Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 9 commits into from Mar 17, 2022
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
45f4c1f
Fix: modules-systemjs transformation
The-x-Theorist Dec 15, 2021
8199f31
var declarations won't be renamed
The-x-Theorist Dec 21, 2021
69c067b
check hoisted outside the for loop
The-x-Theorist Dec 22, 2021
a312fe2
TS warning fixed
The-x-Theorist Jan 3, 2022
15714de
test added for destructuring
The-x-Theorist Jan 4, 2022
6fe46d8
Improve TS types
nicolo-ribaudo Mar 16, 2022
14e9cfd
Don't hoist let/const in blocks
nicolo-ribaudo Mar 16, 2022
aa5593e
Revert changes to `helper-hoist-variables`
nicolo-ribaudo Mar 17, 2022
aeb81ef
Add test
nicolo-ribaudo Mar 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions packages/babel-helper-hoist-variables/src/index.ts
View file
Open in desktop
Expand Up @@ -38,8 +38,16 @@ const visitor = {
> = path.get("declarations");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hoistVariable visitor should be merged with environmentVisitor, too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

let firstId;

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

for (const declar of declarations) {
firstId = declar.node.id;
if (needsRename) {
for (const name of Object.keys(declar.getBindingIdentifiers())) {
declar.scope.rename(name);
}
}

if (declar.node.init) {
nodes.push(
Expand Down
2 changes: 1 addition & 1 deletion .../babel-plugin-transform-modules-systemjs/test/fixtures/systemjs/export-fn-decl/output.mjs
View file
Open in desktop
Expand Up @@ -15,4 +15,4 @@ System.register([], function (_export, _context) {
_export("testProp", testProp = 'test property');
}
};
});
});
8 changes: 8 additions & 0 deletions ...orm-modules-systemjs/test/fixtures/systemjs/module-level-variable-destructuring/input.mjs
View file
Open in desktop
@@ -0,0 +1,8 @@
import { x } from './x.js';

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

new (class extends x {})();
23 changes: 23 additions & 0 deletions ...rm-modules-systemjs/test/fixtures/systemjs/module-level-variable-destructuring/output.mjs
View file
Open in desktop
@@ -0,0 +1,23 @@
System.register(["./x.js"], function (_export, _context) {
"use strict";

var x, _x;

return {
setters: [function (_xJs) {
x = _xJs.x;
}],
execute: function () {
if (true) {
({
x: _x
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What should we change in this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.
  2. When calling hoistVariables, we should remove the third parameter (null) so that it only hoists var and not also let/const.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@JLHwung JLHwung Mar 16, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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):

  1. It changes all the top-level let/const to var
  2. 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).

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

new class extends x {}();
}
};
});
8 changes: 8 additions & 0 deletions ...-plugin-transform-modules-systemjs/test/fixtures/systemjs/module-level-variable/input.mjs
View file
Open in desktop
@@ -0,0 +1,8 @@
import { x } from './x.js';

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

new (class extends x {})();
19 changes: 19 additions & 0 deletions ...plugin-transform-modules-systemjs/test/fixtures/systemjs/module-level-variable/output.mjs
View file
Open in desktop
@@ -0,0 +1,19 @@
System.register(["./x.js"], function (_export, _context) {
"use strict";

var x, _x;

return {
setters: [function (_xJs) {
x = _xJs.x;
}],
execute: function () {
if (true) {
_x = 1;
console.log(_x);
}

new class extends x {}();
}
};
});