Merge pull request #115 from vimeo/foreach-assignment-map

Check loops for nested assignment logic

src/Psalm/Checker/FunctionLikeChecker.php

@@ -162,10 +162,6 @@ abstract class FunctionLikeChecker extends SourceChecker implements StatementsSo
                 $have_emitted = false;
 
                 foreach ($implemented_method_ids as $implemented_method_id) {
-                    if ($have_emitted) {
-                        break;
-                    }
-
                     if ($this->function->name === '__construct') {
                         continue;
                     }
@@ -207,7 +203,7 @@ abstract class FunctionLikeChecker extends SourceChecker implements StatementsSo
                             }
 
                             $have_emitted = true;
-                            break;
+                            break 2;
                         }
 
                         if ($class_storage->user_defined &&
@@ -229,7 +225,7 @@ abstract class FunctionLikeChecker extends SourceChecker implements StatementsSo
                             }
 
                             $have_emitted = true;
-                            break;
+                            break 2;
                         }
 
                         if (!$class_storage->user_defined &&
@@ -252,7 +248,7 @@ abstract class FunctionLikeChecker extends SourceChecker implements StatementsSo
                             }
 
                             $have_emitted = true;
-                            break;
+                            break 2;
                         }
                     }
 

src/Psalm/Checker/Statements/Block/ForChecker.php

@@ -21,6 +21,7 @@ class ForChecker
         Context $context
     ) {
         $for_context = clone $context;
+        $before_context = clone $context;
         $for_context->inside_loop = true;
 
         foreach ($stmt->init as $init) {
@@ -37,7 +38,9 @@ class ForChecker
             $for_context->inside_conditional = false;
         }
 
-        $statements_checker->analyzeLoop($stmt->stmts, $for_context, $context);
+        $changed_vars = Context::getNewOrUpdatedVarIds($before_context, $for_context);
+
+        $statements_checker->analyzeLoop($stmt->stmts, $changed_vars, $for_context, $context);
 
         foreach ($stmt->loop as $expr) {
             if (ExpressionChecker::analyze($statements_checker, $expr, $for_context) === false) {

src/Psalm/Checker/Statements/Block/ForeachChecker.php

@@ -169,6 +169,8 @@ class ForeachChecker
             }
         }
 
+        $before_context = clone $foreach_context;
+
         if ($stmt->keyVar && $stmt->keyVar instanceof PhpParser\Node\Expr\Variable && is_string($stmt->keyVar->name)) {
             $key_var_id = '$' . $stmt->keyVar->name;
             $foreach_context->vars_in_scope[$key_var_id] = $key_type ?: Type::getMixed();
@@ -202,7 +204,9 @@ class ForeachChecker
             );
         }
 
