Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions packages/babel-plugin-transform-for-of/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,11 @@ export default declare((api, options) => {
}

const block = t.toBlock(body);

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

block.body.unshift(assignment);

path.replaceWith(
Expand Down Expand Up @@ -140,6 +145,10 @@ export default declare((api, options) => {
t.inherits(loop, node);
t.ensureBlock(loop);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

const iterationValue = t.memberExpression(
t.cloneNode(right),
t.cloneNode(iterationKey),
Expand Down
3 changes: 3 additions & 0 deletions ...ges/babel-plugin-transform-for-of/test/fixtures/for-of-as-array/for-of-redeclare/input.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
for (let o of arr) {
const arr = o;
}
6 changes: 6 additions & 0 deletions ...es/babel-plugin-transform-for-of/test/fixtures/for-of-as-array/for-of-redeclare/output.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
for (let _i = 0, _arr = arr; _i < _arr.length; _i++) {
let o = _arr[_i];
{
const arr = o;
}
}
5 changes: 5 additions & 0 deletions ...ages/babel-plugin-transform-for-of/test/fixtures/regression/redeclare-array-8913/input.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
function f(...t) {
for (let o of t) {
const t = o;
}
}
8 changes: 8 additions & 0 deletions ...ges/babel-plugin-transform-for-of/test/fixtures/regression/redeclare-array-8913/output.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
function f(...t) {
for (var _i = 0; _i < t.length; _i++) {
let o = t[_i];
{
const t = o;
}
}
}