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.

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);

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 _loop = function () {
-      var _ret = function () {
+      var val = fn(..._arguments);
-        var val = fn(..._arguments);
+      return {
-        return {
+        v: val.test(function () {
-          v: val.test(function () {
+          console.log(val);
-            console.log(val);
+        })
-          })
+      };
-        };
+    };
-      }();
+
+    while (someCondition) {
+      var _ret = _loop();
 
       if (typeof _ret === "object") return _ret.v;
     }
   };
-}
+}

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);

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 () {
+  var bar = function () {
-    function foo() {}
+    return foo;
-    function bar() {
+  };
-      return foo;
+
-    }
+  for (var x in {}) {}
-    for (x in {}) {}
-  })();
 }

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);
+  }
+}

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);
+  }
+}

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();

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();

packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/superswitch/actual.js

@@ -1,16 +1,18 @@
 function foo() {
-  switch (2) {
+  while (true) {
-    case 0: {
+    switch (2) {
-      if (true) {
+      case 0: {
-        return;
+        if (true) {
-      }
+          return;
+        }
 
-      const stuff = new Map();
+        const stuff = new Map();
-      const data = 0;
+        const data = 0;
-      stuff.forEach(() => {
+        stuff.forEach(() => {
-        const d = data;
+          const d = data;
-      });
+        });
-      break;
+        break;
+      }
     }
   }
 }

packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/superswitch/expected.js

@@ -1,29 +1,31 @@
 function foo() {
-  switch (2) {
+  while (true) {
-    case 0:
+    switch (2) {
-      {
+      case 0:
-        var _ret = function () {
+        {
-          if (true) {
+          var _ret = function () {
-            return {
+            if (true) {
-              v: void 0
+              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;
         }
-      }
+    }
   }
 }

packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/switch-callbacks/actual.js

@@ -1,10 +1,12 @@
 function fn() {
-    switch (true) {
+    while (true) {
-    default:
+        switch (true) {
-        let foo = 4;
+        default:
-        if (true) {
+            let foo = 4;
-            let bar = () => foo;
+            if (true) {
-            console.log(bar());
+                let bar = () => foo;
+                console.log(bar());
+            }
         }
     }
 }

packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/switch-callbacks/expected.js

@@ -1,14 +1,16 @@
 function fn() {
-    (function () {
+    while (true) {
-        switch (true) {
+        (function () {
-            default:
+            switch (true) {
-                var foo = 4;
+                default:
-                if (true) {
+                    var foo = 4;
-                    var bar = function () {
+                    if (true) {
-                        return foo;
+                        var bar = function () {
-                    };
+                            return foo;
-                    console.log(bar());
+                        };
-                }
+                        console.log(bar());
-        }
+                    }
-    })();
+            }
+        })();
+    }
 }

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);
+}

packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/throwIfClosureRequired/superswitch/actual.js

@@ -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;
-    }
-  }
-}

packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/throwIfClosureRequired/superswitch/options.json

@@ -1,3 +0,0 @@
-{
-  "throws": "Compiling let/const in this block would add a closure (throwIfClosureRequired)."
-}

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 {
+    return <foo bar={bar} />;
-        v: <foo bar={bar} />
-      };
-    }();
-
-    if (typeof _ret === "object") return _ret.v;
   }
 
   return null;
-}
+}