Fixing Issue #509. Double-evaluation for function calls within IndexNodes as the left-hand-assignment part of a compound-assignment operation.

lib/nodes.js

@@ -48,21 +48,24 @@
     };
     BaseNode.prototype.compileReference = function(o, options) {
       var compiled, pair, reference;
+      options = options || {};
       pair = (function() {
         if (!((this instanceof CallNode || this.contains(function(n) {
           return n instanceof CallNode;
         })) || (this instanceof ValueNode && (!(this.base instanceof LiteralNode) || this.hasProperties())))) {
           return [this, this];
+        } else if (this instanceof ValueNode && options.assignment) {
+          return this.cacheIndexes(o);
         } else {
           reference = literal(o.scope.freeVariable());
           compiled = new AssignNode(reference, this);
           return [compiled, reference];
         }
       }).call(this);
-      if (!(options && options.precompile)) {
-        return pair;
+      if (options.precompile) {
+        return [pair[0].compile(o), pair[1].compile(o)];
       }
-      return [pair[0].compile(o), pair[1].compile(o)];
+      return pair;
     };
     BaseNode.prototype.idt = function(tabs) {
       var idt, num;
@@ -362,6 +365,28 @@
       }
       return node === this;
     };
+    ValueNode.prototype.cacheIndexes = function(o) {
+      var _b, _c, _d, copy, i;
+      copy = new ValueNode(this.base, this.properties.slice(0));
+      _c = copy.properties;
+      for (_b = 0, _d = _c.length; _b < _d; _b++) {
+        (function() {
+          var _e, index, indexVar;
+          var i = _b;
+          var prop = _c[_b];
+          if (prop instanceof IndexNode && prop.contains(function(n) {
+            return n instanceof CallNode;
+          })) {
+            _e = prop.index.compileReference(o);
+            index = _e[0];
+            indexVar = _e[1];
+            this.properties[i] = new IndexNode(index);
+            return (copy.properties[i] = new IndexNode(indexVar));
+          }
+        }).call(this);
+      }
+      return [this, copy];
+    };
     ValueNode.prototype.compile = function(o) {
       return !o.top || this.properties.length ? ValueNode.__superClass__.compile.call(this, o) : this.base.compile(o);
     };
@@ -1256,17 +1281,21 @@
       return "(" + first + ") && (" + shared + " " + this.operator + " " + second + ")";
     };
     OpNode.prototype.compileAssignment = function(o) {
-      var _b, first, second;
-      _b = [this.first.compile(o), this.second.compile(o)];
+      var _b, first, firstVar, second;
+      _b = this.first.compileReference(o, {
+        precompile: true,
+        assignment: true
+      });
       first = _b[0];
-      second = _b[1];
+      firstVar = _b[1];
+      second = this.second.compile(o);
       if (first.match(IDENTIFIER)) {
         o.scope.find(first);
       }
       if (this.operator === '?=') {
-        return ("" + first + " = " + (ExistenceNode.compileTest(o, this.first)) + " ? " + first + " : " + second);
+        return ("" + first + " = " + (ExistenceNode.compileTest(o, this.first)) + " ? " + firstVar + " : " + second);
       }
-      return "" + first + " = " + first + " " + (this.operator.substr(0, 2)) + " " + second;
+      return "" + first + " = " + firstVar + " " + (this.operator.substr(0, 2)) + " " + second;
     };
     OpNode.prototype.compileExistence = function(o) {
       var _b, first, second, test;

src/nodes.coffee

@@ -62,15 +62,18 @@ exports.BaseNode = class BaseNode
   # in multiple places, ensure that the expression is only ever evaluated once,
   # by assigning it to a temporary variable.
   compileReference: (o, options) ->
+    options or= {}
     pair = if not ((this instanceof CallNode or @contains((n) -> n instanceof CallNode)) or
                   (this instanceof ValueNode and (not (@base instanceof LiteralNode) or @hasProperties())))
       [this, this]
+    else if this instanceof ValueNode and options.assignment
+      this.cacheIndexes(o)
     else
       reference = literal o.scope.freeVariable()
       compiled  = new AssignNode reference, this
       [compiled, reference]
-    return pair unless options and options.precompile
-    [pair[0].compile(o), pair[1].compile(o)]
+    return [pair[0].compile(o), pair[1].compile(o)] if options.precompile
+    pair
 
   # Convenience method to grab the current indentation level, plus tabbing in.
   idt: (tabs) ->
@@ -336,6 +339,18 @@ exports.ValueNode = class ValueNode extends BaseNode
     while node instanceof CallNode then node = node.variable
     node is this
 
+  # If the value node has indexes containing function calls, and the value node
+  # needs to be used twice, in compound assignment ... then we need to cache
+  # the value of the indexes.
+  cacheIndexes: (o) ->
+    copy = new ValueNode @base, @properties.slice 0
+    for prop, i in copy.properties
+      if prop instanceof IndexNode and prop.contains((n) -> n instanceof CallNode)
+        [index, indexVar] = prop.index.compileReference o
+        this.properties[i] = new IndexNode index
+        copy.properties[i] = new IndexNode indexVar
+    [this, copy]
+
   # Override compile to unwrap the value when possible.
   compile: (o) ->
     if not o.top or @properties.length then super(o) else @base.compile(o)
@@ -1094,10 +1109,11 @@ exports.OpNode = class OpNode extends BaseNode
   # operands are only evaluated once, even though we have to reference them
   # more than once.
   compileAssignment: (o) ->
-    [first, second] = [@first.compile(o), @second.compile(o)]
+    [first, firstVar] = @first.compileReference o, precompile: yes, assignment: yes
+    second = @second.compile o
     o.scope.find(first) if first.match(IDENTIFIER)
-    return "#first = #{ ExistenceNode.compileTest(o, @first) } ? #first : #second" if @operator is '?='
-    "#first = #first #{ @operator.substr(0, 2) } #second"
+    return "#first = #{ ExistenceNode.compileTest(o, @first) } ? #firstVar : #second" if @operator is '?='
+    "#first = #firstVar #{ @operator.substr(0, 2) } #second"
 
   # If this is an existence operator, we delegate to `ExistenceNode.compileTest`
   # to give us the safe references for the variables.

test/test_operations.coffee

@@ -66,7 +66,7 @@ ok x*-y is 50
 ok x*+y is -50
 
 
-# Half-operators.
+# Compound operators.
 one = two = null
 one or= 1
 two or=  2
@@ -79,3 +79,18 @@ two and=  'two'
 
 ok one is 'one'
 ok two is 'two'
+
+
+# Compound assignment should be careful about caching variables.
+list = [0, 0, 5, 10]
+count = 1
+key = ->
+  count += 1
+
+list[key()] or= 100
+ok list.join(' ') is '0 0 5 10'
+
+count = 0
+
+list[key()] or= 100
+ok list.join(' ') is '0 100 5 10'