Transpiling of for-of leads to problems if array is modified within the loop

[greiner]

How to reproduce

1. Write the following code

   let a = {b: [1, 2, 3]};
   for (let c of a.b)
   {
     console.log(c);
     delete a.b;
   }
   

2. Transpile it using JSHydra

Observed behaviour

The generated code looks like:

var a = {b: [1, 2, 3]};
for (var i = 0; i < a.b.length; ++)
{
  console.log(a.b[i]);
  delete a.b;
}

Hence, running the code leads to:

TypeError: Cannot read property 'length' of undefined

Expected behaviour

The generated code should rather look like:

var a = {b: [1, 2, 3]};
for (var x = a.b, i = 0; i = 0; i < x.length; ++)
{
  console.log(x[i]);
  delete a.b;
}

Mimicking the behavior on Firefox, producing following output:

1
2
3

[trev]

Our JSHydra translator was never meant to ensure 100% correct translation, merely covering our usage. Quite frankly, deleting things while iterating on them is not exactly good style, even if the browser takes care of that scenario. Do you really think this is worth fixing? Is that a real issue you encountered?

[fhd]

This is probably what was fixed here:
​http://codereview.adblockplus.org/6322148272504832/

I'm obviously leaning towards fixing it in JSHydra (and Thomas didn't). I'm slightly worried that this could cause pretty subtle bugs, and it doesn't really seem hard to fix.

[trev]

Then one would also have to store the length of the array in a temp variable, to match the behavior of the for each loop in case the array itself is modified. In other words, the code in the description needs to be converted into:

var a = {b: [1, 2, 3]};
for (var array = a.b, i = 0, l = array.length; i < l; i++)
{
  var c = array[i];
  console.log(c);
  delete a.b;
}

Not sure whether this is worth introducing two additional variables but the behavior should really be the same then.

[greiner]

Actually, for-each-in iterates over enumerable properties so shouldnt' it rather translate to a.b.forEach(...) than to for (var i = 0; i < a.b.length; i++). This would also take care of this issue.

MDN even says "Never use a loop like this on arrays. Only use it on objects."
​https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for_each...in

[trev]

Performance of Array.forEach is bad.

We never use for each loops on anything other than arrays. We don't modify Array.prototype either. That's the only reason why we use for each loops at all. Switching to ​for..of loops might be desirable but it won't solve the necessity to translate loops - no browser other than Firefox currently supports those.

[trev]

Note the #301 will replace for each..in loops by for..of loops. As a side-effect this will change the behavior for the scenario that elements are being removed while looping - the code in comment 3 does not replicate the behavior of a for..of loop in Firefox correctly. The l variable is no longer necessary, the loop should check against array.length each time.

[sebastian]

Closing since we no longer use/maintain jsHydra.

[abpbot]

A commit referencing this issue has landed:
​Issue 54 - Add documentation for the $csp filter option

This commit relates to ​GitLab issue 54.