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

* fix #4889

* test

* move test from 'control_flow' to 'ranges'

* More range tests

Author: zdenko

lib/coffeescript/nodes.js

@@ -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}"
 

src/nodes.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]