From 5c45221bdcad21020676de904b6865ee8d5a7c71 Mon Sep 17 00:00:00 2001
From: Brown <github@muglug.com>
Date: Mon, 6 Jan 2020 16:37:44 -0500
Subject: [PATCH] Improve reconciliation of ||

Ref #2426
---
 .../Expression/BinaryOpAnalyzer.php           | 81 ++++++++++++-------
 tests/TypeReconciliation/ConditionalTest.php  | 15 +---
 2 files changed, 57 insertions(+), 39 deletions(-)

diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php
index 28ec7f28859..f6d505e3ba6 100644
--- a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php
+++ b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php
@@ -295,44 +295,69 @@ function ($c) {
                 return null;
             }
 
-            $pre_referenced_var_ids = $context->referenced_var_ids;
-            $context->referenced_var_ids = [];
+            if (!$stmt->left instanceof PhpParser\Node\Expr\BinaryOp\BooleanOr
+                && !($stmt->left instanceof PhpParser\Node\Expr\BooleanNot
+                    && $stmt->left->expr instanceof PhpParser\Node\Expr\BinaryOp\BooleanAnd)
+            ) {
+                $if_scope = new \Psalm\Internal\Scope\IfScope();
 
-            $pre_assigned_var_ids = $context->assigned_var_ids;
+                try {
+                    $if_conditional_scope = IfAnalyzer::analyzeIfConditional(
+                        $statements_analyzer,
+                        $stmt->left,
+                        $context,
+                        $codebase,
+                        $if_scope,
+                        $context->branch_point ?: (int) $stmt->getAttribute('startFilePos')
+                    );
 
-            $left_context = clone $context;
-            $left_context->parent_context = $context;
-            $left_context->if_context = null;
-            $left_context->assigned_var_ids = [];
+                    $left_context = $if_conditional_scope->if_context;
 
-            if (ExpressionAnalyzer::analyze($statements_analyzer, $stmt->left, $left_context) === false) {
-                return false;
-            }
+                    $left_referenced_var_ids = $if_conditional_scope->cond_referenced_var_ids;
+                    $left_assigned_var_ids = $if_conditional_scope->cond_assigned_var_ids;
+                } catch (\Psalm\Exception\ScopeAnalysisException $e) {
+                    return false;
+                }
+            } else {
+                $pre_referenced_var_ids = $context->referenced_var_ids;
+                $context->referenced_var_ids = [];
 
-            foreach ($left_context->vars_in_scope as $var_id => $type) {
-                if (!isset($context->vars_in_scope[$var_id])) {
-                    if (isset($left_context->assigned_var_ids[$var_id])) {
-                        $context->vars_in_scope[$var_id] = clone $type;
+                $pre_assigned_var_ids = $context->assigned_var_ids;
+
+                $left_context = clone $context;
+                $left_context->parent_context = $context;
+                $left_context->if_context = null;
+                $left_context->assigned_var_ids = [];
+
+                if (ExpressionAnalyzer::analyze($statements_analyzer, $stmt->left, $left_context) === false) {
+                    return false;
+                }
+
+                foreach ($left_context->vars_in_scope as $var_id => $type) {
+                    if (!isset($context->vars_in_scope[$var_id])) {
+                        if (isset($left_context->assigned_var_ids[$var_id])) {
+                            $context->vars_in_scope[$var_id] = clone $type;
+                        }
+                    } else {
+                        $context->vars_in_scope[$var_id] = Type::combineUnionTypes(
+                            $context->vars_in_scope[$var_id],
+                            $type,
+                            $codebase
+                        );
                     }
-                } else {
-                    $context->vars_in_scope[$var_id] = Type::combineUnionTypes(
-                        $context->vars_in_scope[$var_id],
-                        $type,
-                        $codebase
-                    );
                 }
-            }
 
-            if ($context->collect_references) {
-                $context->unreferenced_vars = $left_context->unreferenced_vars;
-            }
+                if ($context->collect_references) {
+                    $context->unreferenced_vars = $left_context->unreferenced_vars;
+                }
 
-            $left_referenced_var_ids = $left_context->referenced_var_ids;
-            $left_context->referenced_var_ids = array_merge($pre_referenced_var_ids, $left_referenced_var_ids);
+                $left_referenced_var_ids = $left_context->referenced_var_ids;
+                $left_context->referenced_var_ids = array_merge($pre_referenced_var_ids, $left_referenced_var_ids);
 
-            $left_assigned_var_ids = array_diff_key($left_context->assigned_var_ids, $pre_assigned_var_ids);
+                $left_assigned_var_ids = array_diff_key($left_context->assigned_var_ids, $pre_assigned_var_ids);
 
-            $left_referenced_var_ids = array_diff_key($left_referenced_var_ids, $left_assigned_var_ids);
+                $left_referenced_var_ids = array_diff_key($left_referenced_var_ids, $left_assigned_var_ids);
+            }
 
             $left_clauses = Algebra::getFormula(
                 \spl_object_id($stmt->left),
diff --git a/tests/TypeReconciliation/ConditionalTest.php b/tests/TypeReconciliation/ConditionalTest.php
index 02e26b988e7..76ecefcded1 100644
--- a/tests/TypeReconciliation/ConditionalTest.php
+++ b/tests/TypeReconciliation/ConditionalTest.php
@@ -2498,7 +2498,7 @@ function foo(): int {
                         return 0;
                     }',
             ],
-            'SKIPPED-assertHardConditional' => [
+            'assertHardConditionalWithString' => [
                 '<?php
                     interface Convertor {
                         function maybeConvert(string $value): ?SomeObject;
@@ -2508,19 +2508,12 @@ interface SomeObject {
                         function isValid(): bool;
                     }
 
-                    /**
-                     * @param mixed $value
-                     */
-                    function exampleWithOr(Convertor $convertor, $value): SomeObject
-                    {
-                        if (!\is_string($value)
-                          || ($value = $convertor->maybeConvert($value)) === null
-                          || !$value->isValid()
-                        ) {
+                    function exampleWithOr(Convertor $convertor, string $value): SomeObject {
+                        if (($value = $convertor->maybeConvert($value)) === null || !$value->isValid()) {
                             throw new Exception();
                         }
 
-                        return $value;
+                        return $value; // $value is SomeObject here and cannot be a string
                     }'
             ],
         ];