Skip to content

Commit

Permalink
Make path.needsParens(true) ignore statement parsing ambiguities.
Browse files Browse the repository at this point in the history
As @spicyj pointed out in
benjamn/recast#81 (comment), the
NodePath.prototype.needsParens method has been conflating precedence logic
with garden-path parser lookahead quirks.

Rather than removing that edge case logic from the function, I have made
it possible to skip the canBeFirstInStatement/firstInStatement checks by
explicitly passing true to needsParens.

Since the behavior of needsParens differs only if client code passes true
for the assumeExpressionContext parameter, this change is completely
backwards-compatible and does not require a minor version bump.
  • Loading branch information
divanutq authored and benjamn committed May 14, 2014
1 parent f9a6599 commit 9b208a5
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 6 deletions.
28 changes: 22 additions & 6 deletions lib/node-path.js
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,20 @@ NPp.getValueProperty = function(name) {
return types.getFieldValue(this.value, name);
};

NPp.needsParens = function() {
/**
* Determine whether this.node needs to be wrapped in parentheses in order
* for a parser to reproduce the same local AST structure.
*
* For instance, in the expression `(1 + 2) * 3`, the BinaryExpression
* whose operator is "+" needs parentheses, because `1 + 2 * 3` would
* parse differently.
*
* If assumeExpressionContext === true, we don't worry about edge cases
* like an anonymous FunctionExpression appearing lexically first in its
* enclosing statement and thus needing parentheses to avoid being parsed
* as a FunctionDeclaration with a missing name.
*/
NPp.needsParens = function(assumeExpressionContext) {
if (!this.parent)
return false;

Expand Down Expand Up @@ -215,11 +228,8 @@ NPp.needsParens = function() {
}
}

if (n.FunctionExpression.check(node) &&
this.firstInStatement())
return true;

if (n.ObjectExpression.check(node) &&
if (assumeExpressionContext !== true &&
!this.canBeFirstInStatement() &&
this.firstInStatement())
return true;

Expand Down Expand Up @@ -266,6 +276,12 @@ function containsCallExpression(node) {
return false;
}

NPp.canBeFirstInStatement = function() {
var node = this.node;
return !n.FunctionExpression.check(node)
&& !n.ObjectExpression.check(node);
};

NPp.firstInStatement = function() {
return firstInStatement(this);
};
Expand Down
18 changes: 18 additions & 0 deletions test/run.js
Original file line number Diff line number Diff line change
Expand Up @@ -489,6 +489,24 @@ describe("NodePath", function() {
var sequenceAssignmentPath = new NodePath(sequenceAssignmentAST);
assert.ok(sequenceAssignmentPath.get("right").needsParens());
});

it("should support .needsParens(true)", function() {
var programPath = new NodePath(parse("(function(){})"));
var funExpPath = programPath.get("body", 0, "expression");
n.FunctionExpression.assert(funExpPath.value);
assert.strictEqual(funExpPath.needsParens(), true);
assert.strictEqual(funExpPath.canBeFirstInStatement(), false);
assert.strictEqual(funExpPath.firstInStatement(), true);
assert.strictEqual(funExpPath.needsParens(true), false);

programPath = new NodePath(parse("({ foo: 42 })"));
var objLitPath = programPath.get("body", 0, "expression");
n.ObjectExpression.assert(objLitPath.value);
assert.strictEqual(objLitPath.needsParens(), true);
assert.strictEqual(objLitPath.canBeFirstInStatement(), false);
assert.strictEqual(objLitPath.firstInStatement(), true);
assert.strictEqual(objLitPath.needsParens(true), false);
});
});

describe("path.replace", function() {
Expand Down

0 comments on commit 9b208a5

Please sign in to comment.