Detect iterator reuse in for() and foreach() loops

Summary:
cpojer fixed a JS instance of this in D481, but we can catch it in PHP with
XHPAST. Add a lint rule to fail if nested loops use the same iterator.

Test Plan:
Ran unit test.

Reviewed By: tomo
Reviewers: aran, pad, cpojer, jungejason, tuomaspelkonen, tomo
Commenters: cpojer
CC: aran, cpojer, epriestley, tomo
Differential Revision: 489

src/lint/linter/xhpast/ArcanistXHPASTLinter.php

@@ -47,6 +47,7 @@ class ArcanistXHPASTLinter extends ArcanistLinter {
   const LINT_TAUTOLOGICAL_EXPRESSION  = 20;
   const LINT_PLUS_OPERATOR_ON_STRINGS = 21;
   const LINT_DUPLICATE_KEYS_IN_ARRAY  = 22;
+  const LINT_REUSED_ITERATORS         = 23;
 
 
   public function getLintNameMap() {
@@ -73,6 +74,7 @@ class ArcanistXHPASTLinter extends ArcanistLinter {
       self::LINT_TAUTOLOGICAL_EXPRESSION  => 'Tautological Expression',
       self::LINT_PLUS_OPERATOR_ON_STRINGS => 'Not String Concatenation',
       self::LINT_DUPLICATE_KEYS_IN_ARRAY  => 'Duplicate Keys in Array',
+      self::LINT_REUSED_ITERATORS         => 'Reuse of Iterator Variable',
     );
   }
 
@@ -152,6 +154,7 @@ class ArcanistXHPASTLinter extends ArcanistLinter {
     $this->lintTautologicalExpressions($root);
     $this->lintPlusOperatorOnStrings($root);
     $this->lintDuplicateKeysInArray($root);
+    $this->lintReusedIterators($root);
   }
 
   private function lintTautologicalExpressions($root) {
@@ -253,6 +256,84 @@ class ArcanistXHPASTLinter extends ArcanistLinter {
     }
   }
 
+  /**
+   * Find cases where loops get nested inside each other but use the same
+   * iterator variable. For example:
+   *
+   *  COUNTEREXAMPLE
+   *  foreach ($list as $thing) {
+   *    foreach ($stuff as $thing) { // <-- Raises an error for reuse of $thing
+   *      // ...
+   *    }
+   *  }
+   *
+   */
+  private function lintReusedIterators($root) {
+    $used_vars = array();
+
+    $for_loops = $root->selectDescendantsOfType('n_FOR');
+    foreach ($for_loops as $for_loop) {
+      $var_map = array();
+
+      // Find all the variables that are assigned to in the for() expression.
+      $for_expr = $for_loop->getChildOfType(0, 'n_FOR_EXPRESSION');
+      $bin_exprs = $for_expr->selectDescendantsOfType('n_BINARY_EXPRESSION');
+      foreach ($bin_exprs as $bin_expr) {
+        if ($bin_expr->getChildByIndex(1)->getConcreteString() == '=') {
+          $var_map[$bin_expr->getChildByIndex(0)->getConcreteString()] = true;
+        }
+      }
+
+      $used_vars[$for_loop->getID()] = $var_map;
+    }
+
+    $foreach_loops = $root->selectDescendantsOfType('n_FOREACH');
+    foreach ($foreach_loops as $foreach_loop) {
+      $var_map = array();
+
+      $foreach_expr = $foreach_loop->getChildOftype(0, 'n_FOREACH_EXPRESSION');
+
+      // We might use one or two vars, i.e. "foreach ($x as $y => $z)" or
+      // "foreach ($x as $y)".
+      $possible_used_vars = array(
+        $foreach_expr->getChildByIndex(1),
+        $foreach_expr->getChildByIndex(2),
+      );
+      foreach ($possible_used_vars as $var) {
+        if ($var->getTypeName() == 'n_EMPTY') {
+          continue;
+        }
+        $name = $var->getConcreteString();
+        $name = trim($name, '&'); // Get rid of ref silliness.
+        $var_map[$name] = true;
+      }
+
+      $used_vars[$foreach_loop->getID()] = $var_map;
+    }
+
+    $all_loops = $for_loops->add($foreach_loops);
+    foreach ($all_loops as $loop) {
+      $child_for_loops = $loop->selectDescendantsOfType('n_FOR');
+      $child_foreach_loops = $loop->selectDescendantsOfType('n_FOREACH');
+      $child_loops = $child_for_loops->add($child_foreach_loops);
+
+      $outer_vars = $used_vars[$loop->getID()];
+      foreach ($child_loops as $inner_loop) {
+        $inner_vars = $used_vars[$inner_loop->getID()];
+        $shared = array_intersect_key($outer_vars, $inner_vars);
+        if ($shared) {
+          $shared_desc = implode(', ', array_keys($shared));
+          $this->raiseLintAtNode(
+            $inner_loop->getChildByIndex(0),
+            self::LINT_REUSED_ITERATORS,
+            "This loop reuses iterator variables ({$shared_desc}) from an ".
+            "outer loop. You might be clobbering the outer iterator. Change ".
+            "the inner loop to use a different iterator name.");
+        }
+      }
+    }
+  }
+
   protected function lintVariableVariables($root) {
     $vvars = $root->selectDescendantsOfType('n_VARIABLE_VARIABLE');
     foreach ($vvars as $vvar) {

src/lint/linter/xhpast/__tests__/data/reused-iterators.lint-test

@@ -0,0 +1,58 @@
+<?php
+
+for ($ii = 0; $ii < $len; $ii++) {
+  foreach ($list as $ii => $jj) {
+    // Reuse of $ii.
+  }
+}
+
+for ($jj = 0; $jj < $len; $jj++) {
+  foreach ($list as $jj) {
+    // Reuse of $jj.
+  }
+}
+
+for ($ii = 0; $ii < $len; $ii++) {
+  foreach ($list as $ii => &$jj) {
+    // Reuse of $ii.
+  }
+}
+
+for ($jj = 0; $jj < $len; $jj++) {
+  foreach ($list as &$jj) {
+    // Reuse of $jj (by reference).
+  }
+}
+
+for ($ii = 0; $ii < $len; $ii++) {
+  for ($jj = 0; $jj < $len; $jj++) {
+    foreach ($list as $kk) {
+      // No reuse.
+    }
+  }
+}
+
+for ($ii = 0; $ii < $len; $ii++) {
+  for ($ii = 0; $ii < $len; $ii++) {
+    // Reuse of $ii, pure for loops.
+  }
+}
+
+for ($ii = 0; $ii < $len; $ii++) {
+  for ($jj = $ii; $jj < $jjlen; $jj++) {
+    // No reuse.
+  }
+}
+
+foreach ($list as $thing) {
+  for ($thing = 0; $thing < $len; $thing++) {
+    // Reuse of $thing, for within foreach.
+  }
+}
+~~~~~~~~~~
+error:4:11
+error:10:11
+error:16:11
+error:22:11
+error:36:7
+error:48:7