Skip to content

Commit

Permalink
[[FIX]] Improve support for __proto__ identifier
Browse files Browse the repository at this point in the history 
Information about identifiers used are stored internally as properties
of objects whose key values are the identifier value. Because of
non-standard implementations of the `__proto__` property, input source
code that uses it as an identifier can have inconsistent side effects
across environments.

When using an object as a lookup table, the built-in
`Object.hasOwnProperty` is generally preferable for detecting property
membership. Unfortunately, due to the non-standard implementations
described above, this check has environment-specific results.

An alternate approach to membership detection is coercing direct
property references to a boolean value. This is not appropriate in cases
where the entries may themselves be "falsey" values, but in the
identifier lookup tables, entries are consistently object literals.

This second approach yields the same results across all legacy
environments and is also compatable with the standardized specification
for the `__proto__` attribute (provided the table is initialized to have
no prototype).

`var o = {};`

                                           | Compliant | Node.js v0.10.40 | IE9   | PhantomJS v1.9
------------------------------------------ | --------- | ---------------- | ----- | --------------
**Before `__proto__` assignment**          |           |                  |       |
!!o.__proto__                              | true      | true             | false | true
'__proto__' in o                           | true      | true             | false | true
Object.hasOwnProperty.call(o, '__proto__') | false     | false            | false | true
Object.keys(o).length                      | 0         | 0                | 0     | 0
**After `__proto__` assignment**           |           |                  |       |
!!o.__proto__                              | true      | true             | true  | true
'__proto__' in o                           | true      | true             | true  | true
Object.hasOwnProperty.call(o, '__proto__') | false     | false            | true  | true
Object.keys(o).length                      | 0         | 0                | 1     | 0

`var o = Object.create(null);`

                                           | Compliant | Node.js v0.10.40 | IE9   | PhantomJS v1.9
------------------------------------------ | --------- | ---------------- | ----- | --------------
**Before `__proto__` assignment**          |           |                  |       |
!!o.__proto__                              | false     | false            | false | false
'__proto__' in o                           | false     | true             | false | true
Object.hasOwnProperty.call(o, '__proto__') | false     | false            | false | true
Object.keys(o).length                      | 0         | 0                | 0     | 0
**After `__proto__` assignment**           |           |                  |       |
!!o.__proto__                              | true      | true             | true  | true
'__proto__' in o                           | true      | true             | true  | true
Object.hasOwnProperty.call(o, '__proto__') | true      | false            | true  | true
Object.keys(o).length                      | 1         | 0                | 1     | 0

Compliant enviroments: latest SpiderMonkey (and Firefox), latest V8 (and
Chrome), Node.js v0.12.7, Iojs v.2.3.4

Update all lookup tables to be created without a prototype, and update
all membership tests to simply coerce direct attribute references.
  • Loading branch information
