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.
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
rename colliding let bindings with for loop init #8937
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/9297/ |
Can you also add complex declarations to the test? e.g. for (let a, { b } = {};;) {
let a, { b } = {};
} |
Added your example, will think of other test cases. Thanks! @nicolo-ribaudo |
@@ -0,0 +1,5 @@ | |||
for (let a, _ref = {}, b = _ref.b;;) { | |||
let a, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should enable the block scoping plug-in, otherwise we are testing the wrong thing 😛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, I thought the block scoping plugin was enabled by default since the test is in that directory... 🤔 I'm guessing it's okay to leave the transform destructuring plugin? w/o the transform destructuring plugin, the destructured variable gets incorrectly renamed regardless.
var { i } = {};
{
var { i: _i } = {};
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it is ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Thank you! |
* master: (222 commits) Set correct methods name Use toPropertyKey in the "decorate" helper Allow function types in type params within arrow return types (babel#8954) Fix message when plugin of a wrong type is passed (babel#8950) rename colliding let bindings with for loop init (babel#8937) edge incomplete support for arrow destructuring (babel babel#8349) (babel#8926) fix single-arg async arrows when retainLines=true (babel#8868) [flow] Explicit inexact objects with `...` (babel#8884) Update preset-env data (babel#8898) Treat break inside block inside loop (babel#8914) fixed "source map" formatting in comment (babel#8878) [skip ci] fix typo in contributing guidelines (babel#8901) [skip ci] fix: Expression x === 'y' && '' should not evaluate to undefined. (babel#8880) fixed an extra word Fixes babel#8865 (babel#8866) v7.1.4 v7.1.3 Bump Babel deps (babel#8770) flow-bin@0.82.0 (babel#8832) Insertafter jsx fix (babel#8833) ... # Conflicts: # packages/babel-parser/src/tokenizer/index.js # packages/babel-parser/test/fixtures/experimental/class-private-properties/failure-numeric-literal/options.json # packages/babel-parser/test/fixtures/experimental/pipeline-operator/invalid-proposal/options.json
Babel transform block scoping doesn't remap let declarations if there's a collision in the loop body with the loop init variable. OutsideLetReferences only keeps track of the loop's init variable so I'm checking the scope of the block path for any colliding variable and renaming it.
This only checks the loop's init variable against the bindings in the loop's body -- so I believe there won't be any additional/unnecessary renaming happening.
Lmk if there's any additional test cases I should add or if there's anything I could do better. Thanks!