[CS2] Fix handling of parameters that are complex (#4430)

* Add failing test per #4406

* If a parameter is a function call, define it in an expression within the function body

* Remove the space between `function` and `*` for generator functions, to follow usual ES idiom

* We can collapse `isCall` into `isComplex`

* Don’t need existence check here

* Correct destructured parameter default evaluation order with an incrementing variable (or more generally any complicated parameter that isComplex)

* Try to pull complex parameters out of the parameter list if their order of execution matters; but don’t pull _all_ complex parameters out of the parameter list, so that we don’t lose parameter default values

* Add lots of comments about node special properties

* Err on the side of caution in deciding whether a complex parameter is allowable in a function parameter list rather than the function body (there are lots more detections we could add to find additional “safe” parameters)

* Follow the ES and CS2 convention of assigning parameter default values only when undefined, not when null or undefined

* Along with arrays and empty objects, also let values whose bases are not complex be allowed in the function parameter list (like `obj.prop`)

* Better way to check for undefined parameters when declaring them in a function body

* Once we’ve put a complex parameter in the function body, all following complex parameters go into the function body; no need to create lots of exceptions of when to choose whether to put a complex param in the body

* Rename `isComplex` to `shouldCache` for clarity

lib/coffee-script/nodes.js

@@ -1,6 +1,6 @@
 // Generated by CoffeeScript 2.0.0-alpha
 (function() {
-  var Access, Arr, Assign, AwaitReturn, Base, Block, BooleanLiteral, Call, Class, Code, CodeFragment, Comment, ExecutableClassBody, Existence, Expansion, ExportAllDeclaration, ExportDeclaration, ExportDefaultDeclaration, ExportNamedDeclaration, ExportSpecifier, ExportSpecifierList, Extends, For, HoistTarget, IdentifierLiteral, If, ImportClause, ImportDeclaration, ImportDefaultSpecifier, ImportNamespaceSpecifier, ImportSpecifier, ImportSpecifierList, In, Index, InfinityLiteral, JS_FORBIDDEN, LEVEL_ACCESS, LEVEL_COND, LEVEL_LIST, LEVEL_OP, LEVEL_PAREN, LEVEL_TOP, Literal, ModuleDeclaration, ModuleSpecifier, ModuleSpecifierList, NEGATE, NO, NaNLiteral, NullLiteral, NumberLiteral, Obj, Op, Param, Parens, PassthroughLiteral, PropertyName, Range, RegexLiteral, RegexWithInterpolations, Return, SIMPLENUM, Scope, Slice, Splat, StatementLiteral, StringLiteral, StringWithInterpolations, SuperCall, Switch, TAB, THIS, TaggedTemplateCall, ThisLiteral, Throw, Try, UTILITIES, UndefinedLiteral, Value, While, YES, YieldReturn, addLocationDataFn, compact, del, ends, extend, flatten, fragmentsToText, isComplexOrAssignable, isLiteralArguments, isLiteralThis, isUnassignable, locationDataToString, merge, multident, ref1, ref2, some, starts, throwSyntaxError, unfoldSoak, utility,
+  var Access, Arr, Assign, AwaitReturn, Base, Block, BooleanLiteral, Call, Class, Code, CodeFragment, Comment, ExecutableClassBody, Existence, Expansion, ExportAllDeclaration, ExportDeclaration, ExportDefaultDeclaration, ExportNamedDeclaration, ExportSpecifier, ExportSpecifierList, Extends, For, HoistTarget, IdentifierLiteral, If, ImportClause, ImportDeclaration, ImportDefaultSpecifier, ImportNamespaceSpecifier, ImportSpecifier, ImportSpecifierList, In, Index, InfinityLiteral, JS_FORBIDDEN, LEVEL_ACCESS, LEVEL_COND, LEVEL_LIST, LEVEL_OP, LEVEL_PAREN, LEVEL_TOP, Literal, ModuleDeclaration, ModuleSpecifier, ModuleSpecifierList, NEGATE, NO, NaNLiteral, NullLiteral, NumberLiteral, Obj, Op, Param, Parens, PassthroughLiteral, PropertyName, Range, RegexLiteral, RegexWithInterpolations, Return, SIMPLENUM, Scope, Slice, Splat, StatementLiteral, StringLiteral, StringWithInterpolations, SuperCall, Switch, TAB, THIS, TaggedTemplateCall, ThisLiteral, Throw, Try, UTILITIES, UndefinedLiteral, Value, While, YES, YieldReturn, addLocationDataFn, compact, del, ends, extend, flatten, fragmentsToText, isLiteralArguments, isLiteralThis, isUnassignable, locationDataToString, merge, multident, ref1, ref2, shouldCacheOrIsAssignable, some, starts, throwSyntaxError, unfoldSoak, utility,
     indexOf = [].indexOf || function(item) { for (var i = 0, l = this.length; i < l; i++) { if (i in this && this[i] === item) return i; } return -1; },
     slice = [].slice;
 
@@ -112,9 +112,9 @@
         return parts;
       }
 
-      cache(o, level, isComplex) {
+      cache(o, level, shouldCache) {
         var complex, ref, sub;
-        complex = isComplex != null ? isComplex(this) : this.isComplex();
+        complex = shouldCache != null ? shouldCache(this) : this.shouldCache();
         if (complex) {
           ref = new IdentifierLiteral(o.scope.freeVariable('ref'));
           sub = new Assign(ref, this);
@@ -313,7 +313,7 @@
 
     Base.prototype.jumps = NO;
 
-    Base.prototype.isComplex = YES;
+    Base.prototype.shouldCache = YES;
 
     Base.prototype.isChainable = NO;
 
@@ -640,7 +640,7 @@
 
     Literal.__super__ = superClass.prototype;
 
-    Literal.prototype.isComplex = NO;
+    Literal.prototype.shouldCache = NO;
 
     return Literal;
 
@@ -944,8 +944,8 @@
         return this.bareLiteral(Range);
       }
 
-      isComplex() {
-        return this.hasProperties() || this.base.isComplex();
+      shouldCache() {
+        return this.hasProperties() || this.base.shouldCache();
       }
 
       isAssignable() {
@@ -1033,18 +1033,18 @@
       cacheReference(o) {
         var base, bref, name, nref, ref3;
         ref3 = this.properties, name = ref3[ref3.length - 1];
-        if (this.properties.length < 2 && !this.base.isComplex() && !(name != null ? name.isComplex() : void 0)) {
+        if (this.properties.length < 2 && !this.base.shouldCache() && !(name != null ? name.shouldCache() : void 0)) {
           return [this, this];
         }
         base = new Value(this.base, this.properties.slice(0, -1));
-        if (base.isComplex()) {
+        if (base.shouldCache()) {
           bref = new IdentifierLiteral(o.scope.freeVariable('base'));
           base = new Value(new Parens(new Assign(bref, base)));
         }
         if (!name) {
           return [base, bref];
         }
-        if (name.isComplex()) {
+        if (name.shouldCache()) {
           nref = new IdentifierLiteral(o.scope.freeVariable('name'));
           name = new Index(new Assign(nref, name.index));
           nref = new Index(nref);
@@ -1083,7 +1083,7 @@
             prop.soak = false;
             fst = new Value(this.base, this.properties.slice(0, i));
             snd = new Value(this.base, this.properties.slice(i));
-            if (fst.isComplex()) {
+            if (fst.shouldCache()) {
               ref = new IdentifierLiteral(o.scope.freeVariable('ref'));
               fst = new Parens(new Assign(ref, fst));
               snd.base = ref;
@@ -1291,13 +1291,13 @@
           return 'super';
         } else if (method != null ? method.klass : void 0) {
           klass = method.klass, name = method.name, variable = method.variable;
-          if (klass.isComplex()) {
+          if (klass.shouldCache()) {
             bref = new IdentifierLiteral(o.scope.parent.freeVariable('base'));
             base = new Value(new Parens(new Assign(bref, klass)));
             variable.base = base;
             variable.properties.splice(0, klass.properties.length);
           }
-          if (name.isComplex() || (name instanceof Index && name.index.isAssignable())) {
+          if (name.shouldCache() || (name instanceof Index && name.index.isAssignable())) {
             nref = new IdentifierLiteral(o.scope.parent.freeVariable('name'));
             name.index = new Assign(nref, name.index);
           }
@@ -1414,7 +1414,7 @@
 
     Access.prototype.children = ['name'];
 
-    Access.prototype.isComplex = NO;
+    Access.prototype.shouldCache = NO;
 
     return Access;
 
@@ -1431,8 +1431,8 @@
         return [].concat(this.makeCode("["), this.index.compileToFragments(o, LEVEL_PAREN), this.makeCode("]"));
       }
 
-      isComplex() {
-        return this.index.isComplex();
+      shouldCache() {
+        return this.index.shouldCache();
       }
 
     };
@@ -1456,15 +1456,15 @@
       }
 
       compileVariables(o) {
-        var isComplex, ref3, ref4, ref5, step;
+        var ref3, ref4, ref5, shouldCache, step;
         o = merge(o, {
           top: true
         });
-        isComplex = del(o, 'isComplex');
-        ref3 = this.cacheToCodeFragments(this.from.cache(o, LEVEL_LIST, isComplex)), this.fromC = ref3[0], this.fromVar = ref3[1];
-        ref4 = this.cacheToCodeFragments(this.to.cache(o, LEVEL_LIST, isComplex)), this.toC = ref4[0], this.toVar = ref4[1];
+        shouldCache = del(o, 'shouldCache');
+        ref3 = this.cacheToCodeFragments(this.from.cache(o, LEVEL_LIST, shouldCache)), this.fromC = ref3[0], this.fromVar = ref3[1];
+        ref4 = this.cacheToCodeFragments(this.to.cache(o, LEVEL_LIST, shouldCache)), this.toC = ref4[0], this.toVar = ref4[1];
         if (step = del(o, 'step')) {
-          ref5 = this.cacheToCodeFragments(step.cache(o, LEVEL_LIST, isComplex)), this.step = ref5[0], this.stepVar = ref5[1];
+          ref5 = this.cacheToCodeFragments(step.cache(o, LEVEL_LIST, shouldCache)), this.step = ref5[0], this.stepVar = ref5[1];
         }
         this.fromNum = this.from.isNumber() ? Number(this.fromVar) : null;
         this.toNum = this.to.isNumber() ? Number(this.toVar) : null;
@@ -1620,7 +1620,7 @@
             prop = new Assign(prop.properties[0].name, prop, 'object');
           }
           if (!(prop instanceof Comment) && !(prop instanceof Assign)) {
-            if (prop.isComplex()) {
+            if (prop.shouldCache()) {
               ref3 = prop.base.cache(o), key = ref3[0], value = ref3[1];
               if (key instanceof IdentifierLiteral) {
                 key = new PropertyName(key.value);
@@ -1933,7 +1933,7 @@
           method.name = variable.properties[0];
         } else {
           methodName = variable.base;
-          method.name = new (methodName.isComplex() ? Index : Access)(methodName);
+          method.name = new (methodName.shouldCache() ? Index : Access)(methodName);
           if (methodName.value === 'constructor') {
             method.ctor = (this.parent ? 'derived' : 'base');
           }
@@ -2107,7 +2107,7 @@
               }
               assign = this.externalCtor = new Assign(new Value, value);
             } else if (!assign.variable["this"]) {
-              name = new (base.isComplex() ? Index : Access)(base);
+              name = new (base.shouldCache() ? Index : Access)(base);
               prototype = new Access(new PropertyName('prototype'));
               variable = new Value(new ThisLiteral(), [prototype, name]);
               assign.variable = variable;
@@ -2529,7 +2529,7 @@
         }
         compiledName = this.variable.compileToFragments(o, LEVEL_LIST);
         if (this.context === 'object') {
-          if (this.variable.isComplex()) {
+          if (this.variable.shouldCache()) {
             compiledName.unshift(this.makeCode('['));
             compiledName.push(this.makeCode(']'));
           } else if (ref8 = fragmentsToText(compiledName), indexOf.call(JS_FORBIDDEN, ref8) >= 0) {
@@ -2782,7 +2782,7 @@
       }
 
       compileNode(o) {
-        var answer, body, condition, exprs, haveSplatParam, i, ifTrue, j, k, len1, len2, m, methodScope, modifiers, name, param, paramNames, params, paramsAfterSplat, ref, ref3, ref4, ref5, ref6, ref7, ref8, signature, splatParamName, thisAssignments, val, wasEmpty;
+        var answer, body, condition, exprs, haveBodyParam, haveSplatParam, i, ifTrue, j, k, len1, len2, m, methodScope, modifiers, name, param, paramNames, params, paramsAfterSplat, ref, ref3, ref4, ref5, ref6, ref7, ref8, signature, splatParamName, thisAssignments, wasEmpty;
         if (this.ctor) {
           if (this.isAsync) {
             this.variable.error('Class constructor may not be async');
@@ -2809,6 +2809,7 @@
         thisAssignments = (ref4 = (ref5 = this.thisAssignments) != null ? ref5.slice() : void 0) != null ? ref4 : [];
         paramsAfterSplat = [];
         haveSplatParam = false;
+        haveBodyParam = false;
         paramNames = [];
         this.eachParamName(function(name, node, param) {
           var target;
@@ -2839,7 +2840,7 @@
             if (param.splat) {
               params.push(ref = param.asReference(o));
               splatParamName = fragmentsToText(ref.compileNode(o));
-              if (param.isComplex()) {
+              if (param.shouldCache()) {
                 exprs.push(new Assign(new Value(param.name), ref, '=', {
                   param: true
                 }));
@@ -2850,25 +2851,37 @@
             }
             o.scope.parameter(splatParamName);
           } else {
-            if (param.isComplex()) {
-              val = ref = param.asReference(o);
-              if (param.value) {
-                val = new Op('?', ref, param.value);
+            if (param.shouldCache() || haveBodyParam) {
+              param.assignedInBody = true;
+              haveBodyParam = true;
+              if (param.value != null) {
+                condition = new Op('==', param, new UndefinedLiteral);
+                ifTrue = new Assign(new Value(param.name), param.value, '=', {
+                  param: true
+                });
+                exprs.push(new If(condition, ifTrue));
+              } else {
+                exprs.push(new Assign(new Value(param.name), param.asReference(o), '=', {
+                  param: true
+                }));
               }
-              exprs.push(new Assign(new Value(param.name), val, '=', {
-                param: true
-              }));
             }
             if (!haveSplatParam) {
-              if (!param.isComplex()) {
-                ref = param.value != null ? new Assign(new Value(param.name), param.value, '=') : param;
+              if (param.shouldCache()) {
+                ref = param.asReference(o);
+              } else {
+                if ((param.value != null) && !param.assignedInBody) {
+                  ref = new Assign(new Value(param.name), param.value, '=');
+                } else {
+                  ref = param;
+                }
               }
               o.scope.parameter(fragmentsToText((param.value != null ? param : ref).compileToFragments(o)));
               params.push(ref);
             } else {
               paramsAfterSplat.push(param);
-              if ((param.value != null) && !param.isComplex()) {
-                condition = new Literal(param.name.value + ' === undefined');
+              if ((param.value != null) && !param.shouldCache()) {
+                condition = new Op('==', param, new UndefinedLiteral);
                 ifTrue = new Assign(new Value(param.name), param.value, '=');
                 exprs.push(new If(condition, ifTrue));
               }
@@ -3076,7 +3089,7 @@
             name = `_${name}`;
           }
           node = new IdentifierLiteral(o.scope.freeVariable(name));
-        } else if (node.isComplex()) {
+        } else if (node.shouldCache()) {
           node = new IdentifierLiteral(o.scope.freeVariable('arg'));
         }
         node = new Value(node);
@@ -3084,8 +3097,8 @@
         return this.reference = node;
       }
 
-      isComplex() {
-        return this.name.isComplex() || this.value instanceof Call;
+      shouldCache() {
+        return this.name.shouldCache();
       }
 
       eachName(iterator, name = this.name) {
@@ -3204,7 +3217,7 @@
 
     Expansion.__super__ = superClass.prototype;
 
-    Expansion.prototype.isComplex = NO;
+    Expansion.prototype.shouldCache = NO;
 
     return Expansion;
 
@@ -3338,7 +3351,7 @@
         return !this.second;
       }
 
-      isComplex() {
+      shouldCache() {
         return !this.isNumber();
       }
 
@@ -3460,7 +3473,7 @@
 
       compileExistence(o) {
         var fst, ref;
-        if (this.first.isComplex()) {
+        if (this.first.shouldCache()) {
           ref = new IdentifierLiteral(o.scope.freeVariable('ref'));
           fst = new Parens(new Assign(ref, this.first));
         } else {
@@ -3537,7 +3550,7 @@
       compileFloorDivision(o) {
         var div, floor, second;
         floor = new Value(new IdentifierLiteral('Math'), [new Access(new PropertyName('floor'))]);
-        second = this.second.isComplex() ? new Parens(this.second) : this.second;
+        second = this.second.shouldCache() ? new Parens(this.second) : this.second;
         div = new Op('/', this.first, second);
         return new Call(floor, [div]).compileToFragments(o);
       }
@@ -3772,8 +3785,8 @@
         return this.body;
       }
 
-      isComplex() {
-        return this.body.isComplex();
+      shouldCache() {
+        return this.body.shouldCache();
       }
 
       compileNode(o) {
@@ -3813,8 +3826,8 @@
         return this;
       }
 
-      isComplex() {
-        return this.body.isComplex();
+      shouldCache() {
+        return this.body.shouldCache();
       }
 
       compileNode(o) {
@@ -3934,7 +3947,7 @@
         kvar = ((this.range || this.from) && name) || index || ivar;
         kvarAssign = kvar !== ivar ? `${kvar} = ` : "";
         if (this.step && !this.range) {
-          ref4 = this.cacheToCodeFragments(this.step.cache(o, LEVEL_LIST, isComplexOrAssignable)), step = ref4[0], stepVar = ref4[1];
+          ref4 = this.cacheToCodeFragments(this.step.cache(o, LEVEL_LIST, shouldCacheOrIsAssignable)), step = ref4[0], stepVar = ref4[1];
           if (this.step.isNumber()) {
             stepNum = Number(stepVar);
           }
@@ -3951,7 +3964,7 @@
             index: ivar,
             name: name,
             step: this.step,
-            isComplex: isComplexOrAssignable
+            shouldCache: shouldCacheOrIsAssignable
           }));
         } else {
           svar = this.source.compile(o, LEVEL_LIST);
@@ -4340,8 +4353,8 @@
     return node instanceof ThisLiteral || (node instanceof Code && node.bound) || node instanceof SuperCall;
   };
 
-  isComplexOrAssignable = function(node) {
-    return node.isComplex() || (typeof node.isAssignable === "function" ? node.isAssignable() : void 0);
+  shouldCacheOrIsAssignable = function(node) {
+    return node.shouldCache() || (typeof node.isAssignable === "function" ? node.isAssignable() : void 0);
   };
 
   unfoldSoak = function(o, parent, name) {

lib/coffee-script/sourcemap.js

@@ -8,9 +8,12 @@
       this.columns = [];
     }
 
-    add(column, arg, options = {}) {
+    add(column, arg, options) {
       var sourceColumn, sourceLine;
       sourceLine = arg[0], sourceColumn = arg[1];
+      if (options === void 0) {
+        options = {};
+      }
       if (this.columns[column] && options.noReplace) {
         return;
       }

src/nodes.coffee

@@ -107,8 +107,8 @@ exports.Base = class Base
   # If `level` is passed, then returns `[val, ref]`, where `val` is the compiled value, and `ref`
   # is the compiled reference. If `level` is not passed, this returns `[val, ref]` where
   # the two values are raw nodes which have not been compiled.
-  cache: (o, level, isComplex) ->
-    complex = if isComplex? then isComplex this else @isComplex()
+  cache: (o, level, shouldCache) ->
+    complex = if shouldCache? then shouldCache this else @shouldCache()
     if complex
       ref = new IdentifierLiteral o.scope.freeVariable 'ref'
       sub = new Assign ref, this
@@ -220,17 +220,43 @@ 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
 
-  unwrap     : THIS
-  unfoldSoak : 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.shouldCache() 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` need not be
+  # cached. `returnFive()`, however, could have side effects as a result of
+  # evaluating it more than once, and therefore we need to cache it. The
+  # parameter is named `shouldCache` rather than `mustCache` because there are
+  # also cases where we might not need to cache but where we want to, for
+  # example a long expression that may well be idempotent but we want to cache
+  # for brevity.
+  shouldCache: YES
+
+  isChainable: NO
+  isAssignable: NO
+  isNumber: NO
+
+  unwrap: THIS
+  unfoldSoak: NO
 
   # Is this node used to assign a certain variable?
   assigns: NO
@@ -481,7 +507,7 @@ exports.Literal = class Literal extends Base
   constructor: (@value) ->
     super()
 
-  isComplex: NO
+  shouldCache: NO
 
   assigns: (name) ->
     name is @value
@@ -625,7 +651,7 @@ exports.Value = class Value extends Base
   # Some boolean checks for the benefit of other nodes.
   isArray        : -> @bareLiteral(Arr)
   isRange        : -> @bareLiteral(Range)
-  isComplex      : -> @hasProperties() or @base.isComplex()
+  shouldCache    : -> @hasProperties() or @base.shouldCache()
   isAssignable   : -> @hasProperties() or @base.isAssignable()
   isNumber       : -> @bareLiteral(NumberLiteral)
   isString       : -> @bareLiteral(StringLiteral)
@@ -668,14 +694,14 @@ exports.Value = class Value extends Base
   # `a()[b()] ?= c` -> `(_base = a())[_name = b()] ? _base[_name] = c`
   cacheReference: (o) ->
     [..., name] = @properties
-    if @properties.length < 2 and not @base.isComplex() and not name?.isComplex()
+    if @properties.length < 2 and not @base.shouldCache() and not name?.shouldCache()
       return [this, this]  # `a` `a.b`
     base = new Value @base, @properties[...-1]
-    if base.isComplex()  # `a().b`
+    if base.shouldCache()  # `a().b`
       bref = new IdentifierLiteral o.scope.freeVariable 'base'
       base = new Value new Parens new Assign bref, base
     return [base, bref] unless name  # `a()`
-    if name.isComplex()  # `a[b()]`
+    if name.shouldCache()  # `a[b()]`
       nref = new IdentifierLiteral o.scope.freeVariable 'name'
       name = new Index new Assign nref, name.index
       nref = new Index nref
@@ -705,7 +731,7 @@ exports.Value = class Value extends Base
         prop.soak = off
         fst = new Value @base, @properties[...i]
         snd = new Value @base, @properties[i..]
-        if fst.isComplex()
+        if fst.shouldCache()
           ref = new IdentifierLiteral o.scope.freeVariable 'ref'
           fst = new Parens new Assign ref, fst
           snd.base = ref
@@ -848,12 +874,12 @@ exports.SuperCall = class SuperCall extends Call
       'super'
     else if method?.klass
       {klass, name, variable} = method
-      if klass.isComplex()
+      if klass.shouldCache()
         bref = new IdentifierLiteral o.scope.parent.freeVariable 'base'
         base = new Value new Parens new Assign bref, klass
         variable.base = base
         variable.properties.splice 0, klass.properties.length
-      if name.isComplex() or (name instanceof Index and name.index.isAssignable())
+      if name.shouldCache() or (name instanceof Index and name.index.isAssignable())
         nref = new IdentifierLiteral o.scope.parent.freeVariable 'name'
         name.index = new Assign nref, name.index
       accesses = [new Access new PropertyName '__super__']
@@ -923,7 +949,7 @@ exports.Access = class Access extends Base
     else
       [@makeCode('['), name..., @makeCode(']')]
 
-  isComplex: NO
+  shouldCache: NO
 
 #### Index
 
@@ -937,8 +963,8 @@ exports.Index = class Index extends Base
   compileToFragments: (o) ->
     [].concat @makeCode("["), @index.compileToFragments(o, LEVEL_PAREN), @makeCode("]")
 
-  isComplex: ->
-    @index.isComplex()
+  shouldCache: ->
+    @index.shouldCache()
 
 #### Range
 
@@ -955,16 +981,14 @@ exports.Range = class Range extends Base
     @exclusive = tag is 'exclusive'
     @equals = if @exclusive then '' else '='
 
-
-
   # Compiles the range's source variables -- where it starts and where it ends.
   # But only if they need to be cached to avoid double evaluation.
   compileVariables: (o) ->
     o = merge o, top: true
-    isComplex = del o, 'isComplex'
-    [@fromC, @fromVar]  =  @cacheToCodeFragments @from.cache o, LEVEL_LIST, isComplex
-    [@toC, @toVar]      =  @cacheToCodeFragments @to.cache o, LEVEL_LIST, isComplex
-    [@step, @stepVar]   =  @cacheToCodeFragments step.cache o, LEVEL_LIST, isComplex if step = del o, 'step'
+    shouldCache = del o, 'shouldCache'
+    [@fromC, @fromVar] = @cacheToCodeFragments @from.cache o, LEVEL_LIST, shouldCache
+    [@toC, @toVar]     = @cacheToCodeFragments @to.cache o, LEVEL_LIST, shouldCache
+    [@step, @stepVar]  = @cacheToCodeFragments step.cache o, LEVEL_LIST, shouldCache if step = del o, 'step'
     @fromNum = if @from.isNumber() then Number @fromVar else null
     @toNum   = if @to.isNumber()   then Number @toVar   else null
     @stepNum = if step?.isNumber() then Number @stepVar else null
@@ -1107,7 +1131,7 @@ exports.Obj = class Obj extends Base
       if prop instanceof Value and prop.this
         prop = new Assign prop.properties[0].name, prop, 'object'
       if prop not instanceof Comment and prop not instanceof Assign
-        if prop.isComplex()
+        if prop.shouldCache()
           [key, value] = prop.base.cache o
           key  = new PropertyName key.value if key instanceof IdentifierLiteral
           prop = new Assign key, value, 'object'
@@ -1315,7 +1339,7 @@ exports.Class = class Class extends Base
       method.name = variable.properties[0]
     else
       methodName  = variable.base
-      method.name = new (if methodName.isComplex() then Index else Access) methodName
+      method.name = new (if methodName.shouldCache() then Index else Access) methodName
       method.ctor = (if @parent then 'derived' else 'base') if methodName.value is 'constructor'
       method.error 'Cannot define a constructor as a bound function' if method.bound and method.ctor
 
@@ -1449,7 +1473,7 @@ exports.ExecutableClassBody = class ExecutableClassBody extends Base
         # The class scope is not available yet, so return the assignment to update later
         assign = @externalCtor = new Assign new Value, value
       else if not assign.variable.this
-        name      = new (if base.isComplex() then Index else Access) base
+        name      = new (if base.shouldCache() then Index else Access) base
         prototype = new Access new PropertyName 'prototype'
         variable  = new Value new ThisLiteral(), [ prototype, name ]
 
@@ -1675,7 +1699,7 @@ exports.Assign = class Assign extends Base
     compiledName = @variable.compileToFragments o, LEVEL_LIST
 
     if @context is 'object'
-      if @variable.isComplex()
+      if @variable.shouldCache()
         compiledName.unshift @makeCode '['
         compiledName.push @makeCode ']'
       else if fragmentsToText(compiledName) in JS_FORBIDDEN
@@ -1897,6 +1921,7 @@ exports.Code = class Code extends Base
     thisAssignments  = @thisAssignments?.slice() ? []
     paramsAfterSplat = []
     haveSplatParam   = no
+    haveBodyParam    = no
 
     # Check for duplicate parameters and separate `this` assignments
     paramNames = []
@@ -1915,6 +1940,10 @@ exports.Code = class Code extends Base
     # function definition; and dealing with splats or expansions, including
     # adding expressions to the function body to declare all parameter
     # variables that would have been after the splat/expansion parameter.
+    # If we encounter a parameter that needs to be declared in the function
+    # body for any reason, for example it’s destructured with `this`, also
+    # declare and assign all subsequent parameters in the function body so that
+    # any non-idempotent parameters are evaluated in the correct order.
     for param, i in @params
       # Was `...` used with this parameter? (Only one such parameter is allowed
       # per function.) Splat/expansion parameters cannot have default values,
@@ -1929,12 +1958,11 @@ exports.Code = class Code extends Base
         if param.splat
           params.push ref = param.asReference o
           splatParamName = fragmentsToText ref.compileNode o
-          if param.isComplex() # Parameter is destructured
+          if param.shouldCache()
             exprs.push new Assign new Value(param.name), ref, '=', param: yes
             # TODO: output destructured parameters as is, and fix destructuring
             # of objects with default values to work in this context (see
-            # Obj.compileNode `if prop.context isnt 'object'`)
-
+            # Obj.compileNode `if prop.context isnt 'object'`).
         else # `param` is an Expansion
           splatParamName = o.scope.freeVariable 'args'
           params.push new Value new IdentifierLiteral splatParamName
@@ -1945,23 +1973,34 @@ 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()
-          # 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.
-          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.shouldCache() or haveBodyParam
+          param.assignedInBody = yes
+          haveBodyParam = yes
+          # This parameter cannot be declared or assigned in the parameter
+          # list. So put a reference in the parameter list and add a statement
+          # to the function body assigning it, e.g.
+          # `(arg) => { var a = arg.a; }`, with a default value if it has one.
+          if param.value?
+            condition = new Op '==', param, new UndefinedLiteral
+            ifTrue = new Assign new Value(param.name), param.value, '=', param: yes
+            exprs.push new If condition, ifTrue
+          else
+            exprs.push new Assign new Value(param.name), param.asReference(o), '=', param: yes
 
         # If this parameter comes before the splat or expansion, it will go
         # in the function definition parameter list.
         unless haveSplatParam
           # If this parameter has a default value, and it hasn’t already been
-          # set by the `isComplex()` block above, define it as a statement in
+          # set by the `shouldCache()` 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()
-            ref = if param.value? then new Assign new Value(param.name), param.value, '=' else param
+          if param.shouldCache()
+            ref = param.asReference o
+          else
+            if param.value? and not param.assignedInBody
+              ref = new Assign new Value(param.name), param.value, '='
+            else
+              ref = param
           # Add this parameter’s reference to the function scope
           o.scope.parameter fragmentsToText (if param.value? then param else ref).compileToFragments o
           params.push ref
@@ -1970,8 +2009,8 @@ exports.Code = class Code extends Base
           # If this parameter had a default value, since it’s no longer in the
           # function parameter list we need to assign its default value
           # (if necessary) as an expression in the body.
-          if param.value? and not param.isComplex()
-            condition = new Literal param.name.value + ' === undefined'
+          if param.value? and not param.shouldCache()
+            condition = new Op '==', param, new UndefinedLiteral
             ifTrue = new Assign new Value(param.name), param.value, '='
             exprs.push new If condition, ifTrue
           # Add this parameter to the scope, since it wouldn’t have been added yet since it was skipped earlier.
@@ -2105,14 +2144,14 @@ exports.Param = class Param extends Base
       name = node.properties[0].name.value
       name = "_#{name}" if name in JS_FORBIDDEN
       node = new IdentifierLiteral o.scope.freeVariable name
-    else if node.isComplex()
+    else if node.shouldCache()
       node = new IdentifierLiteral o.scope.freeVariable 'arg'
     node = new Value node
     node.updateLocationDataIfMissing @locationData
     @reference = node
 
-  isComplex: ->
-    @name.isComplex() or @value instanceof Call
+  shouldCache: ->
+    @name.shouldCache()
 
   # Iterates the name or names of a `Param`.
   # In a sense, a destructured parameter represents multiple JS parameters. This
@@ -2196,7 +2235,7 @@ exports.Splat = class Splat extends Base
 # parameter list.
 exports.Expansion = class Expansion extends Base
 
-  isComplex: NO
+  shouldCache: NO
 
   compileNode: (o) ->
     @error 'Expansion must be used inside a destructuring assignment or parameter list'
@@ -2312,7 +2351,7 @@ exports.Op = class Op extends Base
   isUnary: ->
     not @second
 
-  isComplex: ->
+  shouldCache: ->
     not @isNumber()
 
   # Am I capable of
@@ -2404,7 +2443,7 @@ exports.Op = class Op extends Base
 
   # Keep reference to the left expression, unless this an existential assignment
   compileExistence: (o) ->
-    if @first.isComplex()
+    if @first.shouldCache()
       ref = new IdentifierLiteral o.scope.freeVariable 'ref'
       fst = new Parens new Assign ref, @first
     else
@@ -2455,7 +2494,7 @@ exports.Op = class Op extends Base
 
   compileFloorDivision: (o) ->
     floor = new Value new IdentifierLiteral('Math'), [new Access new PropertyName 'floor']
-    second = if @second.isComplex() then new Parens @second else @second
+    second = if @second.shouldCache() then new Parens @second else @second
     div = new Op '/', @first, second
     new Call(floor, [div]).compileToFragments o
 
@@ -2605,8 +2644,9 @@ exports.Parens = class Parens extends Base
 
   children: ['body']
 
-  unwrap    : -> @body
-  isComplex : -> @body.isComplex()
+  unwrap: -> @body
+
+  shouldCache: -> @body.shouldCache()
 
   compileNode: (o) ->
     expr = @body.unwrap()
@@ -2631,7 +2671,7 @@ exports.StringWithInterpolations = class StringWithInterpolations extends Base
   # _the_ custom logic to output interpolated strings as code.
   unwrap: -> this
 
-  isComplex : -> @body.isComplex()
+  shouldCache: -> @body.shouldCache()
 
   compileNode: (o) ->
     # Assumes that `expr` is `Value` » `StringLiteral` or `Op`
@@ -2719,7 +2759,7 @@ exports.For = class For extends While
     kvar        = ((@range or @from) and name) or index or ivar
     kvarAssign  = if kvar isnt ivar then "#{kvar} = " else ""
     if @step and not @range
-      [step, stepVar] = @cacheToCodeFragments @step.cache o, LEVEL_LIST, isComplexOrAssignable
+      [step, stepVar] = @cacheToCodeFragments @step.cache o, LEVEL_LIST, shouldCacheOrIsAssignable
       stepNum   = Number stepVar if @step.isNumber()
     name        = ivar if @pattern
     varPart     = ''
@@ -2728,7 +2768,7 @@ exports.For = class For extends While
     idt1        = @tab + TAB
     if @range
       forPartFragments = source.compileToFragments merge o,
-        {index: ivar, name, @step, isComplex: isComplexOrAssignable}
+        {index: ivar, name, @step, shouldCache: shouldCacheOrIsAssignable}
     else
       svar    = @source.compile o, LEVEL_LIST
       if (name or @own) and @source.unwrap() not instanceof IdentifierLiteral
@@ -3016,7 +3056,7 @@ isLiteralThis = (node) ->
     (node instanceof Code and node.bound) or
     node instanceof SuperCall
 
-isComplexOrAssignable = (node) -> node.isComplex() or node.isAssignable?()
+shouldCacheOrIsAssignable = (node) -> node.shouldCache() or node.isAssignable?()
 
 # Unfold a node's child if soak, then tuck the node under created `If`
 unfoldSoak = (o, parent, name) ->

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 ]