From d7d9ddc653b7d292970baa9f6de2bf43b7db57ef Mon Sep 17 00:00:00 2001 From: Matt Brown Date: Sun, 17 Jul 2022 12:51:17 -0400 Subject: [PATCH 1/3] Remove special handling for elseifs that breaks for else if --- src/Psalm/Context.php | 2 - .../Statements/Block/IfElse/IfAnalyzer.php | 44 +------------------ .../Statements/Block/IfElseAnalyzer.php | 3 +- tests/TypeReconciliation/TypeAlgebraTest.php | 2 +- 4 files changed, 3 insertions(+), 48 deletions(-) diff --git a/src/Psalm/Context.php b/src/Psalm/Context.php index 286cbf72ee4..75738e3c23d 100644 --- a/src/Psalm/Context.php +++ b/src/Psalm/Context.php @@ -485,8 +485,6 @@ public function update( if ((!$new_type || !$old_type->equals($new_type)) && ($new_type || count($existing_type->getAtomicTypes()) > 1) ) { - $existing_type->substitute($old_type, $new_type); - if ($new_type && $new_type->from_docblock) { $existing_type->setFromDocblock(); } diff --git a/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/IfAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/IfAnalyzer.php index de76e3bb531..d06c4d9b2d5 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/IfAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/IfAnalyzer.php @@ -23,25 +23,20 @@ use Psalm\Node\VirtualArg; use Psalm\Type; 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; /** * @internal @@ -49,8 +44,6 @@ class IfAnalyzer { /** - * @param array $pre_assignment_else_redefined_vars - * * @return false|null */ public static function analyze( @@ -59,8 +52,7 @@ public static function analyze( IfScope $if_scope, IfConditionalScope $if_conditional_scope, Context $if_context, - Context $outer_context, - array $pre_assignment_else_redefined_vars + Context $outer_context ): ?bool { $cond_referenced_var_ids = $if_conditional_scope->cond_referenced_var_ids; @@ -153,8 +145,6 @@ public static function analyze( $outer_context->vars_possibly_in_scope ); - $old_if_context = clone $if_context; - $codebase = $statements_analyzer->getCodebase(); $assigned_var_ids = $if_context->assigned_var_ids; @@ -267,38 +257,6 @@ public static function analyze( } } - // 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) { - $vars_to_update = array_intersect( - array_keys($pre_assignment_else_redefined_vars), - array_keys($if_scope->negated_types) - ); - - $extra_vars_to_update = []; - - // if there's an object-like array in there, we also need to update the root array variable - foreach ($vars_to_update as $var_id) { - $bracked_pos = strpos($var_id, '['); - if ($bracked_pos !== false) { - $extra_vars_to_update[] = substr($var_id, 0, $bracked_pos); - } - } - - if ($extra_vars_to_update) { - $vars_to_update = array_unique(array_merge($extra_vars_to_update, $vars_to_update)); - } - - $outer_context->update( - $old_if_context, - $if_context, - $has_leaving_statements, - $vars_to_update, - $if_scope->updated_vars - ); - } - if (!$has_ending_statements) { $vars_possibly_in_scope = array_diff_key( $if_context->vars_possibly_in_scope, diff --git a/src/Psalm/Internal/Analyzer/Statements/Block/IfElseAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Block/IfElseAnalyzer.php index 91bb49a50b8..117482f7335 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Block/IfElseAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Block/IfElseAnalyzer.php @@ -265,8 +265,7 @@ public static function analyze( $if_scope, $if_conditional_scope, $if_context, - $context, - $pre_assignment_else_redefined_vars + $context ) === false) { return false; } diff --git a/tests/TypeReconciliation/TypeAlgebraTest.php b/tests/TypeReconciliation/TypeAlgebraTest.php index c3cae983726..60aed090452 100644 --- a/tests/TypeReconciliation/TypeAlgebraTest.php +++ b/tests/TypeReconciliation/TypeAlgebraTest.php @@ -212,7 +212,7 @@ function foo(?string $a): void { } }', ], - 'twoVarLogicNotNestedWithElseifCorrectlyReinforcedInIf' => [ + 'SKIPPED-twoVarLogicNotNestedWithElseifCorrectlyReinforcedInIf' => [ 'code' => ' Date: Sun, 17 Jul 2022 13:11:32 -0400 Subject: [PATCH 2/3] Simplify context updates even more --- src/Psalm/Context.php | 26 +++----------------------- 1 file changed, 3 insertions(+), 23 deletions(-) diff --git a/src/Psalm/Context.php b/src/Psalm/Context.php index 75738e3c23d..17329fa6d27 100644 --- a/src/Psalm/Context.php +++ b/src/Psalm/Context.php @@ -459,7 +459,7 @@ public function update( array $vars_to_update, array &$updated_vars ): void { - foreach ($start_context->vars_in_scope as $var_id => $old_type) { + foreach ($start_context->vars_in_scope as $var_id => $_) { // this is only true if there was some sort of type negation if (in_array($var_id, $vars_to_update, true)) { // if we're leaving, we're effectively deleting the possibility of the if types @@ -469,30 +469,10 @@ public function update( $existing_type = $this->vars_in_scope[$var_id] ?? null; - if (!$existing_type) { - if ($new_type) { - $this->vars_in_scope[$var_id] = clone $new_type; - $updated_vars[$var_id] = true; - } - - continue; - } - - $existing_type = clone $existing_type; - - // if the type changed within the block of statements, process the replacement - // also never allow ourselves to remove all types from a union - if ((!$new_type || !$old_type->equals($new_type)) - && ($new_type || count($existing_type->getAtomicTypes()) > 1) - ) { - if ($new_type && $new_type->from_docblock) { - $existing_type->setFromDocblock(); - } - + if (!$existing_type && $new_type) { + $this->vars_in_scope[$var_id] = clone $new_type; $updated_vars[$var_id] = true; } - - $this->vars_in_scope[$var_id] = $existing_type; } } } From e4f73beb0848c120207d2ed71a0e354d4e4b2e6f Mon Sep 17 00:00:00 2001 From: Matthew Brown Date: Sun, 17 Jul 2022 14:06:07 -0400 Subject: [PATCH 3/3] Add comment for skipped test --- tests/TypeReconciliation/TypeAlgebraTest.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/TypeReconciliation/TypeAlgebraTest.php b/tests/TypeReconciliation/TypeAlgebraTest.php index 60aed090452..b25593f9ca5 100644 --- a/tests/TypeReconciliation/TypeAlgebraTest.php +++ b/tests/TypeReconciliation/TypeAlgebraTest.php @@ -212,6 +212,9 @@ function foo(?string $a): void { } }', ], + // skipping this old test because it breaks if the elseif becomes + // an else if, and I don't believe it's a pattern that happens often + // enough to warrant special-casing 'SKIPPED-twoVarLogicNotNestedWithElseifCorrectlyReinforcedInIf' => [ 'code' => '