Repository: jshint/jshint
Pull request #3307 (2018-07-16 15:32:53)

[[FIX]] Correct restriction on function name
The ECMAScript specification details the following early error for
function-defining productions:

> - If the source code matching this production is strict mode code, it
>   is a Syntax Error if BindingIdentifier is present and the
>   StringValue of BindingIdentifier is "eval" or "arguments".

There were two bugs in the original implementation of this error:

- violations from function expressions with empty bodies would not be
  detected
- violations from function declarations contained within strict mode
  code would be reported twice

Correct the first issue by ensuring that the "(isStrict)" property is
accurately set for functions with empty bodies. Correct the second issue
by re-locating the error detection logic to within the processing logic
for expressions and declarations and inserting an additional condition
within the latter.

(This correction does not require modification to the Test262 whitelist
file, although that test suite does offer coverage for this behavior. At
the revision in use by JSHint, the relevant tests are expressed using
`eval` and therefore unusable by parsers. Those tests have since been
reformatted to correct this; they will be incorporated when JSHint next
updates its reference to Test262.)

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

Coverage increased (+0.002%) to 98.755% when pulling **03a589c97d61bc62907bef5cc26238874d4d5d59 on jugglinmike:fix-fn-name-restriction** into **ff71d3cbcb4f7c4a88b1a50130890564e6440484 on jshint:master**.


Patch:
diff --git a/src/jshint.js b/src/jshint.js
index 612fa7a2..d7984701 100644
--- a/src/jshint.js
+++ b/src/jshint.js
@@ -1764,6 +1764,9 @@ var JSHINT = (function() {
         metrics.statementCount += a.length;
 
         indent -= state.option.indent;
+      } else if (isfunc) {
+        // Ensure property is set for functions with empty bodies.
+        state.funct["(isStrict)"] = state.isStrict();
       }
 
       advance("}", t);
@@ -3055,11 +3058,6 @@ var JSHINT = (function() {
       warning("W124", state.tokens.curr);
     }
 
-    if (state.funct["(isStrict)"] === true &&
-      (name === "arguments" || name === "eval")) {
-      error("E008", token);
-    }
-
     state.funct["(metrics)"].verifyMaxStatementsPerFunction();
     state.funct["(metrics)"].verifyMaxComplexityPerFunction();
     state.funct["(unusedOption)"] = state.option.unused;
@@ -3876,27 +3874,40 @@ var JSHINT = (function() {
     if (inblock) {
       warning("W082", state.tokens.curr);
     }
-    var i = optionalidentifier();
+    var nameToken = optionalidentifier() ? state.tokens.curr : null;
 
-    if (i === undefined) {
+    if (!nameToken) {
       warning("W025");
     } else {
-      state.funct["(scope)"].addlabel(i, {
+      state.funct["(scope)"].addlabel(nameToken.value, {
         type: generator ? "generator function" : "function",
         token: state.tokens.curr,
         initialized: true });
 
       if (inexport) {
-        state.funct["(scope)"].setExported(i, state.tokens.prev);
+        state.funct["(scope)"].setExported(nameToken.value, state.tokens.prev);
       }
     }
 
-    doFunction({
-      name: i,
+    var f = doFunction({
+      name: nameToken && nameToken.value,
       statement: this,
       type: generator ? "generator" : null,
       ignoreLoopFunc: inblock // a declaration may already have warned
     });
+
+    // If the function declaration is strict because the surrounding code is
+    // strict, the invalid name will trigger E008 when the scope manager
+    // attempts to create a binding in the strict environment record. An error
+    // should only be signaled here when the function itself enables strict
+    // mode (the scope manager will not report an error because a declaration
+    // does not introduce a binding into the function's environment record).
+    var enablesStrictMode = f["(isStrict)"] && !state.isStrict();
+    if (nameToken && (f["(name)"] === "arguments" || f["(name)"] === "eval") &&
+      enablesStrictMode) {
+      error("E008", nameToken);
+    }
+
     if (state.tokens.next.id === "(" && state.tokens.next.line === state.tokens.curr.line) {
       error("E039");
     }
@@ -3914,8 +3925,17 @@ var JSHINT = (function() {
       generator = true;
     }
 
-    var i = optionalidentifier();
-    doFunction({ name: i, type: generator ? "generator" : null });
+    var nameToken = optionalidentifier() ? state.tokens.curr : null;
+
+    var f = doFunction({
+      name: nameToken && nameToken.value,
+      type: generator ? "generator" : null
+    });
+
+    if (nameToken && (f["(name)"] === "arguments" || f["(name)"] === "eval") &&
+      f["(isStrict)"]) {
+      error("E008", nameToken);
+    }
     return this;
   });
 


Test patch:
diff --git a/tests/unit/parser.js b/tests/unit/parser.js
index b553e1d9..f2bec987 100644
--- a/tests/unit/parser.js
+++ b/tests/unit/parser.js
@@ -3679,17 +3679,49 @@ exports["strict violation - use of arguments and eval"] = function (test) {
   TestRun(test, "as function declaration binding (invalid)")
     .addError(1, 10, "Strict violation.")
     .addError(2, 10, "Strict violation.")
+    .addError(5, 12, "Strict violation.")
+    .addError(6, 12, "Strict violation.")
+    .addError(10, 12, "Strict violation.")
+    .addError(10, 26, "Unnecessary directive \"use strict\".")
+    .addError(11, 12, "Strict violation.")
+    .addError(11, 21, "Unnecessary directive \"use strict\".")
     .test([
       "function arguments() { 'use strict'; }",
-      "function eval() { 'use strict'; }"
+      "function eval() { 'use strict'; }",
+      "(function() {",
+      "  'use strict';",
+      "  function arguments() {}",
+      "  function eval() {}",
+      "}());",
+      "(function() {",
+      "  'use strict';",
+      "  function arguments() { 'use strict'; }",
+      "  function eval() { 'use strict'; }",
+      "}());"
     ]);
 
   TestRun(test, "as function expression binding (invalid)")
     .addError(1, 15, "Strict violation.")
     .addError(2, 15, "Strict violation.")
+    .addError(5, 17, "Strict violation.")
+    .addError(6, 17, "Strict violation.")
+    .addError(10, 17, "Strict violation.")
+    .addError(10, 31, "Unnecessary directive \"use strict\".")
+    .addError(11, 17, "Strict violation.")
+    .addError(11, 26, "Unnecessary directive \"use strict\".")
     .test([
       "void function arguments() { 'use strict'; };",
-      "void function eval() { 'use strict'; };"
+      "void function eval() { 'use strict'; };",
+      "(function() {",
+      "  'use strict';",
+      "  void function arguments() {};",
+      "  void function eval() {};",
+      "}());",
+      "(function() {",
+      "  'use strict';",
+      "  void function arguments() { 'use strict'; };",
+      "  void function eval() { 'use strict'; };",
+      "}());"
     ]);
 
   test.done();