Repository: jshint/jshint
Pull request #3366 (2019-02-19 17:04:29)

[[FIX]] Track FutureReservedWords as idnts in ES5+
ES5 allowed a number identifiers which were previously classified as
FutureReserve to be used as binding identifiers. Ensure that when input
code defines such identifiers, usage counting operates as expected.

Although this patch is intended to correct a regression introduced by
commit f80e049c, it exposes a previously-existing bug. Prior to this
patch, JSHint did not flag FutureReservedWords when used as
IdentifierReferences. Correct this behavior since deferring a correction
to a dedicated commit would require additional and ultimately useless
logic.

---

@rwaldron JSHint creates dedicated entries in the symbol table for each of
ES3's FutureReservedWords. I'd like to lex them as identifiers and attach a
special property. Or maybe just create on symbol for them all to share. Either
change would be more than I'm comfortable making in a patch release, so I
limited the solution to something that we can get out as part of the regression
fix.

This is intended to resolve gh-3362


Patch:
diff --git a/src/jshint.js b/src/jshint.js
index 3b2f5871..2c04ca7c 100644
--- a/src/jshint.js
+++ b/src/jshint.js
@@ -132,6 +132,23 @@ var JSHINT = (function() {
     return true;
   }
 
+  /**
+   * ES3 defined a set of "FutureReservedWords" in order "to allow for the
+   * possibility of future adoption of [proposed] extensions."
+   *
+   * ES5 reduced the set of FutureReservedWords, in some cases by using them to
+   * define new syntactic forms (e.g. `class` and `const`) and in other cases
+   * by simply allowing their use as Identifiers (e.g. `int` and `goto`).
+   * Separately, ES5 introduced new restrictions on certain tokens, but limited
+   * the restriction to strict mode code (e.g. `let` and `yield`).
+   *
+   * This function determines if a given token describes a reserved word
+   * according to the current state of the parser.
+   *
+   * @param {number} context - the parsing context; see `prod-params.js` for
+   *                           more information
+   * @param {Token} token
+   */
   function isReserved(context, token) {
     if (!token.reserved) {
       return false;
@@ -145,19 +162,21 @@ var JSHINT = (function() {
           return false;
         }
 
-        // Some ES5 FutureReservedWord identifiers are active only
-        // within a strict mode environment.
-        if (meta.strictOnly) {
-          if (!state.option.strict && !state.isStrict()) {
-            return false;
-          }
-        }
-
         if (token.isProperty) {
           return false;
         }
       }
+    } else if (meta && meta.es5 && !state.inES5()) {
+      return false;
     }
+
+    // Some identifiers are reserved only within a strict mode environment.
+    if (meta && meta.strictOnly && state.inES5()) {
+      if (!state.option.strict && !state.isStrict()) {
+        return false;
+      }
+    }
+
     if (token.id === "await" && (!(context & prodParams.async) && !state.option.module)) {
       return false;
     }
@@ -1281,9 +1300,7 @@ var JSHINT = (function() {
    *                     support cases where further refinement is necessary)
    */
   function FutureReservedWord(name, meta) {
-    var x = type(name, (meta && meta.nud) || function() {
-      return this;
-    });
+    var x = type(name, (meta && meta.nud) || state.syntax["(identifier)"].nud);
 
     meta = meta || {};
     meta.isFutureReservedWord = true;
@@ -2251,7 +2268,7 @@ var JSHINT = (function() {
     lbp: 0,
     identifier: true,
 
-    nud: function() {
+    nud: function(context) {
       var v = this.value;
 
       // If this identifier is the lone parameter to a shorthand "fat arrow"
@@ -2266,7 +2283,9 @@ var JSHINT = (function() {
         return this;
       }
 
-      if (!state.funct["(comparray)"].check(v)) {
+      if (isReserved(context, this)) {
+        warning("W024", this, v);
+      } else if (!state.funct["(comparray)"].check(v)) {
         state.funct["(scope)"].block.use(v, state.tokens.curr);
       }
       return this;
@@ -2776,7 +2795,7 @@ var JSHINT = (function() {
   });
 
   function classBody(classToken, context) {
-    var props = {};
+    var props = Object.create(null);
     var name, accessorType, token, isStatic, inGenerator, hasConstructor;
 
     if (state.tokens.next.value === "{") {
@@ -4394,7 +4413,7 @@ var JSHINT = (function() {
       return state.syntax["(identifier)"].nud.apply(this, arguments);
     }
   };
-  letstatement.meta = { es5: true, isFutureReservedWord: true, strictOnly: true };
+  letstatement.meta = { es5: true, isFutureReservedWord: false, strictOnly: true };
   letstatement.exps = true;
   letstatement.declaration = true;
   letstatement.useFud = function(context) {
diff --git a/src/lex.js b/src/lex.js
index 1ea296ae..1b1540c7 100644
--- a/src/lex.js
+++ b/src/lex.js
@@ -1582,6 +1582,11 @@ Lexer.prototype = {
         continue;
       }
 
+      if (isCharSet) {
+        index += 1;
+        continue;
+      }
+
       if (char === "{" && !hasInvalidQuantifier) {
         hasInvalidQuantifier = !checkQuantifier();
       }
diff --git a/src/scope-manager.js b/src/scope-manager.js
index 496853b7..f7f92227 100644
--- a/src/scope-manager.js
+++ b/src/scope-manager.js
@@ -345,7 +345,7 @@ var scopeManager = function(state, predefined, exported, declared) {
 
           isFunction = usedLabelType === "function" ||
             usedLabelType === "generator function" ||
-            usedLabelType === "async fuction";
+            usedLabelType === "async function";
 
           // check for re-assigning a function declaration
           if ((isFunction || usedLabelType === "class") && usage["(reassigned)"]) {


Test patch:
diff --git a/tests/unit/core.js b/tests/unit/core.js
index 6f435d98..54bf218f 100644
--- a/tests/unit/core.js
+++ b/tests/unit/core.js
@@ -624,8 +624,6 @@ exports.testReserved = function (test) {
 
   TestRun(test)
     .addError(1, 1, "Expected an identifier and instead saw 'volatile' (a reserved word).")
-    .addError(5, 5, "Expected an identifier and instead saw 'let' (a reserved word).")
-    .addError(10, 7, "Expected an identifier and instead saw 'let' (a reserved word).")
     .addError(13, 13, "Expected an identifier and instead saw 'class' (a reserved word).")
     .addError(14, 5, "Expected an identifier and instead saw 'else' (a reserved word).")
     .addError(15, 5, "Expected an identifier and instead saw 'protected' (a reserved word).")
diff --git a/tests/unit/options.js b/tests/unit/options.js
index 4202991f..ece32ae7 100644
--- a/tests/unit/options.js
+++ b/tests/unit/options.js
@@ -1020,6 +1020,40 @@ exports.unused.basic = function (test) {
   test.done();
 };
 
+// Regression test for gh-3362
+exports.unused.es3Reserved = function (test) {
+  TestRun(test)
+    .addError(1, 5, "'abstract' is defined but never used.")
+    .addError(1, 15, "'boolean' is defined but never used.")
+    .addError(1, 24, "'byte' is defined but never used.")
+    .addError(1, 30, "'char' is defined but never used.")
+    .addError(1, 36, "'double' is defined but never used.")
+    .addError(1, 44, "'final' is defined but never used.")
+    .addError(1, 51, "'float' is defined but never used.")
+    .addError(1, 58, "'goto' is defined but never used.")
+    .addError(1, 64, "'int' is defined but never used.")
+    .addError(2, 5, "'long' is defined but never used.")
+    .addError(2, 11, "'native' is defined but never used.")
+    .addError(2, 19, "'short' is defined but never used.")
+    .addError(2, 26, "'synchronized' is defined but never used.")
+    .addError(2, 40, "'transient' is defined but never used.")
+    .addError(2, 51, "'volatile' is defined but never used.")
+    .test([
+      "var abstract, boolean, byte, char, double, final, float, goto, int;",
+      "var long, native, short, synchronized, transient, volatile;",
+    ], {unused: true});
+
+  TestRun(test)
+    .test([
+      "var abstract, boolean, byte, char, double, final, float, goto, int;",
+      "var long, native, short, synchronized, transient, volatile;",
+      "void (abstract + boolean + byte + char + double + final + float + loat + goto + int);",
+      "void (long + native + short + synchronized + transient + volatile);"
+    ], {unused: true});
+
+  test.done();
+};
+
 // Regression test for gh-2784
 exports.unused.usedThroughShadowedDeclaration = function (test) {
   var code = [
@@ -4087,6 +4121,7 @@ exports.module.await = function(test) {
 
   TestRun(test)
     .addError(1, 1, "Expected an assignment or function call and instead saw an expression.")
+    .addError(1, 1, "Expected an identifier and instead saw 'await' (a reserved word).")
     .addError(1, 6, "Missing semicolon.")
     .addError(1, 1, "Unrecoverable syntax error. (100% scanned).")
     .test("await: while (false) {}", { esversion: 6, module: true });
diff --git a/tests/unit/parser.js b/tests/unit/parser.js
index c5e049b1..dcfd3919 100644
--- a/tests/unit/parser.js
+++ b/tests/unit/parser.js
@@ -866,6 +866,12 @@ exports.regexp.regressions = function (test) {
   TestRun(test).test("var exp = /\\[\\]/;", {esnext: true});
   TestRun(test).test("var exp = /\\[\\]/;", {moz: true});
 
+  // GH-3356
+  TestRun(test).test("void /[/]/;");
+  TestRun(test).test("void /[{]/u;", {esversion: 6});
+  TestRun(test).test("void /[(?=)*]/u;", {esversion: 6});
+  TestRun(test).test("void /[(?!)+]/u;", {esversion: 6});
+
   test.done();
 };
 
@@ -6577,6 +6583,8 @@ exports.classes = function (test) {
     .addError(cexpr + 3, 20, "Expected an identifier and instead saw 'protected' (a reserved word).")
     .addError(cdecl + 4, 15, "Expected an identifier and instead saw 'package' (a reserved word).")
     .addError(cexpr + 4, 15, "Expected an identifier and instead saw 'package' (a reserved word).")
+    .addError(cdecl + 6, 20, "Expected an identifier and instead saw 'interface' (a reserved word).")
+    .addError(cexpr + 6, 27, "Expected an identifier and instead saw 'interface' (a reserved word).")
     .addError(cdeclAssn + 4, 21, "Reassignment of 'Foo15', which is a class. Use 'var' or 'let' to declare bindings that may change.")
     .addError(cdeclAssn + 7, 21, "Reassignment of 'Foo18', which is a class. Use 'var' or 'let' to declare bindings that may change.")
     .addError(cdeclAssn + 7, 43, "Reassignment of 'Foo17', which is a class. Use 'var' or 'let' to declare bindings that may change.")
@@ -6673,6 +6681,87 @@ exports["class and method naming"] = function (test) {
 
   run.test(code, {esnext: true});
 
+  TestRun(test, "hazardous method names (see gh-3358)")
+    .addError(3, 17, "'hasOwnProperty' is a really bad name.")
+    .test([
+      "void class {",
+      "  constructor() {}",
+      "  hasOwnProperty() {}",
+      "  toString() {}",
+      "  toLocaleString() {}",
+      "  valueOf() {}",
+      "  isPrototypeOf() {}",
+      "  propertyIsEnumerable() {}",
+      "};"
+   ], {esversion: 6});
+
+  TestRun(test, "hazardous method names -- true duplicate (see gh-3358)")
+    .addError(4, 11, "Duplicate class method 'toString'.")
+    .test([
+      "void class {",
+      "  toString() {}",
+      "  x() {}",
+      "  toString() {}",
+      "};"
+   ], {esversion: 6});
+
+  TestRun(test, "hazardous static method names (see gh-3358)")
+    .addError(3, 24, "'hasOwnProperty' is a really bad name.")
+    .test([
+      "void class {",
+      "  static constructor() {}",
+      "  static hasOwnProperty() {}",
+      "  static toString() {}",
+      "  static toLocaleString() {}",
+      "  static valueOf() {}",
+      "  static isPrototypeOf() {}",
+      "  static propertyIsEnumerable() {}",
+      "};"
+   ], {esversion: 6});
+
+  TestRun(test, "hazardous static method names -- true duplicate (see gh-3358)")
+    .addError(4, 18, "Duplicate static class method 'toString'.")
+    .test([
+      "void class {",
+      "  static toString() {}",
+      "  static x() {}",
+      "  static toString() {}",
+      "};"
+   ], {esversion: 6});
+
+  TestRun(test, "hazardous accessor method names (see gh-3358)")
+    .addError(2, 21, "'hasOwnProperty' is a really bad name.")
+    .addError(3, 21, "'hasOwnProperty' is a really bad name.")
+    .test([
+      "void class {",
+      "  get hasOwnProperty() {}",
+      "  set hasOwnProperty(_) {}",
+      "  get toString() {}",
+      "  set toString(_) {}",
+      "  get toLocaleString() {}",
+      "  set toLocaleString(_) {}",
+      "  get valueOf() {}",
+      "  set valueOf(_) {}",
+      "  get isPrototypeOf() {}",
+      "  set isPrototypeOf(_) {}",
+      "  get propertyIsEnumerable() {}",
+      "  set propertyIsEnumerable(_) {}",
+      "};"
+   ], {esversion: 6});
+
+  TestRun(test, "hazardous accessor method names -- true duplicate (see gh-3358)")
+    .addError(5, 15, "Duplicate getter method 'toString'.")
+    .addError(6, 15, "Duplicate setter method 'toString'.")
+    .test([
+      "void class {",
+      "  get toString() {}",
+      "  set toString(_) {}",
+      "  static x() {}",
+      "  get toString() {}",
+      "  set toString(_) {}",
+      "};"
+   ], {esversion: 6});
+
   test.done();
 };
 
@@ -6992,6 +7081,20 @@ exports.functionReassignment = function (test) {
     .addError(10, 3, "Reassignment of 'h', which is a function. Use 'var' or 'let' to declare bindings that may change.")
     .test(src);
 
+  TestRun(test, "generator functions")
+    .addError(2, 1, "Reassignment of 'g', which is a generator function. Use 'var' or 'let' to declare bindings that may change.")
+    .test([
+      "function * g () { yield; }",
+      "g = null;"
+    ], { esversion: 6 });
+
+  TestRun(test, "async functions")
+    .addError(2, 1, "Reassignment of 'a', which is a async function. Use 'var' or 'let' to declare bindings that may change.")
+    .test([
+      "async function a () {}",
+      "a = null;"
+    ], { esversion: 8 });
+
   test.done();
 };