Support removing items while iterating for ForInStatement

[KamilaBorowska]

This is probably not really worth it, but JavaScript has a for in statement to iterate through object keys, and it is a part of a language.

I think that to support this statement, you first need to obtain all keys in an object (or use ES6 generators to do it, I guess), all at once. During iteration of those objects, you need to check if a key is still in object as ECMAScript requires deleted keys to not be iterated. Adding keys during iteration is allowed in ES6, however whether it's iterated or not is implementation defined, and it's faster to ignore new keys.

[marten-de-vries]

@xfix Can you provide an example of code that isn't converted? For in should be supported. See e.g. the following test files:

• https://github.com/marten-de-vries/kneden/blob/master/test/fixtures/for-in/actual.js
• https://github.com/marten-de-vries/kneden/blob/master/test/fixtures/for-in/expected.js

As for deleting keys while iterating, I know the standard requires it, but regenerator doesn't support that either as far as I'm aware. I would be open to a PR that remedied that, but it would make the code a lot worse + slower, so at that point it might be good to start looking into making it optional...

[KamilaBorowska]

Oh, I was using for (const item in obj), not for (var item in obj). My fault. I essentially forgot about existence of var.

Also, pretty sure Regenerator does support deleting keys while iterating. The following code works in Regenerator:

var obj = {a: 1, b: 2, c: 3}
for (var i in obj) {
  delete obj.b
  alert(i)
}

[marten-de-vries]

• Hmm, then I did miss that while checking Babel's implementation. Also, just checking, you mean that code works if you add an 'await' inside the body? Because otherwise it doesn't need any conversion IIRC.
• Yes, if you want const, add the es2015 babel preset

[marten-de-vries]

Just tried this with the Babel repl on the babel site. It converts:

async function test() {
  var obj = {a: 1, b: 2, c: 3}
  for (var i in obj) {
    await 1;
    delete obj.b
    alert(i)
  }
}

to

"use strict";

function test() {
  var obj, i;
  return regeneratorRuntime.async(function test$(context$1$0) {
    while (1) switch (context$1$0.prev = context$1$0.next) {
      case 0:
        obj = { a: 1, b: 2, c: 3 };
        context$1$0.t0 = regeneratorRuntime.keys(obj);

      case 2:
        if ((context$1$0.t1 = context$1$0.t0()).done) {
          context$1$0.next = 10;
          break;
        }

        i = context$1$0.t1.value;
        context$1$0.next = 6;
        return regeneratorRuntime.awrap(1);

      case 6:
        delete obj.b;
        alert(i);
        context$1$0.next = 2;
        break;

      case 10:
      case "end":
        return context$1$0.stop();
    }
  }, null, this);
}

... which doesn't seem to check keys multiple times (only one regeneratorRuntime.keys() call).`

Still, there's nothing wrong with being more correct than Regenerator.

Ah, the implementation of .keys() returns a function that handles that. Never mind then.

[marten-de-vries]

Relevant code

The trick is to add a check to that code so a removed item is not iterated on. This most likely includes storing node.right in a temporary variable to prevent evaluating that code multiple times, then adding a check (if statement) over the loop body.

Making this change will make the for-in tests fail (npm run test:js). They should be updated to reflect the change. The trick will be to have a correct implementation, but preferably not too much boilerplate code copied for every for in loop. (I'd like Kneden to remain runtime helper library free - but not at the cost of unreadable code.)

[ljharb]

fwiw, for..in creates a snapshot of all enumerable object keys prior to enumeration, and checks it for deletion before yielding it to the loop.

That means, you could do var keys = []; for (var k in obj) { keys.push(k); } prior to the awaited loop, and simply check if k in obj before entering that iteration. It would mean your loop is O(2n) instead of O(n) but I think that's acceptable.

[marten-de-vries]

Yes, that's the same idea, though written out more clearly. :)

[AprilArcus]

Hi! I wanted to take a stab at this, but found myself confused by the existing behavior of for-in. Currently, the expected output for

 1  async function test(a) {
 2    for (var i in a) {
 3      await i;
 4    }
 5  }

is

 1  function test(a) {
 2    function _recursive() {
 3      if (_items.length) {
 4        return Promise.resolve().then(function () {
 5          i = _items.pop();
 6          return i;
 7        }).then(function () {
 8          return _recursive();
 9        });
10      }
11    }
12
13    var i;
14    return Promise.resolve().then(function () {
15      var _items = [];
16
17      for (var _item in a) {
18        _items.push(_item);
19      }
20
21      _items.reverse();
22
23      return _recursive();
24    }).then(function () {});
25  }

However, on line 3 of the output, _items is out of scope and will throw a ReferenceError. To fix this, line 14 needs to be moved up above line 2 so that _recursive() can close over _items.

It looks like when it's time to build the PromiseChain, the call to path.get('body.body') in index.js:82 is not capturing the _recursive helper function inserted by the call to refactorLoop in the WhileStatement visitor in looprefactor.js:58.

[marten-de-vries]

@AprilArcus Thanks for looking into this!

It's a good find. Only recently I started hoisting up the _recursive function for some reason I don't remember now. It must have regressed then.

To fix it, you'd have to modify the template in looprefactor.js:142 to not include the definition, and additionally, in looprefactor:js:128, you should make sure the variable is defined at that point in a hoisted way, by using this.addVarDecl (see for an example looprefactor:216, the function itself is defined in index.js if I'm not mistaken.)

[AprilArcus]

Okay. What command are you using to rebuild the expected.js files? Running babel inside the test/fixtures/for-in causes it to balk at the untransformed kneden source.

[marten-de-vries]

All test files are manually written currently, to make sure any changes are deliberate. It might be nice to add tests that run transpiled code, though. That would have helped quite a bit with finding the out of scope bug. But that can wait.

[marten-de-vries]

Thanks to @AprilArcus for fixing this!