Fix #2047: Infinite loop possible when for loop with range uses variables

[zdenko]

Fixes #2047. (I hope this is not another "fake bug" 😄).
When variables are used in range and/or by, an infinite loop is possible.
This happens because the condition inside for doesn't check upper and lower range bounds.
This PR corrects this. I also implemented the same check when by is set as a literal number
(for x in [1..5] by -1), to prevent an accidental infinite loop.

Examples:

for x in [1..5] by -1
# for (x = i = 1; 1 <= i && i <= 5; x = i += -1)

for x in [5..1] by 1
# for (x = i = 5; 5 >= i && i >= 1; x = i += 1)

for x in [a..5] by 1
# for (x = i = ref = a; ref <= 5 ? ref <= i && i <= 5 : ref >= i && i >= 5; x = i += 1) 

for x in [1..b] by step
# for (x = i = 1, ref = b, ref1 = step; 1 <= ref ? 1 <= i && i <= ref : 1 >= i && i >= ref; x = i += ref1)

for x in [a..b] by step
# for (x = i = ref = a, ref1 = b, ref2 = step; ref <= ref1 ? ref <= i && i <= ref1 : ref >= i && i >= ref1; x = i += ref2)

[GeoffreyBooth]

This looks good to me, though damn this is dense. @vendethiel, do you mind taking a look?

@zdenko that massive test is very cool, but do you mind adding one additional separate test just for for x in [a..b] by step, i.e. the most complicated case? That way we have confidence that the worst case scenario pointed out by #2047 truly is fixed, and there isn’t just a bug in the big looping test.

[zdenko]

More tests for cases when from, to and step are variables.
Additional condition is added to the for which checks if step isn't zero.
Besides, step is always parsed into number: Number(step) || step|0).
The bitwise operation is used to convert string values into 0 which will be otherwise NaN.

step = "abc"
(Number(step) || step|0) = 0

Full example:

x for x in [a..b] by step

###
for (x = i = ref = a, ref1 = b, ref2 = step; (Number(ref2) || (ref2|0)) !== 0 && (ref <= ref1 ? ref <= i && i <= ref1 : ref >= i && i >= ref1); x = i += (Number(ref2) || (ref2|0))) 
###

[GeoffreyBooth]

Why Number(step) and not parseInt(step, 10)?

[zdenko]

I used Number for two reasons:

1. It can also convert float, e.g. step = "0.25"; for x in [a..b] by step.
2. It will convert the whole string, whereas parseInt parses up to the first non-digit and returns whatever it had parsed, e.g. "12p" => 12. So, in a case when the user makes a mistake it will fail completely instead of giving some result which will not be accurate.

[GeoffreyBooth]

Whoa, we want by to convert strings to floats?

[zdenko]

Actually no. While working on this PR I had a case with an infinite loop if step was a string.
The idea I had was to cast step into the number to avoid infinite loops by accidents, e.g. in case user makes a mistake.
But, now I see the error in my case was due to something else and this is not needed.
I'll correct this.

[vendethiel]

It seems like there are a few too many commits here.

[zdenko]

It seems that something went wrong when I tried to merge master in the branch.
I don't know how to correct this, so I'll appreciate if someone can advise how to fix this.

[GeoffreyBooth]

I merged origin/master into this, and it seems to have fixed the commits/diff @vendethiel

[GeoffreyBooth]

Should we really be supporting step values or variables that aren’t integers? The for loops in these cases becomes code like this:

