Repository: jshint/jshint
Pull request #3175 (2017-08-20 20:42:58)

[[FIX]] Allow closing over immutable bindings
The risk of confusion for functions declared within loops only pertains
to references to mutable bindings. Relax the wanting regarding
"confusing semantics" to only occur in the presence of one or more
references to mutable bindings.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/12914238/badge)](https://coveralls.io/builds/12914238)

Coverage increased (+0.001%) to 97.765% when pulling **78611ac82badbe2023a5b9d97c812d4111c47cf3 on jugglinmike:loopfunc-const** into **9fe8c94b37f8e35fac532bba71f4566d26bf210b on jshint:master**.

rwaldron: Nice work!

Patch:
diff --git a/src/scope-manager.js b/src/scope-manager.js
index 6adbfa0c..ddb49f66 100644
--- a/src/scope-manager.js
+++ b/src/scope-manager.js
@@ -262,7 +262,7 @@ var scopeManager = function(state, predefined, exported, declared) {
         isUnstackingFunctionParams = _current["(type)"] === "functionparams",
         isUnstackingFunctionOuter = _current["(type)"] === "functionouter";
 
-      var i, j;
+      var i, j, isImmutable;
       var currentUsages = _current["(usages)"];
       var currentLabels = _current["(labels)"];
       var usedLabelNameList = Object.keys(currentUsages);
@@ -278,7 +278,7 @@ var scopeManager = function(state, predefined, exported, declared) {
         var usedLabel = currentLabels[usedLabelName];
         if (usedLabel) {
           var usedLabelType = usedLabel["(type)"];
-          var isImmutable = usedLabelType === "const" || usedLabelType === "import";
+          isImmutable = usedLabelType === "const" || usedLabelType === "import";
 
           if (usedLabel["(useOutsideOfScope)"] && !state.option.funcscope) {
             var usedTokens = usage["(tokens)"];
@@ -314,11 +314,14 @@ var scopeManager = function(state, predefined, exported, declared) {
           continue;
         }
 
-        if (isUnstackingFunctionOuter) {
-          state.funct["(isCapturing)"] = true;
-        }
-
         if (subScope) {
+          var labelType = this.labeltype(usedLabelName);
+          isImmutable = labelType === "const" ||
+            (labelType === null && _scopeStack[0]["(predefined)"][usedLabelName] === false);
+          if (isUnstackingFunctionOuter && !isImmutable) {
+            state.funct["(isCapturing)"] = true;
+          }
+
           // not exiting the global scope, so copy the usage down in case its an out of scope usage
           if (!subScope["(usages)"][usedLabelName]) {
             subScope["(usages)"][usedLabelName] = usage;


Test patch:
diff --git a/tests/unit/options.js b/tests/unit/options.js
index 08df11a8..a6e737a3 100644
--- a/tests/unit/options.js
+++ b/tests/unit/options.js
@@ -1408,6 +1408,74 @@ exports.loopfunc = function (test) {
     .addError(3, "Functions declared within loops referencing an outer scoped variable may lead to confusing semantics.")
     .test(src2, { es3: true, loopfunc: false, boss: true });
 
+  TestRun(test, "Allows closing over immutable bindings (ES5)")
+    .addError(6, "Functions declared within loops referencing an outer scoped variable may lead to confusing semantics.")
+    .addError(7, "Functions declared within loops referencing an outer scoped variable may lead to confusing semantics.")
+    .test([
+        "var outerVar;",
+        "",
+        "while (false) {",
+        "  var innerVar;",
+        "",
+        "  void function() { var localVar; return outerVar; };",
+        "  void function() { var localVar; return innerVar; };",
+        "  void function() { var localVar; return localVar; };",
+        "",
+        "}",
+      ]);
+
+  TestRun(test, "Allows closing over immutable bindings (globals)")
+    .addError(8, "Functions declared within loops referencing an outer scoped variable may lead to confusing semantics.")
+    .addError(15, "Functions declared within loops referencing an outer scoped variable may lead to confusing semantics.")
+    .test([
+        "/* globals immutableGlobal: false, mutableGlobal: true */",
+        "while (false) {",
+        "  void function() { return eval; };",
+        "  void function() { return Infinity; };",
+        "  void function() { return NaN; };",
+        "  void function() { return undefined; };",
+        "  void function() { return immutableGlobal; };",
+        "  void function() { return mutableGlobal; };",
+        "}",
+        "",
+        "// Should recognize shadowing",
+        "(function() {",
+        "  var immutableGlobal;",
+        "  while (false) {",
+        "    void function() { return immutableGlobal; };",
+        "  }",
+        "}());"
+      ]);
+
+  TestRun(test, "Allows closing over immutable bindings (ES2015)")
+    .addError(10, "Functions declared within loops referencing an outer scoped variable may lead to confusing semantics.")
+    .addError(11, "Functions declared within loops referencing an outer scoped variable may lead to confusing semantics.")
+    .addError(18, "Functions declared within loops referencing an outer scoped variable may lead to confusing semantics.")
+    .addError(19, "Functions declared within loops referencing an outer scoped variable may lead to confusing semantics.")
+    .test([
+        "let outerLet;",
+        "const outerConst = 0;",
+        "class OuterClass {}",
+        "",
+        "while (false) {",
+        "  let innerLet;",
+        "  const innerConst = 0;",
+        "  class InnerClass {}",
+        "",
+        "  void function() { let localLet; return outerLet; };",
+        "  void function() { let localLet; return innerLet; };",
+        "  void function() { let localLet; return localLet; };",
+        "",
+        "  void function() { const localConst = 0; return outerConst; };",
+        "  void function() { const localConst = 0; return innerConst; };",
+        "  void function() { const localConst = 0; return localConst; };",
+        "",
+        "  void function() { class LocalClass {} return OuterClass; };",
+        "  void function() { class LocalClass {} return InnerClass; };",
+        "  void function() { class LocalClass {} return LocalClass; };",
+        "}"
+      ], { esversion: 2015 });
+
   test.done();
 };