From 48a0bde3df143192d0c7d5cff0438c781fd449d2 Mon Sep 17 00:00:00 2001 From: Ben Alpert Date: Mon, 30 Jan 2017 16:53:56 -0800 Subject: [PATCH] Avoid adding unnecessary closure for block scoping 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/superswitch/actual.js | 24 +++++----- .../fixtures/general/superswitch/expected.js | 46 ++++++++++--------- .../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 ++---- 14 files changed, 189 insertions(+), 101 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/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 e995e0d23496..c2ae386b34dd 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 ae32dbb4d39a..c1079c01cbbf 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 37943664fbf0..270e7e6d2c73 100644 --- a/packages/babel-plugin-transform-es2015-block-scoping/src/index.js +++ b/packages/babel-plugin-transform-es2015-block-scoping/src/index.js @@ -114,8 +114,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]); @@ -550,9 +563,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 1dce9c4c174c..9efbc1091f2b 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 000000000000..f030ac811de4 --- /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 000000000000..a86a2e481147 --- /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/superswitch/actual.js b/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/superswitch/actual.js index 857b8641f415..5dedf9a59244 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 05d7afe3d96c..80b21eba9cdc 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"; - }(); + var stuff = new Map(); + var data = 0; + stuff.forEach(function () { + var d = data; + }); + return "break"; + }(); - switch (_ret) { - case "break": - break; + switch (_ret) { + case "break": + break; - default: - if (typeof _ret === "object") return _ret.v; + 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 c57bdc6e5150..99f8726f058a 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 2efd145af333..40f23daea608 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 000000000000..c28cb3bf748e --- /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 857b8641f415..000000000000 --- 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 d210fdebfc97..000000000000 --- 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 9618e6da028f..ab6ad7d4c478 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 +}