Skip to content

Commit

Permalink
[[FIX]] default params can't reference future arg
Browse files Browse the repository at this point in the history 
Change default argument processing to error if you attempt to access a
future argument or the argument you are assigning yourself.
Fixes #2422
  • Loading branch information
@lukeapage
lukeapage committed Jul 19, 2015
1 parent 3e55c35 commit bc2741c
Show file tree
Hide file tree
Showing 4 changed files with 120 additions and 54 deletions.
50 changes: 29 additions & 21 deletions src/jshint.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2717,11 +2717,11 @@ var JSHINT = (function() {
* single-argument shorthand.
* @param {bool} [options.parsedOpening] Whether the opening parenthesis has
* already been parsed.
* @returns {Array.<{ id: string, token: Token}>}
* @returns {Array.<string>} array of param identifiers
*/
function functionparams(options) {
var next;
var params = [];
var paramsIds = [];
var ident;
var tokens = [];
var t;
Expand All @@ -2730,7 +2730,8 @@ var JSHINT = (function() {
var loneArg = options && options.loneArg;

if (loneArg && loneArg.identifier === true) {
return [ { id: loneArg.value, token: loneArg }];
state.funct["(scope)"].addParam(loneArg.value, loneArg);
return [ loneArg.value ];
}

next = state.tokens.next;
Expand All @@ -2744,20 +2745,30 @@ var JSHINT = (function() {
return;
}

function addParam(addParamArgs) {
state.funct["(scope)"].addParam.apply(state.funct["(scope)"], addParamArgs);
}

for (;;) {
// store the current param(s) of this loop so we can evaluate the default argument before parameters
// are added to the param scope
var currentParams = [];

if (_.contains(["{", "["], state.tokens.next.id)) {
tokens = destructuringExpression();
for (t in tokens) {
t = tokens[t];
if (t.id) {
params.push({ id: t.id, token: t });
paramsIds.push(t.id);
currentParams.push([t.id, t]);
}
}
} else {
if (checkPunctuators(state.tokens.next, ["..."])) pastRest = true;
ident = identifier(true);
if (ident) {
params.push({ id: ident, token: state.tokens.curr });
paramsIds.push(ident);
currentParams.push([ident, state.tokens.curr]);
} else {
// Skip invalid parameter.
while (!checkPunctuators(state.tokens.next, [",", ")"])) advance();
Expand All @@ -2778,14 +2789,18 @@ var JSHINT = (function() {
pastDefault = true;
expression(10);
}

// now we have evaluated the default expression, add the variable to the param scope
currentParams.forEach(addParam);

if (state.tokens.next.id === ",") {
if (pastRest) {
warning("W131", state.tokens.next);
}
comma();
} else {
advance(")", next);
return params;
return paramsIds;
}
}
}
Expand Down Expand Up @@ -2938,16 +2953,11 @@ var JSHINT = (function() {
classExprBinding ? "class" : "function", state.tokens.curr, false);
}

var params = functionparams(options);
var paramsIds;
if (params) {
paramsIds = _.map(params, function(token) {
return token.id;
});
}
// create the param scope (params added in functionparams)
state.funct["(scope)"].stackParams(true);

var paramsIds = functionparams(options);

// create the param scope and adds the labels to it
state.funct["(scope)"].stackParams(params, true);
state.funct["(params)"] = paramsIds;
state.funct["(metrics)"].verifyMaxParametersPerFunction(paramsIds);

Expand Down Expand Up @@ -3806,27 +3816,25 @@ var JSHINT = (function() {
var b;

function doCatch() {
var params = [];

advance("catch");
advance("(");

state.funct["(scope)"].stackParams();

if (checkPunctuators(state.tokens.next, ["[", "{"])) {
var tokens = destructuringExpression();
_.each(tokens, function(token) {
if (token.id) {
params.push({ id: token.id, token: token });
state.funct["(scope)"].addParam(token.id, token, "exception");
}
});
} else if (state.tokens.next.type !== "(identifier)") {
warning("E030", state.tokens.next, state.tokens.next.value);
} else {
// only advance if we have an identifier so we can continue parsing in the most common error - that no param is given.
params.push({ id: identifier(), token: state.tokens.curr, type: "exception" });
state.funct["(scope)"].addParam(identifier(), state.tokens.curr, "exception");
}

state.funct["(scope)"].stackParams(params);

if (state.tokens.next.value === "if") {
if (!state.inMoz()) {
warning("W118", state.tokens.curr, "catch filter");
Expand Down
68 changes: 42 additions & 26 deletions src/scope-manager.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ var scopeManager = function(state, predefined, exported, declared) {
*/
function _checkForUnused() {
// function params are handled specially
// assume that parameters are the only thing declared in the param scope
if (_current["(isParams)"] === "function") {
_checkParams();
return;
Expand Down Expand Up @@ -412,42 +413,57 @@ var scopeManager = function(state, predefined, exported, declared) {
_current = subScope;
},

/**
* Add a param to the current scope
* @param {string} labelName
* @param {Token} token
* @param {string} [type="param"] param type
*/
addParam: function(labelName, token, type) {
type = type || "param";

if (type === "exception") {
// if defined in the current function
var previouslyDefinedLabelType = this.funct.labeltype(labelName);
if (previouslyDefinedLabelType && previouslyDefinedLabelType !== "exception") {
// and has not been used yet in the current function scope
if (!state.option.node) {
warning("W002", state.tokens.next, labelName);
}
}
}

_checkOuterShadow(labelName, token, type);

if (_.has(_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)"]) {
_latedefWarning(type, labelName, token);
} else {
// this is a clear illegal usage for block scoped variables
warning("E056", token, labelName, type);
}
}

_current["(labels)"][labelName] = {
"(type)" : type,
"(token)": token,
"(unused)": true };
_current["(params)"].push(labelName);
},

/**
* Tell the manager we are entering a new scope with parameters
* Call stack after all parameters have been added
* @param isFunction Whether the scope is a function or not
*/
stackParams: function(params, isFunction) {
stackParams: function(isFunction) {
_current = _newScope();
_current["(isParams)"] = isFunction ? "function" : true;
_current["(params)"] = [];

_scopeStack.push(_current);
var functionScope = this.funct;

_.each(params, function(param) {

var type = param.type || "param";

if (type === "exception") {
// if defined in the current function
var previouslyDefinedLabelType = functionScope.labeltype(param.id);
if (previouslyDefinedLabelType && previouslyDefinedLabelType !== "exception") {
// and has not been used yet in the current function scope
if (!state.option.node) {
warning("W002", state.tokens.next, param.id);
}
}
}

_checkOuterShadow(param.id, param.token, type);

_current["(labels)"][param.id] = {
"(type)" : type,
"(token)": param.token,
"(unused)": true };
_current["(params)"].push(param.id);
});
},

getUsedOrDefinedGlobals: function() {
Expand Down
34 changes: 28 additions & 6 deletions tests/unit/core.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1435,17 +1435,39 @@ exports.testPotentialVariableLeak = function (test) {
exports.testDefaultArguments = function (test) {
var src = fs.readFileSync(__dirname + "/fixtures/default-arguments.js", "utf8");
TestRun(test)
.addError(11, "Regular parameters cannot come after default parameters.")
.test(src, { esnext: true });

TestRun(test)
.addError(11, "Regular parameters cannot come after default parameters.")
.addError(14, "'bar' is not defined.")
.addError(14, "'num3' was used before it was declared, which is illegal for 'param' variables.")
.addError(15, "'num4' was used before it was declared, which is illegal for 'param' variables.")
.addError(18, "Regular parameters cannot come after default parameters.")
.addError(27, "'c' is not defined.")
.addError(33, "'d' was used before it was defined.")
.addError(36, "'e' was used before it was declared, which is illegal for 'param' variables.")
.test(src, { esnext: true, undef: true, latedef: true });

TestRun(test)
.addError(14, "'num3' was used before it was declared, which is illegal for 'param' variables.")
.addError(15, "'num4' was used before it was declared, which is illegal for 'param' variables.")
.addError(18, "Regular parameters cannot come after default parameters.")
.addError(36, "'e' was used before it was declared, which is illegal for 'param' variables.")
.test(src, { moz: true });

TestRun(test)
.addError(7, "'default parameters' is only available in ES6 (use esnext option).")
.addError(11, "'default parameters' is only available in ES6 (use esnext option).")
.addError(11, "Regular parameters cannot come after default parameters.")
.addError(12, "'default parameters' is only available in ES6 (use esnext option).")
.addError(13, "'default parameters' is only available in ES6 (use esnext option).")
.addError(14, "'default parameters' is only available in ES6 (use esnext option).")
.addError(14, "'num3' was used before it was declared, which is illegal for 'param' variables.")
.addError(15, "'default parameters' is only available in ES6 (use esnext option).")
.addError(15, "'num4' was used before it was declared, which is illegal for 'param' variables.")
.addError(18, "'default parameters' is only available in ES6 (use esnext option).")
.addError(18, "Regular parameters cannot come after default parameters.")
.addError(26, "'default parameters' is only available in ES6 (use esnext option).")
.addError(31, "'default parameters' is only available in ES6 (use esnext option).")
.addError(33, "'default parameters' is only available in ES6 (use esnext option).")
.addError(35, "'default parameters' is only available in ES6 (use esnext option).")
.addError(36, "'default parameters' is only available in ES6 (use esnext option).")
.addError(36, "'e' was used before it was declared, which is illegal for 'param' variables.")
.test(src, { });

test.done();
Expand Down
22 changes: 21 additions & 1 deletion tests/unit/fixtures/default-arguments.js
View file
Original file line number Diff line number Diff line change
@@ -1,13 +1,20 @@
function Doit() {
}

var foo = true;
Doit.prototype = {
_someProperty: null,

test: function(num, num2=1, num3=2) {
return num === num2;
},

testUseOfVariables: function(num = foo, // Ok, defined above
num1 = num, // Ok, previous argument
num2 = num3, // warn, next argument
num3 = bar, // warn, not defined
num4 = num4) { // warn, self reference
},

testBadDefault: function(num, num2=1, num3) {
return num === num2;
},
Expand All @@ -16,3 +23,16 @@ Doit.prototype = {
return this._someProperty;
},
};
function test1(a = function() {
return c; // warn - wrong scope
}) {
var c;
}
function test2(a = function() {
return d; // cannot tell if it is declared yet at point of evaluation
}, d = true) {
}
function test2(a = e, // next param and outside scope - this acts like a reference in TDZ and throws
e = true) {
}
var e;

0 comments on commit bc2741c

Please sign in to comment.