Fix arrow-function bodies getting excess parens #1128
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This makes it a lot easier to see what's going on when test cases fail, because we can now see which ones passed and which failed in any given group. We leave the "Arithmetic" group as a single test, because it contains a very large number of test cases.
Printing the entire source code for each side of the comparison, rather than just the first node path where they differ, makes it easier to see what went wrong in printing.
I don't quite see why the justNeedsOpeningParen logic would be necessary. If having just an opening parenthesis is good enough for those nodes (and indeed it seems like it is), then it seems like it should be good enough for any node. I.e., we could treat justNeedsOpeningParen as always true. And if we do so, all the tests still pass. So I'm now pretty confident that we can in fact cut that logic out. This code gets quite a bit simpler when we do, so that's nice! Then in particular, cutting out this pair of calls to firstInStatement and canBeFirstInStatement leaves just one remaining call site for each of them. That makes things a lot simpler for us when we go to make changes to how those functions work -- which we're about to do, in order to fix some bugs.
We just cut the only call site that passed this flag, in hasParens.
There's another copy of these a few lines above.
Previously, no tests would fail if you commented out the check for ClassExpression in canBeFirstInStatement. As these new test cases show, that check is indeed necessary.
As it was, if there was a non-`n.Node` value in the middle of the stack (is that even possible?), then we'd recheck the same parent and child as we did on the last iteration. That can't do anything useful; instead, just have each iteration always check either the current parent and child, or nothing.
To get here, the child would have to already be a statement, so we'd have stopped at the previous step. All tests still pass. In fact, all tests still pass if instead of deleting this one replaces the body with `assert.fail()`. So that adds empirical evidence that this case is impossible.
…ement These are fundamentally in exactly the same situation as a CallExpression, a BinaryExpression, etc.
We're going to change how this works, to fix some bugs. Putting the logic here will simplify that.
We were inserting parentheses unnecessarily in the bodies of some
arrow functions. In particular, `() => function(){}` would become
`() => (function(){})`, and `() => class {}.foo.bar` would become
`() => (class {}).foo.bar`.
The cause turns out to be that we were taking the logic that
applies if you have an *expression statement* starting with a
FunctionExpression or ClassExpression -- which does indeed need
parens, to avoid getting parsed as a FunctionDeclaration or
ClassDeclaration respectively -- and applying it the same way if
one of those expressions is instead at the start of a braceless
arrow-function body, aka an ExpressionBody.
In fact, while an ExpressionBody does call for similar intervention
if it starts with a `{` token (so with an ObjectExpression node),
that is the only case where it does.
In the ES spec, this is expressed as lookahead constraints, on the
one hand at ExpressionStatement:
https://tc39.es/ecma262/#prod-ExpressionStatement
and on the other hand at the two productions where ExpressionBody
appears:
https://tc39.es/ecma262/#prod-ConciseBody
https://tc39.es/ecma262/#prod-AsyncConciseBody
Adjust the logic accordingly to distinguish the two situations,
and add tests.
The ExpressionStatement lookahead constraints also point at one
more edge case which we don't yet correctly handle. We'll fix that
in the next commit for the sake of making the logic comprehensive,
along with comments explaining how it corresponds to the spec.
Fixes: benjamn#914
Fixes: benjamn#1082
This is a fun edge case I discovered from reading the ES spec, in particular here: https://tc39.es/ecma262/#prod-ExpressionStatement while working out whether there were cases we were missing in this logic, beyond the three we had. I believe this turns out to be the only such case. Add comments explaining why, with references to the language spec.
This was referenced Jun 12, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #914.
Fixes #1082.
We were inserting parentheses unnecessarily in the bodies of some
arrow functions. In particular,
() => function(){}would become() => (function(){}), and() => class {}.foo.barwould become() => (class {}).foo.bar.The cause turns out to be that we were taking the logic that
applies if you have an expression statement starting with a
FunctionExpression or ClassExpression -- which does indeed need
parens, to avoid getting parsed as a FunctionDeclaration or
ClassDeclaration respectively -- and applying it the same way if
one of those expressions is instead at the start of a braceless
arrow-function body, aka an ExpressionBody.
In fact, while an ExpressionBody does call for similar intervention
if it starts with a
{token (so with an ObjectExpression node),that is the only case where it does.
In the ES spec, this is expressed as lookahead constraints, on the
one hand at ExpressionStatement:
https://tc39.es/ecma262/#prod-ExpressionStatement
and on the other hand at the two productions where ExpressionBody
appears:
https://tc39.es/ecma262/#prod-ConciseBody
https://tc39.es/ecma262/#prod-AsyncConciseBody
Adjust the logic accordingly to distinguish the two situations,
and add tests.
Then, fix the one remaining edge case in this area that comes up when looking at the spec. It's a pretty absurd case (the code we buggily emit is code that TC39 decided it was OK to completely change the meaning of when writing ES6, and you can see why), but fixing it allows us to compare our logic to the spec and conclude that it matches.
Along the way, make some refactors in the
needsParensandhasParenscode to support making these fixes, and some test-infrastructure improvements intest/parens.ts.