Skip to content

Commit

Permalink
Make 'unused' check smarter about unused arguments.
Browse files Browse the repository at this point in the history 
This patch modifies 'unused' checks so that they ignore arguments
that were never used but that were followed by used arguments. For
example:

    function one(a, b) {
        return b;
    }

In this example, since unused 'a' is followed by used 'b', JSHint
assumes that this is a backwards compatibility issue or a specific
API pattern and suppresses the warning.

But this example, on the other hand, will generate a warning for 'b'
because there is no reason to keep it around:

    function one(a, b) {
        return a;
    }

References:

    Closes GH-607
  • Loading branch information
@valueof
valueof committed Aug 16, 2012
1 parent c035bc5 commit 5751c5e
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 61 deletions.
95 changes: 61 additions & 34 deletions jshint.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@
moveTo, mootools, multistr, name, navigator, new, newcap, noarg, node, noempty, nomen,
nonew, nonstandard, nud, onbeforeunload, onblur, onerror, onevar, onecase, onfocus,
onload, onresize, onunload, open, openDatabase, openURL, opener, opera, options, outer, param,
parent, parseFloat, parseInt, passfail, plusplus, postMessage, predef, print, process, prompt,
parent, parseFloat, parseInt, passfail, plusplus, postMessage, pop, predef, print, process, prompt,
proto, prototype, prototypejs, provides, push, quit, quotmark, range, raw, reach, reason, regexp,
readFile, readUrl, regexdash, removeEventListener, replace, report, require,
reserved, resizeBy, resizeTo, resolvePath, resumeUpdates, respond, rhino, right,
Expand Down Expand Up @@ -1745,7 +1745,6 @@ klass: do {


function addlabel(t, type, token) {

if (t === "hasOwnProperty") {
warning("'hasOwnProperty' is a really bad name.");
}
Expand Down Expand Up @@ -2340,18 +2339,22 @@ loop: for (;;) {

function assignop(s) {
symbol(s, 20).exps = true;

return infix(s, function (left, that) {
that.left = left;

if (predefined[left.value] === false &&
scope[left.value]["(global)"] === true) {
warning("Read only.", left);
} else if (left["function"]) {
warning("'{a}' is a function.", left, left.value);
}

if (left) {
if (option.esnext && funct[left.value] === "const") {
warning("Attempting to override '{a}' which is a constant", left, left.value);
}

if (left.id === "." || left.id === "[") {
if (!left.left || left.left.value === "arguments") {
warning("Bad assignment.", that);
Expand All @@ -2365,12 +2368,14 @@ loop: for (;;) {
that.right = expression(19);
return that;
}

if (left === syntax["function"]) {
warning(
"Expected an identifier in an assignment and instead saw a function invocation.",
token);
}
}

error("Bad assignment.", that);
}, 20);
}
Expand Down Expand Up @@ -3330,23 +3335,28 @@ loop: for (;;) {


function functionparams() {
var i, t = nexttoken, p = [];
var next = nexttoken;
var params = [];
var ident;

advance("(");
nospace();

if (nexttoken.id === ")") {
advance(")");
return;
}

for (;;) {
i = identifier(true);
p.push(i);
addlabel(i, "unused", token);
ident = identifier(true);
params.push(ident);
addlabel(ident, "unused", token);
if (nexttoken.id === ",") {
comma();
} else {
advance(")", t);
advance(")", next);
nospace(prevtoken, token);
return p;
return params;
}
}
}
Expand Down Expand Up @@ -4359,30 +4369,35 @@ loop: for (;;) {
implied[name] = newImplied;
};

var warnUnused = function (name, token) {
var line = token.line;
var chr = token.character;

if (option.unused)
warningAt("'{a}' is defined but never used.", line, chr, name);

unuseds.push({
name: name,
line: line,
character: chr
});
};

var checkUnused = function (func, key) {
var type = func[key];
var token = func["(tokens)"][key];

if (key.charAt(0) === "(")
return;

// 'undefined' is a special case for (function (window, undefined) { ... })();
// patterns.

if (key === "undefined")
return;

if (type !== "unused" && type !== "unction")
return;

if (option.unused)
warningAt("'{a}' is defined but never used.", token.line, token.character, key);
// Params are checked separately from other variables.
if (func["(params)"] && func["(params)"].indexOf(key) !== -1)
return;

unuseds.push({
name: key,
line: token.line,
character: token.character
});
warnUnused(key, token);
};

// Check queued 'x is not defined' instances to see if they're still undefined.
Expand All @@ -4402,22 +4417,34 @@ loop: for (;;) {
checkUnused(func, key);
}
}

if (!func["(params)"])
return;

var params = func["(params)"].slice();
var param = params.pop();
var type;

while (param) {
type = func[param];

// 'undefined' is a special case for (function (window, undefined) { ... })();
// patterns.

if (param === "undefined")
return;

if (type !== "unused" && type !== "unction")
return;

warnUnused(param, func["(tokens)"][param]);
param = params.pop();
}
});

for (var key in declared) {
if (is_own(declared, key)) {
if (!is_own(global, key)) {
if (option.unused) {
warningAt("'{a}' is defined but never used.",
declared[key].line, declared[key].character, key);
}

unuseds.push({
name: key,
line: declared[key].line,
character: declared[key].character
});
}
if (is_own(declared, key) && !is_own(global, key)) {
warnUnused(key, declared[key]);
}
}
} catch (e) {
Expand Down
31 changes: 5 additions & 26 deletions tests/regression/jquery.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,13 @@ exports.jQuery_1_7 = function () {
var globals = { DOMParser: false, ActiveXObject: false, define: false };

TestRun()
.addError(77, "'all' is defined but never used.")
.addError(551, "'name' is defined but never used.")
.addError(903, "'i' is defined but never used.")
.addError(1044, "'actual' is defined but never used.")
.addError(1312, "'pCount' is defined but never used.")
.addError(1369, "'events' is defined but never used.")
.addError(1607, "'table' is defined but never used.")
.addError(1710, "'internalKey' is defined but never used.")
.addError(1813, "'internalKey' is defined but never used.")
.addError(2760, "'i' is defined but never used.")
.addError(2787, "'i' is defined but never used.")
.addError(2818, "Expected an assignment or function call and instead saw an expression.")
.addError(2822, "Expected an assignment or function call and instead saw an expression.")
.addError(2859, "'rnamespaces' is defined but never used.")
Expand All @@ -29,36 +25,19 @@ exports.jQuery_1_7 = function () {
.addError(2863, "'rescape' is defined but never used.")
.addError(2900, "'quick' is defined but never used.")
.addError(3269, "'related' is defined but never used.")
.addError(3442, "'data' is defined but never used.")
.addError(3442, "'namespaces' is defined but never used.")
.addError(3449, "'namespaces' is defined but never used.")
.addError(3592, "'selector' is defined but never used.")
.addError(3889, "'i' is defined but never used.")
.addError(4465, "'curLoop' is defined but never used.")
.addError(4496, "'curLoop' is defined but never used.")
.addError(4496, "'inplace' is defined but never used.")
.addError(4496, "'result' is defined but never used.")
.addError(4496, "'not' is defined but never used.")
.addError(4560, "Expected an assignment or function call and instead saw an expression.")
.addError(4574, "'i' is defined but never used.")
.addError(4633, "'elem' is defined but never used.")
.addError(4637, "'elem' is defined but never used.")
.addError(4637, "'match' is defined but never used.")
.addError(4641, "'elem' is defined but never used.")
.addError(4645, "'elem' is defined but never used.")
.addError(4649, "'elem' is defined but never used.")
.addError(4653, "'elem' is defined but never used.")
.addError(4657, "'elem' is defined but never used.")
.addError(4661, "'elem' is defined but never used.")
.addError(4694, "'cache' is defined but never used.")
.addError(4702, "Mixed spaces and tabs.")
.addError(4712, "Expected a 'break' statement before 'case'.")
.addError(4715, "Mixed spaces and tabs.")
.addError(4818, "'all' is defined but never used.")
.addError(4843, "Expected an assignment or function call and instead saw an expression.")
.addError(5552, "'i' is defined but never used.")
.addError(5234, "'nodeCheck' is defined but never used.")
.addError(5267, "'nodeCheck' is defined but never used.")
.addError(5635, "'elem' is defined but never used.")
.addError(5675, "'i' is defined but never used.")
.addError(5691, "'i' is defined but never used.")
.addError(7141, "'i' is defined but never used.")
.addError(6061, "'cur' is defined but never used.")
.addError(9209, "Mixed spaces and tabs.")
.test(src, { undef: true, unused: true }, globals);
};
3 changes: 2 additions & 1 deletion tests/unit/fixtures/unused.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ function main(e, f) {

main(b);

function foo() {
function foo(err, cb) {
main();
cb();
}

4 comments on commit 5751c5e

@telendt
Copy link

Choose a reason for hiding this comment

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

Great news! When can I expect the new release tag including this feature and - more important - when can I expect to see it in node-jshint?

@valueof
Copy link
Member Author

Choose a reason for hiding this comment

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

Around this Sunday: both for the lib and node-jshint.

@goatslacker
Copy link
Member

Choose a reason for hiding this comment

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

👏

@dougwilson
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Please sign in to comment.