From 6404e8a826cc1044841af796b84001bc37e8b393 Mon Sep 17 00:00:00 2001 From: Stephane Rufer Date: Fri, 17 Sep 2021 10:45:10 -0700 Subject: [PATCH 1/3] fix: babel-traverse constant flag in bindings flag bindings in loops as not constant. Fixes: #13760 --- .../constant-elements/for-loop-2/input.js | 11 +++++++++ .../constant-elements/for-loop-2/output.js | 11 +++++++++ .../constant-elements/for-loop-3/input.js | 9 ++++++++ .../constant-elements/for-loop-3/output.js | 9 ++++++++ .../constant-elements/for-loop-4/input.js | 10 ++++++++ .../constant-elements/for-loop-4/output.js | 10 ++++++++ .../constant-elements/for-loop-5/input.js | 11 +++++++++ .../constant-elements/for-loop-5/output.js | 11 +++++++++ packages/babel-traverse/src/scope/binding.ts | 7 ++++++ packages/babel-traverse/test/scope.js | 23 +++++++++++++++++++ 10 files changed, 112 insertions(+) create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop-2/input.js create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop-2/output.js create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop-3/input.js create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop-3/output.js create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop-4/input.js create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop-4/output.js create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop-5/input.js create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop-5/output.js diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop-2/input.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop-2/input.js new file mode 100644 index 000000000000..6f9f41201c54 --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop-2/input.js @@ -0,0 +1,11 @@ +function render() { + const nodes = []; + + for (let i = 0; i < 5; i++) { + const o = "foo"; + const n = i; + nodes.push(
{n}
); + } + + return nodes; +} diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop-2/output.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop-2/output.js new file mode 100644 index 000000000000..6f9f41201c54 --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop-2/output.js @@ -0,0 +1,11 @@ +function render() { + const nodes = []; + + for (let i = 0; i < 5; i++) { + const o = "foo"; + const n = i; + nodes.push(
{n}
); + } + + return nodes; +} diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop-3/input.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop-3/input.js new file mode 100644 index 000000000000..6a3a6907e0a6 --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop-3/input.js @@ -0,0 +1,9 @@ +function render() { + const nodes = []; + + for (const node of nodes) { + nodes.push(
{node}
); + } + + return nodes; +} diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop-3/output.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop-3/output.js new file mode 100644 index 000000000000..6a3a6907e0a6 --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop-3/output.js @@ -0,0 +1,9 @@ +function render() { + const nodes = []; + + for (const node of nodes) { + nodes.push(
{node}
); + } + + return nodes; +} diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop-4/input.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop-4/input.js new file mode 100644 index 000000000000..5ca98a8fdefe --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop-4/input.js @@ -0,0 +1,10 @@ +function render() { + const nodes = []; + + for (const node of nodes) { + const n = node; + nodes.push(
{n}
); + } + + return nodes; +} diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop-4/output.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop-4/output.js new file mode 100644 index 000000000000..5ca98a8fdefe --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop-4/output.js @@ -0,0 +1,10 @@ +function render() { + const nodes = []; + + for (const node of nodes) { + const n = node; + nodes.push(
{n}
); + } + + return nodes; +} diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop-5/input.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop-5/input.js new file mode 100644 index 000000000000..a9c57919f214 --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop-5/input.js @@ -0,0 +1,11 @@ +function render() { + const nodes = []; + + for (let i = 0; i < 5; i++) { + const o = "foo"; + const n = i; + nodes.push(
{o}
); + } + + return nodes; +} diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop-5/output.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop-5/output.js new file mode 100644 index 000000000000..a9c57919f214 --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop-5/output.js @@ -0,0 +1,11 @@ +function render() { + const nodes = []; + + for (let i = 0; i < 5; i++) { + const o = "foo"; + const n = i; + nodes.push(
{o}
); + } + + return nodes; +} diff --git a/packages/babel-traverse/src/scope/binding.ts b/packages/babel-traverse/src/scope/binding.ts index 8ca4cd2c0e2d..9432ecbc6e9f 100644 --- a/packages/babel-traverse/src/scope/binding.ts +++ b/packages/babel-traverse/src/scope/binding.ts @@ -44,6 +44,13 @@ export default class Binding { this.path = path; this.kind = kind; + for (let node of path.getAncestry()) { + if (node.isLoop()) { + debugger; + this.constant = false; + } + } + this.clearValue(); } diff --git a/packages/babel-traverse/test/scope.js b/packages/babel-traverse/test/scope.js index ec1f999dad80..92035e652852 100644 --- a/packages/babel-traverse/test/scope.js +++ b/packages/babel-traverse/test/scope.js @@ -389,6 +389,29 @@ describe("scope", () => { expect( getPath("var a = 1; var a = 2;").scope.getBinding("a").constant, ).toBe(false); + expect( + getPath("for (var n of ns) { var a = 1; }").scope.getBinding("a") + .constant, + ).toBe(false); + expect( + getPath("for (var n in ns) { var a = 1; }").scope.getBinding("a") + .constant, + ).toBe(false); + expect( + getPath("for (var i = 0; i < n; i++) { var a = 1; }").scope.getBinding( + "a", + ).constant, + ).toBe(false); + expect( + getPath("var i = 0; while (i != 1) { var a = 1; }").scope.getBinding( + "a", + ).constant, + ).toBe(false); + expect( + getPath("var i = 0; do { var a = 1; } while (i != 1)").scope.getBinding( + "a", + ).constant, + ).toBe(false); }); test("label", function () { From 0a21a2258f5a7a6c35e4ffa46ccb5ca813dfd11c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=B2=20Ribaudo?= Date: Fri, 7 Oct 2022 17:21:44 +0200 Subject: [PATCH 2/3] Only mark `var`s directly inside loops --- .../for-in/output.js | 5 +- .../fixtures/destructuring/for-in/output.js | 8 ++- .../for-of-shadowed-block-scoped/output.js | 3 +- .../fixtures/destructuring/for-of/output.js | 10 +-- packages/babel-traverse/src/scope/binding.ts | 25 ++++++-- packages/babel-traverse/test/scope.js | 61 ++++++++++++------- 6 files changed, 74 insertions(+), 38 deletions(-) diff --git a/packages/babel-plugin-transform-destructuring/test/fixtures/assumption-iterableIsArray/for-in/output.js b/packages/babel-plugin-transform-destructuring/test/fixtures/assumption-iterableIsArray/for-in/output.js index de8d1198b549..b77b0a544795 100644 --- a/packages/babel-plugin-transform-destructuring/test/fixtures/assumption-iterableIsArray/for-in/output.js +++ b/packages/babel-plugin-transform-destructuring/test/fixtures/assumption-iterableIsArray/for-in/output.js @@ -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]; print("Name: " + name + ", Value: " + value); } diff --git a/packages/babel-plugin-transform-destructuring/test/fixtures/destructuring/for-in/output.js b/packages/babel-plugin-transform-destructuring/test/fixtures/destructuring/for-in/output.js index 6f2e55a2b029..bffddd3e9f2e 100644 --- a/packages/babel-plugin-transform-destructuring/test/fixtures/destructuring/for-in/output.js +++ b/packages/babel-plugin-transform-destructuring/test/fixtures/destructuring/for-in/output.js @@ -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); } diff --git a/packages/babel-plugin-transform-destructuring/test/fixtures/destructuring/for-of-shadowed-block-scoped/output.js b/packages/babel-plugin-transform-destructuring/test/fixtures/destructuring/for-of-shadowed-block-scoped/output.js index 23908dee29eb..3d0f6f30023c 100644 --- a/packages/babel-plugin-transform-destructuring/test/fixtures/destructuring/for-of-shadowed-block-scoped/output.js +++ b/packages/babel-plugin-transform-destructuring/test/fixtures/destructuring/for-of-shadowed-block-scoped/output.js @@ -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"; } diff --git a/packages/babel-plugin-transform-destructuring/test/fixtures/destructuring/for-of/output.js b/packages/babel-plugin-transform-destructuring/test/fixtures/destructuring/for-of/output.js index 570f8b2ef665..24375d436320 100644 --- a/packages/babel-plugin-transform-destructuring/test/fixtures/destructuring/for-of/output.js +++ b/packages/babel-plugin-transform-destructuring/test/fixtures/destructuring/for-of/output.js @@ -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; } diff --git a/packages/babel-traverse/src/scope/binding.ts b/packages/babel-traverse/src/scope/binding.ts index 9432ecbc6e9f..dae019696e7b 100644 --- a/packages/babel-traverse/src/scope/binding.ts +++ b/packages/babel-traverse/src/scope/binding.ts @@ -44,11 +44,8 @@ export default class Binding { this.path = path; this.kind = kind; - for (let node of path.getAncestry()) { - if (node.isLoop()) { - debugger; - this.constant = false; - } + if ((kind === "var" || kind === "hoisted") && isDeclaredInLoop(path)) { + this.reassign(path); } this.clearValue(); @@ -116,3 +113,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; +} diff --git a/packages/babel-traverse/test/scope.js b/packages/babel-traverse/test/scope.js index 92035e652852..63cea944ce9f 100644 --- a/packages/babel-traverse/test/scope.js +++ b/packages/babel-traverse/test/scope.js @@ -389,29 +389,44 @@ describe("scope", () => { expect( getPath("var a = 1; var a = 2;").scope.getBinding("a").constant, ).toBe(false); - expect( - getPath("for (var n of ns) { var a = 1; }").scope.getBinding("a") - .constant, - ).toBe(false); - expect( - getPath("for (var n in ns) { var a = 1; }").scope.getBinding("a") - .constant, - ).toBe(false); - expect( - getPath("for (var i = 0; i < n; i++) { var a = 1; }").scope.getBinding( - "a", - ).constant, - ).toBe(false); - expect( - getPath("var i = 0; while (i != 1) { var a = 1; }").scope.getBinding( - "a", - ).constant, - ).toBe(false); - expect( - getPath("var i = 0; do { var a = 1; } while (i != 1)").scope.getBinding( - "a", - ).constant, - ).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 () { From b1f08dd293583725e59194560c1af07b9a7fed1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=B2=20Ribaudo?= Date: Fri, 7 Oct 2022 17:38:37 +0200 Subject: [PATCH 3/3] Fix CI error --- packages/babel-traverse/src/scope/binding.ts | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/packages/babel-traverse/src/scope/binding.ts b/packages/babel-traverse/src/scope/binding.ts index dae019696e7b..5813b6c58cb0 100644 --- a/packages/babel-traverse/src/scope/binding.ts +++ b/packages/babel-traverse/src/scope/binding.ts @@ -44,7 +44,21 @@ export default class Binding { this.path = path; this.kind = kind; - if ((kind === "var" || kind === "hoisted") && isDeclaredInLoop(path)) { + 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); }