From cbea7b5d1cfc34ebe2f77ec2c953a3aeca0a6541 Mon Sep 17 00:00:00 2001 From: Geoffrey Booth Date: Wed, 1 Feb 2017 06:54:42 -0800 Subject: [PATCH] [CS2] Fix handling of parameters that are complex (#4430) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 | 119 ++++++++++++++----------- lib/coffee-script/sourcemap.js | 5 +- src/nodes.coffee | 158 +++++++++++++++++++++------------ test/functions.coffee | 7 +- 4 files changed, 175 insertions(+), 114 deletions(-) diff --git a/lib/coffee-script/nodes.js b/lib/coffee-script/nodes.js index e477d1bcd0..ff20ca0395 100644 --- a/lib/coffee-script/nodes.js +++ b/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) { diff --git a/lib/coffee-script/sourcemap.js b/lib/coffee-script/sourcemap.js index 9bce3aae36..76ec7047cc 100644 --- a/lib/coffee-script/sourcemap.js +++ b/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; } diff --git a/src/nodes.coffee b/src/nodes.coffee index af9bf11755..0f0e0c7395 100644 --- a/src/nodes.coffee +++ b/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 + + # `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 - 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 @@ -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) -> diff --git a/test/functions.coffee b/test/functions.coffee index cb03bc0a02..89cf3e3abd 100644 --- a/test/functions.coffee +++ b/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 ]