Repository: jshint/jshint
Pull request #3312 (2018-07-22 23:33:44)

[[FIX]] Relax restriction on asgnmnt to arguments
@rwaldron this is another another edge case in service of gh-3272

---

Previously, JSHint issued E031 ("Bad assignment.") when it encountered
assignment to a mapped `arguments` object. Not only is this message
vague, its implementation as an error incorrectly disallows
syntactically valid JavaScript.

Introduce a new warning which more clearly describes the rationale, and
emit the new warning in place of the existing error so that users may
opt out.

Although this change causes changes in messages reported by JSHint, it
is backwards-compatible because the prior behavior was implemented in
terms of an error. Existing users are therefore not reliant on the prior
message or code.


Patch:
diff --git a/src/jshint.js b/src/jshint.js
index 612fa7a2..01351e92 100644
--- a/src/jshint.js
+++ b/src/jshint.js
@@ -1341,7 +1341,7 @@ var JSHINT = (function() {
 
     if (left.id === ".") {
       if (!left.left || left.left.value === "arguments" && !state.isStrict()) {
-        warning("E031", assignToken);
+        warning("W143", assignToken);
       }
 
       state.nameStack.set(state.tokens.prev);
@@ -1351,7 +1351,7 @@ var JSHINT = (function() {
         if (left.id === "{" || !left.left) {
           warning("E031", assignToken);
         } else if (left.left.value === "arguments" && !state.isStrict()) {
-          warning("E031", assignToken);
+          warning("W143", assignToken);
         }
       }
 
@@ -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;
   });
 
diff --git a/src/lex.js b/src/lex.js
index 2d361de6..066a59ec 100644
--- a/src/lex.js
+++ b/src/lex.js
@@ -1690,6 +1690,7 @@ Lexer.prototype = {
         case "~":
         case "#":
         case "]":
+        case "}":
         case "++":
         case "--":
           this.prereg = false;
diff --git a/src/messages.js b/src/messages.js
index f9bc3b5f..ba6dd6ff 100644
--- a/src/messages.js
+++ b/src/messages.js
@@ -230,7 +230,9 @@ var warnings = {
   W139: "Function expressions should not be used as the second operand to instanceof.",
   W140: "Missing comma.",
   W141: "Empty {a}: this is unnecessary and can be removed.",
-  W142: "Empty {a}: consider replacing with `import '{b}';`."
+  W142: "Empty {a}: consider replacing with `import '{b}';`.",
+  W143: "Assignment to properties of a mapped arguments object may cause " +
+    "unexpected changes to formal parameters."
 };
 
 var info = {


Test patch:
diff --git a/tests/test262/expectations.txt b/tests/test262/expectations.txt
index ec074e3c..0e15c6c0 100644
--- a/tests/test262/expectations.txt
+++ b/tests/test262/expectations.txt
@@ -229,9 +229,6 @@ test/built-ins/AsyncFunction/instance-length.js(default)
 test/built-ins/AsyncFunction/instance-length.js(strict mode)
 test/built-ins/AsyncFunction/instance-prototype-property.js(default)
 test/built-ins/AsyncFunction/instance-prototype-property.js(strict mode)
-test/built-ins/ArrayIteratorPrototype/next/args-mapped-expansion-after-exhaustion.js(default)
-test/built-ins/ArrayIteratorPrototype/next/args-mapped-expansion-before-exhaustion.js(default)
-test/built-ins/ArrayIteratorPrototype/next/args-mapped-truncation-before-exhaustion.js(default)
 test/built-ins/Function/prototype/toString/AsyncFunction.js(default)
 test/built-ins/Function/prototype/toString/AsyncFunction.js(strict mode)
 test/built-ins/Function/prototype/toString/async-function-declaration.js(default)
@@ -246,15 +243,6 @@ test/built-ins/Function/prototype/toString/line-terminator-normalisation-LF.js(d
 test/built-ins/Function/prototype/toString/line-terminator-normalisation-LF.js(strict mode)
 test/built-ins/Function/prototype/toString/unicode.js(default)
 test/built-ins/Function/prototype/toString/unicode.js(strict mode)
-test/built-ins/Array/prototype/every/15.4.4.16-2-17.js(default)
-test/built-ins/Array/prototype/forEach/15.4.4.18-2-17.js(default)
-test/built-ins/Array/prototype/indexOf/15.4.4.14-2-17.js(default)
-test/built-ins/Array/prototype/lastIndexOf/15.4.4.15-2-17.js(default)
-test/built-ins/Array/prototype/reduce/15.4.4.21-2-17.js(default)
-test/built-ins/Array/prototype/some/15.4.4.17-2-17.js(default)
-test/built-ins/Object/defineProperties/15.2.3.7-5-b-135.js(default)
-test/built-ins/Object/defineProperties/15.2.3.7-5-b-161.js(default)
-test/built-ins/Object/defineProperties/15.2.3.7-5-b-214.js(default)
 test/built-ins/Object/prototype/valueOf/S15.2.4.4_A14.js(default)
 test/built-ins/Object/prototype/valueOf/S15.2.4.4_A14.js(strict mode)
 test/built-ins/RegExp/S15.10.7_A1_T1.js(default)
@@ -352,30 +340,11 @@ test/language/identifiers/vals-rus-alpha-upper-via-escape-hex.js(default)
 test/language/identifiers/vals-rus-alpha-upper-via-escape-hex.js(strict mode)
 test/language/rest-parameters/array-pattern.js(default)
 test/language/rest-parameters/array-pattern.js(strict mode)
-test/language/rest-parameters/no-alias-arguments.js(default)
 test/language/rest-parameters/object-pattern.js(default)
 test/language/rest-parameters/object-pattern.js(strict mode)
-test/language/arguments-object/10.5-7-b-2-s.js(default)
-test/language/arguments-object/10.5-7-b-3-s.js(default)
-test/language/arguments-object/10.6-10-c-ii-2.js(default)
 test/language/arguments-object/10.6-13-c-1-s.js(strict mode)
 test/language/arguments-object/10.6-1gs.js(strict mode)
 test/language/arguments-object/10.6-2gs.js(strict mode)
-test/language/arguments-object/S10.6_A3_T4.js(default)
-test/language/arguments-object/S10.6_A5_T4.js(default)
-test/language/arguments-object/unmapped/via-params-dflt.js(default)
-test/language/arguments-object/unmapped/via-params-dstr.js(default)
-test/language/arguments-object/unmapped/via-params-rest.js(default)
-test/language/arguments-object/mapped/mapped-arguments-nonconfigurable-4.js(default)
-test/language/arguments-object/mapped/mapped-arguments-nonconfigurable-delete-4.js(default)
-test/language/arguments-object/mapped/mapped-arguments-nonconfigurable-nonwritable-4.js(default)
-test/language/arguments-object/mapped/mapped-arguments-nonconfigurable-strict-delete-4.js(default)
-test/language/arguments-object/mapped/mapped-arguments-nonwritable-nonconfigurable-3.js(default)
-test/language/arguments-object/mapped/nonconfigurable-descriptors-set-value-by-arguments.js(default)
-test/language/arguments-object/mapped/nonconfigurable-nonenumerable-nonwritable-descriptors-set-by-arguments.js(default)
-test/language/arguments-object/mapped/nonconfigurable-nonwritable-descriptors-set-by-arguments.js(default)
-test/language/arguments-object/mapped/nonwritable-nonconfigurable-descriptors-set-by-arguments.js(default)
-test/language/arguments-object/mapped/nonwritable-nonenumerable-nonconfigurable-descriptors-set-by-arguments.js(default)
 test/language/eval-code/indirect/block-decl-strict.js(default)
 test/language/eval-code/indirect/block-decl-strict.js(strict mode)
 test/language/eval-code/indirect/always-non-strict.js(strict mode)
@@ -652,20 +621,12 @@ test/language/expressions/exponentiation/exp-operator-precedence-update-expressi
 test/language/expressions/exponentiation/exp-operator.js(default)
 test/language/expressions/exponentiation/exp-operator.js(strict mode)
 test/language/expressions/delete/11.4.1-5-a-5gs.js(strict mode)
-test/language/expressions/division/S11.5.2_A2.2_T1.js(default)
-test/language/expressions/division/S11.5.2_A2.2_T1.js(strict mode)
-test/language/expressions/division/S11.5.2_A3_T1.5.js(default)
-test/language/expressions/division/S11.5.2_A3_T1.5.js(strict mode)
 test/language/expressions/instanceof/S11.8.6_A3.js(default)
 test/language/expressions/instanceof/S11.8.6_A3.js(strict mode)
-test/language/expressions/postfix-decrement/11.3.2-2-3-s.js(default)
-test/language/expressions/prefix-decrement/11.4.5-2-3-s.js(default)
-test/language/expressions/prefix-increment/11.4.4-2-3-s.js(default)
 test/language/expressions/tagged-template/constructor-invocation.js(default)
 test/language/expressions/tagged-template/constructor-invocation.js(strict mode)
 test/language/expressions/tagged-template/invalid-escape-sequences.js(default)
 test/language/expressions/tagged-template/invalid-escape-sequences.js(strict mode)
-test/language/expressions/yield/formal-parameters-after-reassignment-non-strict.js(default)
 test/language/expressions/yield/rhs-omitted.js(default)
 test/language/expressions/yield/rhs-omitted.js(strict mode)
 test/language/expressions/yield/rhs-primitive.js(default)
@@ -706,7 +667,6 @@ test/language/expressions/function/dstr-dflt-ary-ptrn-rest-obj-id.js(strict mode
 test/language/expressions/function/dstr-dflt-ary-ptrn-rest-obj-prop-id.js(default)
 test/language/expressions/function/dstr-dflt-ary-ptrn-rest-obj-prop-id.js(strict mode)
 test/language/expressions/function/param-dflt-yield-non-strict.js(default)
-test/language/expressions/function/params-dflt-args-unmapped.js(default)
 test/language/expressions/function/scope-param-rest-elem-var-close.js(default)
 test/language/expressions/function/scope-param-rest-elem-var-open.js(default)
 test/language/expressions/function/use-strict-with-non-simple-param.js(default)
@@ -741,7 +701,6 @@ test/language/expressions/generators/dstr-dflt-ary-ptrn-rest-obj-prop-id.js(defa
 test/language/expressions/generators/dstr-dflt-ary-ptrn-rest-obj-prop-id.js(strict mode)
 test/language/expressions/generators/param-dflt-yield.js(default)
 test/language/expressions/generators/param-dflt-yield.js(strict mode)
-test/language/expressions/generators/params-dflt-args-unmapped.js(default)
 test/language/expressions/generators/scope-param-rest-elem-var-close.js(default)
 test/language/expressions/generators/scope-param-rest-elem-var-open.js(default)
 test/language/expressions/generators/use-strict-with-non-simple-param.js(default)
@@ -1050,8 +1009,6 @@ test/language/expressions/object/method-definition/name-prop-name-yield-expr.js(
 test/language/expressions/object/method-definition/name-prop-name-yield-id.js(default)
 test/language/expressions/object/method-definition/object-method-returns-promise.js(default)
 test/language/expressions/object/method-definition/object-method-returns-promise.js(strict mode)
-test/language/expressions/object/method-definition/params-dflt-gen-meth-args-unmapped.js(default)
-test/language/expressions/object/method-definition/params-dflt-meth-args-unmapped.js(default)
 test/language/expressions/object/method-definition/setter-use-strict-with-non-simple-param.js(default)
 test/language/expressions/object/method-definition/setter-use-strict-with-non-simple-param.js(strict mode)
 test/language/expressions/object/method-definition/use-strict-with-non-simple-param.js(default)
@@ -1410,7 +1367,6 @@ test/language/statements/generators/dstr-dflt-ary-ptrn-rest-obj-prop-id.js(defau
 test/language/statements/generators/dstr-dflt-ary-ptrn-rest-obj-prop-id.js(strict mode)
 test/language/statements/generators/param-dflt-yield.js(default)
 test/language/statements/generators/param-dflt-yield.js(strict mode)
-test/language/statements/generators/params-dflt-args-unmapped.js(default)
 test/language/statements/generators/scope-param-rest-elem-var-close.js(default)
 test/language/statements/generators/scope-param-rest-elem-var-open.js(default)
 test/language/statements/generators/use-strict-with-non-simple-param.js(default)
@@ -1434,7 +1390,6 @@ test/language/statements/labeled/decl-gen.js(strict mode)
 test/language/statements/labeled/decl-let.js(default)
 test/language/statements/labeled/decl-let.js(strict mode)
 test/language/statements/labeled/value-yield-non-strict.js(default)
-test/language/statements/function/S13_A13_T3.js(default)
 test/language/statements/function/dstr-ary-ptrn-rest-ary-elem.js(default)
 test/language/statements/function/dstr-ary-ptrn-rest-ary-elem.js(strict mode)
 test/language/statements/function/dstr-ary-ptrn-rest-ary-elision.js(default)
@@ -1464,7 +1419,6 @@ test/language/statements/function/dstr-dflt-ary-ptrn-rest-obj-id.js(strict mode)
 test/language/statements/function/dstr-dflt-ary-ptrn-rest-obj-prop-id.js(default)
 test/language/statements/function/dstr-dflt-ary-ptrn-rest-obj-prop-id.js(strict mode)
 test/language/statements/function/param-dflt-yield-non-strict.js(default)
-test/language/statements/function/params-dflt-args-unmapped.js(default)
 test/language/statements/function/scope-param-rest-elem-var-close.js(default)
 test/language/statements/function/scope-param-rest-elem-var-open.js(default)
 test/language/statements/function/use-strict-with-non-simple-param.js(default)
@@ -1489,7 +1443,6 @@ test/language/statements/return/S12.9_A1_T8.js(default)
 test/language/statements/return/S12.9_A1_T8.js(strict mode)
 test/language/statements/return/S12.9_A1_T9.js(default)
 test/language/statements/return/S12.9_A1_T9.js(strict mode)
-test/language/statements/for-of/arguments-mapped-mutation.js(default)
 test/language/statements/for-of/decl-cls.js(default)
 test/language/statements/for-of/decl-cls.js(strict mode)
 test/language/statements/for-of/decl-fun.js(default)
diff --git a/tests/unit/parser.js b/tests/unit/parser.js
index b553e1d9..c5a815e2 100644
--- a/tests/unit/parser.js
+++ b/tests/unit/parser.js
@@ -188,10 +188,10 @@ exports.assignment = function (test) {
   ];
 
   var run = TestRun(test)
-    .addError(2, 20, "Bad assignment.")
-    .addError(3, 16, "Bad assignment.")
-    .addError(4, 20, "Bad assignment.")
-    .addError(5, 16, "Bad assignment.")
+    .addError(2, 20, "Assignment to properties of a mapped arguments object may cause unexpected changes to formal parameters.")
+    .addError(3, 16, "Assignment to properties of a mapped arguments object may cause unexpected changes to formal parameters.")
+    .addError(4, 20, "Assignment to properties of a mapped arguments object may cause unexpected changes to formal parameters.")
+    .addError(5, 16, "Assignment to properties of a mapped arguments object may cause unexpected changes to formal parameters.")
     .addError(14, 5, "Bad assignment.");
 
   run.test(code, { plusplus: true, es3: true });
@@ -643,6 +643,8 @@ exports.regexp = function (test) {
   TestRun(test).test("var a = 1; var b = a-- / 10;", {esnext: true});
   TestRun(test).test("var a = 1; var b = a-- / 10;", {moz: true});
 
+  TestRun(test, "gh-3308").test("void (function() {} / 0);");
+
   test.done();
 };
 
@@ -2003,7 +2005,7 @@ exports["destructuring globals with syntax error"] = function (test) {
   TestRun(test)
     .addError(1, 19, "Extending prototype of native object: 'Number'.")
     .addError(3, 9, "Bad assignment.")
-    .addError(4, 14, "Bad assignment.")
+    .addError(4, 14, "Assignment to properties of a mapped arguments object may cause unexpected changes to formal parameters.")
     .addError(6, 7, "Do not assign to the exception parameter.")
     .addError(7, 6, "Do not assign to the exception parameter.")
     .addError(9, 9, "Bad assignment.")
@@ -3679,17 +3681,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();