diff --git a/lib/fast-path.ts b/lib/fast-path.ts index 925cbaac..621d9648 100644 --- a/lib/fast-path.ts +++ b/lib/fast-path.ts @@ -41,9 +41,11 @@ interface FastPathType { hasParens(): any; getPrevToken(node: any): any; getNextToken(node: any): any; - needsParens(assumeExpressionContext?: boolean): any; - canBeFirstInStatement(): any; - firstInStatement(): any; + needsParens(): any; + firstInExpressionStatement(): boolean; + firstInExpressionStatementOrExpressionBody( + onlyExpressionStatement?: boolean, + ): boolean; } interface FastPathConstructor { @@ -225,54 +227,12 @@ FPp.map = function map(callback /*, name1, name2, ... */) { return result; }; -// Returns true if the node at the tip of the path is wrapped with -// parentheses, OR if the only reason the node needed parentheses was that -// it couldn't be the first expression in the enclosing statement (see -// FastPath#canBeFirstInStatement), and it has an opening `(` character. -// For example, the FunctionExpression in `(function(){}())` appears to -// need parentheses only because it's the first expression in the AST, but -// since it happens to be preceded by a `(` (which is not apparent from -// the AST but can be determined using FastPath#getPrevToken), there is no -// ambiguity about how to parse it, so it counts as having parentheses, -// even though it is not immediately followed by a `)`. +// Returns true if the node at the tip of the path is preceded by an +// open-paren token `(`. FPp.hasParens = function () { const node = this.getNode(); - const prevToken = this.getPrevToken(node); - if (!prevToken) { - return false; - } - - const nextToken = this.getNextToken(node); - if (!nextToken) { - return false; - } - - if (prevToken.value === "(") { - if (nextToken.value === ")") { - // If the node preceded by a `(` token and followed by a `)` token, - // then of course it has parentheses. - return true; - } - - // If this is one of the few Expression types that can't come first in - // the enclosing statement because of parsing ambiguities (namely, - // FunctionExpression, ObjectExpression, and ClassExpression) and - // this.firstInStatement() returns true, and the node would not need - // parentheses in an expression context because this.needsParens(true) - // returns false, then it just needs an opening parenthesis to resolve - // the parsing ambiguity that made it appear to need parentheses. - const justNeedsOpeningParen = - !this.canBeFirstInStatement() && - this.firstInStatement() && - !this.needsParens(true); - - if (justNeedsOpeningParen) { - return true; - } - } - - return false; + return prevToken && prevToken.value === "("; }; FPp.getPrevToken = function (node) { @@ -311,7 +271,7 @@ FPp.getNextToken = function (node) { // Inspired by require("ast-types").NodePath.prototype.needsParens, but // more efficient because we're iterating backwards through a stack. -FPp.needsParens = function (assumeExpressionContext) { +FPp.needsParens = function () { const node = this.getNode(); // This needs to come before `if (!parent) { return false }` because @@ -541,15 +501,48 @@ FPp.needsParens = function (assumeExpressionContext) { return containsCallExpression(node); } - if ( - assumeExpressionContext !== true && - !this.canBeFirstInStatement() && - this.firstInStatement() - ) { - return true; - } + // The ExpressionStatement production, and the two productions that + // contain ExpressionBody, have lookahead constraints that forbid some + // possibilities for their next node or two: + // https://tc39.es/ecma262/#prod-ExpressionStatement + // https://tc39.es/ecma262/#prod-ConciseBody + // https://tc39.es/ecma262/#prod-AsyncConciseBody + // + // The effect of these is that if we have an expression that appears in + // one of those and would start with a forbidden token sequence, we need + // to insert parens so that the first token is `(` instead. + // + // We choose to do this on the smallest subexpression we can. + switch (node.type) { + case "ObjectExpression": + // Will start with `{`. Therefore can't be the start of an + // ExpressionStatement or (either use of) an ExpressionBody. + return this.firstInExpressionStatementOrExpressionBody(); - return false; + case "FunctionExpression": + case "ClassExpression": + // Will start with the token `function`, tokens `async function`, or + // token `class`. Therefore can't start an ExpressionStatement. + return this.firstInExpressionStatement(); + + case "MemberExpression": + if ( + n.Identifier.check(node.object) && + node.object.name === "let" && + node.computed + ) { + // Will start with the tokens `let [`. Therefore can't start an + // ExpressionStatement. + return this.firstInExpressionStatement(); + } + return false; + + default: + // Will not start with any of the above sequences of tokens, unless it + // starts with a child node that does. If so, that child will take + // care of it (possibly by letting its own child take care of it, etc.) + return false; + } }; function isBinary(node: any) { @@ -585,63 +578,43 @@ function containsCallExpression(node: any): any { return false; } -FPp.canBeFirstInStatement = function () { - const node = this.getNode(); - - if (n.FunctionExpression.check(node)) { - return false; - } - - if (n.ObjectExpression.check(node)) { - return false; - } - - if (n.ClassExpression.check(node)) { - return false; - } - - return true; +FPp.firstInExpressionStatement = function () { + return this.firstInExpressionStatementOrExpressionBody(true); }; -FPp.firstInStatement = function () { +FPp.firstInExpressionStatementOrExpressionBody = function ( + onlyExpressionStatement: boolean = false, +) { const s = this.stack; let parentName, parent; let childName, child; for (let i = s.length - 1; i >= 0; i -= 2) { - if (n.Node.check(s[i])) { - childName = parentName; - child = parent; - parentName = s[i - 1]; - parent = s[i]; + if (!n.Node.check(s[i])) { + continue; } + childName = parentName; + child = parent; + parentName = s[i - 1]; + parent = s[i]; if (!parent || !child) { continue; } - if ( - n.BlockStatement.check(parent) && - parentName === "body" && - childName === 0 - ) { - assert.strictEqual(parent.body[0], child); - return true; - } - if (n.ExpressionStatement.check(parent) && childName === "expression") { assert.strictEqual(parent.expression, child); return true; } - if (n.AssignmentExpression.check(parent) && childName === "left") { - assert.strictEqual(parent.left, child); - return true; - } - if (n.ArrowFunctionExpression.check(parent) && childName === "body") { assert.strictEqual(parent.body, child); - return true; + return !onlyExpressionStatement; + } + + if (n.AssignmentExpression.check(parent) && childName === "left") { + assert.strictEqual(parent.left, child); + continue; } // s[i + 1] and s[i + 2] represent the array between the parent diff --git a/test/parens.ts b/test/parens.ts index dd20af5f..aedca393 100644 --- a/test/parens.ts +++ b/test/parens.ts @@ -15,14 +15,27 @@ function parseExpression(expr: any) { return n.ExpressionStatement.check(ast) ? ast.expression : ast; } -function check(expr: any) { +function check_(expr: string) { const ast = parse(expr); const reprinted = printer.print(ast).code; assert.strictEqual(reprinted, expr); const expressionAst = parseExpression(expr); const generic = printer.printGenerically(expressionAst).code; - types.astNodesAreEquivalent.assert(expressionAst, parseExpression(generic)); + + if (!types.astNodesAreEquivalent(expressionAst, parseExpression(generic))) { + throw new assert.AssertionError({ + message: "Expected values to parse to equivalent ASTs", + actual: generic, + expected: expr, + }); + } +} + +function check(expr: string) { + it(`handles: ${expr}`, () => { + check_(expr); + }); } const operators = [ @@ -53,18 +66,18 @@ const operators = [ describe("parens", function () { it("Arithmetic", function () { - check("1 - 2"); - check(" 2 +2 "); + check_("1 - 2"); + check_(" 2 +2 "); operators.forEach(function (op1) { operators.forEach(function (op2) { - check("(a " + op1 + " b) " + op2 + " c"); - check("a " + op1 + " (b " + op2 + " c)"); + check_("(a " + op1 + " b) " + op2 + " c"); + check_("a " + op1 + " (b " + op2 + " c)"); }); }); }); - it("Unary", function () { + describe("Unary", function () { check("(-a).b"); check("(+a).b"); check("(!a).b"); @@ -74,14 +87,14 @@ describe("parens", function () { check("(delete a.b).c"); }); - it("Binary", function () { + describe("Binary", function () { check("(a && b)()"); check("typeof (a && b)"); check("(a && b)[c]"); check("(a && b).c"); }); - it("Sequence", function () { + describe("Sequence", function () { check("(a, b)()"); check("a(b, (c, d), e)"); check("!(a, b)"); @@ -94,7 +107,7 @@ describe("parens", function () { check("a = (1, 2)"); }); - it("NewExpression", function () { + describe("NewExpression", function () { check("new (a.b())"); check("new (a.b())(c)"); check("new a.b(c)"); @@ -107,7 +120,7 @@ describe("parens", function () { check('(new Date)["getTime"]()'); }); - it("Numbers", function () { + describe("Numbers", function () { check("(1).foo"); check("(-1).foo"); check("+0"); @@ -115,7 +128,7 @@ describe("parens", function () { check("(-Infinity).foo"); }); - it("Assign", function () { + describe("Assign", function () { check("!(a = false)"); check("a + (b = 2) + c"); check("(a = fn)()"); @@ -124,7 +137,7 @@ describe("parens", function () { check("(a = b).c"); }); - it("Function", function () { + describe("Function", function () { check("a(function (){}.bind(this))"); check("(function (){}).apply(this, arguments)"); check("function f() { (function (){}).call(this) }"); @@ -134,12 +147,12 @@ describe("parens", function () { check("a || ((x, y={z:1}) => x + y.z)"); }); - it("ObjectLiteral", function () { + describe("ObjectLiteral", function () { check("a({b:c(d)}.b)"); check("({a:b(c)}).a"); }); - it("ArrowFunctionExpression", () => { + describe("ArrowFunctionExpression", () => { check("(() => {})()"); check("test(() => {})"); @@ -149,7 +162,7 @@ describe("parens", function () { check("(() => {}) + (() => {})"); }); - it("AwaitExpression", function () { + describe("AwaitExpression", function () { check("async () => (await a) && (await b)"); check("async () => +(await a)"); check("async () => (await f)()"); @@ -159,7 +172,7 @@ describe("parens", function () { check("async () => (await a).b"); }); - it("YieldExpression", function () { + describe("YieldExpression", function () { check("function* test () { return (yield a) && (yield b) }"); check("function* test () { return +(yield a) }"); check("function* test () { return (yield f)() }"); @@ -170,14 +183,47 @@ describe("parens", function () { check("function* test () { yield yield foo }"); }); - it("ArrowFunctionExpression", () => { - check("(() => {})()"); - check("test(() => {})"); + describe("ClassExpression", function () { + check("(class { static f() { return 3; } }).f()"); + check("const x = class { static f() { return 3; } }.f()"); + }); - check("(() => {}).test"); - check("test[() => {}]"); + describe("let-bracket ambiguity", () => { + // Test the `let [` lookahead constraint of: + // https://tc39.es/ecma262/#prod-ExpressionStatement - check("(() => {}) + (() => {})"); + // This prints "[5]" (try it!). A MemberExpression `let [x]` appears twice. + check("{var let = [[3]], x = 0; (let [x] = [5]); console.log(let [x])}"); + + // This prints "[3]". The first `let [x]` starts a LexicalDeclaration. + check("{var let = [[3]], x = 0; {let [x] = [5];} console.log(let [x])}"); + + // Focussing in on the key statements: + check("(let [x] = [5])"); // ExpressionStatement, with MemberExpression + check("let [x] = [5]"); // LexicalDeclaration + }); + + describe("ExpressionBody", () => { + // We need parens if an ExpressionBody starts with an ObjectExpression: + check("() => ({ x: 1 })"); // ExpressionBody, with an object literal + check("() => { x: 1 }"); // FunctionBody, with statement labelled "x" + + // But we don't for other kinds of nodes -- even some that *would* need + // parens if they were the start of an ExpressionStatement. + + // Issue #914: FunctionExpression at start of ExpressionBody + check("const e = () => function(g, h) { return i; };"); + check("() => function(){}"); + check("() => function(){}.call(this)"); + + // Issue #1082: ClassExpression at start of ExpressionBody + check("const D = (C) => class extends C {};"); + check("() => class {}"); + check("() => class {}.prototype"); + + // `let [` starting an ExpressionBody + check("() => let[x]"); + check("() => let [x] = [5]"); }); it("ReprintedParens", function () { @@ -333,16 +379,16 @@ describe("parens", function () { assert.strictEqual(printer.print(ast).code, code); }); - it("should be added to callees that are function expressions", function () { + describe("should be added to callees that are function expressions", function () { check("(()=>{})()"); check("(function (){})()"); }); - it("should be added to function expressions at start of sequence", function () { + describe("should be added to function expressions at start of sequence", function () { check("(function (){})(), 0"); }); - it("issues #504 and #512", function () { + describe("issues #504 and #512", function () { check("() => ({})['foo']"); check("() => ({ foo: 123 }[foo] + 2) * 3"); check("() => ({ foo: 123 }['foo'] + 1 - 2 - 10)"); @@ -350,11 +396,11 @@ describe("parens", function () { check("() => (function () { return 456 }())"); }); - it("should be added to bound arrow function expressions", function () { + describe("should be added to bound arrow function expressions", function () { check("(()=>{}).bind(x)"); }); - it("should be added to object destructuring assignment expressions", function () { + describe("should be added to object destructuring assignment expressions", function () { check("({x}={x:1})"); // Issue #533 check("({ foo } = bar)"); @@ -362,7 +408,7 @@ describe("parens", function () { it("regression test for issue #327", function () { const expr = "(function(){}())"; - check(expr); + check_(expr); const ast = parse(expr); const callExpression = ast.program.body[0].expression; @@ -379,7 +425,7 @@ describe("parens", function () { it("regression test for issue #366", function () { const code = "typeof a ? b : c"; - check(code); + check_(code); const ast = parse(code); const exprStmt = ast.program.body[0];