Skip to content

Commit

Permalink
[[FEAT]] Implement new option futurehostile
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@jugglinmike
jugglinmike committed Feb 22, 2015
1 parent 69b3187 commit da52aa0
Show file tree
Hide file tree
Showing 6 changed files with 164 additions and 53 deletions.
19 changes: 15 additions & 4 deletions src/jshint.js
View file
Expand Up @@ -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) {
Expand Down Expand Up @@ -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 });
Expand Down Expand Up @@ -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 || {});
Expand Down
4 changes: 3 additions & 1 deletion src/messages.js
View file
Expand Up @@ -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 = {
Expand Down
8 changes: 8 additions & 0 deletions src/options.js
View file
Expand Up @@ -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
Expand Down
86 changes: 41 additions & 45 deletions src/vars.js
View file
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion tests/regression/thirdparty.js
View file
Expand Up @@ -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();
};
Expand Down
98 changes: 96 additions & 2 deletions tests/unit/options.js
View file
Expand Up @@ -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)
Expand Down Expand Up @@ -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();
};

1 comment on commit da52aa0

@mikermcneil
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jugglinmike very nice work, thank you

Please sign in to comment.