Create additional scope when redeclaring array variable in for..of loop

[kyle1320]

Q	A
Fixed Issues?	Fixes #8913
Patch: Bug Fix?	Yes
Major: Breaking Change?	No
Minor: New Feature?	No
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?	No
License	MIT

When transforming a for..of loop that iterates through an array, redeclaring the array identifier within the loop would result in an ambiguity when assigning the iteration variable at the top of the transformed loop body, which could result in incorrect behavior.

This fix detects whether the array identifier is redeclared within the loop, and if so, wraps the original loop body in an additional block statement to prevent ambiguities when assigning the iteration variable.

[babel-bot]

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/9281/

[babel-bot]

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/9281/

[pomek]

Seems that this PR fixes the issue. But I used a primitive method to check this out:

1. I found REPL that should fail (taken from preset-env: Edge support for arrow param destructuring #8926).

2. I Pasted the entire content of the editor's build: https://github.com/ckeditor/ckeditor5-build-classic/blob/cf4681772537874c5143b8148c3ebbab1f46e6b8/build/ckeditor.js

3. Copied build from the right window on REPL.

4. I modified a sample https://github.com/ckeditor/ckeditor5-build-classic/blob/cf4681772537874c5143b8148c3ebbab1f46e6b8/sample/index.html:

   • <script src="../build/ckeditor.js"></script> was replaced with the code produced by REPL
   • I needed to import babel-regenerator-runtime so I found a CDN with that:

   <script src="https://cdn.jsdelivr.net/npm/babel-regenerator-runtime@6.5.0/runtime.min.js"></script>

5. As I expected, an error mentioned in the issue (for...of array optimization can introduce conflicting variables into the scope #8913) still appears.

6. Now I am going to use REPL from this PR.

7. It just works! 🎉

I am going to apply changes from this PR to my package inside the node_modules/ directory and will check whether it works with Webpack.

[pomek]

I applied the patch to package installed locally and it works! ❤️

[loganfsmyth]

This is a great effort, thanks so much, the main issues are

1. Technically there can be multiple names, and we need to check if any of them collide for (const {a, b} of arr) { const b = 4; }
2. Rather than assigning to block.body it would be better to replace the path.node.body node, I think

Something along the lines of

const iterationKey = scope.generateUidIdentifier("i");

const body = path.get("body");

const hasKeyConflict = 
  body.isBlockStatement() &&
  path.getBindingIdentifiers().some(id => body.scope.hasBinding(id));

let body = hasKeyConflict 
  ? t.blockStatement([node.body])
  : node.body;

let loop = buildForOfArray({
  BODY: body,

I forget the exact return value of path.getBindingIdentifiers() so you might need to tweak that.

Also, we should make sure this also adds the block wrapper for code like

let a, b;
for ({a, b} of arr) {
  const b = 4;
}

since that would also collide. I think this might just work automatically with getBindingIdentifiers but please check if you can.

[kyle1320]

Thanks for the feedback!

As for your first suggestion, I suppose that comes down the scope of the PR. The original issue was caused by a reuse of the array identifier, not the iteration variable. This was causing the array lookup to use the wrong identifier when the inner variable was renamed.

There are a couple of other issues open which specifically deal with the iteration variables being redeclared, namely #8498 and #8839.

I'm rather new to this whole game, so I'm not sure whether it would be preferred to fix those issues in this PR as well, since it is similar, or just leave them to someone else. This PR has already been referenced in #8498 as a related solution. But I'll defer back to you on that.

As for your second point, the reason I used block.body was because block is guaranteed to be a block statement, whereas path.node.body is not. If there is some other reason this is preferred, then I suppose the code could be changed to the following:

if (path.get("body").scope.hasOwnBinding(right.name)) {
    path.node.body = t.blockStatement([t.toBlock(body)]);
}
const block = t.toBlock(path.node.body);

But this seems a bit underhanded to me.

[Timer]

Any update on this? We've had one or two Create React App users hit this. 😄

[danez]

This seems it would be fixed by #9697