From 996a171a4ecfb30a0fae88733b02bec3591f308e Mon Sep 17 00:00:00 2001 From: Simon Lydell Date: Sun, 11 Jan 2015 21:19:24 +0100 Subject: [PATCH] Fix #3778: Make for loops more consistent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The following two lines might seem equivalent: for n in [1, 2, 3] by a then a = 4; n for n in [1, 2, 3] by +a then a = 4; n But they used not to be, because `+a` was cached into a `ref`, while the plain `a` wasn’t. Now even simple identifiers are cached, making the two lines equivalent as expected. --- lib/coffee-script/nodes.js | 35 +++++++++++++++++++++-------------- src/nodes.coffee | 27 ++++++++++++++++----------- test/comprehensions.coffee | 11 +++++++++++ 3 files changed, 48 insertions(+), 25 deletions(-) diff --git a/lib/coffee-script/nodes.js b/lib/coffee-script/nodes.js index 8285724d..2b6be415 100644 --- a/lib/coffee-script/nodes.js +++ b/lib/coffee-script/nodes.js @@ -1,6 +1,6 @@ // Generated by CoffeeScript 1.9.0 (function() { - var Access, Arr, Assign, Base, Block, Call, Class, Code, CodeFragment, Comment, Existence, Expansion, Extends, For, HEXNUM, IDENTIFIER, IDENTIFIER_STR, IS_REGEX, IS_STRING, If, In, Index, LEVEL_ACCESS, LEVEL_COND, LEVEL_LIST, LEVEL_OP, LEVEL_PAREN, LEVEL_TOP, Literal, METHOD_DEF, NEGATE, NO, NUMBER, Obj, Op, Param, Parens, RESERVED, Range, Return, SIMPLENUM, STRICT_PROSCRIBED, Scope, Slice, Splat, Switch, TAB, THIS, Throw, Try, UTILITIES, Value, While, YES, addLocationDataFn, compact, del, ends, extend, flatten, fragmentsToText, isLiteralArguments, isLiteralThis, last, locationDataToString, merge, multident, parseNum, ref1, ref2, some, starts, throwSyntaxError, unfoldSoak, utility, + var Access, Arr, Assign, Base, Block, Call, Class, Code, CodeFragment, Comment, Existence, Expansion, Extends, For, HEXNUM, IDENTIFIER, IDENTIFIER_STR, IS_REGEX, IS_STRING, If, In, Index, LEVEL_ACCESS, LEVEL_COND, LEVEL_LIST, LEVEL_OP, LEVEL_PAREN, LEVEL_TOP, Literal, METHOD_DEF, NEGATE, NO, NUMBER, Obj, Op, Param, Parens, RESERVED, Range, Return, SIMPLENUM, STRICT_PROSCRIBED, Scope, Slice, Splat, Switch, TAB, THIS, Throw, Try, UTILITIES, Value, While, YES, addLocationDataFn, compact, del, ends, extend, flatten, fragmentsToText, isComplexOrAssignable, isLiteralArguments, isLiteralThis, last, locationDataToString, merge, multident, parseNum, ref1, ref2, some, starts, throwSyntaxError, unfoldSoak, utility, extend1 = function(child, parent) { for (var key in parent) { if (hasProp.call(parent, key)) child[key] = parent[key]; } function ctor() { this.constructor = child; } ctor.prototype = parent.prototype; child.prototype = new ctor(); child.__super__ = parent.prototype; return child; }, hasProp = {}.hasOwnProperty, 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; }, @@ -112,19 +112,20 @@ return parts; }; - Base.prototype.cache = function(o, level, reused) { - var ref, sub; - if (!this.isComplex()) { - ref = level ? this.compileToFragments(o, level) : this; - return [ref, ref]; - } else { - ref = new Literal(reused || o.scope.freeVariable('ref')); + Base.prototype.cache = function(o, level, isComplex) { + var complex, ref, sub; + complex = isComplex != null ? isComplex(this) : this.isComplex(); + if (complex) { + ref = new Literal(o.scope.freeVariable('ref')); sub = new Assign(ref, this); if (level) { return [sub.compileToFragments(o, level), [this.makeCode(ref.value)]]; } else { return [sub, ref]; } + } else { + ref = level ? this.compileToFragments(o, level) : this; + return [ref, ref]; } }; @@ -1124,14 +1125,15 @@ } Range.prototype.compileVariables = function(o) { - var ref3, ref4, ref5, ref6, step; + var isComplex, ref3, ref4, ref5, ref6, step; o = merge(o, { top: true }); - ref3 = this.cacheToCodeFragments(this.from.cache(o, LEVEL_LIST)), this.fromC = ref3[0], this.fromVar = ref3[1]; - ref4 = this.cacheToCodeFragments(this.to.cache(o, LEVEL_LIST)), this.toC = ref4[0], this.toVar = ref4[1]; + 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]; if (step = del(o, 'step')) { - ref5 = this.cacheToCodeFragments(step.cache(o, LEVEL_LIST)), this.step = ref5[0], this.stepVar = ref5[1]; + ref5 = this.cacheToCodeFragments(step.cache(o, LEVEL_LIST, isComplex)), this.step = ref5[0], this.stepVar = ref5[1]; } ref6 = [this.fromVar.match(NUMBER), this.toVar.match(NUMBER)], this.fromNum = ref6[0], this.toNum = ref6[1]; if (this.stepVar) { @@ -2773,7 +2775,7 @@ kvar = (this.range && name) || index || ivar; kvarAssign = kvar !== ivar ? kvar + " = " : ""; if (this.step && !this.range) { - ref4 = this.cacheToCodeFragments(this.step.cache(o, LEVEL_LIST)), step = ref4[0], stepVar = ref4[1]; + ref4 = this.cacheToCodeFragments(this.step.cache(o, LEVEL_LIST, isComplexOrAssignable)), step = ref4[0], stepVar = ref4[1]; stepNum = stepVar.match(NUMBER); } if (this.pattern) { @@ -2787,7 +2789,8 @@ forPartFragments = source.compileToFragments(merge(o, { index: ivar, name: name, - step: this.step + step: this.step, + isComplex: isComplexOrAssignable })); } else { svar = this.source.compile(o, LEVEL_LIST); @@ -3189,6 +3192,10 @@ return (node instanceof Literal && node.value === 'this' && !node.asKey) || (node instanceof Code && node.bound) || (node instanceof Call && node.isSuper); }; + isComplexOrAssignable = function(node) { + return node.isComplex() || (typeof node.isAssignable === "function" ? node.isAssignable() : void 0); + }; + unfoldSoak = function(o, parent, name) { var ifn; if (!(ifn = parent[name].unfoldSoak(o))) { diff --git a/src/nodes.coffee b/src/nodes.coffee index 8b02d688..569c4bec 100644 --- a/src/nodes.coffee +++ b/src/nodes.coffee @@ -102,14 +102,15 @@ 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, reused) -> - unless @isComplex() - ref = if level then @compileToFragments o, level else this - [ref, ref] - else - ref = new Literal reused or o.scope.freeVariable 'ref' + cache: (o, level, isComplex) -> + complex = if isComplex? then isComplex this else @isComplex() + if complex + ref = new Literal o.scope.freeVariable 'ref' sub = new Assign ref, this if level then [sub.compileToFragments(o, level), [@makeCode(ref.value)]] else [sub, ref] + else + ref = if level then @compileToFragments o, level else this + [ref, ref] cacheToCodeFragments: (cacheValues) -> [fragmentsToText(cacheValues[0]), fragmentsToText(cacheValues[1])] @@ -793,9 +794,10 @@ exports.Range = class Range extends Base # But only if they need to be cached to avoid double evaluation. compileVariables: (o) -> o = merge o, top: true - [@fromC, @fromVar] = @cacheToCodeFragments @from.cache o, LEVEL_LIST - [@toC, @toVar] = @cacheToCodeFragments @to.cache o, LEVEL_LIST - [@step, @stepVar] = @cacheToCodeFragments step.cache o, LEVEL_LIST if step = del o, 'step' + 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' [@fromNum, @toNum] = [@fromVar.match(NUMBER), @toVar.match(NUMBER)] @stepNum = @stepVar.match(NUMBER) if @stepVar @@ -1971,7 +1973,7 @@ exports.For = class For extends While kvar = (@range 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 + [step, stepVar] = @cacheToCodeFragments @step.cache o, LEVEL_LIST, isComplexOrAssignable stepNum = stepVar.match NUMBER name = ivar if @pattern varPart = '' @@ -1979,7 +1981,8 @@ exports.For = class For extends While defPart = '' idt1 = @tab + TAB if @range - forPartFragments = source.compileToFragments merge(o, {index: ivar, name, @step}) + forPartFragments = source.compileToFragments merge o, + {index: ivar, name, @step, isComplex: isComplexOrAssignable} else svar = @source.compile o, LEVEL_LIST if (name or @own) and not IDENTIFIER.test svar @@ -2291,6 +2294,8 @@ isLiteralThis = (node) -> (node instanceof Code and node.bound) or (node instanceof Call and node.isSuper) +isComplexOrAssignable = (node) -> node.isComplex() or node.isAssignable?() + # Unfold a node's child if soak, then tuck the node under created `If` unfoldSoak = (o, parent, name) -> return unless ifn = parent[name].unfoldSoak o diff --git a/test/comprehensions.coffee b/test/comprehensions.coffee index cda9b12e..6bbbda70 100644 --- a/test/comprehensions.coffee +++ b/test/comprehensions.coffee @@ -555,3 +555,14 @@ test "splats in destructuring in comprehensions", -> test "#156: expansion in destructuring in comprehensions", -> list = [[0, 1, 2], [2, 3, 4], [4, 5, 6]] arrayEq (last for [..., last] in list), [2, 4, 6] + +test "#3778: Consistently always cache for loop range boundaries and steps, even + if they are simple identifiers", -> + a = 1; arrayEq [1, 2, 3], (for n in [1, 2, 3] by a then a = 4; n) + a = 1; arrayEq [1, 2, 3], (for n in [1, 2, 3] by +a then a = 4; n) + a = 1; arrayEq [1, 2, 3], (for n in [a..3] then a = 4; n) + a = 1; arrayEq [1, 2, 3], (for n in [+a..3] then a = 4; n) + a = 3; arrayEq [1, 2, 3], (for n in [1..a] then a = 4; n) + a = 3; arrayEq [1, 2, 3], (for n in [1..+a] then a = 4; n) + a = 1; arrayEq [1, 2, 3], (for n in [1..3] by a then a = 4; n) + a = 1; arrayEq [1, 2, 3], (for n in [1..3] by +a then a = 4; n)