Skip to content

Commit

Permalink
[[FIX]] use of params is not capturing loopfunc
Browse files Browse the repository at this point in the history 
Previously the scope manager only considered the function body for usages
that went outside scope, indicating "capturing" that should trigger
loopfunc. This corrects that.

Also affected are function names, but in the case of declarations there is
a different warning and in the case of expressions the name available to
the function will be correct even within a loop.
  • Loading branch information
@lukeapage @jugglinmike
lukeapage authored and jugglinmike committed Jul 31, 2015
1 parent 9d021ee commit 827e335
Show file tree
Hide file tree
Showing 4 changed files with 71 additions and 61 deletions.
13 changes: 6 additions & 7 deletions src/jshint.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2971,15 +2971,15 @@ var JSHINT = (function() {
// test for unused (unused: false)
// it is a new block scope so that params can override it, it can be block scoped
// but declarations inside the function don't cause already declared error
state.funct["(scope)"].stack("functionouter");
var internallyAccessibleName = name || classExprBinding;
if (internallyAccessibleName) {
state.funct["(scope)"].stack();
state.funct["(scope)"].block.add(internallyAccessibleName,
classExprBinding ? "class" : "function", state.tokens.curr, false);
}

// create the param scope (params added in functionparams)
state.funct["(scope)"].stackParams(true);
state.funct["(scope)"].stack("functionparams");

var paramsInfo = functionparams(options);

Expand Down Expand Up @@ -3016,12 +3016,11 @@ var JSHINT = (function() {
state.funct["(last)"] = state.tokens.curr.line;
state.funct["(lastcharacter)"] = state.tokens.curr.character;

// unstack the params scope
state.funct["(scope)"].unstack(); // also does usage and label checks

// if we have a name, unstack that scope (see above where it is stacked)
if (internallyAccessibleName) {
state.funct["(scope)"].unstack();
}
// unstack the function outer stack
state.funct["(scope)"].unstack();

state.funct = state.funct["(context)"];

Expand Down Expand Up @@ -3871,7 +3870,7 @@ var JSHINT = (function() {
advance("catch");
advance("(");

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

if (checkPunctuators(state.tokens.next, ["[", "{"])) {
var tokens = destructuringPattern();
Expand Down
114 changes: 61 additions & 53 deletions src/scope-manager.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,21 +13,26 @@ var marker = {};
* and resolving when variables are used and undefined
*/
var scopeManager = function(state, predefined, exported, declared) {
function _newScope() {
return {

var _current;
var _scopeStack = [];

function _newScope(type) {
_current = {
"(labels)": Object.create(null),
"(usages)": Object.create(null),
"(breakLabels)": Object.create(null),
"(parent)": _current
"(parent)": _current,
"(type)": type,
"(params)": (type === "functionparams" || type === "catchparams") ? [] : null
};
_scopeStack.push(_current);
}

var _current = _newScope();
_newScope("global");
_current["(predefined)"] = predefined;
_current["(global)"] = true;

var _currentFunct = _current; // this is the block after the params = function
var _scopeStack = [_current];
var _currentFunctBody = _current; // this is the block after the params = function

var usedPredefinedAndGlobals = Object.create(null);
var impliedGlobals = Object.create(null);
Expand Down Expand Up @@ -65,7 +70,7 @@ var scopeManager = function(state, predefined, exported, declared) {
_setupUsages(labelName);

if (token) {
token["(function)"] = _currentFunct;
token["(function)"] = _currentFunctBody;
_current["(usages)"][labelName]["(tokens)"].push(token);
}
}
Expand Down Expand Up @@ -122,7 +127,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") {
if (_current["(type)"] === "functionparams") {
_checkParams();
return;
}
Expand All @@ -143,6 +148,11 @@ var scopeManager = function(state, predefined, exported, declared) {
*/
function _checkParams() {
var params = _current["(params)"];

if (!params) {
return;
}

var param = params.pop();
var unused_opt;

Expand Down Expand Up @@ -186,7 +196,7 @@ var scopeManager = function(state, predefined, exported, declared) {
if (current["(usages)"][labelName]) {
return current["(usages)"][labelName];
}
if (current === _currentFunct) {
if (current === _currentFunctBody) {
break;
}
}
Expand All @@ -200,14 +210,14 @@ var scopeManager = function(state, predefined, exported, declared) {
return;
}

var isGlobal = _currentFunct["(global)"],
isNewFunction = _current["(isParams)"] === "function";
var isGlobal = _currentFunctBody["(type)"] === "global",
isNewFunction = _current["(type)"] === "functionparams";

var outsideCurrentFunction = !isGlobal;
for (var i = 0; i < _scopeStack.length; i++) {
var stackItem = _scopeStack[i];

if (!isNewFunction && _scopeStack[i + 1] === _currentFunct) {
if (!isNewFunction && _scopeStack[i + 1] === _currentFunctBody) {
outsideCurrentFunction = false;
}
if (outsideCurrentFunction && stackItem["(labels)"][labelName]) {
Expand Down Expand Up @@ -244,24 +254,28 @@ var scopeManager = function(state, predefined, exported, declared) {

/**
* Tell the manager we are entering a new block of code
* @param {string} [type] - The type of the block. Valid values are
* "functionparams", "catchparams" and
* "functionouter"
*/
stack: function() {
stack: function(type) {
var previousScope = _current;
_current = _newScope();
_newScope(type);

if (previousScope["(isParams)"] === "function") {
if (!type && previousScope["(type)"] === "functionparams") {

_current["(isFunc)"] = true;
_current["(context)"] = _currentFunct;
_currentFunct = _current;
_current["(isFuncBody)"] = true;
_current["(context)"] = _currentFunctBody;
_currentFunctBody = _current;
}
_scopeStack.push(_current);
},

unstack: function() {
// jshint proto: true
var subScope = _scopeStack.length > 1 ? _scopeStack[_scopeStack.length - 2] : null;
var isUnstackingFunction = _current === _currentFunct;
var isUnstackingFunctionBody = _current === _currentFunctBody,
isUnstackingFunctionParams = _current["(type)"] === "functionparams",
isUnstackingFunctionOuter = _current["(type)"] === "functionouter";

var i, j;
var currentUsages = _current["(usages)"];
Expand Down Expand Up @@ -312,15 +326,15 @@ var scopeManager = function(state, predefined, exported, declared) {
continue;
}

if (isUnstackingFunction) {
if (isUnstackingFunctionOuter) {
state.funct["(isCapturing)"] = true;
}

if (subScope) {
// not exiting the global scope, so copy the usage down in case its an out of scope usage
if (!subScope["(usages)"][usedLabelName]) {
subScope["(usages)"][usedLabelName] = usage;
if (isUnstackingFunction) {
if (isUnstackingFunctionBody) {
subScope["(usages)"][usedLabelName]["(onlyUsedSubFunction)"] = true;
}
} else {
Expand Down Expand Up @@ -384,7 +398,8 @@ var scopeManager = function(state, predefined, exported, declared) {
}

// if we have a sub scope we can copy too and we are still within the function boundary
if (subScope && !isUnstackingFunction && _current["(isParams)"] !== "function") {
if (subScope && !isUnstackingFunctionBody &&
!isUnstackingFunctionParams && !isUnstackingFunctionOuter) {
var labelNames = Object.keys(currentLabels);
for (i = 0; i < labelNames.length; i++) {

Expand All @@ -397,7 +412,7 @@ var scopeManager = function(state, predefined, exported, declared) {
!this.funct.has(defLabelName, { excludeCurrent: true })) {
subScope["(labels)"][defLabelName] = currentLabels[defLabelName];
// we do not warn about out of scope usages in the global scope
if (!_currentFunct["(global)"]) {
if (_currentFunctBody["(type)"] !== "global") {
subScope["(labels)"][defLabelName]["(useOutsideOfScope)"] = true;
}
delete currentLabels[defLabelName];
Expand All @@ -408,10 +423,10 @@ var scopeManager = function(state, predefined, exported, declared) {
_checkForUnused();

_scopeStack.pop();
if (isUnstackingFunction) {
_currentFunct = _scopeStack[_.findLastIndex(_scopeStack, function(scope) {
if (isUnstackingFunctionBody) {
_currentFunctBody = _scopeStack[_.findLastIndex(_scopeStack, function(scope) {
// if function or if global (which is at the bottom so it will only return true if we call back)
return scope["(isFunc)"] || scope["(global)"];
return scope["(isFuncBody)"] || scope["(type)"] === "global";
})];
}

Expand Down Expand Up @@ -451,6 +466,7 @@ var scopeManager = function(state, predefined, exported, declared) {
"(type)" : type,
"(token)": token,
"(unused)": true };

_current["(params)"].push(labelName);
}

Expand All @@ -468,12 +484,16 @@ var scopeManager = function(state, predefined, exported, declared) {

validateParams: function() {
// This method only concerns errors for function parameters
if (_currentFunct["(global)"]) {
if (_currentFunctBody["(type)"] === "global") {
return;
}

var isStrict = state.isStrict();
var currentFunctParamScope = _currentFunct["(parent)"];
var currentFunctParamScope = _currentFunctBody["(parent)"];

if (!currentFunctParamScope["(params)"]) {
return;
}

currentFunctParamScope["(params)"].forEach(function(labelName) {
var label = currentFunctParamScope["(labels)"][labelName];
Expand All @@ -488,19 +508,6 @@ var scopeManager = function(state, predefined, exported, declared) {
});
},

/**
* 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(isFunction) {
_current = _newScope();
_current["(isParams)"] = isFunction ? "function" : true;
_current["(params)"] = [];

_scopeStack.push(_current);
},

getUsedOrDefinedGlobals: function() {
// jshint proto: true
var list = Object.keys(usedPredefinedAndGlobals);
Expand Down Expand Up @@ -596,8 +603,9 @@ var scopeManager = function(state, predefined, exported, declared) {
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 = !!_currentFunct["(parent)"]["(labels)"][labelName];
if (!declaredInCurrentScope && _current === _currentFunctBody &&
_current["(type)"] !== "global") {
declaredInCurrentScope = !!_currentFunctBody["(parent)"]["(labels)"][labelName];
}

// if its not already defined (which is an error, so ignore) and is used in TDZ
Expand Down Expand Up @@ -645,15 +653,15 @@ var scopeManager = function(state, predefined, exported, declared) {
if (declaredInCurrentFunctionScope && labelName !== "__proto__") {

// see https://github.com/jshint/jshint/issues/2400
if (!_currentFunct["(global)"]) {
if (_currentFunctBody["(type)"] !== "global") {
warning("W004", token, labelName);
}
}
}

scopeManagerInst.funct.add(labelName, type, token, true);

if (state.funct["(global)"]) {
if (_currentFunctBody["(type)"] === "global") {
usedPredefinedAndGlobals[labelName] = marker;
}
}
Expand All @@ -680,7 +688,7 @@ var scopeManager = function(state, predefined, exported, declared) {
return current["(labels)"][labelName]["(type)"];
}
var scopeCheck = excludeParams ? _scopeStack[ i - 1 ] : current;
if (scopeCheck && scopeCheck["(isParams)"] === "function") {
if (scopeCheck && scopeCheck["(type)"] === "functionparams") {
return null;
}
}
Expand All @@ -698,15 +706,15 @@ var scopeManager = function(state, predefined, exported, declared) {
if (current["(breakLabels)"][labelName]) {
return true;
}
if (current["(isParams)"] === "function") {
if (current["(type)"] === "functionparams") {
return false;
}
}
return false;
},
/**
* Returns if the label is in the current function scope
* See state.funct.labelType for options
* See scopeManager.funct.labelType for options
*/
has: function(labelName, options) {
return Boolean(this.labeltype(labelName, options));
Expand All @@ -721,7 +729,7 @@ var scopeManager = function(state, predefined, exported, declared) {
"(type)" : type,
"(token)": tok,
"(blockscoped)": false,
"(function)": _currentFunct,
"(function)": _currentFunctBody,
"(unused)": unused };
}
},
Expand All @@ -733,7 +741,7 @@ var scopeManager = function(state, predefined, exported, declared) {
* @returns Boolean
*/
isGlobal: function() {
return _current["(global)"];
return _current["(type)"] === "global";
},

use: function(labelName, token) {
Expand All @@ -742,7 +750,7 @@ var scopeManager = function(state, predefined, exported, declared) {
// this is because function(a) { var a = a; } will resolve to the param, not
// to the unset var
// first check the param is used
var paramScope = _currentFunct["(parent)"];
var paramScope = _currentFunctBody["(parent)"];
if (paramScope && paramScope["(labels)"][labelName] &&
paramScope["(labels)"][labelName]["(type)"] === "param") {

Expand Down
4 changes: 4 additions & 0 deletions tests/unit/fixtures/loopfunc.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,7 @@ function boo(p) {
while (true) {
var d = function z() { return z(); };
}

for (i = 0; i < 5; i++) {
c = function(a,b,i) { return i; };
}
1 change: 0 additions & 1 deletion tests/unit/options.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1198,7 +1198,6 @@ exports.loopfunc = function (test) {
.addError(8, "Don't make functions within a loop.")
.addError(20, "Don't make functions within a loop.")
.addError(25, "Don't make functions within a loop.")
.addError(30, "Don't make functions within a loop.")
.addError(12, "Function declarations should not be placed in blocks. Use a function " +
"expression or move the statement to the top of the outer function.")
.test(src, {es3: true});
Expand Down

0 comments on commit 827e335

Please sign in to comment.