-        $statements_checker->analyzeLoop($stmt->stmts, $foreach_context, $context);
+        $changed_vars = Context::getNewOrUpdatedVarIds($before_context, $foreach_context);
+
+        $statements_checker->analyzeLoop($stmt->stmts, $changed_vars, $foreach_context, $context);
 
         foreach ($context->vars_in_scope as $var => $type) {
             if ($type->isMixed()) {

src/Psalm/Checker/Statements/Block/WhileChecker.php

@@ -67,7 +67,9 @@ class WhileChecker
             $while_context->vars_in_scope = $while_vars_in_scope_reconciled;
         }
 
-        if ($statements_checker->analyzeLoop($stmt->stmts, $while_context, $context) === false) {
+        $while_cond_vars = array_keys($reconcilable_while_types);
+
+        if ($statements_checker->analyzeLoop($stmt->stmts, $while_cond_vars, $while_context, $context) === false) {
             return false;
         }
 

src/Psalm/Checker/Statements/ExpressionChecker.php

@@ -1341,14 +1341,14 @@ class ExpressionChecker
     /**
      * @param  PhpParser\Node\Expr      $stmt
      * @param  string|null              $this_class_name
-     * @param  StatementsSource         $source
+     * @param  StatementsSource|null    $source
      * @param  int|null                 &$nesting
      * @return string|null
      */
     public static function getVarId(
         PhpParser\Node\Expr $stmt,
         $this_class_name,
-        StatementsSource $source,
+        StatementsSource $source = null,
         &$nesting = null
     ) {
         if ($stmt instanceof PhpParser\Node\Expr\Variable && is_string($stmt->name)) {
@@ -1361,15 +1361,17 @@ class ExpressionChecker
         ) {
             if (count($stmt->class->parts) === 1 && in_array($stmt->class->parts[0], ['self', 'static', 'parent'])) {
                 if (!$this_class_name) {
-                    throw new \UnexpectedValueException('$this_class_name should not be null');
+                    $fq_class_name = $stmt->class->parts[0];
+                } else {
+                    $fq_class_name = $this_class_name;
                 }
-
-                $fq_class_name = $this_class_name;
             } else {
-                $fq_class_name = ClassLikeChecker::getFQCLNFromNameObject(
+                $fq_class_name = $source
-                    $stmt->class,
+                    ? ClassLikeChecker::getFQCLNFromNameObject(
-                    $source
+                        $stmt->class,
-                );
+                        $source
+                    )
+                    : implode('\\', $stmt->class->parts);
             }
 
             return $fq_class_name . '::$' . $stmt->name;
@@ -1396,13 +1398,13 @@ class ExpressionChecker
     /**
      * @param  PhpParser\Node\Expr      $stmt
      * @param  string|null              $this_class_name
-     * @param  StatementsSource         $source
+     * @param  StatementsSource|null    $source
      * @return string|null
      */
     public static function getArrayVarId(
         PhpParser\Node\Expr $stmt,
         $this_class_name,
-        StatementsSource $source
+        StatementsSource $source = null
     ) {
         if ($stmt instanceof PhpParser\Node\Expr\Assign) {
             return self::getArrayVarId($stmt->var, $this_class_name, $source);

src/Psalm/Checker/StatementsChecker.php

@@ -329,36 +329,67 @@ class StatementsChecker extends SourceChecker implements StatementsSource
      * Checks an array of statements in a loop
      *
      * @param  array<PhpParser\Node\Stmt|PhpParser\Node\Expr>   $stmts
+     * @param  array<int, string>                               $asserted_vars
      * @param  Context                                          $loop_context
      * @param  Context                                          $outer_context
      * @return void
      */
     public function analyzeLoop(
         array $stmts,
+        array $asserted_vars,
         Context $loop_context,
         Context $outer_context
     ) {
-        // record all the vars that existed before we did the first pass through the loop
+        $traverser = new PhpParser\NodeTraverser;
-        $pre_loop_context = clone $loop_context;
 
-        IssueBuffer::startRecording();
+        $assignment_mapper = new \Psalm\Visitor\AssignmentMapVisitor($loop_context->self);
-        $this->analyze($stmts, $loop_context, $outer_context);
+        $traverser->addVisitor($assignment_mapper);
-        $recorded_issues = IssueBuffer::clearRecordingLevel();
-        IssueBuffer::stopRecording();
 
-        if ($recorded_issues) {
+        $traverser->traverse($stmts);
-            do {
+
+        $assignment_map = $assignment_mapper->getAssignmentMap();
+
+        $assignment_depth = 0;
+
+        if ($assignment_map) {
+            $first_var_id = array_keys($assignment_map)[0];
+
+            $assignment_depth = self::getAssignmentMapDepth($first_var_id, $assignment_map);
+        }
+
+        if ($assignment_depth === 0) {
+            $this->analyze($stmts, $loop_context, $outer_context);
+        } else {
+            // record all the vars that existed before we did the first pass through the loop
+            $pre_loop_context = clone $loop_context;
+            $pre_outer_context = clone $outer_context;
+
+            IssueBuffer::startRecording();
+            $this->analyze($stmts, $loop_context, $outer_context);
+            $recorded_issues = IssueBuffer::clearRecordingLevel();
+            IssueBuffer::stopRecording();
+
+            for ($i = 0; $i < $assignment_depth; $i++) {
                 $vars_to_remove = [];
 
-                // widen the foreach context type with the initial context type
+                $has_changes = false;
-                foreach ($loop_context->vars_in_scope as $var_id => $type) {
-                    if (isset($pre_loop_context->vars_in_scope[$var_id])) {
-                        $loop_context->vars_in_scope[$var_id] = Type::combineUnionTypes(
-                            $type,
-                            $pre_loop_context->vars_in_scope[$var_id]
-                        );
 
-                        if (isset($outer_context->vars_in_scope[$var_id])) {
+                foreach ($loop_context->vars_in_scope as $var_id => $type) {
+                    if (in_array($var_id, $asserted_vars)) {
+                        // set the vars to whatever the while/foreach loop expects them to be
+                        if ((string)$type !== (string)$pre_loop_context->vars_in_scope[$var_id]) {
+                            $loop_context->vars_in_scope[$var_id] = $pre_loop_context->vars_in_scope[$var_id];
+                            $has_changes = true;
+                        }
+                    } elseif (isset($pre_outer_context->vars_in_scope[$var_id])) {
+                        $pre_outer = (string)$pre_outer_context->vars_in_scope[$var_id];
+
+                        if ((string)$type !== $pre_outer ||
+                            (string)$outer_context->vars_in_scope[$var_id] !== $pre_outer
+                        ) {
+                            $has_changes = true;
+
+                            // widen the foreach context type with the initial context type
                             $loop_context->vars_in_scope[$var_id] = Type::combineUnionTypes(
                                 $loop_context->vars_in_scope[$var_id],
                                 $outer_context->vars_in_scope[$var_id]
@@ -369,6 +400,11 @@ class StatementsChecker extends SourceChecker implements StatementsSource
                     }
                 }
 
+                // if there are no changes to the types, no need to re-examine
+                if (!$has_changes) {
+                    break;
+                }
+
                 // remove vars that were defined in the foreach
                 foreach ($vars_to_remove as $var_id) {
                     unset($loop_context->vars_in_scope[$var_id]);
@@ -376,11 +412,9 @@ class StatementsChecker extends SourceChecker implements StatementsSource
 
                 IssueBuffer::startRecording();
                 $this->analyze($stmts, $loop_context, $outer_context);
-                $last_recorded_issues_count = count($recorded_issues);
                 $recorded_issues = IssueBuffer::clearRecordingLevel();
-
                 IssueBuffer::stopRecording();
-            } while (count($recorded_issues) < $last_recorded_issues_count);
+            }
 
             if ($recorded_issues) {
                 foreach ($recorded_issues as $recorded_issue) {
@@ -391,6 +425,33 @@ class StatementsChecker extends SourceChecker implements StatementsSource
         }
     }
 
+    /**
+     * @param  string                               $first_var_id
+     * @param  array<string, array<string, bool>>   $assignment_map
+     * @return int
+     */
+    private static function getAssignmentMapDepth($first_var_id, array $assignment_map)
+    {
+        $max_depth = 0;
+
+        $assignment_var_ids = $assignment_map[$first_var_id];
+        unset($assignment_map[$first_var_id]);
+
+        foreach ($assignment_var_ids as $assignment_var_id => $_) {
+            $depth = 1;
+
+            if (isset($assignment_map[$assignment_var_id])) {
+                $depth = 1 + self::getAssignmentMapDepth($assignment_var_id, $assignment_map);
+            }
+
+            if ($depth > $max_depth) {
+                $max_depth = $depth;
+            }
+        }
+
+        return $max_depth;
+    }
+
     /**
      * @param   PhpParser\Node\Stmt\Static_ $stmt
      * @param   Context                     $context
@@ -474,7 +535,7 @@ class StatementsChecker extends SourceChecker implements StatementsSource
     {
         $do_context = clone $context;
 
-        if ($this->analyzeLoop($stmt->stmts, $do_context, $context) === false) {
+        if ($this->analyzeLoop($stmt->stmts, [], $do_context, $context) === false) {
             return false;
         }
 

src/Psalm/Checker/TypeChecker.php

@@ -500,16 +500,16 @@ class TypeChecker
                 if ((string) $result_type === 'array') {
                     $result_type = Type::getArray();
                 }
+
+                if ($result_type === false) {
+                    return false;
+                }
             }
 
             if ($result_type === null) {
                 continue;
             }
 
-            if ($result_type === false) {
-                return false;
-            }
-
             if ((string)$result_type !== $before_adjustment) {
                 $changed_types[] = $key;
             }

src/Psalm/Context.php

@@ -206,6 +206,26 @@ class Context
         return $redefined_vars;
     }
 
+    /**
+     * @param  Context $original_context
+     * @param  Context $new_context
+     * @return array<int, string>
+     */
+    public static function getNewOrUpdatedVarIds(Context $original_context, Context $new_context)
+    {
+        $redefined_var_ids = [];
+
+        foreach ($new_context->vars_in_scope as $var_id => $context_type) {
+            if (!isset($original_context->vars_in_scope[$var_id]) ||
+                (string)$original_context->vars_in_scope[$var_id] !== (string)$context_type
+            ) {
+                $redefined_var_ids[] = $var_id;
+            }
+        }
+
+        return $redefined_var_ids;
+    }
+
     /**
      * @param  string $remove_var_id
      * @return void

src/Psalm/Visitor/AssignmentMapVisitor.php

@@ -0,0 +1,50 @@
+<?php
+namespace Psalm\Visitor;
+
+use PhpParser;
+use Psalm\Checker\Statements\ExpressionChecker;
+
+class AssignmentMapVisitor extends PhpParser\NodeVisitorAbstract implements PhpParser\NodeVisitor
+{
+    /**
+     * @var array<string, array<string, bool>>
+     */
+    protected $assignment_map = [];
+
+    /**
+     * @var string|null
+     */
+    protected $this_class_name;
+
+    /**
+     * @param string|null $this_class_name
+     */
+    public function __construct($this_class_name)
+    {
+        $this->this_class_name = $this_class_name;
+    }
+
+    public function enterNode(PhpParser\Node $node)
+    {
+        if ($node instanceof PhpParser\Node\Expr\Assign) {
+            $left_var_id = ExpressionChecker::getVarId($node->var, $this->this_class_name);
+            $right_var_id = ExpressionChecker::getVarId($node->expr, $this->this_class_name);
+
+            if ($left_var_id) {
+                $this->assignment_map[$left_var_id][(string)$right_var_id] = true;
+            }
+
+            return PhpParser\NodeTraverser::DONT_TRAVERSE_CHILDREN;
+        }
+
+        return null;
+    }
+
+    /**
+     * @return array<string, array<string, bool>>
+     */
+    public function getAssignmentMap()
+    {
+        return $this->assignment_map;
+    }
+}

tests/LoopScopeTest.php

@@ -502,6 +502,54 @@ class LoopScopeTest extends PHPUnit_Framework_TestCase
         $file_checker->visitAndAnalyzeMethods();
     }
 
+    /**
+     * @return void
+     */
+    public function testImplicitFourthLoop()
+    {
+        $stmts = self::$parser->parse('<?php
+        function test(): int {
+          $x = 0;
+          $y = 1;
+          $z = 2;
+          for ($i = 0; $i < 3; $i++) {
+            $x = $y;
+            $y = $z;
+            $z = 5;
+          }
+          return $x;
+        }
+        ');
+
+        $file_checker = new FileChecker('somefile.php', $this->project_checker, $stmts);
+        $file_checker->visitAndAnalyzeMethods();
+    }
+
+    /**
+     * @expectedException        \Psalm\Exception\CodeException
+     * @expectedExceptionMessage InvalidReturnType
+     * @return                   void
+     */
+    public function testImplicitFourthLoopWithBadReturnType()
+    {
+        $stmts = self::$parser->parse('<?php
+        function test(): int {
+          $x = 0;
+          $y = 1;
+          $z = 2;
+          for ($i = 0; $i < 3; $i++) {
+            $x = $y;
+            $y = $z;
+            $z = "hello";
+          }
+          return $x;
+        }
+        ');
+
+        $file_checker = new FileChecker('somefile.php', $this->project_checker, $stmts);
+        $file_checker->visitAndAnalyzeMethods();
+    }
+
     /**
      * @return void
      */