Fix #4354 - allow assignments on RHS of || in if conditional

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

@@ -18,6 +18,7 @@
 use Psalm\Issue\RedundantCondition;
 use Psalm\IssueBuffer;
 use Psalm\Internal\Scope\IfScope;
+use Psalm\Internal\Scope\IfConditionalScope;
 use Psalm\Type;
 use Psalm\Type\Algebra;
 use Psalm\Type\Reconciler;
@@ -77,6 +78,23 @@ public static function analyze(
 
         $if_scope = new IfScope();
 
+        // We need to clone the original context for later use if we're exiting in this if conditional
+        if (!$stmt->else && !$stmt->elseifs && $stmt->cond instanceof PhpParser\Node\Expr\BinaryOp\BooleanOr) {
+            $final_actions = ScopeAnalyzer::getControlActions(
+                $stmt->stmts,
+                $statements_analyzer->node_data,
+                $codebase->config->exit_functions,
+                $context->break_types
+            );
+
+            $has_leaving_statements = $final_actions === [ScopeAnalyzer::ACTION_END]
+                || (count($final_actions) && !in_array(ScopeAnalyzer::ACTION_NONE, $final_actions, true));
+
+            if ($has_leaving_statements) {
+                $if_scope->mic_drop_context = clone $context;
+            }
+        }
+
         try {
             $if_conditional_scope = self::analyzeIfConditional(
                 $statements_analyzer,
@@ -334,6 +352,7 @@ function (array $carry, Clause $clause): array {
             $statements_analyzer,
             $stmt,
             $if_scope,
+            $if_conditional_scope,
             $if_context,
             $old_if_context,
             $context,
@@ -489,7 +508,7 @@ public static function analyzeIfConditional(
         Codebase $codebase,
         IfScope $if_scope,
         ?int $branch_point
-    ): \Psalm\Internal\Scope\IfConditionalScope {
+    ): IfConditionalScope {
         $entry_clauses = [];
 
         // used when evaluating elseifs
@@ -746,6 +765,7 @@ protected static function analyzeIfBlock(
         StatementsAnalyzer $statements_analyzer,
         PhpParser\Node\Stmt\If_ $stmt,
         IfScope $if_scope,
+        IfConditionalScope $if_conditional_scope,
         Context $if_context,
         Context $old_if_context,
         Context $outer_context,
@@ -859,6 +879,46 @@ protected static function analyzeIfBlock(
         }
 
         if ($has_leaving_statements && !$has_break_statement && !$stmt->else && !$stmt->elseifs) {
+            // If we're assigning inside
+            if ($if_conditional_scope->cond_assigned_var_ids
+                && $stmt->cond instanceof PhpParser\Node\Expr\BinaryOp\BooleanOr
+                && $if_scope->mic_drop_context
+            ) {
+                $exprs = self::getDefinitelyEvaluatedOredExpressions($stmt->cond);
+
+                // if there was no assignment in the first expression it's safe to proceed
+                $old_node_data = $statements_analyzer->node_data;
+                $statements_analyzer->node_data = clone $old_node_data;
+
+                foreach ($exprs as $expr) {
+                    $fake_negated_expr = new PhpParser\Node\Expr\FuncCall(
+                        new PhpParser\Node\Name\FullyQualified('assert'),
+                        [new PhpParser\Node\Arg(
+                            new PhpParser\Node\Expr\BooleanNot($expr, $expr->getAttributes()),
+                            false,
+                            false,
+                            $expr->getAttributes()
+                        )],
+                        $expr->getAttributes()
+                    );
+
+                    ExpressionAnalyzer::analyze(
+                        $statements_analyzer,
+                        $fake_negated_expr,
+                        $if_scope->mic_drop_context
+                    );
+                }
+
+                $statements_analyzer->node_data = $old_node_data;
+
+                foreach ($if_conditional_scope->cond_assigned_var_ids as $var_id => $_) {
+                    if (isset($if_scope->mic_drop_context->vars_in_scope[$var_id])) {
+                        $outer_context->vars_in_scope[$var_id]
+                            = clone $if_scope->mic_drop_context->vars_in_scope[$var_id];
+                    }
+                }
+            }
+
             if ($if_scope->negated_types) {
                 $changed_var_ids = [];
 
@@ -1789,4 +1849,25 @@ private static function getDefinitelyEvaluatedExpressionInsideIf(PhpParser\Node\
 
         return $stmt;
     }
+
+    /**
+     * Returns all expressions inside an ored expression
+     * @return non-empty-list<PhpParser\Node\Expr>
+     */
+    private static function getDefinitelyEvaluatedOredExpressions(PhpParser\Node\Expr $stmt): array
+    {
+        if ($stmt instanceof PhpParser\Node\Expr\BinaryOp\BooleanOr
+            || $stmt instanceof PhpParser\Node\Expr\BinaryOp\LogicalOr
+            || $stmt instanceof PhpParser\Node\Expr\BinaryOp\LogicalXor
+        ) {
+            return array_merge(
+                self::getDefinitelyEvaluatedOredExpressions($stmt->left),
+                self::getDefinitelyEvaluatedOredExpressions($stmt->right)
+            );
+        }
+
+        return [$stmt];
+    }
+
+
 }

src/Psalm/Internal/Analyzer/Statements/Expression/CallAnalyzer.php

@@ -497,8 +497,6 @@ public static function getFunctionIdsFromCallableArg(
             return [$fq_class_name . '::' . $method_name_arg->value];
         }
 
-        $class_arg_type = null;
-
         if (!$file_source instanceof StatementsAnalyzer
             || !($class_arg_type = $file_source->node_data->getType($class_arg))
         ) {

src/Psalm/Internal/Codebase/InternalCallMapHandler.php

@@ -140,8 +140,6 @@ public static function getMatchingCallableFromCallMapOptions(
                     continue;
                 }
 
-                $arg_type = null;
-
                 if (!$nodes
                     || !($arg_type = $nodes->getType($arg->value))
                 ) {

src/Psalm/Internal/Scope/IfScope.php

@@ -84,4 +84,9 @@ class IfScope
      * @var string[]
      */
     public $final_actions = [];
+
+    /**
+     * @var ?\Psalm\Context
+     */
+    public $mic_drop_context;
 }

tests/TypeReconciliation/ConditionalTest.php

@@ -2865,7 +2865,25 @@ function skipTwo(IntLinkedList $l) : ?int {
                 [],
                 [],
                 '8.0'
-            ]
+            ],
+            'allowBasicOrAssignment' => [
+                '<?php
+                    function test(): int {
+                        if (rand(0, 1) || ($a = rand(0, 10)) === 0) {
+                            return 0;
+                        }
+
+                        return $a;
+                    }
+
+                    function test2(?string $comment): ?string {
+                        if ($comment === null || preg_match("/.*/", $comment, $match) === 0) {
+                            return null;
+                        }
+
+                        return $match[0];
+                    }'
+            ],
         ];
     }