Fix if propagation (#8326)

* Add failing unit test

* Fix test

* Revert "Add comment for skipped test"

This reverts commit e4f73be.

* Revert "Simplify context updates even more"

This reverts commit a32e63f.

* Revert "Remove special handling for elseifs that breaks for else if"

This reverts commit d7d9ddc.

* Fix test

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 => $_) {
+        foreach ($start_context->vars_in_scope as $var_id => $old_type) {
             // 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,10 +469,32 @@ public function update(
 
                 $existing_type = $this->vars_in_scope[$var_id] ?? null;
 
-                if (!$existing_type && $new_type) {
-                    $this->vars_in_scope[$var_id] = clone $new_type;
+                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)
+                ) {
+                    $existing_type->substitute($old_type, $new_type);
+
+                    if ($new_type && $new_type->from_docblock) {
+                        $existing_type->setFromDocblock();
+                    }
+
                     $updated_vars[$var_id] = true;
                 }
+
+                $this->vars_in_scope[$var_id] = $existing_type;
             }
         }
     }

src/Psalm/Internal/Analyzer/Statements/Block/IfElse/IfAnalyzer.php

@@ -23,27 +23,34 @@
 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
  */
 class IfAnalyzer
 {
     /**
+     * @param  array<string, Union> $pre_assignment_else_redefined_vars
+     *
      * @return false|null
      */
     public static function analyze(
@@ -52,7 +59,8 @@ public static function analyze(
         IfScope $if_scope,
         IfConditionalScope $if_conditional_scope,
         Context $if_context,
-        Context $outer_context
+        Context $outer_context,
+        array $pre_assignment_else_redefined_vars
     ): ?bool {
         $cond_referenced_var_ids = $if_conditional_scope->cond_referenced_var_ids;
 
@@ -142,6 +150,8 @@ 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;
@@ -254,6 +264,38 @@ 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,

src/Psalm/Internal/Analyzer/Statements/Block/IfElseAnalyzer.php

@@ -264,7 +264,8 @@ public static function analyze(
             $if_scope,
             $if_conditional_scope,
             $if_context,
-            $context
+            $context,
+            $pre_assignment_else_redefined_vars
         ) === false) {
             return false;
         }

tests/TypeReconciliation/ConditionalTest.php

@@ -17,6 +17,21 @@ class ConditionalTest extends TestCase
     public function providerValidCodeParse(): iterable
     {
         return [
+            'arrayAssignmentPropagation' => [
+                'code' => '<?php
+                    $dummy = ["test" => 123];
+
+                    /** @var array{test: ?int} */
+                    $a = ["test" => null];
+
+                    if ($a["test"] === null) {
+                        $a = $dummy;
+                    }
+                    $var = $a["test"];',
+                'assertions' => [
+                    '$var' => 'int',
+                ],
+            ],
             'intIsMixed' => [
                 'code' => '<?php
                     /** @param mixed $a */

tests/TypeReconciliation/TypeAlgebraTest.php

@@ -212,10 +212,7 @@ 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' => [
+            'twoVarLogicNotNestedWithElseifCorrectlyReinforcedInIf' => [
                 'code' => '<?php
                     class A {}
                     class B extends A {}