Opened on 03/11/2014 at 05:12:53 PM

Closed on 07/07/2017 at 04:26:09 PM

Last modified on 05/16/2018 at 12:06:03 PM

#54 closed defect (rejected)

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

Reported by: greiner Assignee:
Priority: P4 Milestone:
Module: Automation Keywords: jshydra
Cc: trev, sebastian Blocked By:
Blocking: Platform: Unknown
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description (last modified by sebastian)

How to reproduce

  1. Write the following code
    let a = {b: [1, 2, 3]};
    for (let c of a.b)
      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; ++)
  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; ++)
  delete a.b;

Mimicking the behavior on Firefox, producing following output:


Attachments (0)

Change History (11)

comment:1 Changed on 03/11/2014 at 08:53:50 PM by trev

  • Cc trev added

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?

comment:2 Changed on 03/12/2014 at 11:15:12 PM by fhd

This is probably what was fixed here:

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.

comment:3 Changed on 03/13/2014 at 07:14:27 AM by trev

  • Priority changed from Unknown to P4

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];
  delete a.b;

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

comment:4 Changed on 03/14/2014 at 05:07:29 PM by 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."

comment:5 Changed on 03/14/2014 at 06:15:13 PM by 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.

comment:6 Changed on 04/12/2014 at 08:28:22 AM by trev

  • Ready unset

Note the #301 will replace for 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.

comment:7 Changed on 07/09/2014 at 12:38:11 PM by philll

  • Platform set to Firefox

comment:8 Changed on 02/20/2015 at 08:56:23 AM by sebastian

  • Cc sebastian added
  • Description modified (diff)
  • Platform changed from Firefox to Unknown
  • Ready set
  • Summary changed from Transpiling of for-each leads to problems if array is modified within the loop to Transpiling of for-of leads to problems if array is modified within the loop

comment:9 Changed on 03/30/2015 at 09:43:33 AM by greiner

  • Owner greiner deleted

comment:10 Changed on 07/07/2017 at 04:26:09 PM by sebastian

  • Resolution set to rejected
  • Status changed from new to closed
  • Tester set to Unknown

Closing since we no longer use/maintain jsHydra.

comment:11 Changed on 05/16/2018 at 12:03:57 PM by abpbot

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

This commit relates to GitLab issue 54.

Last edited on 05/16/2018 at 12:06:03 PM by kzar

Add Comment

Modify Ticket

Change Properties
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from (none).
Note: See TracTickets for help on using tickets.