[systemjs] Fix nested `let`/`const` shadowing imported bindings #14057

The-x-Theorist · 2021-12-15T16:36:25Z

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

Variables will be renamed in the hoisting operation, and variables declared inside blockStatment will be different, hence they won't collide with any other identifier.

babel-bot · 2021-12-15T16:39:23Z

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

nicolo-ribaudo · 2021-12-21T18:10:55Z

packages/babel-helper-hoist-variables/src/index.ts

+        declar.parentPath.parentPath.isBlockStatement() &&
+        declar.parent.kind !== "var"


Nit: we can hoist this check outside of the for loop:

const needsRename = path.node.kind !== "var" && path.parentPath.isBlockStatement()

Actually, I think we can just edit the Scope visitor above to check kind === "let" || kind === "const".

Okay, I will change that.

nicolo-ribaudo · 2021-12-21T18:11:18Z

...posal-do-expressions/test/fixtures/do-expressions/variable-declaration-start-to-iife/exec.js

@@ -7,4 +7,4 @@ expect(
    bar;
  }
 ).toBe("foo");
-expect(bar).toBe("foo");
+// expect(bar).toBe("foo");


This behavior should not change.

nicolo-ribaudo · 2021-12-31T17:49:39Z

packages/babel-helper-hoist-variables/src/index.ts

    for (const declar of declarations) {
      firstId = declar.node.id;
+      if (needsRename) {
+        declar.scope.rename(declar.node.id.name);


The TS warning here is correct; I think this breaks, for example, with const { x } = {}. You probably need to do

if (needsRename) { for (const name of Object.keys(declar.getBindingIdentifiers()) { declar.scope.rename(name); } }

Okay, I will do that.

Also add a test for this!

Okay, I will add the test

I think that it's working now.

nicolo-ribaudo

Thanks!

JLHwung · 2022-03-03T19:44:21Z

...sform-modules-systemjs/test/fixtures/systemjs/module-level-variable-destructuring/output.mjs

+    execute: function () {
+      if (true) {
+        ({
+          x: _x


Why const { x } = { x: 1 } is also transformed? I would expect _x is registered in the same scope of the local x declaration:

if (true) { const { x: _x } = { x: 1 }; console.log(_x); }

Hoisted vars are renamed, So I think that's the reason for this.

What should we change in this?

I have been thinking about this, and the proper fix probably is in the systemjs plugin itself.

We should not hoist all variables (var and let/const), but only var and top-level let/const. We can do this by:

In the big loop over the program body (in the Program.exit visitor), we convert all the top-level let/const bindings to var. This is safe, they are not in nested blocks.

When calling hoistVariables, we should remove the third parameter (null) so that it only hoists var and not also let/const.

For (1), be careful because export let x = 1; and export { x }; let x = 1; are handled in two different code paths and both should still work.

If third parameter is removed from the hoistVariable it removes all the _export calls from the outputs, and doesn't transform the input?

Yeah we will still need to hoist these export bindings and set up the metadata from which the _export calls are built. I suggest we copy the code from the hoistVariable to the systemjs transform, and implement the custom hoisting logic mentioned in #14057 (comment). Or we can add a new option in the helper, though I lean to the first option since the hoisting logic here is not reused by other plugins.

I'm surprised that the current helper doesn't work, because we only need _export for things that become var in step 1 🤔
Do you mind pushing the current (non working) code you have?

I don't have a working branch. We can infer the behaviour of hoistVariable helper from the implementation:

babel/packages/babel-helper-hoist-variables/src/index.ts

Lines 23 to 32 in dacaab1

Scope(path: NodePath, state: State) {

if (state.kind === "let") path.skip();

},

FunctionParent(path: NodePath) {

path.skip();

},

VariableDeclaration(path: NodePath<t.VariableDeclaration>, state: State) {

if (state.kind && path.node.kind !== state.kind) return;

The third variable kind controls two behaviours: 1) Whether we should check a BlockParent to see if it contains to-be-hoisted variables, and 2) whether we should hoist the let-kind variable declarations.

In systemjs transform, what we want to achieve is to hoist var variables within block parent (so kind has to be "let"), but leave let-kind declarations within block parent untouched.

If we set kind to "var", only var-kind declarations will be hoisted, the top level let-kinds are not.

If we set kind to null, all declarations will be hoisted unless nested within FunctionParent. But we want to preserve let within block parent

If we set kind to "let", only let-kind declarations will be hoisted

Therefore we still need to extend the hoistVariable helper behaviour.

@The-x-Theorist I hope you don't mind, I pushed two commits to your branch.

14e9cfd (#14057) is how I was thinking to fix the problem, as described in #14057 (comment):

It changes all the top-level let/const to var

It only hoists vars

The updated tests look good, but it probably needs a few new tests to make sure that both const x = 1; export { x } and export const x = 1; work (unless we already have these tests).

I'll re-review this

JLHwung · 2022-03-16T23:56:45Z

packages/babel-plugin-transform-modules-systemjs/src/index.ts

+                    // Convert top-level variable declarations to "var",
+                    // because they must be hoisted
+                    declar.node.kind = "var";
+                  }


A top level var declaration can be nested within for statement:

for (var x = 1;false;); export { x }

or it could be within a do expression.

They already have var, there is no need to manually change them.

JLHwung · 2022-03-16T23:57:51Z

packages/babel-helper-hoist-variables/src/index.ts

@@ -38,8 +38,16 @@ const visitor = {
    > = path.get("declarations");


The hoistVariable visitor should be merged with environmentVisitor, too.

Lets do it in another PR since it's separate from this bug 👍

nicolo-ribaudo · 2022-03-17T11:21:48Z

@The-x-Theorist I reverted your hoistVariable changes but I still kept your tests since they ensure that we fixed the bug. Thank you!

Fix: modules-systemjs transformation

45f4c1f

var declarations won't be renamed

8199f31

nicolo-ribaudo added area: modules PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Dec 21, 2021

nicolo-ribaudo reviewed Dec 21, 2021

View reviewed changes

check hoisted outside the for loop

69c067b

The-x-Theorist requested a review from nicolo-ribaudo December 22, 2021 09:24

nicolo-ribaudo reviewed Dec 31, 2021

View reviewed changes

TS warning fixed

a312fe2

The-x-Theorist requested a review from nicolo-ribaudo January 3, 2022 11:02

test added for destructuring

15714de

nicolo-ribaudo previously approved these changes Jan 5, 2022

View reviewed changes

nicolo-ribaudo requested a review from JLHwung February 13, 2022 15:03

JLHwung reviewed Mar 3, 2022

View reviewed changes

nicolo-ribaudo added 2 commits March 16, 2022 22:31

Improve TS types

6fe46d8

Don't hoist let/const in blocks

14e9cfd

JLHwung reviewed Mar 16, 2022

View reviewed changes

JLHwung approved these changes Mar 16, 2022

View reviewed changes

nicolo-ribaudo added 2 commits March 17, 2022 12:19

Revert changes to helper-hoist-variables

aa5593e

Add test

aeb81ef

nicolo-ribaudo changed the title ~~Fix: modules-systemjs transformation~~ [systemjs] Fix nested let/const shadowing imported bindings Mar 17, 2022

nicolo-ribaudo approved these changes Mar 17, 2022

View reviewed changes

nicolo-ribaudo merged commit 410c9ac into babel:main Mar 17, 2022

The-x-Theorist deleted the modules-systemjs-transformation branch March 23, 2022 10:22

github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jun 23, 2022

github-actions bot locked as resolved and limited conversation to collaborators Jun 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[systemjs] Fix nested `let`/`const` shadowing imported bindings #14057

[systemjs] Fix nested `let`/`const` shadowing imported bindings #14057

The-x-Theorist commented Dec 15, 2021 •

edited by gitpod-io bot

babel-bot commented Dec 15, 2021 •

edited

nicolo-ribaudo Dec 21, 2021

nicolo-ribaudo Dec 21, 2021

The-x-Theorist Dec 22, 2021

nicolo-ribaudo Dec 21, 2021

nicolo-ribaudo Dec 31, 2021

The-x-Theorist Jan 3, 2022

nicolo-ribaudo Jan 3, 2022

The-x-Theorist Jan 4, 2022

The-x-Theorist Jan 5, 2022

nicolo-ribaudo left a comment

JLHwung Mar 3, 2022

The-x-Theorist Mar 7, 2022

The-x-Theorist Mar 8, 2022

nicolo-ribaudo Mar 8, 2022 •

edited

nicolo-ribaudo Mar 8, 2022

The-x-Theorist Mar 15, 2022

JLHwung Mar 16, 2022

nicolo-ribaudo Mar 16, 2022

JLHwung Mar 16, 2022 •

edited

nicolo-ribaudo Mar 16, 2022

JLHwung Mar 16, 2022

nicolo-ribaudo Mar 17, 2022

JLHwung Mar 16, 2022

nicolo-ribaudo Mar 17, 2022

nicolo-ribaudo commented Mar 17, 2022

		declar.parentPath.parentPath.isBlockStatement() &&
		declar.parent.kind !== "var"

	Scope(path: NodePath, state: State) {
	if (state.kind === "let") path.skip();
	},

	FunctionParent(path: NodePath) {
	path.skip();
	},

	VariableDeclaration(path: NodePath<t.VariableDeclaration>, state: State) {
	if (state.kind && path.node.kind !== state.kind) return;

		@@ -38,8 +38,16 @@ const visitor = {
		> = path.get("declarations");

[systemjs] Fix nested let/const shadowing imported bindings #14057

[systemjs] Fix nested let/const shadowing imported bindings #14057

Conversation

The-x-Theorist commented Dec 15, 2021 • edited by gitpod-io bot

babel-bot commented Dec 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolo-ribaudo Mar 8, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JLHwung Mar 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolo-ribaudo commented Mar 17, 2022

[systemjs] Fix nested `let`/`const` shadowing imported bindings #14057

[systemjs] Fix nested `let`/`const` shadowing imported bindings #14057

The-x-Theorist commented Dec 15, 2021 •

edited by gitpod-io bot

babel-bot commented Dec 15, 2021 •

edited

nicolo-ribaudo Mar 8, 2022 •

edited

JLHwung Mar 16, 2022 •

edited