Complain about Reuse of By-ref Iterators

Summary:
Added a lint rule that warns about reusing iterator reference
variables.

Test Plan:
- Add a file with examples found in with https://secure.phabricator.com/T2536
- Did not make a unit test yet

Reviewers: vrana, bill, epriestley

Reviewed By: epriestley

CC: aran, epriestley, Korvin

Maniphest Tasks: T2536

Differential Revision: https://secure.phabricator.com/D5179

src/lint/linter/ArcanistXHPASTLinter.php

@@ -46,6 +46,7 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
   const LINT_SLOWNESS                  = 36;
   const LINT_CLOSING_CALL_PAREN        = 37;
   const LINT_CLOSING_DECL_PAREN        = 38;
+  const LINT_REUSED_ITERATOR_REFERENCE = 39;
 
   public function getLintNameMap() {
     return array(
@@ -85,6 +86,7 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
       self::LINT_SLOWNESS                  => 'Slow Construct',
       self::LINT_CLOSING_CALL_PAREN        => 'Call Formatting',
       self::LINT_CLOSING_DECL_PAREN        => 'Declaration Formatting',
+      self::LINT_REUSED_ITERATOR_REFERENCE => 'Reuse of Iterator References',
     );
   }
 
@@ -112,6 +114,7 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
       self::LINT_COMMENT_SPACING           => $advice,
       self::LINT_CLOSING_CALL_PAREN        => $warning,
       self::LINT_CLOSING_DECL_PAREN        => $warning,
+      self::LINT_REUSED_ITERATOR_REFERENCE => $warning,
 
       // This is disabled by default because it implies a very strict policy
       // which isn't necessary in the general case.
@@ -184,6 +187,7 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
       'lintCommentSpaces' => self::LINT_COMMENT_SPACING,
       'lintHashComments' => self::LINT_COMMENT_STYLE,
       'lintReusedIterators' => self::LINT_REUSED_ITERATORS,
+      'lintReusedIteratorReferences' => self::LINT_REUSED_ITERATOR_REFERENCE,
       'lintVariableVariables' => self::LINT_VARIABLE_VARIABLE,
       'lintUndeclaredVariables' => array(
         self::LINT_EXTRACT_USE,
@@ -874,6 +878,177 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
     }
   }
 
