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.

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

fix: mark var declarations in loops as not constant #15027

Merged
merged 3 commits into from Oct 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Expand Up @@ -5,7 +5,8 @@ for (var _ref in obj) {
}

for (var _ref2 in obj) {
name = _ref2[0];
value = _ref2[1];
var _ref3 = _ref2;
name = _ref3[0];
value = _ref3[1];
Comment on lines 7 to +10
Copy link
Member Author

Choose a reason for hiding this comment

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

Input:

for ([name, value] in obj) {

This output changes because the destructuring transform first transforms it to

for (var _ref2 in obj) {
  [name, value] = _ref2;

and then _ref2 isn't considered constant anymore.

We can avoid this in Babel 8, by injecting let instead of var.

print("Name: " + name + ", Value: " + value);
}
Expand Up @@ -7,9 +7,11 @@ for (var _ref in obj) {
}

for (var _ref3 in obj) {
var _ref4 = babelHelpers.slicedToArray(_ref3, 2);
var _ref4 = _ref3;

name = _ref4[0];
value = _ref4[1];
var _ref5 = babelHelpers.slicedToArray(_ref4, 2);

name = _ref5[0];
value = _ref5[1];
print("Name: " + name + ", Value: " + value);
}
Expand Up @@ -13,7 +13,8 @@ for (const _ref of [O]) {
var _;

for (var _ref2 of [O]) {
_ = _ref2[a];
var _ref3 = _ref2;
_ = _ref3[a];
{
const a = "A";
}
Expand Down
Expand Up @@ -7,10 +7,12 @@ for (var _ref of test.expectation.registers) {
}

for (var _ref3 of test.expectation.registers) {
var _ref4 = babelHelpers.slicedToArray(_ref3, 3);
var _ref4 = _ref3;

name = _ref4[0];
before = _ref4[1];
after = _ref4[2];
var _ref5 = babelHelpers.slicedToArray(_ref4, 3);

name = _ref5[0];
before = _ref5[1];
after = _ref5[2];
void 0;
}
@@ -0,0 +1,11 @@
function render() {
const nodes = [];

for (let i = 0; i < 5; i++) {
const o = "foo";
const n = i;
nodes.push(<div>{n}</div>);
}

return nodes;
}
@@ -0,0 +1,11 @@
function render() {
const nodes = [];

for (let i = 0; i < 5; i++) {
const o = "foo";
const n = i;
nodes.push(<div>{n}</div>);
}

return nodes;
}
@@ -0,0 +1,9 @@
function render() {
const nodes = [];

for (const node of nodes) {
nodes.push(<div>{node}</div>);
}

return nodes;
}
@@ -0,0 +1,9 @@
function render() {
const nodes = [];

for (const node of nodes) {
nodes.push(<div>{node}</div>);
}

return nodes;
}
@@ -0,0 +1,10 @@
function render() {
const nodes = [];

for (const node of nodes) {
const n = node;
nodes.push(<div>{n}</div>);
}

return nodes;
}
@@ -0,0 +1,10 @@
function render() {
const nodes = [];

for (const node of nodes) {
const n = node;
nodes.push(<div>{n}</div>);
}

return nodes;
}
@@ -0,0 +1,11 @@
function render() {
const nodes = [];

for (let i = 0; i < 5; i++) {
const o = "foo";
const n = i;
nodes.push(<div>{o}</div>);
}

return nodes;
}
@@ -0,0 +1,11 @@
function render() {
const nodes = [];

for (let i = 0; i < 5; i++) {
const o = "foo";
const n = i;
nodes.push(<div>{o}</div>);
}

return nodes;
}
36 changes: 36 additions & 0 deletions packages/babel-traverse/src/scope/binding.ts
Expand Up @@ -44,6 +44,24 @@ export default class Binding {
this.path = path;
this.kind = kind;

if (
(kind === "var" || kind === "hoisted") &&
// https://github.com/rollup/rollup/issues/4654
// Rollup removes the path argument from this call. Add an
// unreachable IIFE (that rollup doesn't know is unreachable)
// with side effects, to prevent it from messing up with arguments.
// You can reproduce this with
// BABEL_8_BREAKING=true make prepublish-build
isDeclaredInLoop(
path ||
(() => {
throw new Error("Internal Babel error: unreachable ");
})(),
)
) {
this.reassign(path);
}

this.clearValue();
}

Expand Down Expand Up @@ -109,3 +127,21 @@ export default class Binding {
this.referenced = !!this.references;
}
}

function isDeclaredInLoop(path: NodePath) {
for (
let { parentPath, key } = path;
parentPath;
{ parentPath, key } = parentPath
) {
if (parentPath.isFunctionParent()) return false;
if (
parentPath.isWhile() ||
parentPath.isForXStatement() ||
(parentPath.isForStatement() && key === "body")
) {
return true;
}
}
return false;
}
38 changes: 38 additions & 0 deletions packages/babel-traverse/test/scope.js
Expand Up @@ -391,6 +391,44 @@ describe("scope", () => {
).toBe(false);
});

it("variable constantness in loops", () => {
let scopePath = null;
const isAConstant = code => {
let path = getPath(code);
if (scopePath) path = path.get(scopePath);
return path.scope.getBinding("a").constant;
};

expect(isAConstant("for (_ of ns) { var a = 1; }")).toBe(false);
expect(isAConstant("for (_ in ns) { var a = 1; }")).toBe(false);
expect(isAConstant("for (;;) { var a = 1; }")).toBe(false);
expect(isAConstant("while (1) { var a = 1; }")).toBe(false);
expect(isAConstant("do { var a = 1; } while (1)")).toBe(false);

expect(isAConstant("for (var a of ns) {}")).toBe(false);
expect(isAConstant("for (var a in ns) {}")).toBe(false);
expect(isAConstant("for (var a;;) {}")).toBe(true);

scopePath = "body.0.body.expression";
expect(isAConstant("for (_ of ns) () => { var a = 1; }")).toBe(true);
expect(isAConstant("for (_ in ns) () => { var a = 1; }")).toBe(true);
expect(isAConstant("for (;;) () => { var a = 1; }")).toBe(true);
expect(isAConstant("while (1) () => { var a = 1; }")).toBe(true);
expect(isAConstant("do () => { var a = 1; }; while (1)")).toBe(true);

scopePath = "body.0.body";
expect(isAConstant("for (_ of ns) { let a = 1; }")).toBe(true);
expect(isAConstant("for (_ in ns) { let a = 1; }")).toBe(true);
expect(isAConstant("for (;;) { let a = 1; }")).toBe(true);
expect(isAConstant("while (1) { let a = 1; }")).toBe(true);
expect(isAConstant("do { let a = 1; } while (1)")).toBe(true);

scopePath = "body.0";
expect(isAConstant("for (let a of ns) {}")).toBe(true);
expect(isAConstant("for (let a in ns) {}")).toBe(true);
expect(isAConstant("for (let a;;) {}")).toBe(true);
});

test("label", function () {
expect(getPath("foo: { }").scope.getBinding("foo")).toBeUndefined();
expect(getPath("foo: { }").scope.getLabel("foo").type).toBe(
Expand Down