Whoa, we want by to convert strings to floats?

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.

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.

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).

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.

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
###

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?

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.

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
...
*/

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.

I removed "string" tests.