Repository: jshint/jshint
Pull request #2172 (2015-02-13 16:09:46)

[[FEAT]] Implement new option `futurehostile`
A recent change to JSHint included the ECMAScript 6 global identifiers
in JSHint's collection of "predefined" variables. This disallowed the
definition of those global values even in contexts that did not
implement them.

The intent of this change was to help users avoid bugs when migrating to
the new version of the language. Re-using an existing warning mechanism
caused confusion among many users [1][2][3] for two reasons:

1. The generated warning messages were somewhat misleading (i.e. the
   warning "Redefinition of 'Promise'." is inaccurate in ES5
   environments)
2. The method of disabling the warnings did not accurately communicate
   the intent of the developer (i.e. setting `predef: -Promise` does not
   prompt the user to consider the user to consider if creating their
   own global variable named `Promise` is appropriate)

A dedicated warning message more clearly describes the best practice
that motivates this particular constraint on global variable names. An
explicit option makes it less likely that users will disable this
warning without understanding the motivation.

Intoduce a new option, `futurehostile`, and dedicated warning message to
better communicate the intent of restricting usage of future-reserved
identifiers.

[1] GH-1747 redefinition of Promise (W079)
[2] GH-1995 ES6 globals are activated even though esnext=false
[3] GH-2171 Remove Set from the ECMAScript5 reserved words

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

Coverage increased (+0.03%) to 94.86% when pulling **9b06eaf930794d9eb2ddfb957272919aaac78438 on jugglinmike:futurehostile** into **4e59553f7e2e09ba6c15643fdcb5b4416301148f on jshint:master**.

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

Coverage increased (+0.03%) to 94.86% when pulling **c750f3fddd1458857ff728494feb899ad461ef6c on jugglinmike:futurehostile** into **4e59553f7e2e09ba6c15643fdcb5b4416301148f on jshint:master**.

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

Coverage increased (+0.03%) to 94.86% when pulling **c750f3fddd1458857ff728494feb899ad461ef6c on jugglinmike:futurehostile** into **4e59553f7e2e09ba6c15643fdcb5b4416301148f on jshint:master**.

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

Coverage increased (+0.03%) to 94.86% when pulling **0f1c8e0138c58fdae29d2d008405a23d78d762c3 on jugglinmike:futurehostile** into **4e59553f7e2e09ba6c15643fdcb5b4416301148f on jshint:master**.

jugglinmike: @rwaldron @caitp Thanks for the review! A couple things:

- Removing `System` is a breaking change. I think this is fair because developers should generally recognize that ES6 is a moving target and that enabling `esnext` will tend to decrease stability
- The most recent commit repurposes the existing `ecmaIdentifiers` attribute. Nobody asked for this, but after moving `JSON` to ES5, it only seemed appropriate to put everything else in ES3 so we'd have a consistent interface for referencing all of these globals.
rwaldron: @jugglinmike this needs a rebase 
jugglinmike: @rwaldron Sure thing; should I squash while I'm at it?