for (x = i = ref = b, ref1 = a, ref2 = step; ref2 !== 0 && (ref <= ref1 ? ref <= i && i <= ref1 : ref >= i && i >= ref1); x = i += ref2) {

with the relevant part being the end, x = i += ref2. So basically our iterator is getting incremented by the value of step, as it should be, but I’m pretty sure in that i += part it was assumed that the incrementing value would be an integer.

Get rid of the r = in these tests and you’ll see why. Because of the r =, there’s an implicit results array that x is getting pushed to, so we never need to reference an index on the array we’re looping through. However, without that:

arr = [1..3]
step = '0.1'
for x in arr by step
  console.log x

var arr, i, len, ref, step, x;

arr = [1, 2, 3];

step = '0.1';

ref = step;
for ((ref > 0 ? (i = 0, len = arr.length) : i = arr.length - 1); ref > 0 ? i < len : i >= 0; i += ref) {
  x = arr[i];
  console.log(x);
}

That x = arr[i] line doesn’t work if i isn’t an integer.

[zdenko]

I see what you mean. But, the real difference is whether we use Identifier or Range in the for loop.

arr = [1..2]
step = 0.5
for x in arr by step
  console.log x

###
ref = step;
for ((ref > 0 ? (i = 0, len = arr.length) : i = arr.length - 1); ref > 0 ? i < len : i >= 0; i += ref) {
  x = arr[i];
  console.log(x);
}

Ouput:
1
undefined
2
undefined
###

a = 1
b = 2
step = 0.5
for x in [a..b] by step
  console.log x

###
for (x = i = ref = a, ref1 = b, ref2 = step; ref2 !== 0 && (ref <= ref1 ? ref <= i && i <= ref1 : ref >= i && i >= ref1); x = i += ref2) {
  console.log(x);
}

Ouput:
1
1.5
2
###

The former will check arr.length and the latter the upper bound of the range, e.g. b.
With Range I would expect this result. For example, Ruby and Python support float values for step in ranges.

I guess the question is if we should throw an error when the step isn't integer when not used with Range (e.g. for x in arr by 0.5).

[GeoffreyBooth]

I guess the question is if we should throw an error when the step isn’t integer when not used with Range

But how would we know that, if step is a variable?

What we could do, at least, is not cast it into a number. Then it’s on the user to supply a step variable that’s either an integer (if iterating over an array) or an integer or float (if iterating over a range) and the runtime will throw the error if step isn’t the right type.

[zdenko]

It looks that step is never cast into a number. In case step isn't a number, at least first item from the array or range will pass.

arr = [10..1]
step = "a"
for x in arr by step
  console.log x

### 
output:
1
###

for x in [5..1] by step
  console.log x

### 
output:
5
###

arr = [10..1]
step = "-0.5"
for x in arr by step
  console.log x

### 
output:
1
###

for x in [5..1] by step
  console.log x

### 
output:
5
###

arr = [10..1]
step = -0.5
for x in arr by step
  console.log x

### 
output:
1
undefined
2
undefined
3
undefined
4
undefined
5
undefined
6
undefined
7
undefined
8
undefined
9
undefined
10
###

for x in [5..1] by step
  console.log x

### 
output:
5
4.5
4
3.5
3
2.5
2
1.5
1
###

[GeoffreyBooth]

So in other words, the iterator that gets created is 0 by default, and after the first loop += step gets applied to it, which breaks it?

[GeoffreyBooth]

So I guess dealing with the case where the step variable isn’t numeric needn’t be part of the scope of this PR, since it no longer is casting the step variable to a number. The current compiler also has the issue where a non-numeric step variable will allow the first iteration of a loop over an array to be run, but none thereafter. I don’t know if that’s a bug, necessarily, but it surprised me.

I think we should rewrite these tests that use string-based steps, though, as they’re misleading; when I see step = "0.500000" I’m assuming you’re testing that that string value will be cast as a float. Looking at this test:

a = 2
b = 1
step = "0.5000000"
r = (x for x in [b..a] by step)
arrayEq r, [1]

Why would I expect r to equal [1]? What are you really testing here? A quick read of this test implies that r should be [ 1, 1.5, 2 ], assuming that by is supposed to cast the string or if I didn’t notice that the step was a string rather than a float. The same goes for the other “step is a string” tests. It might help if some of the more complicated ones have comments explaining what they’re intending to test, and/or why the output is as expected.

[zdenko]

In the previous commit, I cast step into a number, and the purpose of this test was checking the result ([ 1, 1.5, 2 ]). After your comment about the casting, to which I agree, I refactored the code. The test stayed, and I just corrected the expected result.
My idea was to cast step into a number, for cases like this, i.e. user made a mistake, or some library returned a string instead of the number, etc.
But, I think this isn't the scope of this PR, which only tries to prevent the cases where step or range boundaries can produce an infinite loop.

The issue of casting step should be considered from two angles: range and array and should be the part of another PR.

This JSexample will also output just the first item:

var arr = [3,2,1]
for (var i = 0; i < arr.length; i += "-1") {
  console.log(arr[i])
}
// 3

And, if you replace "-1" with the number -1 you'll get infinite loop

var arr = [3,2,1]
for (var i = 0; i < arr.length; i += "-1") {
  console.log(arr[i])
}
/*
undefined
undefined
undefined
undefined
undefined
undefined
undefined
...
*/

[GeoffreyBooth]

So for the purposes of wrapping up this PR, can we take out the “string step“ tests? And we can discuss that in a future issue/PR if you’d like.

[zdenko]

I removed "string" tests.