From faaf7690f64aef40865e1f81a3fd34f51db4a50c Mon Sep 17 00:00:00 2001 From: Matthew Brown Date: Fri, 28 Jan 2022 18:30:47 -0500 Subject: [PATCH] Remove mic-drop hack from if analysis (#7484) * Remove mic-drop hack from if analysis * Remove more special handling * Remove some unnecessary ElseAnalyzer code * Add back necessary code * Fix return type of method never returning null * Add a comment * Simplify && handling * Add comments to make stuff clearer * Move if-specfic logic to more appropriate setting --- src/Psalm/Context.php | 2 +- .../Block/IfConditionalAnalyzer.php | 23 +- .../Statements/Block/IfElse/ElseAnalyzer.php | 49 ++-- .../Statements/Block/IfElse/IfAnalyzer.php | 234 ++++++++---------- .../Statements/Block/IfElseAnalyzer.php | 150 ++++------- .../Expression/AssignmentAnalyzer.php | 27 +- .../Expression/BinaryOp/AndAnalyzer.php | 34 ++- .../Expression/BinaryOp/OrAnalyzer.php | 28 ++- .../Expression/IncDecExpressionAnalyzer.php | 2 +- .../Statements/ExpressionAnalyzer.php | 4 +- .../Internal/Codebase/VariableUseGraph.php | 1 + src/Psalm/Internal/Scope/IfScope.php | 5 + tests/Loop/DoTest.php | 1 + tests/TypeReconciliation/ConditionalTest.php | 1 + .../RedundantConditionTest.php | 13 + tests/TypeReconciliation/TypeAlgebraTest.php | 18 +- tests/UnusedVariableTest.php | 36 ++- 17 files changed, 297 insertions(+), 331 deletions(-) diff --git a/src/Psalm/Context.php b/src/Psalm/Context.php index 23718645ded..ebc68666a2d 100644 --- a/src/Psalm/Context.php +++ b/src/Psalm/Context.php @@ -349,7 +349,7 @@ class Context /** * @var Context|null */ - public $if_context; + public $if_body_context; /** * @var bool diff --git a/src/Psalm/Internal/Analyzer/Statements/Block/IfConditionalAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Block/IfConditionalAnalyzer.php index 7c672ca138d..2973fcaf96d 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Block/IfConditionalAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Block/IfConditionalAnalyzer.php @@ -114,14 +114,12 @@ public static function analyze( $outer_context->inside_conditional = true; - if ($externally_applied_if_cond_expr) { - if (ExpressionAnalyzer::analyze( - $statements_analyzer, - $externally_applied_if_cond_expr, - $outer_context - ) === false) { - throw new ScopeAnalysisException(); - } + if (ExpressionAnalyzer::analyze( + $statements_analyzer, + $externally_applied_if_cond_expr, + $outer_context + ) === false) { + throw new ScopeAnalysisException(); } $first_cond_assigned_var_ids = $outer_context->assigned_var_ids; @@ -143,7 +141,10 @@ public static function analyze( } $if_conditional_context = clone $if_context; - $if_conditional_context->if_context = $if_context; + + // here we set up a context specifically for the statements in the first `if`, which can + // be affected by statements in the if condition + $if_conditional_context->if_body_context = $if_context; if ($codebase->alter_code) { $if_context->branch_point = $branch_point; @@ -236,7 +237,7 @@ public static function analyze( * Returns statements that are definitely evaluated before any statements after the end of the * if/elseif/else blocks */ - private static function getDefinitelyEvaluatedExpressionAfterIf(PhpParser\Node\Expr $stmt): ?PhpParser\Node\Expr + private static function getDefinitelyEvaluatedExpressionAfterIf(PhpParser\Node\Expr $stmt): PhpParser\Node\Expr { if ($stmt instanceof PhpParser\Node\Expr\BinaryOp\Equal || $stmt instanceof PhpParser\Node\Expr\BinaryOp\Identical @@ -280,7 +281,7 @@ private static function getDefinitelyEvaluatedExpressionAfterIf(PhpParser\Node\E * Returns statements that are definitely evaluated before any statements inside * the if block */ - private static function getDefinitelyEvaluatedExpressionInsideIf(PhpParser\Node\Expr $stmt): ?PhpParser\Node\Expr + private static function getDefinitelyEvaluatedExpressionInsideIf(PhpParser\Node\Expr $stmt): PhpParser\Node\Expr { if ($stmt instanceof PhpParser\Node\Expr\BinaryOp\Equal || $stmt instanceof PhpParser\Node\Expr\BinaryOp\Identical diff --git a/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/ElseAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/ElseAnalyzer.php index 4d878054555..06c88b06cb1 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/ElseAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/ElseAnalyzer.php @@ -59,16 +59,6 @@ public static function analyze( $else_types = Algebra::getTruthsFromFormula($else_context->clauses); - if (!$else && !$else_types) { - $if_scope->final_actions = array_merge([ScopeAnalyzer::ACTION_NONE], $if_scope->final_actions); - $if_scope->assigned_var_ids = []; - $if_scope->new_vars = []; - $if_scope->redefined_vars = []; - $if_scope->reasonable_clauses = []; - - return null; - } - $original_context = clone $else_context; if ($else_types) { @@ -120,19 +110,19 @@ public static function analyze( ) { return false; } + } - foreach ($else_context->parent_remove_vars as $var_id => $_) { - $outer_context->removeVarFromConflictingClauses($var_id); - } + foreach ($else_context->parent_remove_vars as $var_id => $_) { + $outer_context->removeVarFromConflictingClauses($var_id); } /** @var array */ $new_assigned_var_ids = $else_context->assigned_var_ids; - $else_context->assigned_var_ids = $pre_stmts_assigned_var_ids; + $else_context->assigned_var_ids += $pre_stmts_assigned_var_ids; /** @var array */ $new_possibly_assigned_var_ids = $else_context->possibly_assigned_var_ids; - $else_context->possibly_assigned_var_ids = $pre_possibly_assigned_var_ids + $new_possibly_assigned_var_ids; + $else_context->possibly_assigned_var_ids += $pre_possibly_assigned_var_ids; if ($else) { foreach ($else_context->byref_constraints as $var_id => $byref_constraint) { @@ -185,7 +175,7 @@ public static function analyze( $new_assigned_var_ids, $new_possibly_assigned_var_ids, [], - (bool) $else + true ); $if_scope->reasonable_clauses = []; @@ -210,24 +200,21 @@ public static function analyze( $possibly_assigned_var_ids = $new_possibly_assigned_var_ids; - if ($has_leaving_statements && $else_context->loop_scope) { - if (!$has_continue_statement && !$has_break_statement) { - $if_scope->new_vars_possibly_in_scope = array_merge( - $vars_possibly_in_scope, - $if_scope->new_vars_possibly_in_scope - ); + if ($has_leaving_statements) { + if ($else_context->loop_scope) { + if (!$has_continue_statement && !$has_break_statement) { + $if_scope->new_vars_possibly_in_scope = array_merge( + $vars_possibly_in_scope, + $if_scope->new_vars_possibly_in_scope + ); + } - $if_scope->possibly_assigned_var_ids = array_merge( - $possibly_assigned_var_ids, - $if_scope->possibly_assigned_var_ids + $else_context->loop_scope->vars_possibly_in_scope = array_merge( + $vars_possibly_in_scope, + $else_context->loop_scope->vars_possibly_in_scope ); } - - $else_context->loop_scope->vars_possibly_in_scope = array_merge( - $vars_possibly_in_scope, - $else_context->loop_scope->vars_possibly_in_scope - ); - } elseif (!$has_leaving_statements) { + } else { $if_scope->new_vars_possibly_in_scope = array_merge( $vars_possibly_in_scope, $if_scope->new_vars_possibly_in_scope diff --git a/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/IfAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/IfAnalyzer.php index fda3b851617..095b885209f 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/IfAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/IfAnalyzer.php @@ -7,11 +7,10 @@ use Psalm\Codebase; use Psalm\Context; use Psalm\Internal\Algebra; -use Psalm\Internal\Analyzer\FunctionLikeAnalyzer; use Psalm\Internal\Analyzer\ScopeAnalyzer; use Psalm\Internal\Analyzer\Statements\ExpressionAnalyzer; use Psalm\Internal\Analyzer\StatementsAnalyzer; -use Psalm\Internal\Analyzer\TraitAnalyzer; +use Psalm\Internal\Clause; use Psalm\Internal\Scope\IfConditionalScope; use Psalm\Internal\Scope\IfScope; use Psalm\Internal\Type\Comparator\UnionTypeComparator; @@ -26,15 +25,21 @@ use Psalm\Type\Reconciler; use Psalm\Type\Union; +use function array_combine; use function array_diff_key; use function array_filter; use function array_intersect; use function array_intersect_key; +use function array_key_exists; use function array_keys; use function array_merge; +use function array_reduce; use function array_unique; use function count; use function in_array; +use function preg_match; +use function preg_quote; +use function spl_object_id; use function strpos; use function substr; @@ -54,10 +59,106 @@ public static function analyze( IfScope $if_scope, IfConditionalScope $if_conditional_scope, Context $if_context, - Context $old_if_context, Context $outer_context, array $pre_assignment_else_redefined_vars ): ?bool { + $cond_referenced_var_ids = $if_conditional_scope->cond_referenced_var_ids; + + $active_if_types = []; + + $reconcilable_if_types = Algebra::getTruthsFromFormula( + $if_context->clauses, + spl_object_id($stmt->cond), + $cond_referenced_var_ids, + $active_if_types + ); + + if (array_filter( + $outer_context->clauses, + fn($clause): bool => (bool)$clause->possibilities + )) { + $omit_keys = array_reduce( + $outer_context->clauses, + /** + * @param array $carry + * @return array + */ + fn(array $carry, Clause $clause): array => array_merge($carry, array_keys($clause->possibilities)), + [] + ); + + $omit_keys = array_combine($omit_keys, $omit_keys); + $omit_keys = array_diff_key($omit_keys, Algebra::getTruthsFromFormula($outer_context->clauses)); + + $cond_referenced_var_ids = array_diff_key( + $cond_referenced_var_ids, + $omit_keys + ); + } + + // if the if has an || in the conditional, we cannot easily reason about it + if ($reconcilable_if_types) { + $changed_var_ids = []; + + $if_vars_in_scope_reconciled = + Reconciler::reconcileKeyedTypes( + $reconcilable_if_types, + $active_if_types, + $if_context->vars_in_scope, + $if_context->references_in_scope, + $changed_var_ids, + $cond_referenced_var_ids, + $statements_analyzer, + $statements_analyzer->getTemplateTypeMap() ?: [], + $if_context->inside_loop, + $outer_context->check_variables + ? new CodeLocation( + $statements_analyzer->getSource(), + $stmt->cond instanceof PhpParser\Node\Expr\BooleanNot + ? $stmt->cond->expr + : $stmt->cond, + $outer_context->include_location + ) : null + ); + + $if_context->vars_in_scope = $if_vars_in_scope_reconciled; + + foreach ($reconcilable_if_types as $var_id => $_) { + $if_context->vars_possibly_in_scope[$var_id] = true; + } + + if ($changed_var_ids) { + $if_context->clauses = Context::removeReconciledClauses($if_context->clauses, $changed_var_ids)[0]; + + foreach ($changed_var_ids as $changed_var_id => $_) { + foreach ($if_context->vars_in_scope as $var_id => $_) { + if (preg_match('/' . preg_quote($changed_var_id, '/') . '[\]\[\-]/', $var_id) + && !array_key_exists($var_id, $changed_var_ids) + && !array_key_exists($var_id, $cond_referenced_var_ids) + ) { + $if_context->removePossibleReference($var_id); + } + } + } + } + + $if_scope->if_cond_changed_var_ids = $changed_var_ids; + } + + $if_context->reconciled_expression_clauses = []; + + $outer_context->vars_possibly_in_scope = array_merge( + $if_context->vars_possibly_in_scope, + $outer_context->vars_possibly_in_scope + ); + + $outer_context->referenced_var_ids = array_merge( + $if_context->referenced_var_ids, + $outer_context->referenced_var_ids + ); + + $old_if_context = clone $if_context; + $codebase = $statements_analyzer->getCodebase(); $assigned_var_ids = $if_context->assigned_var_ids; @@ -77,7 +178,7 @@ public static function analyze( $outer_context->removeVarFromConflictingClauses($var_id); } - $final_actions = ScopeAnalyzer::getControlActions( + $if_scope->if_actions = $final_actions = ScopeAnalyzer::getControlActions( $stmt->stmts, $statements_analyzer->node_data, [] @@ -91,6 +192,7 @@ public static function analyze( $has_break_statement = $final_actions === [ScopeAnalyzer::ACTION_BREAK]; $has_continue_statement = $final_actions === [ScopeAnalyzer::ACTION_CONTINUE]; + $if_scope->if_actions = $final_actions; $if_scope->final_actions = $final_actions; /** @var array */ @@ -128,8 +230,6 @@ public static function analyze( } } - $mic_drop = false; - if (!$has_leaving_statements) { self::updateIfScope( $codebase, @@ -168,27 +268,13 @@ public static function analyze( $if_conditional_scope->assigned_in_conditional_var_ids ); } - - if (!$stmt->else && !$stmt->elseifs) { - $mic_drop = self::handleMicDrop( - $statements_analyzer, - $stmt->cond, - $if_scope, - $outer_context, - $new_assigned_var_ids - ); - - $outer_context->clauses = Algebra::simplifyCNF( - array_merge($outer_context->clauses, $if_scope->negated_clauses) - ); - } } } // update the parent context as necessary, but only if we can safely reason about type negation. // We only update vars that changed both at the start of the if block and then again by an assignment // in the if statement. - if ($if_scope->negated_types && !$mic_drop) { + if ($if_scope->negated_types) { $vars_to_update = array_intersect( array_keys($pre_assignment_else_redefined_vars), array_keys($if_scope->negated_types) @@ -208,19 +294,6 @@ public static function analyze( $vars_to_update = array_unique(array_merge($extra_vars_to_update, $vars_to_update)); } - //update $if_context vars to include the pre-assignment else vars - if (!$stmt->else && !$has_leaving_statements) { - foreach ($pre_assignment_else_redefined_vars as $var_id => $type) { - if (isset($if_context->vars_in_scope[$var_id])) { - $if_context->vars_in_scope[$var_id] = Type::combineUnionTypes( - $if_context->vars_in_scope[$var_id], - $type, - $codebase - ); - } - } - } - $outer_context->update( $old_if_context, $if_context, @@ -263,99 +336,6 @@ public static function analyze( return null; } - /** - * This handles the situation when returning inside an - * if block with no else or elseifs - * - * @param array $new_assigned_var_ids - */ - private static function handleMicDrop( - StatementsAnalyzer $statements_analyzer, - PhpParser\Node\Expr $cond, - IfScope $if_scope, - Context $post_if_context, - array $new_assigned_var_ids - ): bool { - if (!$if_scope->negated_types) { - return false; - } - - $newly_reconciled_var_ids = []; - - $post_if_context_vars_reconciled = Reconciler::reconcileKeyedTypes( - $if_scope->negated_types, - [], - $post_if_context->vars_in_scope, - $post_if_context->references_in_scope, - $newly_reconciled_var_ids, - [], - $statements_analyzer, - $statements_analyzer->getTemplateTypeMap() ?: [], - $post_if_context->inside_loop, - new CodeLocation( - $statements_analyzer->getSource(), - $cond instanceof PhpParser\Node\Expr\BooleanNot - ? $cond->expr - : $cond, - $post_if_context->include_location, - false - ) - ); - - foreach ($newly_reconciled_var_ids as $changed_var_id => $_) { - $post_if_context->removeVarFromConflictingClauses($changed_var_id); - } - - $newly_reconciled_var_ids += $new_assigned_var_ids; - - foreach ($newly_reconciled_var_ids as $var_id => $_) { - $if_scope->negated_clauses = Context::filterClauses( - $var_id, - $if_scope->negated_clauses - ); - } - - foreach ($newly_reconciled_var_ids as $var_id => $_) { - $first_appearance = $statements_analyzer->getFirstAppearance($var_id); - - if ($first_appearance - && isset($post_if_context->vars_in_scope[$var_id]) - && isset($post_if_context_vars_reconciled[$var_id]) - && $post_if_context->vars_in_scope[$var_id]->hasMixed() - && !$post_if_context_vars_reconciled[$var_id]->hasMixed() - ) { - if (!$post_if_context->collect_initializations - && !$post_if_context->collect_mutations - && $statements_analyzer->getFilePath() === $statements_analyzer->getRootFilePath() - ) { - $parent_source = $statements_analyzer->getSource(); - - $functionlike_storage = $parent_source instanceof FunctionLikeAnalyzer - ? $parent_source->getFunctionLikeStorage($statements_analyzer) - : null; - - if (!$functionlike_storage - || (!$parent_source->getSource() instanceof TraitAnalyzer - && !isset($functionlike_storage->param_lookup[substr($var_id, 1)])) - ) { - $codebase = $statements_analyzer->getCodebase(); - $codebase->analyzer->decrementMixedCount($statements_analyzer->getFilePath()); - } - } - - IssueBuffer::remove( - $statements_analyzer->getFilePath(), - 'MixedAssignment', - $first_appearance->raw_file_start - ); - } - } - - $post_if_context->vars_in_scope = $post_if_context_vars_reconciled; - - return true; - } - /** * @param array $assigned_in_conditional_var_ids */ diff --git a/src/Psalm/Internal/Analyzer/Statements/Block/IfElseAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Block/IfElseAnalyzer.php index 316b91a8961..73046ada3fc 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Block/IfElseAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Block/IfElseAnalyzer.php @@ -10,27 +10,26 @@ use Psalm\Internal\Algebra; use Psalm\Internal\Algebra\FormulaGenerator; use Psalm\Internal\Analyzer\AlgebraAnalyzer; +use Psalm\Internal\Analyzer\FunctionLikeAnalyzer; use Psalm\Internal\Analyzer\ScopeAnalyzer; use Psalm\Internal\Analyzer\Statements\Block\IfElse\ElseAnalyzer; use Psalm\Internal\Analyzer\Statements\Block\IfElse\ElseIfAnalyzer; use Psalm\Internal\Analyzer\Statements\Block\IfElse\IfAnalyzer; use Psalm\Internal\Analyzer\StatementsAnalyzer; +use Psalm\Internal\Analyzer\TraitAnalyzer; use Psalm\Internal\Clause; use Psalm\Internal\Scope\IfScope; +use Psalm\IssueBuffer; use Psalm\Node\Expr\VirtualBooleanNot; use Psalm\Type; use Psalm\Type\Reconciler; -use function array_combine; use function array_diff; -use function array_diff_key; use function array_filter; use function array_intersect_key; -use function array_key_exists; use function array_keys; use function array_map; use function array_merge; -use function array_reduce; use function array_unique; use function array_values; use function count; @@ -38,6 +37,7 @@ use function preg_match; use function preg_quote; use function spl_object_id; +use function substr; /** * @internal @@ -106,10 +106,11 @@ public static function analyze( $context->branch_point ?: (int) $stmt->getAttribute('startFilePos') ); + // this is the context for stuff that happens within the `if` block $if_context = $if_conditional_scope->if_context; + // this is the context for stuff that happens after the `if` block $post_if_context = $if_conditional_scope->post_if_context; - $cond_referenced_var_ids = $if_conditional_scope->cond_referenced_var_ids; $assigned_in_conditional_var_ids = $if_conditional_scope->assigned_in_conditional_var_ids; } catch (ScopeAnalysisException $e) { return false; @@ -226,100 +227,6 @@ function (Clause $c) use ($mixed_var_ids, $cond_object_id): Clause { ) ); - $active_if_types = []; - - $reconcilable_if_types = Algebra::getTruthsFromFormula( - $if_context->clauses, - spl_object_id($stmt->cond), - $cond_referenced_var_ids, - $active_if_types - ); - - if (array_filter( - $context->clauses, - fn($clause): bool => (bool)$clause->possibilities - )) { - $omit_keys = array_reduce( - $context->clauses, - /** - * @param array $carry - * @return array - */ - fn(array $carry, Clause $clause): array => array_merge($carry, array_keys($clause->possibilities)), - [] - ); - - $omit_keys = array_combine($omit_keys, $omit_keys); - $omit_keys = array_diff_key($omit_keys, Algebra::getTruthsFromFormula($context->clauses)); - - $cond_referenced_var_ids = array_diff_key( - $cond_referenced_var_ids, - $omit_keys - ); - } - - // if the if has an || in the conditional, we cannot easily reason about it - if ($reconcilable_if_types) { - $changed_var_ids = []; - - $if_vars_in_scope_reconciled = - Reconciler::reconcileKeyedTypes( - $reconcilable_if_types, - $active_if_types, - $if_context->vars_in_scope, - $if_context->references_in_scope, - $changed_var_ids, - $cond_referenced_var_ids, - $statements_analyzer, - $statements_analyzer->getTemplateTypeMap() ?: [], - $if_context->inside_loop, - $context->check_variables - ? new CodeLocation( - $statements_analyzer->getSource(), - $stmt->cond instanceof PhpParser\Node\Expr\BooleanNot - ? $stmt->cond->expr - : $stmt->cond, - $context->include_location - ) : null - ); - - $if_context->vars_in_scope = $if_vars_in_scope_reconciled; - - foreach ($reconcilable_if_types as $var_id => $_) { - $if_context->vars_possibly_in_scope[$var_id] = true; - } - - if ($changed_var_ids) { - $if_context->clauses = Context::removeReconciledClauses($if_context->clauses, $changed_var_ids)[0]; - - foreach ($changed_var_ids as $changed_var_id => $_) { - foreach ($if_context->vars_in_scope as $var_id => $_) { - if (preg_match('/' . preg_quote($changed_var_id, '/') . '[\]\[\-]/', $var_id) - && !array_key_exists($var_id, $changed_var_ids) - && !array_key_exists($var_id, $cond_referenced_var_ids) - ) { - $if_context->removePossibleReference($var_id); - } - } - } - } - - $if_scope->if_cond_changed_var_ids = $changed_var_ids; - } - - $if_context->reconciled_expression_clauses = []; - - $old_if_context = clone $if_context; - $context->vars_possibly_in_scope = array_merge( - $if_context->vars_possibly_in_scope, - $context->vars_possibly_in_scope - ); - - $context->referenced_var_ids = array_merge( - $if_context->referenced_var_ids, - $context->referenced_var_ids - ); - $temp_else_context = clone $post_if_context; $changed_var_ids = []; @@ -362,7 +269,6 @@ function (Clause $c) use ($mixed_var_ids, $cond_object_id): Clause { $if_scope, $if_conditional_scope, $if_context, - $old_if_context, $context, $pre_assignment_else_redefined_vars ) === false) { @@ -405,6 +311,50 @@ function (Clause $c) use ($mixed_var_ids, $cond_object_id): Clause { return false; } + if (count($if_scope->if_actions) && !in_array(ScopeAnalyzer::ACTION_NONE, $if_scope->if_actions, true) + && !$stmt->elseifs + ) { + $context->clauses = $else_context->clauses; + foreach ($else_context->vars_in_scope as $var_id => $type) { + $context->vars_in_scope[$var_id] = clone $type; + } + + foreach ($pre_assignment_else_redefined_vars as $var_id => $reconciled_type) { + $first_appearance = $statements_analyzer->getFirstAppearance($var_id); + + if ($first_appearance + && isset($post_if_context->vars_in_scope[$var_id]) + && $post_if_context->vars_in_scope[$var_id]->hasMixed() + && !$reconciled_type->hasMixed() + ) { + if (!$post_if_context->collect_initializations + && !$post_if_context->collect_mutations + && $statements_analyzer->getFilePath() === $statements_analyzer->getRootFilePath() + ) { + $parent_source = $statements_analyzer->getSource(); + + $functionlike_storage = $parent_source instanceof FunctionLikeAnalyzer + ? $parent_source->getFunctionLikeStorage($statements_analyzer) + : null; + + if (!$functionlike_storage + || (!$parent_source->getSource() instanceof TraitAnalyzer + && !isset($functionlike_storage->param_lookup[substr($var_id, 1)])) + ) { + $codebase = $statements_analyzer->getCodebase(); + $codebase->analyzer->decrementMixedCount($statements_analyzer->getFilePath()); + } + } + + IssueBuffer::remove( + $statements_analyzer->getFilePath(), + 'MixedAssignment', + $first_appearance->raw_file_start + ); + } + } + } + if ($context->loop_scope) { $context->loop_scope->final_actions = array_unique( array_merge( diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php index 797de49243d..b3daa26555c 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php @@ -112,7 +112,8 @@ public static function analyze( ?Union $assign_value_type, Context $context, ?PhpParser\Comment\Doc $doc_comment, - array $not_ignored_docblock_var_ids = [] + array $not_ignored_docblock_var_ids = [], + ?PhpParser\Node\Expr $assign_expr = null ) { $var_id = ExpressionIdentifier::getVarId( $assign_var, @@ -616,6 +617,26 @@ public static function analyze( $added_taints ); } + + if ($assign_expr) { + $new_parent_node = DataFlowNode::getForAssignment( + 'assignment_expr', + new CodeLocation($statements_analyzer->getSource(), $assign_expr) + ); + + $data_flow_graph->addNode($new_parent_node); + + foreach ($context->vars_in_scope[$var_id]->parent_nodes as $old_parent_node) { + $data_flow_graph->addPath( + $old_parent_node, + $new_parent_node, + '=', + ); + } + + $assign_value_type = clone $assign_value_type; + $assign_value_type->parent_nodes = [$new_parent_node->id => $new_parent_node]; + } } } } @@ -1083,7 +1104,7 @@ public static function assignByRefParam( $by_ref_out_type->parent_nodes += $existing_type->parent_nodes; } - if (!$existing_type->isEmptyArray()) { + if (!$context->inside_conditional) { $context->vars_in_scope[$var_id] = $by_ref_out_type; if (!($stmt_type = $statements_analyzer->node_data->getType($stmt)) @@ -1505,7 +1526,7 @@ private static function analyzeDestructuringAssignment( $assignment_node->id => $assignment_node ]; } else { - if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph + if ($data_flow_graph instanceof TaintFlowGraph && in_array('TaintedInput', $statements_analyzer->getSuppressedIssues()) ) { $context->vars_in_scope[$list_var_id]->parent_nodes = []; diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOp/AndAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOp/AndAnalyzer.php index 7e3937c38ac..793a1ea162e 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOp/AndAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOp/AndAnalyzer.php @@ -180,42 +180,38 @@ public static function analyze( ); } - if ($context->if_context && !$context->inside_negation) { + if ($context->if_body_context && !$context->inside_negation) { + $if_body_context = $context->if_body_context; $context->vars_in_scope = $right_context->vars_in_scope; - $if_context = $context->if_context; - - foreach ($right_context->vars_in_scope as $var_id => $type) { - if (!isset($if_context->vars_in_scope[$var_id])) { - $if_context->vars_in_scope[$var_id] = $type; - } elseif (isset($context->vars_in_scope[$var_id])) { - $if_context->vars_in_scope[$var_id] = $context->vars_in_scope[$var_id]; - } - } + $if_body_context->vars_in_scope = array_merge( + $if_body_context->vars_in_scope, + $context->vars_in_scope + ); - $if_context->referenced_var_ids = array_merge( + $if_body_context->referenced_var_ids = array_merge( + $if_body_context->referenced_var_ids, $context->referenced_var_ids, - $if_context->referenced_var_ids ); - $if_context->assigned_var_ids = array_merge( + $if_body_context->assigned_var_ids = array_merge( + $if_body_context->assigned_var_ids, $context->assigned_var_ids, - $if_context->assigned_var_ids ); - $if_context->reconciled_expression_clauses = array_merge( - $if_context->reconciled_expression_clauses, + $if_body_context->reconciled_expression_clauses = array_merge( + $if_body_context->reconciled_expression_clauses, array_map( fn($c) => $c->hash, $partitioned_clauses[1] ) ); - $if_context->vars_possibly_in_scope = array_merge( + $if_body_context->vars_possibly_in_scope = array_merge( + $if_body_context->vars_possibly_in_scope, $context->vars_possibly_in_scope, - $if_context->vars_possibly_in_scope ); - $if_context->updateChecks($context); + $if_body_context->updateChecks($context); } else { $context->vars_in_scope = $left_context->vars_in_scope; } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOp/OrAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOp/OrAnalyzer.php index f5ef20d7a8f..78fa57971a6 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOp/OrAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOp/OrAnalyzer.php @@ -64,6 +64,8 @@ public static function analyze( $post_leaving_if_context = null; + // we cap this at max depth of 4 to prevent quadratic behaviour + // when analysing || || || || if (!$stmt->left instanceof PhpParser\Node\Expr\BinaryOp\BooleanOr || !$stmt->left->left instanceof PhpParser\Node\Expr\BinaryOp\BooleanOr || !$stmt->left->left->left instanceof PhpParser\Node\Expr\BinaryOp\BooleanOr @@ -100,7 +102,7 @@ public static function analyze( $post_leaving_if_context = clone $context; $left_context = clone $context; - $left_context->if_context = null; + $left_context->if_body_context = null; $left_context->assigned_var_ids = []; if (ExpressionAnalyzer::analyze($statements_analyzer, $stmt->left, $left_context) === false) { @@ -259,7 +261,7 @@ public static function analyze( ); } - $right_context->if_context = null; + $right_context->if_body_context = null; $pre_referenced_var_ids = $right_context->referenced_var_ids; $right_context->referenced_var_ids = []; @@ -369,18 +371,18 @@ public static function analyze( $right_context->assigned_var_ids ); - if ($context->if_context) { - $if_context = $context->if_context; + if ($context->if_body_context) { + $if_body_context = $context->if_body_context; foreach ($right_context->vars_in_scope as $var_id => $type) { - if (isset($if_context->vars_in_scope[$var_id])) { - $if_context->vars_in_scope[$var_id] = Type::combineUnionTypes( + if (isset($if_body_context->vars_in_scope[$var_id])) { + $if_body_context->vars_in_scope[$var_id] = Type::combineUnionTypes( $type, - $if_context->vars_in_scope[$var_id], + $if_body_context->vars_in_scope[$var_id], $codebase ); } elseif (isset($left_context->vars_in_scope[$var_id])) { - $if_context->vars_in_scope[$var_id] = Type::combineUnionTypes( + $if_body_context->vars_in_scope[$var_id] = Type::combineUnionTypes( $type, $left_context->vars_in_scope[$var_id], $codebase @@ -388,17 +390,17 @@ public static function analyze( } } - $if_context->referenced_var_ids = array_merge( + $if_body_context->referenced_var_ids = array_merge( $context->referenced_var_ids, - $if_context->referenced_var_ids + $if_body_context->referenced_var_ids ); - $if_context->assigned_var_ids = array_merge( + $if_body_context->assigned_var_ids = array_merge( $context->assigned_var_ids, - $if_context->assigned_var_ids + $if_body_context->assigned_var_ids ); - $if_context->updateChecks($context); + $if_body_context->updateChecks($context); } $context->vars_possibly_in_scope = array_merge( diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/IncDecExpressionAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/IncDecExpressionAnalyzer.php index 4ad506e9207..aff24fec7c1 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/IncDecExpressionAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/IncDecExpressionAnalyzer.php @@ -129,7 +129,7 @@ public static function analyze( if ($stmt instanceof PreInc || $stmt instanceof PreDec) { $old_node_data->setType( $stmt, - $statements_analyzer->node_data->getType($operation) ?? Type::getMixed() + $statements_analyzer->node_data->getType($fake_assignment) ?? Type::getMixed() ); } diff --git a/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php index a33d2e3c973..0d3c64c542f 100644 --- a/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php @@ -180,7 +180,9 @@ private static function handleExpression( $stmt->expr, null, $context, - $stmt->getDocComment() + $stmt->getDocComment(), + [], + !$from_stmt ? $stmt : null ); if ($assignment_type === false) { diff --git a/src/Psalm/Internal/Codebase/VariableUseGraph.php b/src/Psalm/Internal/Codebase/VariableUseGraph.php index 2e8c192e180..d07772d9bad 100644 --- a/src/Psalm/Internal/Codebase/VariableUseGraph.php +++ b/src/Psalm/Internal/Codebase/VariableUseGraph.php @@ -160,6 +160,7 @@ private function getChildNodes( || $path->type === 'use-inside-conditional' || $path->type === 'use-inside-isset' || $path->type === 'arg' + || $path->type === 'comparison' ) { return null; } diff --git a/src/Psalm/Internal/Scope/IfScope.php b/src/Psalm/Internal/Scope/IfScope.php index 93de1e9359b..696e405ee10 100644 --- a/src/Psalm/Internal/Scope/IfScope.php +++ b/src/Psalm/Internal/Scope/IfScope.php @@ -76,6 +76,11 @@ class IfScope */ public $reasonable_clauses = []; + /** + * @var string[] + */ + public $if_actions = []; + /** * @var string[] */ diff --git a/tests/Loop/DoTest.php b/tests/Loop/DoTest.php index 35fbbc0f7df..45f6056ce12 100644 --- a/tests/Loop/DoTest.php +++ b/tests/Loop/DoTest.php @@ -121,6 +121,7 @@ function foo(string $b) {} break; } + /** @psalm-suppress MixedArgument */ foo($a); } while (rand(0,100) === 10);', diff --git a/tests/TypeReconciliation/ConditionalTest.php b/tests/TypeReconciliation/ConditionalTest.php index 733743da3e7..ff02c810be4 100644 --- a/tests/TypeReconciliation/ConditionalTest.php +++ b/tests/TypeReconciliation/ConditionalTest.php @@ -2083,6 +2083,7 @@ class C { /** * @psalm-suppress MixedReturnStatement * @psalm-suppress MixedInferredReturnType + * @psalm-suppress MixedArrayAccess */ public static function get(string $k1, string $k2) : ?string { if (!isset(static::$cache[$k1][$k2])) { diff --git a/tests/TypeReconciliation/RedundantConditionTest.php b/tests/TypeReconciliation/RedundantConditionTest.php index 919b747eeb2..1038e354c87 100644 --- a/tests/TypeReconciliation/RedundantConditionTest.php +++ b/tests/TypeReconciliation/RedundantConditionTest.php @@ -1527,6 +1527,19 @@ function f(array $p) : void { }', 'error_message' => 'RedundantCondition' ], + 'secondFalsyTwiceWithoutChangeWithElse' => [ + 'code' => ' 'RedundantCondition' + ], ]; } } diff --git a/tests/TypeReconciliation/TypeAlgebraTest.php b/tests/TypeReconciliation/TypeAlgebraTest.php index ef44446565b..c1e2a763da3 100644 --- a/tests/TypeReconciliation/TypeAlgebraTest.php +++ b/tests/TypeReconciliation/TypeAlgebraTest.php @@ -19,18 +19,16 @@ public function providerValidCodeParse(): iterable return [ 'twoVarLogicSimple' => [ 'code' => ' [ @@ -1194,8 +1192,6 @@ function foo(?string $a, ?string $b, ?string $c): void { ], 'threeVarLogicWithException' => [ 'code' => ' 'PossiblyNullArgument', + 'error_message' => 'RedundantCondition', ], 'invertedTwoVarLogicNotNestedWithVarChange' => [ 'code' => ' [ - 'code' => ' 10) { - break; - } else {} - } - }', - ], 'referenceUseUsesReferencedVariable' => [ 'code' => ' [ + 'code' => ' 10) { + break; + } else {} + } + }', + ], + 'usedPlusInUnaryAddition' => [ + 'code' => ' 10) { + break; + } else {} + } + }', + ], ]; }