diff --git a/config.xsd b/config.xsd index 5d965e2e7..2a3136547 100644 --- a/config.xsd +++ b/config.xsd @@ -141,6 +141,7 @@ + diff --git a/src/Psalm/Checker/AlgebraChecker.php b/src/Psalm/Checker/AlgebraChecker.php index 9d1ebf9f8..0a0956d37 100644 --- a/src/Psalm/Checker/AlgebraChecker.php +++ b/src/Psalm/Checker/AlgebraChecker.php @@ -4,6 +4,9 @@ namespace Psalm\Checker; use PhpParser; use Psalm\Checker\Statements\Expression\AssertionFinder; use Psalm\Clause; +use Psalm\CodeLocation; +use Psalm\IssueBuffer; +use Psalm\Issue\ParadoxicalCondition; use Psalm\StatementsSource; class AlgebraChecker @@ -156,25 +159,73 @@ class AlgebraChecker return; } - $clause->impossibilities = array_map( - /** - * @param array $types - * @return array - */ - function (array $types) { - return array_map( - /** - * @param string $type - * @return string - */ - function ($type) { - return TypeChecker::negateType($type); - }, - $types - ); - }, - $clause->possibilities - ); + $impossibilities = []; + + foreach ($clause->possibilities as $var_id => $possiblity) { + $impossibility = []; + + foreach ($possiblity as $type) { + if ($type[0] !== '^') { + $impossibility[] = TypeChecker::negateType($type); + } + } + + if ($impossibility) { + $impossibilities[$var_id] = $impossibility; + } + } + + $clause->impossibilities = $impossibilities; + } + + /** + * This looks to see if there are any clauses in one formula that contradict + * clauses in another formula + * + * e.g. + * if ($a) { } + * elseif ($a) { } + * + * @param array $formula1 + * @param array $formula2 + * @param StatementsChecker $statements_checker, + * @param PhpParser\Node $stmt + * @return void + */ + public static function checkForParadox( + array $formula1, + array $formula2, + StatementsChecker $statements_checker, + PhpParser\Node $stmt + ) { + // remove impossible types + foreach ($formula1 as $clause_a) { + if (!$clause_a->reconcilable || $clause_a->wedge) { + continue; + } + + self::calculateNegation($clause_a); + + foreach ($formula2 as $clause_b) { + if ($clause_a === $clause_b || !$clause_b->reconcilable || $clause_b->wedge) { + continue; + } + + if ($clause_a->impossibilities == $clause_b->possibilities) { + if (IssueBuffer::accepts( + new ParadoxicalCondition( + 'Encountered a paradox when evaluating the conditional', + new CodeLocation($statements_checker, $stmt) + ), + $statements_checker->getSuppressedIssues() + )) { + // fall through + } + + return; + } + } + } } /** diff --git a/src/Psalm/Checker/Statements/Block/ForChecker.php b/src/Psalm/Checker/Statements/Block/ForChecker.php index 552a69b64..86b265ffc 100644 --- a/src/Psalm/Checker/Statements/Block/ForChecker.php +++ b/src/Psalm/Checker/Statements/Block/ForChecker.php @@ -63,7 +63,7 @@ class ForChecker $for_context->vars_in_scope[$var] ); - $context->removeVarFromClauses($var); + $context->removeVarFromConflictingClauses($var); } } diff --git a/src/Psalm/Checker/Statements/Block/ForeachChecker.php b/src/Psalm/Checker/Statements/Block/ForeachChecker.php index 063553adb..341495fbc 100644 --- a/src/Psalm/Checker/Statements/Block/ForeachChecker.php +++ b/src/Psalm/Checker/Statements/Block/ForeachChecker.php @@ -228,7 +228,7 @@ class ForeachChecker $foreach_context->vars_in_scope[$var] ); - $context->removeVarFromClauses($var); + $context->removeVarFromConflictingClauses($var); } } diff --git a/src/Psalm/Checker/Statements/Block/IfChecker.php b/src/Psalm/Checker/Statements/Block/IfChecker.php index f78af5957..2be921bd2 100644 --- a/src/Psalm/Checker/Statements/Block/IfChecker.php +++ b/src/Psalm/Checker/Statements/Block/IfChecker.php @@ -98,6 +98,9 @@ class IfChecker $statements_checker ); + // this will see whether any of the clauses in set A conflict with the clauses in set B + AlgebraChecker::checkForParadox($context->clauses, $if_clauses, $statements_checker, $stmt->cond); + $if_context->clauses = AlgebraChecker::simplifyCNF(array_merge($context->clauses, $if_clauses)); $if_scope->negated_clauses = AlgebraChecker::negateFormula($if_clauses); @@ -351,7 +354,7 @@ class IfChecker ); foreach ($changed_vars as $changed_var) { - $outer_context->removeVarFromClauses($changed_var); + $outer_context->removeVarFromConflictingClauses($changed_var); } if ($outer_context_vars_reconciled === false) { @@ -460,10 +463,14 @@ class IfChecker $statements_checker ); + $entry_clauses = array_merge($original_context->clauses, $if_scope->negated_clauses); + + // this will see whether any of the clauses in set A conflict with the clauses in set B + AlgebraChecker::checkForParadox($entry_clauses, $elseif_clauses, $statements_checker, $elseif->cond); + $elseif_context->clauses = AlgebraChecker::simplifyCNF( array_merge( - $original_context->clauses, - $if_scope->negated_clauses, + $entry_clauses, $elseif_clauses ) ); diff --git a/src/Psalm/Checker/Statements/Block/WhileChecker.php b/src/Psalm/Checker/Statements/Block/WhileChecker.php index 9ab64730f..8c97f4448 100644 --- a/src/Psalm/Checker/Statements/Block/WhileChecker.php +++ b/src/Psalm/Checker/Statements/Block/WhileChecker.php @@ -39,6 +39,8 @@ class WhileChecker $statements_checker ); + $while_context->parent_context = $context; + $while_context->clauses = AlgebraChecker::simplifyCNF(array_merge($context->clauses, $while_clauses)); $reconcilable_while_types = AlgebraChecker::getTruthsFromFormula($while_context->clauses); @@ -113,7 +115,7 @@ class WhileChecker $context->vars_in_scope = $vars_in_scope_reconciled; foreach ($changed_vars as $changed_var) { - $context->removeVarFromClauses($changed_var); + $context->removeVarFromConflictingClauses($changed_var); } } } diff --git a/src/Psalm/Checker/Statements/Expression/AssignmentChecker.php b/src/Psalm/Checker/Statements/Expression/AssignmentChecker.php index 5d3afd4c6..1bb0c4bd8 100644 --- a/src/Psalm/Checker/Statements/Expression/AssignmentChecker.php +++ b/src/Psalm/Checker/Statements/Expression/AssignmentChecker.php @@ -101,7 +101,7 @@ class AssignmentChecker if ($assign_value && ExpressionChecker::analyze($statements_checker, $assign_value, $context) === false) { if ($var_id) { if ($array_var_id) { - $context->removeDescendents($array_var_id); + $context->removeDescendents($array_var_id, null, $assign_value_type); } // if we're not exiting immediately, make everything mixed @@ -123,10 +123,13 @@ class AssignmentChecker } if ($array_var_id && isset($context->vars_in_scope[$array_var_id])) { - if ((string)$context->vars_in_scope[$array_var_id] !== (string)$assign_value_type) { - // removes dependennt vars from $context - $context->removeDescendents($array_var_id); - } + // removes dependennt vars from $context + $context->removeDescendents( + $array_var_id, + $context->vars_in_scope[$array_var_id], + $assign_value_type, + $statements_checker->getFileChecker() + ); } if ($assign_value_type->isMixed()) { diff --git a/src/Psalm/Checker/TypeChecker.php b/src/Psalm/Checker/TypeChecker.php index bb129e958..3b0aa69b2 100644 --- a/src/Psalm/Checker/TypeChecker.php +++ b/src/Psalm/Checker/TypeChecker.php @@ -1070,9 +1070,7 @@ class TypeChecker */ public static function simplifyUnionType(Type\Union $union, FileChecker $file_checker) { - $union_type_count = count($union->types); - - if ($union_type_count === 1 || ($union_type_count === 2 && $union->isNullable())) { + if (count($union->types) === 1) { return $union; } diff --git a/src/Psalm/Context.php b/src/Psalm/Context.php index b9e016aa1..2b9a4765b 100644 --- a/src/Psalm/Context.php +++ b/src/Psalm/Context.php @@ -1,6 +1,9 @@ vars_possibly_in_scope[$remove_var_id]); if (isset($this->vars_in_scope[$remove_var_id])) { - $type = $this->vars_in_scope[$remove_var_id]; + $existing_type = $this->vars_in_scope[$remove_var_id]; unset($this->vars_in_scope[$remove_var_id]); - $this->removeDescendents($remove_var_id, $type); + $this->removeDescendents($remove_var_id, $existing_type); } } /** - * @param string $remove_var_id + * @param string $remove_var_id + * @param Union|null $new_type + * @param FileChecker|null $file_checker * @return void */ - public function removeVarFromClauses($remove_var_id) - { + public function removeVarFromConflictingClauses( + $remove_var_id, + Union $new_type = null, + FileChecker $file_checker = null + ) { $clauses_to_keep = []; + $new_type_string = (string)$new_type; + foreach ($this->clauses as $clause) { - if (!isset($clause->possibilities[$remove_var_id])) { + \Psalm\Checker\AlgebraChecker::calculateNegation($clause); + + if (!isset($clause->possibilities[$remove_var_id]) || + $clause->possibilities[$remove_var_id] === [$new_type_string] + ) { $clauses_to_keep[] = $clause; + } elseif ($file_checker && $new_type && !in_array('empty', $clause->possibilities[$remove_var_id])) { + $type_changed = false; + + // if the clause contains any possibilities that would be altered + foreach ($clause->possibilities[$remove_var_id] as $type) { + $result_type = \Psalm\Checker\TypeChecker::reconcileTypes( + $type, + clone $new_type, + null, + $file_checker, + null, + [], + $failed_reconciliation + ); + + if ((string)$result_type !== $new_type_string) { + $type_changed = true; + break; + } + } + + if (!$type_changed) { + $clauses_to_keep[] = $clause; + } } } $this->clauses = $clauses_to_keep; if ($this->parent_context) { - $this->parent_context->removeVarFromClauses($remove_var_id); + $this->parent_context->removeVarFromConflictingClauses($remove_var_id); } } /** * @param string $remove_var_id - * @param \Psalm\Type\Union|null $type + * @param \Psalm\Type\Union|null $existing_type + * @param \Psalm\Type\Union|null $new_type + * @param FileChecker|null $file_checker * @return void */ - public function removeDescendents($remove_var_id, \Psalm\Type\Union $type = null) - { - if (!$type && isset($this->vars_in_scope[$remove_var_id])) { - $type = $this->vars_in_scope[$remove_var_id]; + public function removeDescendents( + $remove_var_id, + Union $existing_type = null, + Union $new_type = null, + FileChecker $file_checker = null + ) { + if (!$existing_type && isset($this->vars_in_scope[$remove_var_id])) { + $existing_type = $this->vars_in_scope[$remove_var_id]; } - if (!$type) { + if (!$existing_type) { return; } - $this->removeVarFromClauses($remove_var_id); + $this->removeVarFromConflictingClauses($remove_var_id, $new_type, $file_checker); - if ($type->hasArray() || $type->isMixed()) { + if ($existing_type->hasArray() || $existing_type->isMixed()) { $vars_to_remove = []; foreach ($this->vars_in_scope as $var_id => $_) { diff --git a/src/Psalm/Issue/ParadoxicalCondition.php b/src/Psalm/Issue/ParadoxicalCondition.php new file mode 100644 index 000000000..02db71eec --- /dev/null +++ b/src/Psalm/Issue/ParadoxicalCondition.php @@ -0,0 +1,6 @@ +project_checker, $stmts); $file_checker->visitAndAnalyzeMethods(); } + + /** + * @expectedException \Psalm\Exception\CodeException + * @expectedExceptionMessage ParadoxicalCondition + * @return void + */ + public function testRepeatedIfStatements() + { + $stmts = self::$parser->parse('project_checker, $stmts); + $file_checker->visitAndAnalyzeMethods(); + } + + /** + * @expectedException \Psalm\Exception\CodeException + * @expectedExceptionMessage ParadoxicalCondition + * @return void + */ + public function testRepeatedConditionals() + { + $stmts = self::$parser->parse('project_checker, $stmts); + $file_checker->visitAndAnalyzeMethods(); + } + + /** + * This shouldn't throw an error + * + * @return void + */ + public function testDifferentValueChecks() + { + $stmts = self::$parser->parse('project_checker, $stmts); + $file_checker->visitAndAnalyzeMethods(); + } + + /** + * This shouldn't throw an error + * + * @return void + */ + public function testRepeatedSet() + { + $stmts = self::$parser->parse('project_checker, $stmts); + $file_checker->visitAndAnalyzeMethods(); + } + + /** + * @return void + */ + public function testRepeatedSetInsideWhile() + { + $stmts = self::$parser->parse('project_checker, $stmts); + $file_checker->visitAndAnalyzeMethods(); + } }