@jugglinmike @lukeapage
jugglinmike authored and lukeapage committed Jul 19, 2015
1 parent 56c19a5 commit 925a983
Show file tree
Hide file tree
Showing 3 changed files with 174 additions and 30 deletions.
16 changes: 8 additions & 8 deletions src/jshint.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3063,7 +3063,7 @@ var JSHINT = (function() {
// Check for lonely setters if in the ES5 mode.
if (state.inES5()) {
for (var name in props) {
if (_.has(props, name) && props[name].setterToken && !props[name].getterToken) {
if (props[name] && props[name].setterToken && !props[name].getterToken) {
warning("W078", props[name].setterToken);
}
}
Expand All @@ -3088,7 +3088,7 @@ var JSHINT = (function() {
(function(x) {
x.nud = function() {
var b, f, i, p, t, isGeneratorMethod = false, nextVal;
var props = {}; // All properties, including accessors
var props = Object.create(null); // All properties, including accessors

b = state.tokens.curr.line !== startLine(state.tokens.next);
if (b) {
Expand Down Expand Up @@ -3583,8 +3583,8 @@ var JSHINT = (function() {
var isStatic;
var isGenerator;
var getset;
var props = {};
var staticProps = {};
var props = Object.create(null);
var staticProps = Object.create(null);
var computed;
for (var i = 0; state.tokens.next.id !== "}"; ++i) {
name = state.tokens.next;
Expand Down Expand Up @@ -4618,10 +4618,10 @@ var JSHINT = (function() {
name = tkn.value;
}

if (props[name] && _.has(props, name)) {
if (props[name]) {
warning("W075", state.tokens.next, msg, name);
} else {
props[name] = {};
props[name] = Object.create(null);
}

props[name].basic = true;
Expand Down Expand Up @@ -4653,12 +4653,12 @@ var JSHINT = (function() {
state.tokens.curr.accessorType = accessorType;
state.nameStack.set(tkn);

if (props[name] && _.has(props, name)) {
if (props[name]) {
if (props[name].basic || props[name][flagName]) {
warning("W075", state.tokens.next, msg, name);
}
} else {
props[name] = {};
props[name] = Object.create(null);
}

props[name][flagName] = tkn;
Expand Down
82 changes: 60 additions & 22 deletions src/scope-manager.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@
var _ = require("lodash");
var events = require("events");

// Used to denote membership in lookup tables (a primitive value such as `true`
// would be silently rejected for the property name "__proto__" in some
// environments)
var marker = {};

/**
* Creates a scope manager that handles variables and labels, storing usages
* and resolving when variables are used and undefined
Expand All @@ -25,7 +30,7 @@ var scopeManager = function(state, predefined, exported, declared) {
var _scopeStack = [_current];

var usedPredefinedAndGlobals = Object.create(null);
var impliedGlobals = {};
var impliedGlobals = Object.create(null);
var unuseds = [];
var emitter = new events.EventEmitter();

Expand All @@ -49,7 +54,7 @@ var scopeManager = function(state, predefined, exported, declared) {
if (token) {
token["(function)"] = _currentFunct;
}
if (!_.has(_current["(usages)"], labelName)) {
if (!_current["(usages)"][labelName]) {
_current["(usages)"][labelName] = {
"(modified)": [],
"(reassigned)": [],
Expand Down Expand Up @@ -117,7 +122,7 @@ var scopeManager = function(state, predefined, exported, declared) {
}
var curentLabels = _current["(labels)"];
for (var labelName in curentLabels) {
if (_.has(curentLabels, labelName)) {
if (curentLabels[labelName]) {
if (curentLabels[labelName]["(type)"] !== "exception" &&
curentLabels[labelName]["(unused)"]) {
_warnUnused(labelName, curentLabels[labelName]["(token)"], "var");
Expand Down Expand Up @@ -162,7 +167,7 @@ var scopeManager = function(state, predefined, exported, declared) {
function _getLabel(labelName) {
for (var i = _scopeStack.length - 1 ; i >= 0; --i) {
var scopeLabels = _scopeStack[i]["(labels)"];
if (_.has(scopeLabels, labelName)) {
if (scopeLabels[labelName]) {
return scopeLabels;
}
}
Expand All @@ -172,7 +177,7 @@ var scopeManager = function(state, predefined, exported, declared) {
// used so far in this whole function and any sub functions
for (var i = _scopeStack.length - 1; i >= 0; i--) {
var current = _scopeStack[i];
if (_.has(current["(usages)"], labelName)) {
if (current["(usages)"][labelName]) {
return current["(usages)"][labelName];
}
if (current === _currentFunct) {
Expand All @@ -199,10 +204,10 @@ var scopeManager = function(state, predefined, exported, declared) {
if (!isNewFunction && _scopeStack[i + 1] === _currentFunct) {
outsideCurrentFunction = false;
}
if (outsideCurrentFunction && _.has(stackItem["(labels)"], labelName)) {
if (outsideCurrentFunction && stackItem["(labels)"][labelName]) {
warning("W123", token, labelName);
}
if (_.has(stackItem["(breakLabels)"], labelName)) {
if (stackItem["(breakLabels)"][labelName]) {
warning("W123", token, labelName);
}
}
Expand Down Expand Up @@ -248,19 +253,24 @@ var scopeManager = function(state, predefined, exported, declared) {
},

unstack: function() {

// jshint proto: true
var subScope = _scopeStack.length > 1 ? _scopeStack[_scopeStack.length - 2] : null;
var isUnstackingFunction = _current === _currentFunct;

var i, j;
var currentUsages = _current["(usages)"];
var currentLabels = _current["(labels)"];
var usedLabelNameList = Object.keys(currentUsages);

if (currentUsages.__proto__ && usedLabelNameList.indexOf("__proto__") === -1) {
usedLabelNameList.push("__proto__");
}

for (i = 0; i < usedLabelNameList.length; i++) {
var usedLabelName = usedLabelNameList[i];

var usage = currentUsages[usedLabelName];
var usedLabel = _.has(currentLabels, usedLabelName) && currentLabels[usedLabelName];
var usedLabel = currentLabels[usedLabelName];
if (usedLabel) {

if (usedLabel["(useOutsideOfScope)"] && !state.option.funcscope) {
Expand Down Expand Up @@ -300,7 +310,7 @@ var scopeManager = function(state, predefined, exported, declared) {

if (subScope) {
// not exiting the global scope, so copy the usage down in case its an out of scope usage
if (!_.has(subScope["(usages)"], usedLabelName)) {
if (!subScope["(usages)"][usedLabelName]) {
subScope["(usages)"][usedLabelName] = usage;
if (isUnstackingFunction) {
subScope["(usages)"][usedLabelName]["(onlyUsedSubFunction)"] = true;
Expand All @@ -315,13 +325,13 @@ var scopeManager = function(state, predefined, exported, declared) {
}
} else {
// this is exiting global scope, so we finalise everything here - we are at the end of the file
if (_.has(_current["(predefined)"], usedLabelName)) {
if (typeof _current["(predefined)"][usedLabelName] === "boolean") {

// remove the declared token, so we know it is used
delete declared[usedLabelName];

// note it as used so it can be reported
usedPredefinedAndGlobals[usedLabelName] = true;
usedPredefinedAndGlobals[usedLabelName] = marker;

// check for re-assigning a read-only (set to false) predefined
if (_current["(predefined)"][usedLabelName] === false && usage["(reassigned)"]) {
Expand All @@ -342,7 +352,7 @@ var scopeManager = function(state, predefined, exported, declared) {
if (state.option.undef && !undefinedToken.ignoreUndef) {
warning("W117", undefinedToken, usedLabelName);
}
if (_.has(impliedGlobals, usedLabelName)) {
if (impliedGlobals[usedLabelName]) {
impliedGlobals[usedLabelName].line.push(undefinedToken.line);
} else {
impliedGlobals[usedLabelName] = {
Expand Down Expand Up @@ -439,15 +449,43 @@ var scopeManager = function(state, predefined, exported, declared) {
},

getUsedOrDefinedGlobals: function() {
return Object.keys(usedPredefinedAndGlobals);
// jshint proto: true
var list = Object.keys(usedPredefinedAndGlobals);

// If `__proto__` is used as a global variable name, its entry in the
// lookup table may not be enumerated by `Object.keys` (depending on the
// environment).
if (usedPredefinedAndGlobals.__proto__ === marker &&
list.indexOf("__proto__") === -1) {
list.push("__proto__");
}

return list;
},

/**
* Gets an array of implied globals
* @returns {Array.<{ name: string, line: Array.<number>}>}
*/
getImpliedGlobals: function() {
return _.values(impliedGlobals);
// jshint proto: true
var values = _.values(impliedGlobals);
var hasProto = false;

// If `__proto__` is an implied global variable, its entry in the lookup
// table may not be enumerated by `_.values` (depending on the
// environment).
if (impliedGlobals.__proto__) {
hasProto = values.some(function(value) {
return value.name === "__proto__";
});

if (!hasProto) {
values.push(impliedGlobals.__proto__);
}
}

return values;
},

/**
Expand Down Expand Up @@ -502,15 +540,15 @@ var scopeManager = function(state, predefined, exported, declared) {
// if is block scoped (let or const)
if (isblockscoped) {

var declaredInCurrentScope = _.has(_current["(labels)"], labelName);
var declaredInCurrentScope = _current["(labels)"][labelName];
// for block scoped variables, params are seen in the current scope as the root function
// scope, so check these too.
if (!declaredInCurrentScope && _current === _currentFunct && !_current["(global)"]) {
declaredInCurrentScope = _.has(_currentFunct["(parent)"]["(labels)"], labelName);
declaredInCurrentScope = !!_currentFunct["(parent)"]["(labels)"][labelName];
}

// if its not already defined (which is an error, so ignore) and is used in TDZ
if (!declaredInCurrentScope && _.has(_current["(usages)"], labelName)) {
if (!declaredInCurrentScope && _current["(usages)"][labelName]) {
var usage = _current["(usages)"][labelName];
// if its in a sub function it is not necessarily an error, just latedef
if (usage["(onlyUsedSubFunction)"]) {
Expand Down Expand Up @@ -563,7 +601,7 @@ var scopeManager = function(state, predefined, exported, declared) {
scopeManagerInst.funct.add(labelName, type, token, true);

if (state.funct["(global)"]) {
usedPredefinedAndGlobals[labelName] = true;
usedPredefinedAndGlobals[labelName] = marker;
}
}
},
Expand All @@ -584,7 +622,7 @@ var scopeManager = function(state, predefined, exported, declared) {
var currentScopeIndex = _scopeStack.length - (options && options.excludeCurrent ? 2 : 1);
for (var i = currentScopeIndex; i >= 0; i--) {
var current = _scopeStack[i];
if (_.has(current["(labels)"], labelName) &&
if (current["(labels)"][labelName] &&
(!onlyBlockscoped || current["(labels)"][labelName]["(blockscoped)"])) {
return current["(labels)"][labelName]["(type)"];
}
Expand All @@ -604,7 +642,7 @@ var scopeManager = function(state, predefined, exported, declared) {
for (var i = _scopeStack.length - 1; i >= 0; i--) {
var current = _scopeStack[i];

if (_.has(current["(breakLabels)"], labelName)) {
if (current["(breakLabels)"][labelName]) {
return true;
}
if (current["(isParams)"] === "function") {
Expand Down Expand Up @@ -652,7 +690,7 @@ var scopeManager = function(state, predefined, exported, declared) {
// to the unset var
// first check the param is used
var paramScope = _currentFunct["(parent)"];
if (paramScope && _.has(paramScope["(labels)"], labelName) &&
if (paramScope && paramScope["(labels)"][labelName] &&
paramScope["(labels)"][labelName]["(type)"] === "param") {

// then check its not used
Expand Down
Loading

0 comments on commit 925a983

Please sign in to comment.