From 2b7da10793d425e38479349b25be4d7ac1c75ead Mon Sep 17 00:00:00 2001 From: vedantroy Date: Thu, 30 Jan 2020 00:49:43 -0500 Subject: [PATCH 01/13] Correctly transpile export bindings for some for-of loops --- .../src/rewrite-live-references.js | 28 +++++++++++++++++++ .../fixtures/misc/for-of-export/input.mjs | 12 ++++++++ .../fixtures/misc/for-of-export/output.js | 20 +++++++++++++ 3 files changed, 60 insertions(+) create mode 100644 packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-export/input.mjs create mode 100644 packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-export/output.js diff --git a/packages/babel-helper-module-transforms/src/rewrite-live-references.js b/packages/babel-helper-module-transforms/src/rewrite-live-references.js index 95f1daadd158..f252502f5009 100644 --- a/packages/babel-helper-module-transforms/src/rewrite-live-references.js +++ b/packages/babel-helper-module-transforms/src/rewrite-live-references.js @@ -306,4 +306,32 @@ const rewriteReferencesVisitor = { } }, }, + ForOfStatement(path) { + const { scope, node } = path; + const { body } = node + const { exported } = this + + //TODO: Handle case where the loop identifier has the same name as the exported variable but is actually different + // Example: + // export let foo = 3; + // while(true) { + // let foo = 3; + // // don't transpile this + // for (foo of [1, 2, 3]) {} + // } + if (node.left.type === 'Identifier' && exported.get(node.left.name)) { + const oldLoopVarName = node.left.name + const newLoopVarId = scope.generateUidIdentifier(oldLoopVarName) + const assign = t.assignmentExpression('=', node.left, newLoopVarId) + const block = t.toBlock(body) + block.body.unshift(assign) + path.replaceWith( + t.forOfStatement( + t.variableDeclaration('let', [t.variableDeclarator(newLoopVarId)]), + node.right, + block + ) + ) + } + } }; diff --git a/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-export/input.mjs b/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-export/input.mjs new file mode 100644 index 000000000000..088c4500866e --- /dev/null +++ b/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-export/input.mjs @@ -0,0 +1,12 @@ +export let foo = 42; + +for (foo of [1, 2, 3]) { + +} + +for (foo of [1, 2, 3]) { + // This loop body is transformed incorrectly + // correct output should be + // exports.foo = _foo2 + let foo = 3; +} diff --git a/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-export/output.js b/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-export/output.js new file mode 100644 index 000000000000..409526d3f92d --- /dev/null +++ b/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-export/output.js @@ -0,0 +1,20 @@ +"use strict"; + +Object.defineProperty(exports, "__esModule", { + value: true +}); +exports.foo = void 0; +let foo = 42; +exports.foo = foo; + +for (let _foo of [1, 2, 3]) { + exports.foo = foo = _foo +} + +for (let _foo2 of [1, 2, 3]) { + foo = _foo2 + // This loop body is transformed incorrectly + // correct output should be + // exports.foo = _foo2 + let foo = 3; +} From f51494d748971f681a4848bdff63baa68e029b9f Mon Sep 17 00:00:00 2001 From: vedantroy Date: Thu, 30 Jan 2020 14:46:19 -0500 Subject: [PATCH 02/13] Correctly transform non-destructured for of loops to update exported variables --- .../src/rewrite-live-references.js | 54 +++++++++++-------- 1 file changed, 32 insertions(+), 22 deletions(-) diff --git a/packages/babel-helper-module-transforms/src/rewrite-live-references.js b/packages/babel-helper-module-transforms/src/rewrite-live-references.js index f252502f5009..6b9a8a285201 100644 --- a/packages/babel-helper-module-transforms/src/rewrite-live-references.js +++ b/packages/babel-helper-module-transforms/src/rewrite-live-references.js @@ -308,30 +308,40 @@ const rewriteReferencesVisitor = { }, ForOfStatement(path) { const { scope, node } = path; - const { body } = node - const { exported } = this - - //TODO: Handle case where the loop identifier has the same name as the exported variable but is actually different - // Example: - // export let foo = 3; - // while(true) { - // let foo = 3; - // // don't transpile this - // for (foo of [1, 2, 3]) {} - // } - if (node.left.type === 'Identifier' && exported.get(node.left.name)) { - const oldLoopVarName = node.left.name - const newLoopVarId = scope.generateUidIdentifier(oldLoopVarName) - const assign = t.assignmentExpression('=', node.left, newLoopVarId) - const block = t.toBlock(body) - block.body.unshift(assign) + const { body } = node; + const { exported, scope: programScope, metadata } = this; + + if ( + t.isIdentifier(path.node.left) && + exported.get(node.left.name) && + // Ensure the exported variable is not re-declared in this scope + programScope.getBinding(path.node.left.name) === + scope.getBinding(path.node.left.name) + ) { + const oldLoopVarName = node.left.name; + const newLoopVarId = scope.generateUidIdentifier(oldLoopVarName); + let assignmentExpr; + if (path.get("body").scope.hasOwnBinding(oldLoopVarName)) { + // exported variable is re-declared in loop body, we need to manually build the exported assignment + assignmentExpr = buildBindingExportAssignmentExpression( + metadata, + exported.get(oldLoopVarName), + newLoopVarId, + ); + } else { + // transform for "(foo of []) {}" into "(_foo of []) {foo = _foo}", babel will handle exporting later + assignmentExpr = t.assignmentExpression("=", node.left, newLoopVarId); + } + + const block = t.toBlock(body); + block.body.unshift(assignmentExpr); path.replaceWith( t.forOfStatement( - t.variableDeclaration('let', [t.variableDeclarator(newLoopVarId)]), + t.variableDeclaration("let", [t.variableDeclarator(newLoopVarId)]), node.right, - block - ) - ) + block, + ), + ); } - } + }, }; From f61009c94e6da4089635d14aa039a794d35735a7 Mon Sep 17 00:00:00 2001 From: vedantroy Date: Thu, 30 Jan 2020 14:58:54 -0500 Subject: [PATCH 03/13] Add tests --- .../test/fixtures/misc/for-of-export/input.mjs | 14 +++++++------- .../test/fixtures/misc/for-of-export/output.js | 17 ++++++++++------- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-export/input.mjs b/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-export/input.mjs index 088c4500866e..17bd8291b7d6 100644 --- a/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-export/input.mjs +++ b/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-export/input.mjs @@ -1,12 +1,12 @@ -export let foo = 42; +export let foo; -for (foo of [1, 2, 3]) { +for (foo of []) {} +for (foo of []) { + let foo = 3; } -for (foo of [1, 2, 3]) { - // This loop body is transformed incorrectly - // correct output should be - // exports.foo = _foo2 +{ let foo = 3; -} + for (foo of []) {} +} \ No newline at end of file diff --git a/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-export/output.js b/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-export/output.js index 409526d3f92d..a12281430a1d 100644 --- a/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-export/output.js +++ b/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-export/output.js @@ -4,17 +4,20 @@ Object.defineProperty(exports, "__esModule", { value: true }); exports.foo = void 0; -let foo = 42; +let foo; exports.foo = foo; -for (let _foo of [1, 2, 3]) { +for (let _foo of []) { exports.foo = foo = _foo } -for (let _foo2 of [1, 2, 3]) { - foo = _foo2 - // This loop body is transformed incorrectly - // correct output should be - // exports.foo = _foo2 +for (let _foo2 of []) { + exports.foo = _foo2 let foo = 3; } + +{ + let foo = 3; + + for (foo of []) {} +} From 998a4771972a415706696ec87a6434cdba4a562c Mon Sep 17 00:00:00 2001 From: vedantroy Date: Thu, 30 Jan 2020 15:28:48 -0500 Subject: [PATCH 04/13] Don't replace entire for of loop --- .../src/rewrite-live-references.js | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/packages/babel-helper-module-transforms/src/rewrite-live-references.js b/packages/babel-helper-module-transforms/src/rewrite-live-references.js index 6b9a8a285201..9b0f30fb2ac3 100644 --- a/packages/babel-helper-module-transforms/src/rewrite-live-references.js +++ b/packages/babel-helper-module-transforms/src/rewrite-live-references.js @@ -308,7 +308,6 @@ const rewriteReferencesVisitor = { }, ForOfStatement(path) { const { scope, node } = path; - const { body } = node; const { exported, scope: programScope, metadata } = this; if ( @@ -333,15 +332,12 @@ const rewriteReferencesVisitor = { assignmentExpr = t.assignmentExpression("=", node.left, newLoopVarId); } - const block = t.toBlock(body); - block.body.unshift(assignmentExpr); - path.replaceWith( - t.forOfStatement( + path + .get("left") + .replaceWith( t.variableDeclaration("let", [t.variableDeclarator(newLoopVarId)]), - node.right, - block, - ), - ); + ); + path.get("body").unshiftContainer("body", assignmentExpr); } }, }; From ec006f4e14ad405f7e121e0ffbfcf8641dda2988 Mon Sep 17 00:00:00 2001 From: vedantroy Date: Thu, 30 Jan 2020 22:46:32 -0500 Subject: [PATCH 05/13] Correctly transform destructured for-of/for-in exports --- .../src/rewrite-live-references.js | 74 +++++++++++++------ .../fixtures/misc/for-of-in-export/input.mjs | 27 +++++++ .../fixtures/misc/for-of-in-export/output.js | 59 +++++++++++++++ 3 files changed, 139 insertions(+), 21 deletions(-) create mode 100644 packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/input.mjs create mode 100644 packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/output.js diff --git a/packages/babel-helper-module-transforms/src/rewrite-live-references.js b/packages/babel-helper-module-transforms/src/rewrite-live-references.js index 9b0f30fb2ac3..a60990416e64 100644 --- a/packages/babel-helper-module-transforms/src/rewrite-live-references.js +++ b/packages/babel-helper-module-transforms/src/rewrite-live-references.js @@ -306,38 +306,70 @@ const rewriteReferencesVisitor = { } }, }, - ForOfStatement(path) { + "ForOfStatement|ForInStatement"(path) { const { scope, node } = path; + const { left } = node; const { exported, scope: programScope, metadata } = this; - + let newLoopVarExpr; + const oldNameNewIdPairs = []; if ( - t.isIdentifier(path.node.left) && - exported.get(node.left.name) && + t.isIdentifier(left) && + exported.get(left.name) && // Ensure the exported variable is not re-declared in this scope - programScope.getBinding(path.node.left.name) === - scope.getBinding(path.node.left.name) + programScope.getBinding(left.name) === scope.getBinding(left.name) ) { - const oldLoopVarName = node.left.name; + const oldLoopVarName = left.name; const newLoopVarId = scope.generateUidIdentifier(oldLoopVarName); - let assignmentExpr; - if (path.get("body").scope.hasOwnBinding(oldLoopVarName)) { + newLoopVarExpr = newLoopVarId; + //TODO: is cloneNode needed? + oldNameNewIdPairs.push([oldLoopVarName, t.cloneNode(newLoopVarId)]); + } else if (t.isObjectPattern(left)) { + // TODO: Do I need to handle computed properties? + const newProps = []; + for (const prop of left.properties) { + if ( + t.isIdentifier(prop.key) && + exported.get(prop.key.name) && + programScope.getBinding(prop.key.name) === + scope.getBinding(prop.key.name) + ) { + const oldLoopVarName = prop.key.name; + const id = scope.generateUidIdentifier(oldLoopVarName); + newProps.push(t.objectProperty(t.identifier(oldLoopVarName), id)); + //TODO: is cloneNode needed? + oldNameNewIdPairs.push([oldLoopVarName, t.cloneNode(id)]); + } else { + newProps.push(prop); + } + } + const newObjectPattern = t.objectPattern(newProps); + newLoopVarExpr = newObjectPattern; + } else { + return; + } + path + .get("left") + .replaceWith( + t.variableDeclaration("let", [t.variableDeclarator(newLoopVarExpr)]), + ); + const assignExprs = []; + for (const [name, newId] of oldNameNewIdPairs) { + if (path.get("body").scope.hasOwnBinding(name)) { // exported variable is re-declared in loop body, we need to manually build the exported assignment - assignmentExpr = buildBindingExportAssignmentExpression( - metadata, - exported.get(oldLoopVarName), - newLoopVarId, + assignExprs.push( + buildBindingExportAssignmentExpression( + metadata, + exported.get(name), + newId, + ), ); } else { // transform for "(foo of []) {}" into "(_foo of []) {foo = _foo}", babel will handle exporting later - assignmentExpr = t.assignmentExpression("=", node.left, newLoopVarId); - } - - path - .get("left") - .replaceWith( - t.variableDeclaration("let", [t.variableDeclarator(newLoopVarId)]), + assignExprs.push( + t.assignmentExpression("=", t.identifier(name), newId), ); - path.get("body").unshiftContainer("body", assignmentExpr); + } } + path.get("body").unshiftContainer("body", assignExprs); }, }; diff --git a/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/input.mjs b/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/input.mjs new file mode 100644 index 000000000000..b3e8732b9854 --- /dev/null +++ b/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/input.mjs @@ -0,0 +1,27 @@ +export let foo; +export let bar; + +for (foo of []) {} + +for (foo in {}) {} + +for ({foo} of {}) {} + +for ({foo} of {}) { + let foo = 3; +} + +for ({foo, bar} of {}) {} + +for ({foo, bar} of {}) { + let bar = 3; +} + +for (foo of []) { + let foo = 3; +} + +{ + let foo = 3; + for (foo of []) {} +} \ No newline at end of file diff --git a/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/output.js b/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/output.js new file mode 100644 index 000000000000..503814fdc7ec --- /dev/null +++ b/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/output.js @@ -0,0 +1,59 @@ +"use strict"; + +Object.defineProperty(exports, "__esModule", { + value: true +}); +exports.bar = exports.foo = void 0; +let foo; +exports.foo = foo; +let bar; +exports.bar = bar; + +for (let _foo of []) { + exports.foo = foo = _foo +} + +for (let _foo2 in {}) { + exports.foo = foo = _foo2 +} + +for (let { + foo: _foo3 +} of {}) { + exports.foo = foo = _foo3 +} + +for (let { + foo: _foo4 +} of {}) { + exports.foo = _foo4 + let foo = 3; +} + +for (let { + foo: _foo5, + bar: _bar +} of {}) { + exports.foo = foo = _foo5 + exports.bar = bar = _bar +} + +for (let { + foo: _foo6, + bar: _bar2 +} of {}) { + exports.foo = foo = _foo6 + exports.bar = _bar2 + let bar = 3; +} + +for (let _foo7 of []) { + exports.foo = _foo7 + let foo = 3; +} + +{ + let foo = 3; + + for (foo of []) {} +} From 7be543a38bfbe0838c76cb9c2cddf3fabc8e96c8 Mon Sep 17 00:00:00 2001 From: vedantroy Date: Fri, 31 Jan 2020 01:53:18 -0500 Subject: [PATCH 06/13] Update exported variables in array pattern and more fixes --- .../src/rewrite-live-references.js | 77 ++++++++++++++++--- .../fixtures/misc/for-of-export/input.mjs | 12 --- .../fixtures/misc/for-of-export/output.js | 23 ------ 3 files changed, 68 insertions(+), 44 deletions(-) delete mode 100644 packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-export/input.mjs delete mode 100644 packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-export/output.js diff --git a/packages/babel-helper-module-transforms/src/rewrite-live-references.js b/packages/babel-helper-module-transforms/src/rewrite-live-references.js index a60990416e64..feb4bce98317 100644 --- a/packages/babel-helper-module-transforms/src/rewrite-live-references.js +++ b/packages/babel-helper-module-transforms/src/rewrite-live-references.js @@ -310,7 +310,6 @@ const rewriteReferencesVisitor = { const { scope, node } = path; const { left } = node; const { exported, scope: programScope, metadata } = this; - let newLoopVarExpr; const oldNameNewIdPairs = []; if ( t.isIdentifier(left) && @@ -320,11 +319,16 @@ const rewriteReferencesVisitor = { ) { const oldLoopVarName = left.name; const newLoopVarId = scope.generateUidIdentifier(oldLoopVarName); - newLoopVarExpr = newLoopVarId; //TODO: is cloneNode needed? oldNameNewIdPairs.push([oldLoopVarName, t.cloneNode(newLoopVarId)]); + path + .get("left") + .replaceWith( + t.variableDeclaration("let", [t.variableDeclarator(newLoopVarId)]), + ); } else if (t.isObjectPattern(left)) { // TODO: Do I need to handle computed properties? + let didTransform = false; const newProps = []; for (const prop of left.properties) { if ( @@ -333,29 +337,43 @@ const rewriteReferencesVisitor = { programScope.getBinding(prop.key.name) === scope.getBinding(prop.key.name) ) { + didTransform = true; const oldLoopVarName = prop.key.name; const id = scope.generateUidIdentifier(oldLoopVarName); newProps.push(t.objectProperty(t.identifier(oldLoopVarName), id)); //TODO: is cloneNode needed? oldNameNewIdPairs.push([oldLoopVarName, t.cloneNode(id)]); + // necessary so "for ({x : _x} of []) {}" doesn't throw "_x" undefined error + path.insertBefore( + t.variableDeclaration("let", [t.variableDeclarator(id)]), + ); } else { newProps.push(prop); } } - const newObjectPattern = t.objectPattern(newProps); - newLoopVarExpr = newObjectPattern; + if (!didTransform) { + return; + } + path.get("left").replaceWith(t.objectPattern(newProps)); + } else if (t.isArrayPattern(left)) { + const renamedVars = new Map(); + // array patterns are tricky since "[foo, [foo, ...foo]]" is valid + // and will update "foo", if it is exported. To solve this, we have a separate recursive function. + transformArrayPattern(left, scope, programScope, exported, renamedVars); + for (const [oldName, newId] of renamedVars.entries()) { + path.insertBefore( + t.variableDeclaration("let", [t.variableDeclarator(newId)]), + ); + oldNameNewIdPairs.push([oldName, newId]); + } } else { return; } - path - .get("left") - .replaceWith( - t.variableDeclaration("let", [t.variableDeclarator(newLoopVarExpr)]), - ); const assignExprs = []; for (const [name, newId] of oldNameNewIdPairs) { if (path.get("body").scope.hasOwnBinding(name)) { // exported variable is re-declared in loop body, we need to manually build the exported assignment + // example: for(foo of []) { let foo = 42 } assignExprs.push( buildBindingExportAssignmentExpression( metadata, @@ -373,3 +391,44 @@ const rewriteReferencesVisitor = { path.get("body").unshiftContainer("body", assignExprs); }, }; + +function transformArrayPattern( + pattern, + scope, + programScope, + exported, + renamedVars, // example: for the pattern [foo, [foo, ...foo]], we want to rename each "foo" to "_foo" + // key: the original name "foo", value: identifier object with name "_foo" +) { + for (const element of pattern.elements) { + if (t.isArrayPattern(element)) { + transformArrayPattern( + element, + scope, + programScope, + exported, + renamedVars, + ); + } else { + const base = t.isIdentifier(element) + ? element + : t.isRestElement(element) + ? element.argument + : null; + if (base === null) { + continue; + } + const oldName = base.name; + if (renamedVars.get(oldName)) { + base.name = renamedVars.get(oldName).name; + } else if ( + exported.get(oldName) && + programScope.getBinding(oldName) === scope.getBinding(oldName) + ) { + const id = scope.generateUidIdentifier(oldName); + base.name = id.name; + renamedVars.set(oldName, id); + } + } + } +} diff --git a/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-export/input.mjs b/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-export/input.mjs deleted file mode 100644 index 17bd8291b7d6..000000000000 --- a/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-export/input.mjs +++ /dev/null @@ -1,12 +0,0 @@ -export let foo; - -for (foo of []) {} - -for (foo of []) { - let foo = 3; -} - -{ - let foo = 3; - for (foo of []) {} -} \ No newline at end of file diff --git a/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-export/output.js b/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-export/output.js deleted file mode 100644 index a12281430a1d..000000000000 --- a/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-export/output.js +++ /dev/null @@ -1,23 +0,0 @@ -"use strict"; - -Object.defineProperty(exports, "__esModule", { - value: true -}); -exports.foo = void 0; -let foo; -exports.foo = foo; - -for (let _foo of []) { - exports.foo = foo = _foo -} - -for (let _foo2 of []) { - exports.foo = _foo2 - let foo = 3; -} - -{ - let foo = 3; - - for (foo of []) {} -} From 5bdbe2877b5895c3bf7f88f12dcc29cdd493ef08 Mon Sep 17 00:00:00 2001 From: vedantroy Date: Fri, 31 Jan 2020 02:04:28 -0500 Subject: [PATCH 07/13] Refresh test output --- .../fixtures/misc/for-of-in-export/output.js | 67 ++++++++++--------- 1 file changed, 35 insertions(+), 32 deletions(-) diff --git a/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/output.js b/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/output.js index 503814fdc7ec..5ba89ae78f0b 100644 --- a/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/output.js +++ b/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/output.js @@ -5,55 +5,58 @@ Object.defineProperty(exports, "__esModule", { }); exports.bar = exports.foo = void 0; let foo; -exports.foo = foo; -let bar; -exports.bar = bar; +exports.bar = exports.foo = foo; for (let _foo of []) { - exports.foo = foo = _foo + exports.bar = exports.foo = foo = _foo } -for (let _foo2 in {}) { - exports.foo = foo = _foo2 +for (let _foo2 in []) { + exports.bar = exports.foo = foo = _foo2 } -for (let { - foo: _foo3 -} of {}) { - exports.foo = foo = _foo3 +for (let _foo3 of []) { + exports.bar = exports.foo = _foo3 + let foo = 42; } -for (let { +let _foo4; + +for ({ foo: _foo4 -} of {}) { - exports.foo = _foo4 - let foo = 3; +} of []) { + exports.bar = exports.foo = foo = _foo4 } -for (let { +let qux; + +let _foo5; + +for ({ foo: _foo5, - bar: _bar -} of {}) { - exports.foo = foo = _foo5 - exports.bar = bar = _bar + bar, + qux +} of []) { + exports.bar = exports.foo = foo = _foo5 } -for (let { - foo: _foo6, - bar: _bar2 -} of {}) { - exports.foo = foo = _foo6 - exports.bar = _bar2 - let bar = 3; +{ + let foo; + + for (foo of []) {} + + for ([foo] of []) {} } -for (let _foo7 of []) { - exports.foo = _foo7 - let foo = 3; +let _foo6; + +for ([_foo6, qux, [_foo6, ..._foo6]] of []) { + exports.bar = exports.foo = foo = _foo6 } -{ - let foo = 3; +let _foo7; - for (foo of []) {} +for ([_foo7, qux, [_foo7, ..._foo7]] of []) { + exports.bar = exports.foo = _foo7 + let foo = 42; } From 794f6f3e53be72601b62e4748ab02b02b4e1a8cf Mon Sep 17 00:00:00 2001 From: vedantroy Date: Fri, 31 Jan 2020 13:53:09 -0500 Subject: [PATCH 08/13] Update tests and rebase on master --- .../fixtures/misc/for-of-in-export/input.mjs | 28 +++++----- .../fixtures/misc/for-of-in-export/output.js | 51 ++++++++++--------- 2 files changed, 42 insertions(+), 37 deletions(-) diff --git a/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/input.mjs b/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/input.mjs index b3e8732b9854..bcaa7e9992df 100644 --- a/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/input.mjs +++ b/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/input.mjs @@ -1,27 +1,31 @@ export let foo; -export let bar; +export {foo as bar} for (foo of []) {} -for (foo in {}) {} - -for ({foo} of {}) {} - -for ({foo} of {}) { +for (foo of []) { let foo = 3; } -for ({foo, bar} of {}) {} +for (foo of []) { + foo = 3; +} + +for (foo in {}) {} + +for ({foo} of {}) {} for ({foo, bar} of {}) { let bar = 3; } -for (foo of []) { - let foo = 3; -} - { let foo = 3; for (foo of []) {} -} \ No newline at end of file +} + +let qux; + +for ([foo] of []) {} + +for ([foo, qux, [...foo]] of []) {} \ No newline at end of file diff --git a/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/output.js b/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/output.js index 5ba89ae78f0b..d523b419df92 100644 --- a/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/output.js +++ b/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/output.js @@ -11,52 +11,53 @@ for (let _foo of []) { exports.bar = exports.foo = foo = _foo } -for (let _foo2 in []) { - exports.bar = exports.foo = foo = _foo2 +for (let _foo2 of []) { + exports.bar = exports.foo = _foo2 + let foo = 3; } for (let _foo3 of []) { - exports.bar = exports.foo = _foo3 - let foo = 42; + exports.bar = exports.foo = foo = _foo3 + exports.bar = exports.foo = foo = 3; } -let _foo4; - -for ({ - foo: _foo4 -} of []) { +for (let _foo4 in {}) { exports.bar = exports.foo = foo = _foo4 } -let qux; - let _foo5; for ({ - foo: _foo5, - bar, - qux -} of []) { + foo: _foo5 +} of {}) { exports.bar = exports.foo = foo = _foo5 } +let _foo6; + +for ({ + foo: _foo6, + bar +} of {}) { + exports.bar = exports.foo = foo = _foo6 + let bar = 3; +} + { - let foo; + let foo = 3; for (foo of []) {} - - for ([foo] of []) {} } +let qux; -let _foo6; +let _foo7; -for ([_foo6, qux, [_foo6, ..._foo6]] of []) { - exports.bar = exports.foo = foo = _foo6 +for ([_foo7] of []) { + exports.bar = exports.foo = foo = _foo7 } -let _foo7; +let _foo8; -for ([_foo7, qux, [_foo7, ..._foo7]] of []) { - exports.bar = exports.foo = _foo7 - let foo = 42; +for ([_foo8, qux, [..._foo8]] of []) { + exports.bar = exports.foo = foo = _foo8 } From 949d2f60f1288ac0d866b89f6fbd3620733ec516 Mon Sep 17 00:00:00 2001 From: vedantroy Date: Fri, 31 Jan 2020 16:03:08 -0500 Subject: [PATCH 09/13] Refactor ForOf|ForIn visitor --- .../src/rewrite-live-references.js | 137 +++--------------- .../fixtures/misc/for-of-in-export/input.mjs | 30 ++-- .../fixtures/misc/for-of-in-export/output.js | 81 ++++++----- 3 files changed, 80 insertions(+), 168 deletions(-) diff --git a/packages/babel-helper-module-transforms/src/rewrite-live-references.js b/packages/babel-helper-module-transforms/src/rewrite-live-references.js index feb4bce98317..6af0018eb1bf 100644 --- a/packages/babel-helper-module-transforms/src/rewrite-live-references.js +++ b/packages/babel-helper-module-transforms/src/rewrite-live-references.js @@ -309,126 +309,33 @@ const rewriteReferencesVisitor = { "ForOfStatement|ForInStatement"(path) { const { scope, node } = path; const { left } = node; - const { exported, scope: programScope, metadata } = this; - const oldNameNewIdPairs = []; - if ( - t.isIdentifier(left) && - exported.get(left.name) && - // Ensure the exported variable is not re-declared in this scope - programScope.getBinding(left.name) === scope.getBinding(left.name) - ) { - const oldLoopVarName = left.name; - const newLoopVarId = scope.generateUidIdentifier(oldLoopVarName); - //TODO: is cloneNode needed? - oldNameNewIdPairs.push([oldLoopVarName, t.cloneNode(newLoopVarId)]); - path - .get("left") - .replaceWith( - t.variableDeclaration("let", [t.variableDeclarator(newLoopVarId)]), - ); - } else if (t.isObjectPattern(left)) { - // TODO: Do I need to handle computed properties? - let didTransform = false; - const newProps = []; - for (const prop of left.properties) { + const { exported, scope: programScope } = this; + + // if it's a variable declaration, such as for (let {foo} of []) {} + // then no transformation is needed + if (!t.isVariableDeclaration(left)) { + const bodyPath = path.get("body"); + const loopBodyScope = bodyPath.scope; + for (const name of Object.keys(t.getOuterBindingIdentifiers(left))) { if ( - t.isIdentifier(prop.key) && - exported.get(prop.key.name) && - programScope.getBinding(prop.key.name) === - scope.getBinding(prop.key.name) + exported.get(name) && + programScope.getBinding(name) === scope.getBinding(name) ) { - didTransform = true; - const oldLoopVarName = prop.key.name; - const id = scope.generateUidIdentifier(oldLoopVarName); - newProps.push(t.objectProperty(t.identifier(oldLoopVarName), id)); - //TODO: is cloneNode needed? - oldNameNewIdPairs.push([oldLoopVarName, t.cloneNode(id)]); - // necessary so "for ({x : _x} of []) {}" doesn't throw "_x" undefined error - path.insertBefore( - t.variableDeclaration("let", [t.variableDeclarator(id)]), - ); - } else { - newProps.push(prop); + if (loopBodyScope.hasOwnBinding(name)) { + loopBodyScope.rename(name); + } } } - if (!didTransform) { - return; - } - path.get("left").replaceWith(t.objectPattern(newProps)); - } else if (t.isArrayPattern(left)) { - const renamedVars = new Map(); - // array patterns are tricky since "[foo, [foo, ...foo]]" is valid - // and will update "foo", if it is exported. To solve this, we have a separate recursive function. - transformArrayPattern(left, scope, programScope, exported, renamedVars); - for (const [oldName, newId] of renamedVars.entries()) { - path.insertBefore( - t.variableDeclaration("let", [t.variableDeclarator(newId)]), - ); - oldNameNewIdPairs.push([oldName, newId]); - } - } else { - return; - } - const assignExprs = []; - for (const [name, newId] of oldNameNewIdPairs) { - if (path.get("body").scope.hasOwnBinding(name)) { - // exported variable is re-declared in loop body, we need to manually build the exported assignment - // example: for(foo of []) { let foo = 42 } - assignExprs.push( - buildBindingExportAssignmentExpression( - metadata, - exported.get(name), - newId, - ), - ); - } else { - // transform for "(foo of []) {}" into "(_foo of []) {foo = _foo}", babel will handle exporting later - assignExprs.push( - t.assignmentExpression("=", t.identifier(name), newId), + const newLoopId = scope.generateUidIdentifier(); + bodyPath.unshiftContainer( + "body", + t.expressionStatement(t.assignmentExpression("=", left, newLoopId)), + ); + path + .get("left") + .replaceWith( + t.variableDeclaration("let", [t.variableDeclarator(newLoopId)]), ); - } } - path.get("body").unshiftContainer("body", assignExprs); }, }; - -function transformArrayPattern( - pattern, - scope, - programScope, - exported, - renamedVars, // example: for the pattern [foo, [foo, ...foo]], we want to rename each "foo" to "_foo" - // key: the original name "foo", value: identifier object with name "_foo" -) { - for (const element of pattern.elements) { - if (t.isArrayPattern(element)) { - transformArrayPattern( - element, - scope, - programScope, - exported, - renamedVars, - ); - } else { - const base = t.isIdentifier(element) - ? element - : t.isRestElement(element) - ? element.argument - : null; - if (base === null) { - continue; - } - const oldName = base.name; - if (renamedVars.get(oldName)) { - base.name = renamedVars.get(oldName).name; - } else if ( - exported.get(oldName) && - programScope.getBinding(oldName) === scope.getBinding(oldName) - ) { - const id = scope.generateUidIdentifier(oldName); - base.name = id.name; - renamedVars.set(oldName, id); - } - } - } -} diff --git a/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/input.mjs b/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/input.mjs index bcaa7e9992df..a0e59f691add 100644 --- a/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/input.mjs +++ b/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/input.mjs @@ -3,29 +3,23 @@ export {foo as bar} for (foo of []) {} -for (foo of []) { - let foo = 3; -} for (foo of []) { - foo = 3; -} - -for (foo in {}) {} - -for ({foo} of {}) {} - -for ({foo, bar} of {}) { - let bar = 3; + let foo; } -{ - let foo = 3; - for (foo of []) {} +for ({foo} of []) {} +for ({test: {foo}} of []) { + let foo; } +for ({foo: bar} of []) {} let qux; +for([foo, [...foo], qux] of []) {} -for ([foo] of []) {} - -for ([foo, qux, [...foo]] of []) {} \ No newline at end of file +{ + for ({foo} of []) {} + for({test: {foo}, qux} of []) {} + let foo; + for({foo} of []) {} +} diff --git a/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/output.js b/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/output.js index d523b419df92..5376df4974e2 100644 --- a/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/output.js +++ b/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/output.js @@ -7,57 +7,68 @@ exports.bar = exports.foo = void 0; let foo; exports.bar = exports.foo = foo; -for (let _foo of []) { - exports.bar = exports.foo = foo = _foo +for (let _temp of []) { + exports.bar = exports.foo = foo = _temp; } -for (let _foo2 of []) { - exports.bar = exports.foo = _foo2 - let foo = 3; -} +for (let _temp2 of []) { + exports.bar = exports.foo = foo = _temp2; -for (let _foo3 of []) { - exports.bar = exports.foo = foo = _foo3 - exports.bar = exports.foo = foo = 3; + let _foo; } -for (let _foo4 in {}) { - exports.bar = exports.foo = foo = _foo4 +for (let _temp3 of []) { + ({ + foo + } = _temp3); + exports.bar = exports.foo = foo; } -let _foo5; +for (let _temp4 of []) { + ({ + test: { + foo + } + } = _temp4); + exports.bar = exports.foo = foo; -for ({ - foo: _foo5 -} of {}) { - exports.bar = exports.foo = foo = _foo5 + let _foo2; } -let _foo6; - -for ({ - foo: _foo6, - bar -} of {}) { - exports.bar = exports.foo = foo = _foo6 - let bar = 3; +for (let _temp5 of []) { + ({ + foo: bar + } = _temp5); } -{ - let foo = 3; +let qux; - for (foo of []) {} +for (let _temp6 of []) { + [foo, [...foo], qux] = _temp6; + exports.bar = exports.foo = foo; } -let qux; -let _foo7; +{ + for (let _temp7 of []) { + ({ + foo + } = _temp7); + } -for ([_foo7] of []) { - exports.bar = exports.foo = foo = _foo7 -} + for (let _temp8 of []) { + ({ + test: { + foo + }, + qux + } = _temp8); + } -let _foo8; + let foo; -for ([_foo8, qux, [..._foo8]] of []) { - exports.bar = exports.foo = foo = _foo8 + for (let _temp9 of []) { + ({ + foo + } = _temp9); + } } From 468e9edcd73ff68044e9c38050c66f9673da444d Mon Sep 17 00:00:00 2001 From: vedantroy Date: Fri, 31 Jan 2020 16:13:20 -0500 Subject: [PATCH 10/13] Don't transform re-declared exported vars --- .../src/rewrite-live-references.js | 5 ++ .../fixtures/misc/for-of-in-export/input.mjs | 21 +++---- .../fixtures/misc/for-of-in-export/output.js | 59 ++++++++----------- 3 files changed, 39 insertions(+), 46 deletions(-) diff --git a/packages/babel-helper-module-transforms/src/rewrite-live-references.js b/packages/babel-helper-module-transforms/src/rewrite-live-references.js index 6af0018eb1bf..c2f31113c2bd 100644 --- a/packages/babel-helper-module-transforms/src/rewrite-live-references.js +++ b/packages/babel-helper-module-transforms/src/rewrite-live-references.js @@ -314,6 +314,7 @@ const rewriteReferencesVisitor = { // if it's a variable declaration, such as for (let {foo} of []) {} // then no transformation is needed if (!t.isVariableDeclaration(left)) { + let didTransform = false; const bodyPath = path.get("body"); const loopBodyScope = bodyPath.scope; for (const name of Object.keys(t.getOuterBindingIdentifiers(left))) { @@ -321,11 +322,15 @@ const rewriteReferencesVisitor = { exported.get(name) && programScope.getBinding(name) === scope.getBinding(name) ) { + didTransform = true; if (loopBodyScope.hasOwnBinding(name)) { loopBodyScope.rename(name); } } } + if (!didTransform) { + return; + } const newLoopId = scope.generateUidIdentifier(); bodyPath.unshiftContainer( "body", diff --git a/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/input.mjs b/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/input.mjs index a0e59f691add..fe09208eea2c 100644 --- a/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/input.mjs +++ b/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/input.mjs @@ -2,24 +2,21 @@ export let foo; export {foo as bar} for (foo of []) {} - - +for (foo in []) {} for (foo of []) { let foo; } - for ({foo} of []) {} -for ({test: {foo}} of []) { +for ({foo} of []) { + let foo; +} +for ({test: {foo}} of []) {} +for ([foo, [...foo]] of []) {} +for ([foo, [...foo]] of []) { let foo; } -for ({foo: bar} of []) {} - -let qux; -for([foo, [...foo], qux] of []) {} { - for ({foo} of []) {} - for({test: {foo}, qux} of []) {} let foo; - for({foo} of []) {} -} + for(foo of []) {} +} \ No newline at end of file diff --git a/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/output.js b/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/output.js index 5376df4974e2..05dbbd6cc0f0 100644 --- a/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/output.js +++ b/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/output.js @@ -11,64 +11,55 @@ for (let _temp of []) { exports.bar = exports.foo = foo = _temp; } -for (let _temp2 of []) { +for (let _temp2 in []) { exports.bar = exports.foo = foo = _temp2; +} + +for (let _temp3 of []) { + exports.bar = exports.foo = foo = _temp3; let _foo; } -for (let _temp3 of []) { +for (let _temp4 of []) { ({ foo - } = _temp3); + } = _temp4); exports.bar = exports.foo = foo; } -for (let _temp4 of []) { +for (let _temp5 of []) { ({ - test: { - foo - } - } = _temp4); + foo + } = _temp5); exports.bar = exports.foo = foo; let _foo2; } -for (let _temp5 of []) { +for (let _temp6 of []) { ({ - foo: bar - } = _temp5); + test: { + foo + } + } = _temp6); + exports.bar = exports.foo = foo; } -let qux; - -for (let _temp6 of []) { - [foo, [...foo], qux] = _temp6; +for (let _temp7 of []) { + [foo, [...foo]] = _temp7; exports.bar = exports.foo = foo; } -{ - for (let _temp7 of []) { - ({ - foo - } = _temp7); - } +for (let _temp8 of []) { + [foo, [...foo]] = _temp8; + exports.bar = exports.foo = foo; - for (let _temp8 of []) { - ({ - test: { - foo - }, - qux - } = _temp8); - } + let _foo3; +} +{ let foo; - for (let _temp9 of []) { - ({ - foo - } = _temp9); - } + for (foo of []) {} } From be442d98794687d92ba53f2ec5e362d93b55cb8c Mon Sep 17 00:00:00 2001 From: Vedant Roy Date: Fri, 31 Jan 2020 21:46:17 -0500 Subject: [PATCH 11/13] Generate better name for loop id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Nicolò Ribaudo --- .../src/rewrite-live-references.js | 2 +- .../fixtures/misc/for-of-in-export/output.js | 38 +++++++++---------- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/packages/babel-helper-module-transforms/src/rewrite-live-references.js b/packages/babel-helper-module-transforms/src/rewrite-live-references.js index c2f31113c2bd..a39f02823239 100644 --- a/packages/babel-helper-module-transforms/src/rewrite-live-references.js +++ b/packages/babel-helper-module-transforms/src/rewrite-live-references.js @@ -331,7 +331,7 @@ const rewriteReferencesVisitor = { if (!didTransform) { return; } - const newLoopId = scope.generateUidIdentifier(); + const newLoopId = t.identifier(scope.generateUidBasedOnNode(left)); bodyPath.unshiftContainer( "body", t.expressionStatement(t.assignmentExpression("=", left, newLoopId)), diff --git a/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/output.js b/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/output.js index 05dbbd6cc0f0..bb8330350160 100644 --- a/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/output.js +++ b/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/for-of-in-export/output.js @@ -7,55 +7,55 @@ exports.bar = exports.foo = void 0; let foo; exports.bar = exports.foo = foo; -for (let _temp of []) { - exports.bar = exports.foo = foo = _temp; +for (let _foo of []) { + exports.bar = exports.foo = foo = _foo; } -for (let _temp2 in []) { - exports.bar = exports.foo = foo = _temp2; +for (let _foo2 in []) { + exports.bar = exports.foo = foo = _foo2; } -for (let _temp3 of []) { - exports.bar = exports.foo = foo = _temp3; +for (let _foo4 of []) { + exports.bar = exports.foo = foo = _foo4; - let _foo; + let _foo3; } -for (let _temp4 of []) { +for (let _foo5 of []) { ({ foo - } = _temp4); + } = _foo5); exports.bar = exports.foo = foo; } -for (let _temp5 of []) { +for (let _foo7 of []) { ({ foo - } = _temp5); + } = _foo7); exports.bar = exports.foo = foo; - let _foo2; + let _foo6; } -for (let _temp6 of []) { +for (let _test of []) { ({ test: { foo } - } = _temp6); + } = _test); exports.bar = exports.foo = foo; } -for (let _temp7 of []) { - [foo, [...foo]] = _temp7; +for (let _ref of []) { + [foo, [...foo]] = _ref; exports.bar = exports.foo = foo; } -for (let _temp8 of []) { - [foo, [...foo]] = _temp8; +for (let _ref2 of []) { + [foo, [...foo]] = _ref2; exports.bar = exports.foo = foo; - let _foo3; + let _foo8; } { From 53fad8e6365475006a041fbc422296a69a417f45 Mon Sep 17 00:00:00 2001 From: vedantroy Date: Tue, 4 Feb 2020 16:24:25 -0500 Subject: [PATCH 12/13] Idiomatically generate UidIdentifier --- .../src/rewrite-live-references.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/babel-helper-module-transforms/src/rewrite-live-references.js b/packages/babel-helper-module-transforms/src/rewrite-live-references.js index a39f02823239..e498d112ca21 100644 --- a/packages/babel-helper-module-transforms/src/rewrite-live-references.js +++ b/packages/babel-helper-module-transforms/src/rewrite-live-references.js @@ -311,8 +311,6 @@ const rewriteReferencesVisitor = { const { left } = node; const { exported, scope: programScope } = this; - // if it's a variable declaration, such as for (let {foo} of []) {} - // then no transformation is needed if (!t.isVariableDeclaration(left)) { let didTransform = false; const bodyPath = path.get("body"); @@ -331,7 +329,7 @@ const rewriteReferencesVisitor = { if (!didTransform) { return; } - const newLoopId = t.identifier(scope.generateUidBasedOnNode(left)); + const newLoopId = scope.generateUidIdentifierBasedOnNode(left); bodyPath.unshiftContainer( "body", t.expressionStatement(t.assignmentExpression("=", left, newLoopId)), From 14b8c14125a894ead698b0a9c136aa249afed005 Mon Sep 17 00:00:00 2001 From: vedantroy Date: Tue, 4 Feb 2020 16:51:25 -0500 Subject: [PATCH 13/13] Update scope after replacing loop declaration --- .../src/rewrite-live-references.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/babel-helper-module-transforms/src/rewrite-live-references.js b/packages/babel-helper-module-transforms/src/rewrite-live-references.js index e498d112ca21..b98953f70662 100644 --- a/packages/babel-helper-module-transforms/src/rewrite-live-references.js +++ b/packages/babel-helper-module-transforms/src/rewrite-live-references.js @@ -339,6 +339,7 @@ const rewriteReferencesVisitor = { .replaceWith( t.variableDeclaration("let", [t.variableDeclarator(newLoopId)]), ); + scope.registerDeclaration(path.get("left")); } }, };