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
Do not remove let bindings even they are wrapped in closure #10343
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/11340/ |
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.
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-spread
should be blamed but after digging you would find that it comes fromblock-scoping
which 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-scoping
and 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-scoping
transforms. 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
removeUnusedExcludeKeys
function that is invoked only inloose
mode. 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
foo
is inside an arrow function expression. However, theblock-scoping
transform 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-spread
loose 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.