Fix #4889: for...range loop condition (#4891)

* fix #4889

* test

* move test from 'control_flow' to 'ranges'

* More range tests

lib/coffeescript/nodes.js

@@ -2031,7 +2031,7 @@
       // When compiled normally, the range returns the contents of the *for loop*
       // needed to iterate over the values in the range. Used by comprehensions.
       compileNode(o) {
-        var cond, condPart, from, gt, idx, idxName, known, lowerBound, lt, namedIndex, stepCond, stepPart, to, upperBound, varPart;
+        var cond, condPart, from, gt, idx, idxName, known, lowerBound, lt, namedIndex, ref1, ref2, stepCond, stepNotZero, stepPart, to, upperBound, varPart;
         if (!this.fromVar) {
           this.compileVariables(o);
         }
@@ -2054,12 +2054,11 @@
         // Generate the condition.
         [from, to] = [this.fromNum, this.toNum];
         // Always check if the `step` isn't zero to avoid the infinite loop.
-        stepCond = this.stepNum ? `${this.stepNum} !== 0` : `${this.stepVar} !== 0`;
-        condPart = known ? this.step == null ? from <= to ? `${lt} ${to}` : `${gt} ${        // from < to
-to}` : (lowerBound = `${from} <= ${idx} && ${lt} ${// from > to
-to}`, upperBound = `${from} >= ${idx} && ${gt} ${to}`, from <= to ? `${stepCond} && ${lowerBound}` : `${stepCond} && ${// from < to
-upperBound}`) : (lowerBound = `${this.fromVar} <= ${idx} && ${lt} ${// from > to
-this.toVar}`, upperBound = `${this.fromVar} >= ${idx} && ${gt} ${this.toVar}`, `${stepCond} && (${this.fromVar} <= ${this.toVar} ? ${lowerBound} : ${upperBound})`);
+        stepNotZero = `${(ref1 = this.stepNum) != null ? ref1 : this.stepVar} !== 0`;
+        stepCond = `${(ref2 = this.stepNum) != null ? ref2 : this.stepVar} > 0`;
+        lowerBound = `${lt} ${(known ? to : this.toVar)}`;
+        upperBound = `${gt} ${(known ? to : this.toVar)}`;
+        condPart = this.step != null ? `${stepNotZero} && (${stepCond} ? ${lowerBound} : ${upperBound})` : known ? `${(from <= to ? lt : gt)} ${to}` : `(${this.fromVar} <= ${this.toVar} ? ${lowerBound} : ${upperBound})`;
         cond = this.stepVar ? `${this.stepVar} > 0` : `${this.fromVar} <= ${this.toVar}`;
         // Generate the step.
         stepPart = this.stepVar ? `${idx} += ${this.stepVar}` : known ? namedIndex ? from <= to ? `++${idx}` : `--${idx}` : from <= to ? `${idx}++` : `${idx}--` : namedIndex ? `${cond} ? ++${idx} : --${idx}` : `${cond} ? ${idx}++ : ${idx}--`;

src/nodes.coffee

@@ -1365,23 +1365,18 @@ exports.Range = class Range extends Base
     # Generate the condition.
     [from, to] = [@fromNum, @toNum]
     # Always check if the `step` isn't zero to avoid the infinite loop.
-    stepCond = if @stepNum then "#{@stepNum} !== 0" else "#{@stepVar} !== 0"
+    stepNotZero = "#{ @stepNum ? @stepVar } !== 0"
+    stepCond = "#{ @stepNum ? @stepVar } > 0"
+    lowerBound = "#{lt} #{ if known then to else @toVar }"
+    upperBound = "#{gt} #{ if known then to else @toVar }"
     condPart =
-      if known
-        unless @step?
-          if from <= to then "#{lt} #{to}" else "#{gt} #{to}"
-        else
-          # from < to
-          lowerBound = "#{from} <= #{idx} && #{lt} #{to}"
-          # from > to
-          upperBound = "#{from} >= #{idx} && #{gt} #{to}"
-          if from <= to then "#{stepCond} && #{lowerBound}" else "#{stepCond} && #{upperBound}"
-      else
-        # from < to
-        lowerBound = "#{@fromVar} <= #{idx} && #{lt} #{@toVar}"
-        # from > to
-        upperBound = "#{@fromVar} >= #{idx} && #{gt} #{@toVar}"
-        "#{stepCond} && (#{@fromVar} <= #{@toVar} ? #{lowerBound} : #{upperBound})"
+       if @step?
+         "#{stepNotZero} && (#{stepCond} ? #{lowerBound} : #{upperBound})"
+       else
+         if known
+           "#{ if from <= to then lt else gt } #{to}"
+         else
+          "(#{@fromVar} <= #{@toVar} ? #{lowerBound} : #{upperBound})"
 
     cond = if @stepVar then "#{@stepVar} > 0" else "#{@fromVar} <= #{@toVar}"
 

test/ranges.coffee

@@ -130,43 +130,43 @@ test "#2047: Infinite loop possible when `for` loop with `range` uses variables"
 
   testData = [
     [1, 5, 1, [1..5]]
-    [1, 5, -1, [1]]
+    [1, 5, -1, []]
     [1, 5, up, [1..5]]
-    [1, 5, down, [1]]
+    [1, 5, down, []]
 
     [a, 5, 1, [1..5]]
-    [a, 5, -1, [1]]
+    [a, 5, -1, []]
     [a, 5, up, [1..5]]
-    [a, 5, down, [1]]
+    [a, 5, down, []]
 
     [1, b, 1, [1..5]]
-    [1, b, -1, [1]]
+    [1, b, -1, []]
     [1, b, up, [1..5]]
-    [1, b, down, [1]]
+    [1, b, down, []]
 
     [a, b, 1, [1..5]]
-    [a, b, -1, [1]]
+    [a, b, -1, []]
     [a, b, up, [1..5]]
-    [a, b, down, [1]]
+    [a, b, down, []]
 
-    [5, 1, 1, [5]]
+    [5, 1, 1, []]
     [5, 1, -1, [5..1]]
-    [5, 1, up, [5]]
+    [5, 1, up, []]
     [5, 1, down,  [5..1]]
 
-    [5, a, 1, [5]]
+    [5, a, 1, []]
     [5, a, -1, [5..1]]
-    [5, a, up, [5]]
+    [5, a, up, []]
     [5, a, down, [5..1]]
 
-    [b, 1, 1, [5]]
+    [b, 1, 1, []]
     [b, 1, -1, [5..1]]
-    [b, 1, up, [5]]
+    [b, 1, up, []]
     [b, 1, down, [5..1]]
 
-    [b, a, 1, [5]]
+    [b, a, 1, []]
     [b, a, -1, [5..1]]
-    [b, a, up, [5]]
+    [b, a, up, []]
     [b, a, down, [5..1]]
   ]
 
@@ -182,10 +182,10 @@ test "#2047: from, to and step as variables", ->
   arrayEq r, [1..5]
 
   r = (x for x in [a..b] by down)
-  arrayEq r, [1]
+  arrayEq r, []
 
   r = (x for x in [b..a] by up)
-  arrayEq r, [5]
+  arrayEq r, []
 
   r = (x for x in [b..a] by down)
   arrayEq r, [5..1]
@@ -202,3 +202,43 @@ test "#4884: Range not declaring var for the 'i'", ->
     idx + 1
 
   eq global.i, undefined
+
+test "#4889: `for` loop unexpected behavior", ->
+  n = 1
+  result = []
+  for i in [0..n]
+    result.push i
+    for j in [(i+1)..n]
+      result.push j
+
+  arrayEq result, [0,1,1,2,1]
+
+test "#4889: `for` loop unexpected behavior with `by 1` on second loop", ->
+  n = 1
+  result = []
+  for i in [0..n]
+    result.push i
+    for j in [(i+1)..n] by 1
+      result.push j
+
+  arrayEq result, [0,1,1]
+
+test "countdown example from docs", ->
+  countdown = (num for num in [10..1])
+  arrayEq countdown, [10,9,8,7,6,5,4,3,2,1]
+
+test "counting up when the range goes down returns an empty array", ->
+  countdown = (num for num in [10..1] by 1)
+  arrayEq countdown, []
+
+test "counting down when the range goes up returns an empty array", ->
+  countup = (num for num in [1..10] by -1)
+  arrayEq countup, []
+
+test "counting down by too much returns just the first value", ->
+  countdown = (num for num in [10..1] by -100)
+  arrayEq countdown, [10]
+
+test "counting up by too much returns just the first value", ->
+  countup = (num for num in [1..10] by 100)
+  arrayEq countup, [1]