Do not remove let bindings even they are wrapped in closure#10343
Do not remove let bindings even they are wrapped in closure#10343nicolo-ribaudo merged 3 commits intobabel:masterfrom
Conversation
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/11340/ |
nicolo-ribaudo
left a comment
There was a problem hiding this comment.
Nice writeup!
I checked out this PR locally and found that you didn't consider a case: when bindings are declared inside the loop body.
for (const ref of {}) {
const {foo, ...bar} = ref;
() => foo;
bar;
}After that the function is generated, the loop body still has the old bindings:
babel.transformSync(input, {
configFile: false,
plugins: [
load("plugin-transform-block-scoping"),
{
visitor: {
Function(path) {
const loopBody = path.parentPath.parentPath
.getNextSibling()
.get("body");
console.log(loopBody.parentPath.toString());
console.log(loopBody.scope.bindings);
},
},
},
],
});Logs
for (var ref of {}) {
_loop(ref);
}
[ 'foo', 'bar' ]
|
@nicolo-ribaudo Addressed in 0db9416. Actually the issue you mentioned above also exists in current master, so it should be another bug. 🤦♂️... Looking into babel/packages/babel-plugin-transform-block-scoping/src/index.js Lines 450 to 452 in eb3767d When scope is attached to a ForOfStatement, any variable declaration in the loop body is not visible to ForOfStatement and would be skipped here. Given that I have changed
|
Oh I didn't notice it 😅 |
0db9416 to
ce91919
Compare
|
I changed the |
I would like to talk more details of #10339 than I did usually because it is an interesting bug: It is an issue that, on the surface, the
object-rest-spreadshould be blamed but after digging you would find that it comes fromblock-scopingwhich seems completely unrelated.I have simplified the OP's snippet a bit
The minimum configuration to reproduce this bug is
note that the plugin order matters here. Theoretically a loose approximation of this configuration would be a two-pass transformation: First transform with
transform-block-scopingand then transform the intermediate result withproposal-object-rest-spread. The Babel REPL works good on two-pass transformation, which means there must be something wrong after theblock-scopingtransforms. Something that does not touch the syntax but the abstract metadata.The error in #10339 comes from
babel/packages/babel-plugin-proposal-object-rest-spread/src/index.js
Lines 113 to 116 in eb3767d
in the
removeUnusedExcludeKeysfunction that is invoked only inloosemode. This routine will lookup the binding informations of the ObjectPattern{ foo, ...bar }in the for-of statements. For unknown reasons the binding informations are corrupted so exception is thrown.The block-scoping transform will try to wrap the loop body into a closure once a let reference is used inside a function expressions in the loop body. Which means
will be transformed to
since
foois inside an arrow function expression. However, theblock-scopingtransform incorrectly removes the binding after the loop body is wrapped -- maybe because we tend to feel it is useless since the whole loop body is wrapped in a new scope.This operation creates dichotomy between the actual code and the abstract syntax tree: The variable declarator is still in our code while its binding information is removed. This error has been hidden for a while until it is luckily detected by
object-rest-spreadloose mode, which tries to manipulate the code according to the binding information.In the end, the fix is pretty straightforward: do not remove the binding information and always synchronize code with AST.