Call AST #5117
Call AST #5117
Conversation
| @@ -520,7 +520,7 @@ grammar = | |||
| # The list of arguments to a function call. | |||
| Arguments: [ | |||
| o 'CALL_START CALL_END', -> [] | |||
| o 'CALL_START ArgList OptComma CALL_END', -> $2 | |||
| o 'CALL_START ArgList OptComma CALL_END', -> $2.implicit = $1.generated; $2 | |||
There was a problem hiding this comment.
Preserve whether the call was implicit through the grammar
| return | ||
| callee: @variable.ast() | ||
| arguments: arg.ast() for arg in @args | ||
| optional: !!@soak |
There was a problem hiding this comment.
optional is similar to what we saw with MemberExpression (this is how Babel was representing the proposed JS version of soak operators, though they now seem to have switched to distinguishing via type (here would be OptionalCallExpression) as of Babel 7)
There was a problem hiding this comment.
So shouldn’t we go with what Babel 7 is using?
There was a problem hiding this comment.
Probably, but I'd like to see if I can find an explanation as to why they switched
There was a problem hiding this comment.
If you're curious, babel/babel#7256 discusses the switch (to OptionalMemberExpression/OptionalCallExpression). Haven't finished reading through it yet, but one thing that has implications if we want to mimic it is that OptionalMemberExpression "cascades" (so eg in a?.b.c, both the inner (ie a?.b) and outer (a?.b.c) are OptionalMemberExpressions - the inner has optional: true corresponding to it actually having a ?, while the outer has optional: false)
There was a problem hiding this comment.
Well if this is the Babel spec now, I would think that we should follow it, yes?
There was a problem hiding this comment.
@GeoffreyBooth yes I think so. But I'd like to tackle that as a separate PR as there's some complexity there. And in the meantime I'll keep putting up individual node class PRs
There was a problem hiding this comment.
Maybe add a test for the new form so we don't forget to do it?
There was a problem hiding this comment.
@GeoffreyBooth sure added a TODO comment on a test that should end up using OptionalMemberExpression (unless you meant commit a failing test?)
There was a problem hiding this comment.
I was thinking we'd add a commented out failing test, but either way works.
src/rewriter.coffee
Outdated
| @@ -655,7 +655,7 @@ exports.Rewriter = class Rewriter | |||
| # primitive string and separately passing any expected token data properties | |||
| exposeTokenDataToGrammar: -> | |||
| @scanTokens (token, i) -> | |||
| if token.data and Object.keys(token.data).length or token[0] is 'JS' and token.generated | |||
| if token.data and Object.keys(token.data).length or token[0] in ['JS', 'CALL_START'] and token.generated | |||
There was a problem hiding this comment.
Can you please explain this change? JS is a special case because I create empty JS tokens to hold comments, but shouldn’t CALL_START have a data property like all the other tokens we’re passing through the parser with extra data?
There was a problem hiding this comment.
From my understanding, generated is a preexisting pattern that sets generated directly on token rather than using the token data mechanism. So this wasn't catching it
I guess one option would be to update all current uses of generated to be set as token data? Not sure if that's the type of refactor you'd rather treat separately
There was a problem hiding this comment.
Maybe rather than looking for token tag, should we look for .generated?
if token.generated or (token.data and Object.keys(token.data).length isnt 0)I guess the question is, what’s the harm in applying this for all generated tokens rather than only generated JS and CALL_START ones. Are those two special because they’re generated in the rewriter? Are they the only token types generated in the rewriter, and if not, should we include the other types (or if not, why not)?
Just wanting to make sure we understand what’s going on here. A comment explaining it might be good to add.
Also I think you’re right in that generated is different than the other data, in that it’s metadata of about the token parsing process itself rather than a property of the token like whether a boolean was true or yes.
# Conflicts: # test/abstract_syntax_tree.coffee # test/abstract_syntax_tree_location_data.coffee
| return | ||
| callee: @variable.ast() | ||
| arguments: arg.ast() for arg in @args | ||
| optional: !!@soak |
There was a problem hiding this comment.
Probably, but I'd like to see if I can find an explanation as to why they switched
src/rewriter.coffee
Outdated
| @@ -655,7 +655,7 @@ exports.Rewriter = class Rewriter | |||
| # primitive string and separately passing any expected token data properties | |||
| exposeTokenDataToGrammar: -> | |||
| @scanTokens (token, i) -> | |||
| if token.data and Object.keys(token.data).length or token[0] is 'JS' and token.generated | |||
| if token.data and Object.keys(token.data).length or token[0] in ['JS', 'CALL_START'] and token.generated | |||
There was a problem hiding this comment.
From my understanding, generated is a preexisting pattern that sets generated directly on token rather than using the token data mechanism. So this wasn't catching it
I guess one option would be to update all current uses of generated to be set as token data? Not sure if that's the type of refactor you'd rather treat separately
There was a problem hiding this comment.
@GeoffreyBooth updated to avoid the "whitelist" of tokens that should expose generated to the grammar
| return | ||
| callee: @variable.ast() | ||
| arguments: arg.ast() for arg in @args | ||
| optional: !!@soak |
There was a problem hiding this comment.
If you're curious, babel/babel#7256 discusses the switch (to OptionalMemberExpression/OptionalCallExpression). Haven't finished reading through it yet, but one thing that has implications if we want to mimic it is that OptionalMemberExpression "cascades" (so eg in a?.b.c, both the inner (ie a?.b) and outer (a?.b.c) are OptionalMemberExpressions - the inner has optional: true corresponding to it actually having a ?, while the outer has optional: false)
@GeoffreyBooth AST for
Calls (includingnew)Based on
op-ast, here is just the diff against that branch