Do not remove let bindings even they are wrapped in closure

[JLHwung]

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

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 from block-scoping which seems completely unrelated.

I have simplified the OP's snippet a bit

for (const {foo, ...bar} of {}) {
  () => foo;
  bar;
}

The minimum configuration to reproduce this bug is

{
  "plugins": ["transform-block-scoping", ["proposal-object-rest-spread", { "loose": true }]]
}

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 with proposal-object-rest-spread. The Babel REPL works good on two-pass transformation, which means there must be something wrong after the block-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

	if (
	path.scope.getBinding(bindingName).references > ZERO_REFS ||
	!bindingParentPath.isObjectProperty()
	) {

in the removeUnusedExcludeKeys function that is invoked only in loose 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

for (const {foo, ...bar} of {}) {
  () => foo;
  bar;
}

will be transformed to

var _loop = function (foo, bar) {
  () => foo;
  bar;
};

for (var { foo, ...bar } of qux) {
  _loop(foo, bar);
}

since foo is inside an arrow function expression. However, the block-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.

[babel-bot]

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

[nicolo-ribaudo]

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' ]

[JLHwung]

@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

	const ref = letRefs[key];
	const binding = scope.getBinding(ref.name);
	if (!binding) continue;

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 needsClosure only works in loop expression, the goal of removing those bindings that would be defined in loop body closure has never been met.

I have changed this.scope to this.blockPath.scope so that it works as expected. While it may look like a subtle but drastic change, the only behavior difference will be only in the ForOfStatement case if we look through the BlockScoping usage here.

Pattern	Equivalence
Loop	No: blockPath.scope !== this.scope
CatchClause	Yes: CatchClause and the its body shares the same scope
BlockStatement/SwitchStatement/Program	Yes: blockPath === path

[nicolo-ribaudo]

Actually the issue you mentioned above also exists in current master

Oh I didn't notice it 😅
Anyway, thanks for fixing it!

[nicolo-ribaudo]

I changed the exec test because the assertion in Program did never run.