Patch:
diff --git a/src/jshint.js b/src/jshint.js
index 43d29d42..8b09cc88 100644
--- a/src/jshint.js
+++ b/src/jshint.js
@@ -197,8 +197,12 @@ var JSHINT = (function() {
 
     processenforceall();
 
+    if (!state.option.es3) {
+      combine(predefined, vars.ecmaIdentifiers[5]);
+    }
+
     if (state.option.esnext) {
-      combine(predefined, vars.newEcmaIdentifiers);
+      combine(predefined, vars.ecmaIdentifiers[6]);
     }
 
     if (state.option.couch) {
@@ -3364,8 +3368,15 @@ var JSHINT = (function() {
           if (state.option.inESNext() && funct[t.id] === "const") {
             warning("E011", null, t.id);
           }
-          if (funct["(global)"] && predefined[t.id] === false) {
-            warning("W079", t.token, t.id);
+          if (funct["(global)"]) {
+            if (predefined[t.id] === false) {
+              warning("W079", t.token, t.id);
+            } else if (!state.option.futurehostile) {
+              if ((!state.option.inES5() && vars.ecmaIdentifiers[5][t.id] === false) ||
+                (!state.option.inESNext() && vars.ecmaIdentifiers[6][t.id] === false)) {
+                warning("W129", t.token, t.id);
+              }
+            }
           }
           if (t.id) {
             addlabel(t.id, { type: "unused", token: t.token });
@@ -4919,7 +4930,7 @@ var JSHINT = (function() {
     }
 
     predefined = Object.create(null);
-    combine(predefined, vars.ecmaIdentifiers);
+    combine(predefined, vars.ecmaIdentifiers[3]);
     combine(predefined, vars.reservedVars);
 
     combine(predefined, g || {});
diff --git a/src/messages.js b/src/messages.js
index 5aa1f2c5..f5e65749 100644
--- a/src/messages.js
+++ b/src/messages.js
@@ -202,7 +202,9 @@ var warnings = {
   W125: "This line contains non-breaking spaces: http://jshint.com/doc/options/#nonbsp",
   W126: "Unnecessary grouping operator.",
   W127: "Unexpected use of a comma operator.",
-  W128: "Empty array elements require elision=true."
+  W128: "Empty array elements require elision=true.",
+  W129: "'{a}' is defined in a future version of JavaScript. Use a " +
+    "different variable name to avoid migration issues."
 };
 
 var info = {
diff --git a/src/options.js b/src/options.js
index 3a5cccf2..a5175556 100644
--- a/src/options.js
+++ b/src/options.js
@@ -352,6 +352,14 @@ exports.bool = {
     */
     evil        : true,
 
+    /**
+     * This option supresses warnings about the use of identifiers which are
+     * defined in future versions of JavaScript. Although overwriting them has
+     * no effect in contexts where they are not implemented, this practice can
+     * cause issues when migrating codebases to newer versions of the language.
+     */
+    futurehostile: true,
+
     /**
      * This option prohibits the use of unary increment and decrement
      * operators.  Some people think that `++` and `--` reduces the quality of
diff --git a/src/vars.js b/src/vars.js
index 91ac6565..08865c21 100644
--- a/src/vars.js
+++ b/src/vars.js
@@ -10,51 +10,47 @@ exports.reservedVars = {
 };
 
 exports.ecmaIdentifiers = {
-  Array              : false,
-  Boolean            : false,
-  Date               : false,
-  decodeURI          : false,
-  decodeURIComponent : false,
-  encodeURI          : false,
-  encodeURIComponent : false,
-  Error              : false,
-  "eval"             : false,
-  EvalError          : false,
-  Function           : false,
-  hasOwnProperty     : false,
-  isFinite           : false,
-  isNaN              : false,
-  JSON               : false,
-  Map                : false,
-  Math               : false,
-  Number             : false,
-  Object             : false,
-  Proxy              : false,
-  Promise            : false,
-  parseInt           : false,
-  parseFloat         : false,
-  RangeError         : false,
-  ReferenceError     : false,
-  RegExp             : false,
-  Set                : false,
-  String             : false,
-  SyntaxError        : false,
-  TypeError          : false,
-  URIError           : false,
-  WeakMap            : false,
-  WeakSet            : false
-};
-
-exports.newEcmaIdentifiers = {
-  Set     : false,
-  Map     : false,
-  WeakMap : false,
-  WeakSet : false,
-  Proxy   : false,
-  Promise : false,
-  Reflect : false,
-  Symbol  : false,
-  System  : false
+  3: {
+    Array              : false,
+    Boolean            : false,
+    Date               : false,
+    decodeURI          : false,
+    decodeURIComponent : false,
+    encodeURI          : false,
+    encodeURIComponent : false,
+    Error              : false,
+    "eval"             : false,
+    EvalError          : false,
+    Function           : false,
+    hasOwnProperty     : false,
+    isFinite           : false,
+    isNaN              : false,
+    Math               : false,
+    Number             : false,
+    Object             : false,
+    parseInt           : false,
+    parseFloat         : false,
+    RangeError         : false,
+    ReferenceError     : false,
+    RegExp             : false,
+    String             : false,
+    SyntaxError        : false,
+    TypeError          : false,
+    URIError           : false
+  },
+  5: {
+    JSON               : false
+  },
+  6: {
+    Map                : false,
+    Promise            : false,
+    Proxy              : false,
+    Reflect            : false,
+    Set                : false,
+    Symbol             : false,
+    WeakMap            : false,
+    WeakSet            : false
+  }
 };
 
 // Global variables commonly provided by a web browser environment.


Test patch:
diff --git a/tests/regression/thirdparty.js b/tests/regression/thirdparty.js
index 45f888cc..f5cf927b 100644
--- a/tests/regression/thirdparty.js
+++ b/tests/regression/thirdparty.js
@@ -185,7 +185,7 @@ exports.json2 = function (test) {
   TestRun(test)
     .addError(177, "'key' is defined but never used.")
     .addError(191, "'key' is defined but never used.")
-    .test(src, { singleGroups: true, undef: true, unused: true, laxbreak: true }, { JSON: true });
+    .test(src, { singleGroups: true, undef: true, unused: true, laxbreak: true, predef: ["-JSON"] }, { JSON: true });
 
   test.done();
 };
diff --git a/tests/unit/options.js b/tests/unit/options.js
index 4260762e..62bf6740 100644
--- a/tests/unit/options.js
+++ b/tests/unit/options.js
@@ -1818,8 +1818,7 @@ exports.esnextPredefs = function (test) {
     '/* global alert: true */',
     'var mySym = Symbol("name");',
     'var myBadSym = new Symbol("name");',
-    'alert(Reflect);',
-    'alert(System);'
+    'alert(Reflect);'
   ];
 
   TestRun(test)
@@ -2183,3 +2182,98 @@ exports.badInlineOptionValue = function (test) {
 
   test.done();
 };
+
+exports.futureHostile = function (test) {
+  var code = [
+    "var JSON = {};",
+    "var Map = function() {};",
+    "var Promise = function() {};",
+    "var Proxy = function() {};",
+    "var Reflect = function() {};",
+    "var Set = function() {};",
+    "var Symbol = function() {};",
+    "var WeakMap = function() {};",
+    "var WeakSet = function() {};"
+  ];
+
+  TestRun(test, "ES3 without option")
+    .addError(1, "'JSON' is defined in a future version of JavaScript. Use a different variable name to avoid migration issues.")
+    .addError(2, "'Map' is defined in a future version of JavaScript. Use a different variable name to avoid migration issues.")
+    .addError(3, "'Promise' is defined in a future version of JavaScript. Use a different variable name to avoid migration issues.")
+    .addError(4, "'Proxy' is defined in a future version of JavaScript. Use a different variable name to avoid migration issues.")
+    .addError(5, "'Reflect' is defined in a future version of JavaScript. Use a different variable name to avoid migration issues.")
+    .addError(6, "'Set' is defined in a future version of JavaScript. Use a different variable name to avoid migration issues.")
+    .addError(7, "'Symbol' is defined in a future version of JavaScript. Use a different variable name to avoid migration issues.")
+    .addError(8, "'WeakMap' is defined in a future version of JavaScript. Use a different variable name to avoid migration issues.")
+    .addError(9, "'WeakSet' is defined in a future version of JavaScript. Use a different variable name to avoid migration issues.")
+    .test(code, { es3: true, es5: false });
+
+  TestRun(test, "ES3 with option")
+    .test(code, { es3: true, es5: false, futurehostile: true });
+
+  TestRun(test, "ES5 without option")
+    .addError(1, "Redefinition of 'JSON'.")
+    .addError(2, "'Map' is defined in a future version of JavaScript. Use a different variable name to avoid migration issues.")
+    .addError(3, "'Promise' is defined in a future version of JavaScript. Use a different variable name to avoid migration issues.")
+    .addError(4, "'Proxy' is defined in a future version of JavaScript. Use a different variable name to avoid migration issues.")
+    .addError(5, "'Reflect' is defined in a future version of JavaScript. Use a different variable name to avoid migration issues.")
+    .addError(6, "'Set' is defined in a future version of JavaScript. Use a different variable name to avoid migration issues.")
+    .addError(7, "'Symbol' is defined in a future version of JavaScript. Use a different variable name to avoid migration issues.")
+    .addError(8, "'WeakMap' is defined in a future version of JavaScript. Use a different variable name to avoid migration issues.")
+    .addError(9, "'WeakSet' is defined in a future version of JavaScript. Use a different variable name to avoid migration issues.")
+    .test(code, {});
+
+  TestRun(test, "ES5 with option")
+    .addError(1, "Redefinition of 'JSON'.")
+    .test(code, {
+      futurehostile: true
+    });
+
+  TestRun(test, "ES5 with opt-out")
+    .test(code, {
+      futurehostile: true,
+      predef: ["-JSON"]
+    });
+
+  TestRun(test, "ESNext without option")
+    .addError(1, "Redefinition of 'JSON'.")
+    .addError(2, "Redefinition of 'Map'.")
+    .addError(3, "Redefinition of 'Promise'.")
+    .addError(4, "Redefinition of 'Proxy'.")
+    .addError(5, "Redefinition of 'Reflect'.")
+    .addError(6, "Redefinition of 'Set'.")
+    .addError(7, "Redefinition of 'Symbol'.")
+    .addError(8, "Redefinition of 'WeakMap'.")
+    .addError(9, "Redefinition of 'WeakSet'.")
+    .test(code, { esnext: true });
+
+  TestRun(test, "ESNext with option")
+    .addError(1, "Redefinition of 'JSON'.")
+    .addError(2, "Redefinition of 'Map'.")
+    .addError(3, "Redefinition of 'Promise'.")
+    .addError(4, "Redefinition of 'Proxy'.")
+    .addError(5, "Redefinition of 'Reflect'.")
+    .addError(6, "Redefinition of 'Set'.")
+    .addError(7, "Redefinition of 'Symbol'.")
+    .addError(8, "Redefinition of 'WeakMap'.")
+    .addError(9, "Redefinition of 'WeakSet'.")
+    .test(code, { esnext: true, futurehostile: true });
+
+  TestRun(test, "ESNext with opt-out")
+    .test(code, {
+      esnext: true,
+      predef: [
+        "-JSON",
+        "-Map",
+        "-Promise",
+        "-Proxy",
+        "-Reflect",
+        "-Set",
+        "-Symbol",
+        "-WeakMap",
+        "-WeakSet"
+      ]
+    });
+
+  test.done();
+};