Repository: jshint/jshint
Pull request #3173 (2017-08-20 20:38:33)

[[CHORE]] Improve reporting in project test suite
Previously, the project's test suite generated error messages which
included formatting directives related to text rendering. These
directives no longer modify the style of the text rendered to the
terminal; instead, they only introduce additional noise in the failure
reports.

- Remove the formatting directives to improve the readability of error
  reports
- Expand the logic that generates error messages to improve the
  readability of the test suite's source code
- Suppress test title in cases where it is not specified
- Consistently report expected character position

---

This has bugged me for years. For the longest time, I assumed that the noise in
the test suite's error reports was due to a misconfiguration in my system. I
imagine that contributors either made the same assumption or simply did not
want to dig in to the problem here.

I've previously bisected the project history, and I can't find any point where
the formatting directives produced the intended effect. My best guess is that
the functionality was built around some chance interaction between modules that
overrode built-in functions.

In any event, the noise is very distracting. Here's an example of what test
suite ouput looks like for a failure, prior to applying this patch:

    ✖ instanceOfLiterals

    Assertion Message: 
      TestRun: [bold]{undefined}
      [yellow]{Errors defined, but not thrown by JSHINT}
     [bold]{Line 1, Char undefined} A made up error.
      [yellow]{Errors thrown by JSHINT, but not defined in test run}
      [bold]{Line 1, Char 13} Expected an assignment or function call and instead saw an expression.
      [yellow]{Errors with wrong line number}
      [bold]{Line 1} Expected an identifier and instead saw ';'. [red]{not in line(s)} [bold]{2}
      [yellow]{Duplicated errors}
      [bold]{Line 1, Char 14} Missing semicolon.
      [bold]{Line 1, Char 14} Missing semicolon.


And here is the report as generated with this patch applied:

    ✖ instanceOfLiterals

    Assertion Message: 
      Errors defined, but not thrown by JSHint:
        {Line 1, Char undefined} A made up error.
      Errors thrown by JSHint, but not defined in test run:
        {Line 1, Char 13} Expected an assignment or function call and instead saw an expression.
      Errors with wrong line number:
        {Line 1, Char 13} Expected an identifier and instead saw ';'. {not in line(s)} {2}
      Duplicated errors:
        {Line 1, Char 14} Missing semicolon.
        {Line 1, Char 14} Missing semicolon.

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

Coverage remained the same at 97.764% when pulling **4cb4aba93041de46622e51569ff98294d9e2c3ae on jugglinmike:improve-error-reporting** into **9fe8c94b37f8e35fac532bba71f4566d26bf210b on jshint:master**.


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/helpers/testhelper.js b/tests/helpers/testhelper.js
index 11fc94ed..d5460947 100644
--- a/tests/helpers/testhelper.js
+++ b/tests/helpers/testhelper.js
@@ -103,6 +103,7 @@ exports.setup.testRun = function (test, name) {
         if (lines.length) {
           return {
             line: er.line,
+            character: er.character,
             message: er.reason,
             definedIn: lines
           };
@@ -130,32 +131,42 @@ exports.setup.testRun = function (test, name) {
         });
       });
 
+      var errorDetails = "";
+
+      if (unthrownErrors.length > 0) {
+        errorDetails += "\n  Errors defined, but not thrown by JSHint:\n" +
+          unthrownErrors.map(function (el) {
+            return "    {Line " + el.line + ", Char " + el.character + "} " + el.message;
+          }).join("\n");
+      }
+
+      if (undefinedErrors.length > 0) {
+        errorDetails += "\n  Errors thrown by JSHint, but not defined in test run:\n" +
+          undefinedErrors.map(function (el) {
+            return "    {Line " + el.line + ", Char " + el.character + "} " + el.reason;
+          }).join("\n");
+      }
+
+      if (wrongLineNumbers.length > 0) {
+        errorDetails += "\n  Errors with wrong line number:\n" +
+          wrongLineNumbers.map(function (el) {
+            return "    {Line " + el.line + ", Char " + el.character + "} " + el.message + " {not in line(s)} {" + el.definedIn.join(", ") + "}";
+          }).join("\n");
+      }
+
+      if (duplicateErrors.length > 0) {
+        errorDetails += "\n  Duplicated errors:\n" +
+          duplicateErrors.map(function (el) {
+            return "    {Line " + el.line + ", Char " + el.character + "} " + el.reason;
+          }).join("\n");
+      }
+
       test.ok(
-        undefinedErrors.length === 0 &&
-          unthrownErrors.length === 0 &&
-          wrongLineNumbers.length === 0 &&
-          duplicateErrors.length === 0,
-
-        (name === null ? "" : "\n  TestRun: [bold]{" + name + "}") +
-        unthrownErrors.map(function (el, idx) {
-          return (idx === 0 ? "\n  [yellow]{Errors defined, but not thrown by JSHINT}\n" : "") +
-            " [bold]{Line " + el.line + ", Char " + el.character + "} " + el.message;
-        }).join("\n") +
-        undefinedErrors.map(function (el, idx) {
-          return (idx === 0 ? "\n  [yellow]{Errors thrown by JSHINT, but not defined in test run}\n" : "") +
-            "  [bold]{Line " + el.line + ", Char " + el.character + "} " + el.reason;
-        }).join("\n") +
-        wrongLineNumbers.map(function (el, idx) {
-          return (idx === 0 ? "\n  [yellow]{Errors with wrong line number}\n" : "") +
-            "  [bold]{Line " + el.line + "} " + el.message + " [red]{not in line(s)} [bold]{" + el.definedIn.join(", ") + "}";
-        }).join("\n") +
-        duplicateErrors.map(function (el, idx) {
-          return (idx === 0 ? "\n  [yellow]{Duplicated errors}\n": "") +
-            "  [bold]{Line " + el.line + ", Char " + el.character + "} " + el.reason;
-        }).join("\n") + "\n"
+        errorDetails === "",
+        (name ? "\n  TestRun: '" + name + "'" : "") + errorDetails
       );
     }
   };
 
   return helperObj;
-};
\ No newline at end of file
+};
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();
 };