+  /**
+   * Find cases where a foreach loop is being iterated using a variable
+   * reference and the same variable is used outside of the loop without
+   * calling unset() or reassigning the variable to another variable
+   * reference.
+   *
+   *  COUNTEREXAMPLE
+   *  foreach ($ar as &$a) {
+   *    // ...
+   *  }
+   *  $a = 1; // <-- Raises an error for using $a
+   *
+   */
+  protected function lintReusedIteratorReferences($root) {
+
+    $fdefs = $root->selectDescendantsOfType('n_FUNCTION_DECLARATION');
+    $mdefs = $root->selectDescendantsOfType('n_METHOD_DECLARATION');
+    $defs = $fdefs->add($mdefs);
+
+    foreach ($defs as $def) {
+
+      $body = $def->getChildByIndex(5);
+      if ($body->getTypeName() == 'n_EMPTY') {
+        // Abstract method declaration.
+        continue;
+      }
+
+      $exclude = array();
+
+      // Exclude uses of variables, unsets, and foreach loops
+      // within closures - they are checked on their own
+      $func_defs = $body->selectDescendantsOfType('n_FUNCTION_DECLARATION');
+      foreach ($func_defs as $func_def) {
+        $vars = $func_def->selectDescendantsOfType('n_VARIABLE');
+        foreach ($vars as $var) {
+          $exclude[$var->getID()] = true;
+        }
+
+        $unset_lists = $func_def->selectDescendantsOfType('n_UNSET_LIST');
+        foreach ($unset_lists as $unset_list) {
+          $exclude[$unset_list->getID()] = true;
+        }
+
+        $foreaches = $func_def->selectDescendantsOfType('n_FOREACH');
+        foreach ($foreaches as $foreach) {
+          $exclude[$foreach->getID()] = true;
+        }
+      }
+
+      // Find all variables that are unset within the scope
+      $unset_vars = array();
+      $unset_lists = $body->selectDescendantsOfType('n_UNSET_LIST');
+      foreach ($unset_lists as $unset_list) {
+        if (isset($exclude[$unset_list->getID()])) {
+          continue;
+        }
+
+        $unset_list_vars = $unset_list->selectDescendantsOfType('n_VARIABLE');
+        foreach ($unset_list_vars as $var) {
+          $concrete = $this->getConcreteVariableString($var);
+          $unset_vars[$concrete][] = $var->getOffset();
+          $exclude[$var->getID()] = true;
+        }
+      }
+
+      // Find all reference variables in foreach expressions
+      $reference_vars = array();
+      $foreaches = $body->selectDescendantsOfType('n_FOREACH');
+      foreach ($foreaches as $foreach) {
+        if (isset($exclude[$foreach->getID()])) {
+          continue;
+        }
+
+        $foreach_expr = $foreach->getChildOfType(0, 'n_FOREACH_EXPRESSION');
+        $var = $foreach_expr->getChildByIndex(2);
+        if ($var->getTypeName() != 'n_VARIABLE_REFERENCE') {
+          continue;
+        }
+
+        $reference = $var->getChildByIndex(0);
+        if ($reference->getTypeName() != 'n_VARIABLE') {
+          continue;
+        }
+
+        $reference_name = $this->getConcreteVariableString($reference);
+        $reference_vars[$reference_name][] = $reference->getOffset();
+        $exclude[$reference->getID()] = true;
+
+        // Exclude uses of the reference variable within the foreach loop
+        $foreach_vars = $foreach->selectDescendantsOfType('n_VARIABLE');
+        foreach ($foreach_vars as $var) {
+          $name = $this->getConcreteVariableString($var);
+          if ($name == $reference_name) {
+            $exclude[$var->getID()] = true;
+          }
+        }
+      }
+
+      // Allow usage if the reference variable is assigned to another
+      // reference variable
+      $binary = $body->selectDescendantsOfType('n_BINARY_EXPRESSION');
+      foreach ($binary as $expr) {
+        if ($expr->getChildByIndex(1)->getConcreteString() != '=') {
+          continue;
+        }
+        $lval = $expr->getChildByIndex(0);
+        if ($lval->getTypeName() != 'n_VARIABLE') {
+          continue;
+        }
+        $rval = $expr->getChildByIndex(2);
+        if ($rval->getTypeName() != 'n_VARIABLE_REFERENCE') {
+          continue;
+        }
+
+        // Counts as unsetting a variable
+        $concrete = $this->getConcreteVariableString($lval);
+        $unset_vars[$concrete][] = $lval->getOffset();
+        $exclude[$lval->getID()] = true;
+      }
+
+      $all_vars = array();
+      $all = $body->selectDescendantsOfType('n_VARIABLE');
+      foreach ($all as $var) {
+        if (isset($exclude[$var->getID()])) {
+          continue;
+        }
+
+        $name = $this->getConcreteVariableString($var);
+
+        if (!isset($reference_vars[$name])) {
+          continue;
+        }
+
+        // Find the closest reference offset to this variable
+        $reference_offset = null;
+        foreach ($reference_vars[$name] as $offset) {
+          if ($offset < $var->getOffset()) {
+            $reference_offset = $offset;
+          } else {
+            break;
+          }
+        }
+        if (!$reference_offset) {
+          continue;
+        }
+
+        // Check if an unset exists between reference and usage of this
+        // variable
+        $warn = true;
+        if (isset($unset_vars[$name])) {
+          foreach ($unset_vars[$name] as $unset_offset) {
+            if ($unset_offset > $reference_offset &&
+                $unset_offset < $var->getOffset()) {
+                $warn = false;
+                break;
+            }
+          }
+        }
+        if ($warn) {
+          $this->raiseLintAtNode(
+            $var,
+            self::LINT_REUSED_ITERATOR_REFERENCE,
+            'This variable was used already as a by-reference iterator ' .
+            'variable. Such variables survive outside the foreach loop, ' .
+            'do not reuse.');
+        }
+      }
+
+    }
+  }
+
   protected function lintVariableVariables($root) {
     $vvars = $root->selectDescendantsOfType('n_VARIABLE_VARIABLE');
     foreach ($vvars as $vvar) {
@@ -1131,7 +1306,11 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
           //    foreach ($a as $b => &$c) { ... }
           // Reach into the n_VARIABLE_REFERENCE node to grab the n_VARIABLE
           // node.
-          $foreach_vars[] = $value_var->getChildOfType(0, 'n_VARIABLE');
+          $var = $value_var->getChildByIndex(0);
+          if ($var->getTypeName() == 'n_VARIABLE_VARIABLE') {
+            $var = $var->getChildByIndex(0);
+          }
+          $foreach_vars[] = $var;
         }
 
         // Remove all uses of the iterators inside of the foreach() loop from

src/lint/linter/__tests__/xhpast/reused-iterator-reference.lint-test

@@ -0,0 +1,137 @@
+<?php
+
+function assign() {
+  $ar = array();
+  foreach ($ar as &$a) { }
+  $a = 1; // Reuse of $a.
+}
+
+function expr() {
+  $ar = array();
+  foreach ($ar as &$a) { }
+  $b = $a; // Reuse of $a.
+}
+
+function func_call() {
+  $ar = array();
+  foreach ($ar as &$a) { }
+  $b = x($a); // Reuse of $a.
+}
+
+function x($b) { }
+
+function iterator_reuse() {
+  $ar1 = array();
+  $ar2 = array();
+  foreach ($ar1 as &$a) { }
+  foreach ($ar2 as $a) { } // Reuse of $a
+}
+
+function key_value() {
+  $ar = array();
+  foreach ($ar as $k => &$v) { }
+  $v++; // Reuse of $v
+}
+
+function key_value2() {
+  $ar = array();
+  foreach ($ar as $k => $v) { }
+  $v++;
+}
+
+function unset() {
+  $ar = array();
+  foreach ($ar as &$a) { }
+  unset($a);
+  $a++;
+}
+
+function unset2() {
+  $ar = array();
+  foreach ($ar as &$a) { }
+  $a++; // Reuse of $a
+  unset($a);
+}
+
+function twice_ref() {
+  $ar1 = array();
+  $ar2 = array();
+  foreach ($ar1 as &$b) { }
+  foreach ($ar2 as &$b) { }
+}
+
+function assign_ref(&$a) {
+  $ar = array();
+  foreach ($ar as &$b) { }
+  $b = &$a;
+}
+
+function assign_ref2(&$a) {
+  $ar = array();
+  foreach ($ar as &$b) { }
+  $b = &$a;
+  $c = $b;
+}
+
+function use_inside() {
+  $ar = array();
+  foreach ($ar as &$a) {
+    $a++;
+  }
+}
+
+function variable_variable() {
+  $ar = array();
+  foreach ($ar as &$$a) { }
+  $a++;
+  $$a++;
+}
+
+function closure1() {
+  $ar = array();
+  foreach ($ar as &$a) { }
+  function($a) {
+    $a++;
+  };
+}
+
+function closure2() {
+  function() {
+    $ar = array();
+    foreach ($ar as &$a) { }
+  };
+  $a++;
+}
+
+function closure3() {
+  function() {
+    $ar = array();
+    foreach ($ar as &$a) { }
+    $a++; // Reuse of $a
+  };
+}
+
+function closure4() {
+  $ar = array();
+  foreach ($ar as &$a) { }
+  function($a) {
+    unset($a);
+  };
+  $a++; // Reuse of $a
+}
+
+~~~~~~~~~~
+warning:6:3
+warning:12:8
+warning:18:10
+warning:27:20
+warning:33:3
+warning:52:3
+error:85:20
+error:87:3
+disabled:93:3
+disabled:99:3
+disabled:107:3
+warning:110:5
+disabled:117:3
+warning:120:3

src/lint/linter/__tests__/xhpast/undeclared-variables.lint-test

@@ -171,6 +171,7 @@ error:46:5
 error:47:5
 error:48:10
 error:53:10 worst ever
+warning:65:3
 error:91:3 This stuff is basically testing the lexer/parser for function decls.
 error:108:15 Variables in instance derefs should be checked, static should not.
 error:121:3 isset() and empty() should not trigger errors.