diff --git a/src/Psalm/Checker/FunctionLikeChecker.php b/src/Psalm/Checker/FunctionLikeChecker.php index 1af4753cf..63b272b04 100644 --- a/src/Psalm/Checker/FunctionLikeChecker.php +++ b/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; } } diff --git a/src/Psalm/Checker/Statements/Block/ForChecker.php b/src/Psalm/Checker/Statements/Block/ForChecker.php index 1d2c18f09..552a69b64 100644 --- a/src/Psalm/Checker/Statements/Block/ForChecker.php +++ b/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) { diff --git a/src/Psalm/Checker/Statements/Block/ForeachChecker.php b/src/Psalm/Checker/Statements/Block/ForeachChecker.php index 9b45de6d1..063553adb 100644 --- a/src/Psalm/Checker/Statements/Block/ForeachChecker.php +++ b/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()) { diff --git a/src/Psalm/Checker/Statements/Block/WhileChecker.php b/src/Psalm/Checker/Statements/Block/WhileChecker.php index 08bec825e..aeb72fb2d 100644 --- a/src/Psalm/Checker/Statements/Block/WhileChecker.php +++ b/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; } diff --git a/src/Psalm/Checker/Statements/ExpressionChecker.php b/src/Psalm/Checker/Statements/ExpressionChecker.php index 9f9b9d70c..d1a89f721 100644 --- a/src/Psalm/Checker/Statements/ExpressionChecker.php +++ b/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( - $stmt->class, - $source - ); + $fq_class_name = $source + ? ClassLikeChecker::getFQCLNFromNameObject( + $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); diff --git a/src/Psalm/Checker/StatementsChecker.php b/src/Psalm/Checker/StatementsChecker.php index b266bd74c..0a11825e2 100644 --- a/src/Psalm/Checker/StatementsChecker.php +++ b/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 $stmts + * @param array $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 - $pre_loop_context = clone $loop_context; + $traverser = new PhpParser\NodeTraverser; - IssueBuffer::startRecording(); - $this->analyze($stmts, $loop_context, $outer_context); - $recorded_issues = IssueBuffer::clearRecordingLevel(); - IssueBuffer::stopRecording(); + $assignment_mapper = new \Psalm\Visitor\AssignmentMapVisitor($loop_context->self); + $traverser->addVisitor($assignment_mapper); - if ($recorded_issues) { - do { + $traverser->traverse($stmts); + + $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 - 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] - ); + $has_changes = false; - 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> $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; } diff --git a/src/Psalm/Checker/TypeChecker.php b/src/Psalm/Checker/TypeChecker.php index ac2ffc5d4..aef43bf9e 100644 --- a/src/Psalm/Checker/TypeChecker.php +++ b/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; } diff --git a/src/Psalm/Context.php b/src/Psalm/Context.php index f72db6c6b..6255f89ca 100644 --- a/src/Psalm/Context.php +++ b/src/Psalm/Context.php @@ -206,6 +206,26 @@ class Context return $redefined_vars; } + /** + * @param Context $original_context + * @param Context $new_context + * @return array + */ + 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 diff --git a/src/Psalm/Visitor/AssignmentMapVisitor.php b/src/Psalm/Visitor/AssignmentMapVisitor.php new file mode 100644 index 000000000..d7f857123 --- /dev/null +++ b/src/Psalm/Visitor/AssignmentMapVisitor.php @@ -0,0 +1,50 @@ +> + */ + 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> + */ + public function getAssignmentMap() + { + return $this->assignment_map; + } +} diff --git a/tests/LoopScopeTest.php b/tests/LoopScopeTest.php index 519002b6d..9cb38148e 100644 --- a/tests/LoopScopeTest.php +++ b/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('project_checker, $stmts); + $file_checker->visitAndAnalyzeMethods(); + } + + /** + * @expectedException \Psalm\Exception\CodeException + * @expectedExceptionMessage InvalidReturnType + * @return void + */ + public function testImplicitFourthLoopWithBadReturnType() + { + $stmts = self::$parser->parse('project_checker, $stmts); + $file_checker->visitAndAnalyzeMethods(); + } + /** * @return void */