Make 'forin' check recognize negative checks with continue

This patch modifies the 'forin' check so that it correctly recognizes
negative conditional checks using continue. For example:

for (var key in collection) {
    if (! collection.hasOwnProperty(key)) {
        continue;
    }
    doSomething();
}

The conditional itself is not analyzed beyond checking whether it
starts with a negation operator or not. Results of the if analysis
are stored in a stack that is evaluated after the for loop was parsed
completely. As a result, the improved check also works for nested loops.

References:
    Closes GH-229

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 {

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();
 }
-

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();