From 14d3c2e256246c306d916a171c2d1301c9b7989c Mon Sep 17 00:00:00 2001 From: Ben Alpert Date: Mon, 13 Feb 2017 13:46:00 -0800 Subject: [PATCH] Avoid adding unnecessary closure for block scoping (#5246) When you write ``` for (const x of l) { setTimeout(() => x); } ``` we need to add a closure because the variable is meant to be block-scoped and recreated each time the block runs. We do this. However, we also add the closure when no loop is present. This isn't necessary, because if no loop is present then each piece of code runs at most once. I changed the transform to only add a closure if a variable is referenced from within a loop. --- .../misc/regression-2364/actual.js | 2 +- .../misc/regression-2364/expected.js | 22 ++++---- .../src/index.js | 27 +++++++++- .../fixtures/general/issue-2174/expected.js | 14 +++--- .../general/loops-and-no-loops/actual.js | 34 +++++++++++++ .../general/loops-and-no-loops/expected.js | 42 ++++++++++++++++ .../fixtures/general/sibling-scopes/actual.js | 11 ++++ .../general/sibling-scopes/expected.js | 15 ++++++ .../fixtures/general/superswitch/actual.js | 24 +++++---- .../fixtures/general/superswitch/expected.js | 50 ++++++++++--------- .../general/switch-callbacks/actual.js | 14 +++--- .../general/switch-callbacks/expected.js | 26 +++++----- .../for-const-closure/expected.js | 6 +++ .../superswitch/actual.js | 16 ------ .../superswitch/options.json | 3 -- .../expected.js | 14 ++---- 16 files changed, 217 insertions(+), 103 deletions(-) create mode 100644 packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/loops-and-no-loops/actual.js create mode 100644 packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/loops-and-no-loops/expected.js create mode 100644 packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/sibling-scopes/actual.js create mode 100644 packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/sibling-scopes/expected.js create mode 100644 packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/throwIfClosureRequired/for-const-closure/expected.js delete mode 100644 packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/throwIfClosureRequired/superswitch/actual.js delete mode 100644 packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/throwIfClosureRequired/superswitch/options.json diff --git a/packages/babel-core/test/fixtures/transformation/misc/regression-2364/actual.js b/packages/babel-core/test/fixtures/transformation/misc/regression-2364/actual.js index e995e0d234..c2ae386b34 100644 --- a/packages/babel-core/test/fixtures/transformation/misc/regression-2364/actual.js +++ b/packages/babel-core/test/fixtures/transformation/misc/regression-2364/actual.js @@ -1,6 +1,6 @@ function wrapper(fn) { return (...args) => { - if (someCondition) { + while (someCondition) { const val = fn(...args); return val.test(() => { console.log(val); diff --git a/packages/babel-core/test/fixtures/transformation/misc/regression-2364/expected.js b/packages/babel-core/test/fixtures/transformation/misc/regression-2364/expected.js index ae32dbb4d3..c1079c01cb 100644 --- a/packages/babel-core/test/fixtures/transformation/misc/regression-2364/expected.js +++ b/packages/babel-core/test/fixtures/transformation/misc/regression-2364/expected.js @@ -2,17 +2,19 @@ function wrapper(fn) { return function () { var _arguments = arguments; - if (someCondition) { - var _ret = function () { - var val = fn(..._arguments); - return { - v: val.test(function () { - console.log(val); - }) - }; - }(); + var _loop = function () { + var val = fn(..._arguments); + return { + v: val.test(function () { + console.log(val); + }) + }; + }; + + while (someCondition) { + var _ret = _loop(); if (typeof _ret === "object") return _ret.v; } }; -} \ No newline at end of file +} diff --git a/packages/babel-plugin-transform-es2015-block-scoping/src/index.js b/packages/babel-plugin-transform-es2015-block-scoping/src/index.js index b1227d3a52..f633054622 100644 --- a/packages/babel-plugin-transform-es2015-block-scoping/src/index.js +++ b/packages/babel-plugin-transform-es2015-block-scoping/src/index.js @@ -112,8 +112,21 @@ function isVar(node) { } const letReferenceBlockVisitor = traverse.visitors.merge([{ + Loop: { + enter(path, state) { + state.loopDepth++; + }, + exit(path, state) { + state.loopDepth--; + }, + }, Function(path, state) { - path.traverse(letReferenceFunctionVisitor, state); + // References to block-scoped variables only require added closures if it's + // possible for the code to run more than once -- otherwise it is safe to + // simply rename the variables. + if (state.loopDepth > 0) { + path.traverse(letReferenceFunctionVisitor, state); + } return path.skip(); } }, tdzVisitor]); @@ -549,9 +562,19 @@ class BlockScoping { const state = { letReferences: this.letReferences, closurify: false, - file: this.file + file: this.file, + loopDepth: 0, }; + const loopOrFunctionParent = this.blockPath.find( + (path) => path.isLoop() || path.isFunction() + ); + if (loopOrFunctionParent && loopOrFunctionParent.isLoop()) { + // There is a loop ancestor closer than the closest function, so we + // consider ourselves to be in a loop. + state.loopDepth++; + } + // traverse through this block, stopping on functions and checking if they // contain any local let references this.blockPath.traverse(letReferenceBlockVisitor, state); diff --git a/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/issue-2174/expected.js b/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/issue-2174/expected.js index 1dce9c4c17..9efbc1091f 100644 --- a/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/issue-2174/expected.js +++ b/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/issue-2174/expected.js @@ -1,11 +1,9 @@ if (true) { - var x; + var foo = function () {}; - (function () { - function foo() {} - function bar() { - return foo; - } - for (x in {}) {} - })(); + var bar = function () { + return foo; + }; + + for (var x in {}) {} } diff --git a/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/loops-and-no-loops/actual.js b/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/loops-and-no-loops/actual.js new file mode 100644 index 0000000000..f030ac811d --- /dev/null +++ b/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/loops-and-no-loops/actual.js @@ -0,0 +1,34 @@ +function foo() { + const x = 5; + console.log(x); + + { + const x = 7; + setTimeout(() => x, 0); + } +} + +function bar() { + const x = 5; + console.log(x); + + for (let i = 0; i < 7; i++) { + { + const x = i; + setTimeout(() => x, 0); + } + } +} + +function baz() { + const x = 5; + console.log(x); + + for (let i = 0; i < 7; i++) { + var qux = function qux(y) { + const x = y; + setTimeout(() => x, 0); + }; + qux(i); + } +} diff --git a/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/loops-and-no-loops/expected.js b/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/loops-and-no-loops/expected.js new file mode 100644 index 0000000000..a86a2e4811 --- /dev/null +++ b/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/loops-and-no-loops/expected.js @@ -0,0 +1,42 @@ +function foo() { + var x = 5; + console.log(x); + + { + var _x = 7; + setTimeout(function () { + return _x; + }, 0); + } +} + +function bar() { + var x = 5; + console.log(x); + + for (var i = 0; i < 7; i++) { + { + (function () { + var x = i; + setTimeout(function () { + return x; + }, 0); + })(); + } + } +} + +function baz() { + var x = 5; + console.log(x); + + for (var i = 0; i < 7; i++) { + var qux = function qux(y) { + var x = y; + setTimeout(function () { + return x; + }, 0); + }; + qux(i); + } +} diff --git a/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/sibling-scopes/actual.js b/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/sibling-scopes/actual.js new file mode 100644 index 0000000000..e3020dada7 --- /dev/null +++ b/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/sibling-scopes/actual.js @@ -0,0 +1,11 @@ +var f1, f2; +{ + let z = 'z1 value'; + f1 = function() { return z; }; +} +{ + let z = 'z2 value'; + f2 = function() { return z; }; +} +f1(); +f2(); diff --git a/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/sibling-scopes/expected.js b/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/sibling-scopes/expected.js new file mode 100644 index 0000000000..8bbdfc7ddf --- /dev/null +++ b/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/sibling-scopes/expected.js @@ -0,0 +1,15 @@ +var f1, f2; +{ + var z = 'z1 value'; + f1 = function () { + return z; + }; +} +{ + var _z = 'z2 value'; + f2 = function () { + return _z; + }; +} +f1(); +f2(); \ No newline at end of file diff --git a/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/superswitch/actual.js b/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/superswitch/actual.js index 857b8641f4..5dedf9a592 100644 --- a/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/superswitch/actual.js +++ b/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/superswitch/actual.js @@ -1,16 +1,18 @@ function foo() { - switch (2) { - case 0: { - if (true) { - return; - } + while (true) { + switch (2) { + case 0: { + if (true) { + return; + } - const stuff = new Map(); - const data = 0; - stuff.forEach(() => { - const d = data; - }); - break; + const stuff = new Map(); + const data = 0; + stuff.forEach(() => { + const d = data; + }); + break; + } } } } diff --git a/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/superswitch/expected.js b/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/superswitch/expected.js index 05d7afe3d9..80b21eba9c 100644 --- a/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/superswitch/expected.js +++ b/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/superswitch/expected.js @@ -1,29 +1,31 @@ function foo() { - switch (2) { - case 0: - { - var _ret = function () { - if (true) { - return { - v: void 0 - }; + while (true) { + switch (2) { + case 0: + { + var _ret = function () { + if (true) { + return { + v: void 0 + }; + } + + var stuff = new Map(); + var data = 0; + stuff.forEach(function () { + var d = data; + }); + return "break"; + }(); + + switch (_ret) { + case "break": + break; + + default: + if (typeof _ret === "object") return _ret.v; } - - var stuff = new Map(); - var data = 0; - stuff.forEach(function () { - var d = data; - }); - return "break"; - }(); - - switch (_ret) { - case "break": - break; - - default: - if (typeof _ret === "object") return _ret.v; } - } + } } } diff --git a/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/switch-callbacks/actual.js b/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/switch-callbacks/actual.js index c57bdc6e51..99f8726f05 100644 --- a/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/switch-callbacks/actual.js +++ b/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/switch-callbacks/actual.js @@ -1,10 +1,12 @@ function fn() { - switch (true) { - default: - let foo = 4; - if (true) { - let bar = () => foo; - console.log(bar()); + while (true) { + switch (true) { + default: + let foo = 4; + if (true) { + let bar = () => foo; + console.log(bar()); + } } } } diff --git a/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/switch-callbacks/expected.js b/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/switch-callbacks/expected.js index 2efd145af3..40f23daea6 100644 --- a/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/switch-callbacks/expected.js +++ b/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/switch-callbacks/expected.js @@ -1,14 +1,16 @@ function fn() { - (function () { - switch (true) { - default: - var foo = 4; - if (true) { - var bar = function () { - return foo; - }; - console.log(bar()); - } - } - })(); + while (true) { + (function () { + switch (true) { + default: + var foo = 4; + if (true) { + var bar = function () { + return foo; + }; + console.log(bar()); + } + } + })(); + } } diff --git a/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/throwIfClosureRequired/for-const-closure/expected.js b/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/throwIfClosureRequired/for-const-closure/expected.js new file mode 100644 index 0000000000..c28cb3bf74 --- /dev/null +++ b/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/throwIfClosureRequired/for-const-closure/expected.js @@ -0,0 +1,6 @@ +for (var i = 0; i < 5; i++) { + var l = i; + setTimeout(function () { + console.log(l); + }, 1); +} \ No newline at end of file diff --git a/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/throwIfClosureRequired/superswitch/actual.js b/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/throwIfClosureRequired/superswitch/actual.js deleted file mode 100644 index 857b8641f4..0000000000 --- a/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/throwIfClosureRequired/superswitch/actual.js +++ /dev/null @@ -1,16 +0,0 @@ -function foo() { - switch (2) { - case 0: { - if (true) { - return; - } - - const stuff = new Map(); - const data = 0; - stuff.forEach(() => { - const d = data; - }); - break; - } - } -} diff --git a/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/throwIfClosureRequired/superswitch/options.json b/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/throwIfClosureRequired/superswitch/options.json deleted file mode 100644 index d210fdebfc..0000000000 --- a/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/throwIfClosureRequired/superswitch/options.json +++ /dev/null @@ -1,3 +0,0 @@ -{ - "throws": "Compiling let/const in this block would add a closure (throwIfClosureRequired)." -} diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/dont-hoist-block-scoped-variables/expected.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/dont-hoist-block-scoped-variables/expected.js index 9618e6da02..ab6ad7d4c4 100644 --- a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/dont-hoist-block-scoped-variables/expected.js +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/dont-hoist-block-scoped-variables/expected.js @@ -1,17 +1,11 @@ function render(flag) { if (flag) { - var _ret = function () { - var bar = "bar"; + var bar = "bar"; - [].map(() => bar); + [].map(() => bar); - return { - v: - }; - }(); - - if (typeof _ret === "object") return _ret.v; + return ; } return null; -} \ No newline at end of file +}