[CS2] Fix handling of parameters that are complex

src/nodes.coffee

@@ -220,17 +220,39 @@ exports.Base = class Base
 
   # Default implementations of the common node properties and methods. Nodes
   # will override these with custom logic, if needed.
+
+  # `children` are the properties to recurse into when tree walking. The
+  # `children` list *is* the structure of the AST. The `parent` pointer, and
+  # the pointer to the `children` are how you can traverse the tree.
   children: []
 
-  isStatement     : NO
-  jumps           : NO
-  isComplex       : YES
-  isChainable     : NO
-  isAssignable    : NO
-  isNumber        : NO
+  # `isStatement` has to do with “everything is an expression”. A few things
+  # can’t be expressions, such as `break`. Things that `isStatement` returns
+  # `true` for are things that can’t be used as expressions. There are some
+  # error messages that come from `nodes.coffee` due to statements ending up
+  # in expression position.
+  isStatement: NO
+
+  # `jumps` tells you if an expression, or an internal part of an expression
+  # has a flow control construct (like `break`, or `continue`, or `return`,
+  # or `throw`) that jumps out of the normal flow of control and can’t be
+  # used as a value. This is important because things like this make no sense;
+  # we have to disallow them.
+  jumps: NO
+
+  # 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?”
+  isComplex: YES
 
-  unwrap     : THIS
-  unfoldSoak : NO
+  isChainable: NO
+  isAssignable: NO
+  isNumber: NO
+
+  unwrap: THIS
+  unfoldSoak: NO
 
   # Is this node used to assign a certain variable?
   assigns: NO
@@ -1946,12 +1968,18 @@ exports.Code = class Code extends Base
       # the function definition.
       else
         if param.isComplex()
-          # 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.
+          # This parameter cannot be declared or assigned in the parameter
+          # list. 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.
           val = ref = param.asReference o
-          val = new Op '?', ref, param.value if param.value
-          exprs.push new Assign new Value(param.name), val, '=', param: yes
+          if param.value?
+            condition = new Literal param.name.value + ' === undefined'
+            ifTrue = new Assign new Value(param.name), param.value, '=', param: yes
+            exprs.push new If condition, ifTrue
+          else
+            val = new Op '?', ref, param.value if param.value
+            exprs.push new Assign new Value(param.name), val, '=', param: yes
 
         # If this parameter comes before the splat or expansion, it will go
         # in the function definition parameter list.
@@ -2112,7 +2140,39 @@ exports.Param = class Param extends Base
     @reference = node
 
   isComplex: ->
-    @name.isComplex() or @value instanceof Call
+    # Should this parameter be declared in the function body, rather than in
+    # the parameter list? We don’t want to pull out any parameter that
+    # has a value, or else we break default values; but we want things like
+    # generator functions and iterators to evaluate in the correct order,
+    # which they might not do unless we pull them out into the function body.
+    if @name.isComplex()
+      yes
+    else if @value? and @value.isComplex()
+      if @value instanceof Value
+        unless @value.base.isComplex()
+          no
+        else
+          # Objects and arrays are always complex, but unless they contain any
+          # complex children, they’re safe to leave in the parameter list.
+          if @value.base instanceof Arr or @value.base instanceof Obj
+            hasComplexChild = no
+            @value.base.traverseChildren no, (node) ->
+              if node.isComplex()
+                # This can be further refined. An empty object will evaluate as
+                # not having any complex children, but as long as it has at least
+                # one property it will be considered complex.
+                hasComplexChild = yes
+                no # Stop traversing.
+            hasComplexChild
+          else
+            yes
+      else
+        # Other types of `@value`s in the CoffeeScript codebase:
+        # `Call`, `Class`, `SuperCall`, `Op`.
+        # All of these should be declared and assigned in the function body.
+        yes
+    else
+      no
 
   # Iterates the name or names of a `Param`.
   # In a sense, a destructured parameter represents multiple JS parameters. This

test/functions.coffee

@@ -342,7 +342,12 @@ test "#1038 Optimize trailing return statements", ->
                                                    return
                                                  """)
 
-test "#4406 Destructured parameter default evaluation order", ->
+test "#4406 Destructured parameter default evaluation order with incrementing variable", ->
+  i = 0
+  f = ({ a = ++i }, b = ++i) -> [a, b]
+  arrayEq f({}), [1, 2]
+
+test "#4406 Destructured parameter default evaluation order with generator function", ->
   current = 0
   next    = -> ++current
   foo = ({ a = next() }, b = next()) -> [ a, b ]