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

[CS2] Fix handling of parameters that are function calls #4427

Merged
Show file tree
Hide file tree
Changes from 3 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
15 changes: 9 additions & 6 deletions lib/coffee-script/nodes.js

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

19 changes: 13 additions & 6 deletions src/nodes.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -1945,7 +1945,7 @@ exports.Code = class Code extends Base
# encountered, add these other parameters to the list to be output in
# the function definition.
else
if param.isComplex()
if param.isComplex() or param.isCall()
Copy link
Collaborator

Choose a reason for hiding this comment

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

So a call does not count as "complex"? What happens if we change .isComplex() to also check for calls instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just couldn’t remember what isComplex meant 😄 But I did remember that we documented it here, which says:

If node.isComplex() is false, it is safe to use node more than once. Otherwise you need to store the value of node in a variable and output that variable several times instead. Kind of like this: 5 is not complex. returnFive() is complex. It’s all about “do we need to cache this expression?”

So there it is—returnFive() is exactly our use case. Thanks for the suggestion!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

# This parameter is destructured. So add a statement to the function
# body assigning it, e.g. `(arg) => { var a = arg.a; }` or with a
# default value if it has one.
Expand All @@ -1960,7 +1960,7 @@ exports.Code = class Code extends Base
# set by the `isComplex()` block above, define it as a statement in
# the function body. This parameter comes after the splat parameter,
# so we can’t define its default value in the parameter list.
unless param.isComplex()
unless param.isComplex() or param.isCall()
ref = if param.value? then new Assign new Value(param.name), param.value, '=' else param
# Add this parameter’s reference to the function scope
o.scope.parameter fragmentsToText (if param.value? then param else ref).compileToFragments o
Expand Down Expand Up @@ -1993,10 +1993,12 @@ exports.Code = class Code extends Base

# Assemble the output
modifiers = []
modifiers.push 'static' if @isMethod and @isStatic
modifiers.push 'async' if @isAsync
modifiers.push 'function' if not @isMethod and not @bound
modifiers.push '*' if @isGenerator
modifiers.push 'static' if @isMethod and @isStatic
modifiers.push 'async' if @isAsync
unless @isMethod or @bound
modifiers.push "function#{if @isGenerator then '*' else ''}"
else if @isGenerator
modifiers.push '*'

signature = [@makeCode '(']
for param, i in params
Expand Down Expand Up @@ -2110,6 +2112,11 @@ exports.Param = class Param extends Base
isComplex: ->
@name.isComplex()

isCall: ->
# We need to be careful that parameters whose default value is a function
# call do not call that function more than once.
@value? and @value instanceof Call

# Iterates the name or names of a `Param`.
# In a sense, a destructured parameter represents multiple JS parameters. This
# method allows to iterate them all.
Expand Down
6 changes: 6 additions & 0 deletions test/functions.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -341,3 +341,9 @@ test "#1038 Optimize trailing return statements", ->
foo()
return
""")

test "#4406 Destructured parameter default evaluation order", ->
current = 0
next = -> ++current
foo = ({ a = next() }, b = next()) -> [ a, b ]
arrayEq foo({}), [1, 2]