Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Call AST #5117

Merged
merged 19 commits into from
Oct 15, 2018
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/coffeescript/grammar.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

28 changes: 28 additions & 0 deletions lib/coffeescript/nodes.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

30 changes: 18 additions & 12 deletions lib/coffeescript/parser.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 6 additions & 6 deletions lib/coffeescript/rewriter.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion src/grammar.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preserve whether the call was implicit through the grammar

]

# A reference to the *this* current object.
Expand Down
14 changes: 14 additions & 0 deletions src/nodes.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -1285,6 +1285,7 @@ exports.Call = class Call extends Base
constructor: (@variable, @args = [], @soak, @token) ->
super()

@implicit = @args.implicit
@isNew = no
if @variable instanceof Value and @variable.isNotCallable()
@variable.error "literal is not a function"
Expand Down Expand Up @@ -1426,6 +1427,19 @@ exports.Call = class Call extends Base
fragments.push @makeCode(' />')
fragments

astType: ->
if @isNew
'NewExpression'
else
'CallExpression'

astProperties: ->
return
callee: @variable.ast()
arguments: arg.ast() for arg in @args
optional: !!@soak
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So shouldn’t we go with what Babel 7 is using?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably, but I'd like to see if I can find an explanation as to why they switched

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well if this is the Babel spec now, I would think that we should follow it, yes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a test for the new form so we don't forget to do it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GeoffreyBooth sure added a TODO comment on a test that should end up using OptionalMemberExpression (unless you meant commit a failing test?)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking we'd add a commented out failing test, but either way works.

implicit: !!@implicit
GeoffreyBooth marked this conversation as resolved.
Show resolved Hide resolved

#### Super

# Takes care of converting `super()` calls into calls against the prototype's
Expand Down
2 changes: 1 addition & 1 deletion src/rewriter.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@GeoffreyBooth GeoffreyBooth Oct 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

token[1] = new String token[1]
token[1][key] = val for own key, val of (token.data ? {})
token[1].generated = yes if token.generated
Expand Down
Loading