diff --git a/src/jshint.js b/src/jshint.js index e722f737e9..72f2e1fd42 100644 --- a/src/jshint.js +++ b/src/jshint.js @@ -3686,10 +3686,34 @@ var JSHINT = (function () { increaseComplexityCount(); state.condition = true; advance("("); - checkCondAssignment(expression(0)); + var expr = expression(0); + checkCondAssignment(expr); + + // When the if is within a for-in loop, check if the condition + // starts with a negation operator + var forinifcheck = null; + if (state.option.forin && state.forinifcheckneeded) { + state.forinifcheckneeded = false; // We only need to analyze the first if inside the loop + forinifcheck = state.forinifchecks[state.forinifchecks.length - 1]; + if (expr.type === "(punctuator)" && expr.value === "!") { + forinifcheck.type = "(negative)"; + } else { + forinifcheck.type = "(positive)"; + } + } + advance(")", t); state.condition = false; - block(true, true); + var s = block(true, true); + + // When the if is within a for-in loop and the condition has a negative form, + // check if the body contains nothing but a continue statement + if (forinifcheck && forinifcheck.type === "(negative)") { + if (s && s.length === 1 && s[0].type === "(identifier)" && s[0].value === "continue") { + forinifcheck.type = "(negative-with-continue)"; + } + } + if (state.tokens.next.id === "else") { advance("else"); if (state.tokens.next.id === "if" || state.tokens.next.id === "switch") { @@ -4009,11 +4033,41 @@ var JSHINT = (function () { advance(nextop.value); expression(20); advance(")", t); + + if (nextop.value === "in" && state.option.forin) { + state.forinifcheckneeded = true; + + if (state.forinifchecks === undefined) { + state.forinifchecks = []; + } + + // Push a new for-in-if check onto the stack. The type will be modified + // when the loop's body is parsed and a suitable if statement exists. + state.forinifchecks.push({ + type: "(none)" + }); + } + s = block(true, true); - if (nextop.value === "in" && state.option.forin && s && - (s.length > 1 || typeof s[0] !== "object" || s[0].value !== "if")) { - warning("W089", this); + + if (nextop.value === "in" && state.option.forin) { + if (state.forinifchecks && state.forinifchecks.length > 0) { + var check = state.forinifchecks.pop(); + + if (// No if statement or not the first statement in loop body + s && s.length > 0 && (typeof s[0] !== "object" || s[0].value !== "if") || + // Positive if statement is not the only one in loop body + check.type === "(positive)" && s.length > 1 || + // Negative if statement but no continue + check.type === "(negative)") { + warning("W089", this); + } + } + + // Reset the flag in case no if statement was contained in the loop body + state.forinifcheckneeded = false; } + funct["(breakage)"] -= 1; funct["(loopage)"] -= 1; } else { diff --git a/tests/unit/fixtures/forin.js b/tests/unit/fixtures/forin.js index de33d658d6..6ac4a8d39c 100644 --- a/tests/unit/fixtures/forin.js +++ b/tests/unit/fixtures/forin.js @@ -4,6 +4,52 @@ for (var key in objects) { } } +for (var key in objects) { + if (! objects.hasOwnProperty(key)) { + continue; + } + hey(); +} + +// More statements in loop body than just the if +for (var key in objects) { + if (objects.hasOwnProperty(key)) { + hey(); + } + hey(); +} + +// No continue +for (var key in objects) { + if (! objects.hasOwnProperty(key)) { + hey(); + } + hey(); +} + +// Nested loops +for (var key in objects) { + if (! objects.hasOwnProperty(key)) { + continue; + } + + // No if statement + for (var key2 in objects) { + hey(); + + for (var key3 in objects) { + if (objects.hasOwnProperty(key3)) { + // No continue + for (var key4 in objects) { + if (! objects.hasOwnProperty(key4)) { + hey(); + } + } + } + } + } +} + var hasOwn = Object.prototype.hasOwnProperty; for (var p in o) { @@ -25,6 +71,5 @@ for ( key in objects ) { } // Let's make sure we continue scanning the rest of the file. for (key in objects) { - hey(); + hey(); } - diff --git a/tests/unit/options.js b/tests/unit/options.js index ec7e8eaad8..48a7135bac 100644 --- a/tests/unit/options.js +++ b/tests/unit/options.js @@ -696,7 +696,11 @@ exports.forin = function (test) { // Make sure it fails when forin is true TestRun(test) - .addError(27, msg) + .addError(15, msg) + .addError(23, msg) + .addError(37, msg) + .addError(43, msg) + .addError(73, msg) .test(src, { es3: true, forin: true }); test.done();