From e39209386c662e77fccf276a0d6f471d61bbf5b7 Mon Sep 17 00:00:00 2001 From: Christopher Chedeau Date: Mon, 1 May 2017 14:32:52 -0700 Subject: [PATCH] Only add parenthesis on ternaries inside of arrow functions if doesn't break (#1450) This was added in order to follow some eslint rule but it's only confusing when it doesn't break, when it breaks the indentation makes it clear what is happening and you don't need parenthesis. Fixes #1379 --- src/fast-path.js | 56 +----------------- src/printer.js | 19 +++++- src/util.js | 58 ++++++++++++++++++- .../__snapshots__/jsfmt.spec.js.snap | 2 +- .../__snapshots__/jsfmt.spec.js.snap | 32 ++++++++++ tests/ternaries/parenthesis.js | 4 ++ 6 files changed, 113 insertions(+), 58 deletions(-) diff --git a/src/fast-path.js b/src/fast-path.js index 6d83ccad..65cd9a4c 100644 --- a/src/fast-path.js +++ b/src/fast-path.js @@ -6,6 +6,7 @@ var util = require("./util"); var n = types.namedTypes; var isArray = types.builtInTypes.array; var isNumber = types.builtInTypes.number; +var startsWithNoLookaheadToken = util.startsWithNoLookaheadToken; function FastPath(value) { assert.ok(this instanceof FastPath); @@ -464,7 +465,6 @@ FPp.needsParens = function() { case "ExportDefaultDeclaration": case "AwaitExpression": case "JSXSpreadAttribute": - case "ArrowFunctionExpression": return true; case "NewExpression": @@ -554,58 +554,4 @@ function containsCallExpression(node) { return false; } -// Tests if an expression starts with `{`, or (if forbidFunctionAndClass holds) `function` or `class`. -// Will be overzealous if there's already necessary grouping parentheses. -function startsWithNoLookaheadToken(node, forbidFunctionAndClass) { - node = getLeftMost(node); - switch (node.type) { - case "FunctionExpression": - case "ClassExpression": - return forbidFunctionAndClass; - case "ObjectExpression": - return true; - case "MemberExpression": - return startsWithNoLookaheadToken(node.object, forbidFunctionAndClass); - case "TaggedTemplateExpression": - if (node.tag.type === "FunctionExpression") { - // IIFEs are always already parenthesized - return false; - } - return startsWithNoLookaheadToken(node.tag, forbidFunctionAndClass); - case "CallExpression": - if (node.callee.type === "FunctionExpression") { - // IIFEs are always already parenthesized - return false; - } - return startsWithNoLookaheadToken(node.callee, forbidFunctionAndClass); - case "ConditionalExpression": - return startsWithNoLookaheadToken(node.test, forbidFunctionAndClass); - case "UpdateExpression": - return ( - !node.prefix && - startsWithNoLookaheadToken(node.argument, forbidFunctionAndClass) - ); - case "BindExpression": - return ( - node.object && - startsWithNoLookaheadToken(node.object, forbidFunctionAndClass) - ); - case "SequenceExpression": - return startsWithNoLookaheadToken( - node.expressions[0], - forbidFunctionAndClass - ); - default: - return false; - } -} - -function getLeftMost(node) { - if (node.left) { - return getLeftMost(node.left); - } else { - return node; - } -} - module.exports = FastPath; diff --git a/src/printer.js b/src/printer.js index f3dd181a..3a3f7ab5 100644 --- a/src/printer.js +++ b/src/printer.js @@ -386,12 +386,29 @@ function genericPrintNoParens(path, options, print, args) { // with the opening (. const shouldAddSoftLine = args && args.expandLastArg; + // In order to avoid confusion between + // a => a ? a : a + // a <= a ? a : a + const shouldAddParens = + n.body.type === "ConditionalExpression" && + !util.startsWithNoLookaheadToken( + n.body, + /* forbidFunctionAndClass */ false + ); + return group( concat([ concat(parts), group( concat([ - indent(concat([line, body])), + indent( + concat([ + line, + shouldAddParens ? ifBreak("", "(") : "", + body, + shouldAddParens ? ifBreak("", ")") : "" + ]) + ), shouldAddSoftLine ? concat([ ifBreak(shouldPrintComma(options, "all") ? "," : ""), diff --git a/src/util.js b/src/util.js index 190438dd..1d32ba6c 100644 --- a/src/util.js +++ b/src/util.js @@ -267,6 +267,61 @@ function getPrecedence(op) { return PRECEDENCE[op]; } + +// Tests if an expression starts with `{`, or (if forbidFunctionAndClass holds) `function` or `class`. +// Will be overzealous if there's already necessary grouping parentheses. +function startsWithNoLookaheadToken(node, forbidFunctionAndClass) { + node = getLeftMost(node); + switch (node.type) { + case "FunctionExpression": + case "ClassExpression": + return forbidFunctionAndClass; + case "ObjectExpression": + return true; + case "MemberExpression": + return startsWithNoLookaheadToken(node.object, forbidFunctionAndClass); + case "TaggedTemplateExpression": + if (node.tag.type === "FunctionExpression") { + // IIFEs are always already parenthesized + return false; + } + return startsWithNoLookaheadToken(node.tag, forbidFunctionAndClass); + case "CallExpression": + if (node.callee.type === "FunctionExpression") { + // IIFEs are always already parenthesized + return false; + } + return startsWithNoLookaheadToken(node.callee, forbidFunctionAndClass); + case "ConditionalExpression": + return startsWithNoLookaheadToken(node.test, forbidFunctionAndClass); + case "UpdateExpression": + return ( + !node.prefix && + startsWithNoLookaheadToken(node.argument, forbidFunctionAndClass) + ); + case "BindExpression": + return ( + node.object && + startsWithNoLookaheadToken(node.object, forbidFunctionAndClass) + ); + case "SequenceExpression": + return startsWithNoLookaheadToken( + node.expressions[0], + forbidFunctionAndClass + ); + default: + return false; + } +} + +function getLeftMost(node) { + if (node.left) { + return getLeftMost(node.left); + } else { + return node; + } +} + module.exports = { getPrecedence, isExportDeclaration, @@ -286,5 +341,6 @@ module.exports = { locEnd, setLocStart, setLocEnd, - htmlEscapeInsideAngleBracket + htmlEscapeInsideAngleBracket, + startsWithNoLookaheadToken }; diff --git a/tests/last_argument_expansion/__snapshots__/jsfmt.spec.js.snap b/tests/last_argument_expansion/__snapshots__/jsfmt.spec.js.snap index d56fb130..c1cec4c0 100644 --- a/tests/last_argument_expansion/__snapshots__/jsfmt.spec.js.snap +++ b/tests/last_argument_expansion/__snapshots__/jsfmt.spec.js.snap @@ -104,7 +104,7 @@ true }), require("postcss-url")({ url: url => - (url.startsWith("/") || /^[a-z]+:/.test(url) ? url : \`/static/\${url}\`) + url.startsWith("/") || /^[a-z]+:/.test(url) ? url : \`/static/\${url}\` }) ] }); diff --git a/tests/ternaries/__snapshots__/jsfmt.spec.js.snap b/tests/ternaries/__snapshots__/jsfmt.spec.js.snap index 8f8b6be6..054d4c38 100644 --- a/tests/ternaries/__snapshots__/jsfmt.spec.js.snap +++ b/tests/ternaries/__snapshots__/jsfmt.spec.js.snap @@ -3,23 +3,55 @@ exports[`parenthesis.js 1`] = ` debug ? this.state.isVisible ? "partially visible" : "hidden" : null; debug ? this.state.isVisible && somethingComplex ? "partially visible" : "hidden" : null; + +a => a ? () => {a} : () => {a} +a => a ? a : a +a => a ? aasdasdasdasdasdasdaaasdasdasdasdasdasdasdasdasdasdasdasdasdaaaaaa : a ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ debug ? (this.state.isVisible ? "partially visible" : "hidden") : null; debug ? this.state.isVisible && somethingComplex ? "partially visible" : "hidden" : null; +a => + a + ? () => { + a; + } + : () => { + a; + }; +a => (a ? a : a); +a => + a ? aasdasdasdasdasdasdaaasdasdasdasdasdasdasdasdasdasdasdasdasdaaaaaa : a; + `; exports[`parenthesis.js 2`] = ` debug ? this.state.isVisible ? "partially visible" : "hidden" : null; debug ? this.state.isVisible && somethingComplex ? "partially visible" : "hidden" : null; + +a => a ? () => {a} : () => {a} +a => a ? a : a +a => a ? aasdasdasdasdasdasdaaasdasdasdasdasdasdasdasdasdasdasdasdasdaaaaaa : a ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ debug ? (this.state.isVisible ? "partially visible" : "hidden") : null; debug ? this.state.isVisible && somethingComplex ? "partially visible" : "hidden" : null; +a => + a + ? () => { + a; + } + : () => { + a; + }; +a => (a ? a : a); +a => + a ? aasdasdasdasdasdasdaaasdasdasdasdasdasdasdasdasdasdasdasdasdaaaaaa : a; + `; exports[`test.js 1`] = ` diff --git a/tests/ternaries/parenthesis.js b/tests/ternaries/parenthesis.js index 38e87106..c792c3c6 100644 --- a/tests/ternaries/parenthesis.js +++ b/tests/ternaries/parenthesis.js @@ -1,2 +1,6 @@ debug ? this.state.isVisible ? "partially visible" : "hidden" : null; debug ? this.state.isVisible && somethingComplex ? "partially visible" : "hidden" : null; + +a => a ? () => {a} : () => {a} +a => a ? a : a +a => a ? aasdasdasdasdasdasdaaasdasdasdasdasdasdasdasdasdasdasdasdasdaaaaaa : a