Fix RiskyTruthyFalsyComparison reporting irrelevant errors when there…

… is no explicit truthy/falsy type
vimeo · Feb 20, 2024 · c6ada53 · c6ada53
1 parent 503ccd8
commit c6ada53
Show file tree

Hide file tree

Showing 5 changed files with 30 additions and 5 deletions.
diff --git a/docs/running_psalm/issues/RiskyTruthyFalsyComparison.md b/docs/running_psalm/issues/RiskyTruthyFalsyComparison.md
@@ -1,6 +1,6 @@
 # RiskyTruthyFalsyComparison
 
-Emitted when comparing a value with multiple types that can both contain truthy and falsy values.
+Emitted when comparing a value with multiple types, where at least one type can be only truthy or falsy and other types can contain both truthy and falsy values.
 
 ```php
 <?php
@@ -22,7 +22,7 @@ function foo($arg) {
 
 ## Why this is bad
 
-The truthy/falsy type of one variable is often forgotten and not handled explicitly causing hard to track down errors.
+The truthy/falsy type of a variable is often forgotten and not handled explicitly causing hard to track down errors.
 
 ## How to fix
 

diff --git a/src/Psalm/Internal/Analyzer/Statements/Block/IfConditionalAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Block/IfConditionalAnalyzer.php
@@ -372,16 +372,18 @@ public static function handleParadoxicalCondition(
                 && !($stmt instanceof PhpParser\Node\Expr\BinaryOp\Identical)
                 && !($stmt instanceof PhpParser\Node\Expr\BooleanNot)) {
                 if (count($type->getAtomicTypes()) > 1) {
+                    $has_truthy_or_falsy_exclusive_type = false;
                     $both_types = $type->getBuilder();
                     foreach ($both_types->getAtomicTypes() as $key => $atomic_type) {
                         if ($atomic_type->isTruthy()
                             || $atomic_type->isFalsy()
                             || $atomic_type instanceof TBool) {
                             $both_types->removeType($key);
+                            $has_truthy_or_falsy_exclusive_type = true;
                         }
                     }
 
-                    if (count($both_types->getAtomicTypes()) > 0) {
+                    if (count($both_types->getAtomicTypes()) > 0 && $has_truthy_or_falsy_exclusive_type) {
                         $both_types = $both_types->freeze();
                         IssueBuffer::maybeAdd(
                             new RiskyTruthyFalsyComparison(

diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/BooleanNotAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/BooleanNotAnalyzer.php
@@ -46,16 +46,18 @@ public static function analyze(
                 $stmt_type = new TTrue($expr_type->from_docblock);
             } else {
                 if (count($expr_type->getAtomicTypes()) > 1) {
+                    $has_truthy_or_falsy_exclusive_type = false;
                     $both_types = $expr_type->getBuilder();
                     foreach ($both_types->getAtomicTypes() as $key => $atomic_type) {
                         if ($atomic_type->isTruthy()
                             || $atomic_type->isFalsy()
                             || $atomic_type instanceof TBool) {
                             $both_types->removeType($key);
+                            $has_truthy_or_falsy_exclusive_type = true;
                         }
                     }
 
-                    if (count($both_types->getAtomicTypes()) > 0) {
+                    if (count($both_types->getAtomicTypes()) > 0 && $has_truthy_or_falsy_exclusive_type) {
                         $both_types = $both_types->freeze();
                         IssueBuffer::maybeAdd(
                             new RiskyTruthyFalsyComparison(

diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/EmptyAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/EmptyAnalyzer.php
@@ -65,16 +65,18 @@ public static function analyze(
                 $stmt_type = new TTrue($expr_type->from_docblock);
             } else {
                 if (count($expr_type->getAtomicTypes()) > 1) {
+                    $has_truthy_or_falsy_exclusive_type = false;
                     $both_types = $expr_type->getBuilder();
                     foreach ($both_types->getAtomicTypes() as $key => $atomic_type) {
                         if ($atomic_type->isTruthy()
                             || $atomic_type->isFalsy()
                             || $atomic_type instanceof TBool) {
                             $both_types->removeType($key);
+                            $has_truthy_or_falsy_exclusive_type = true;
                         }
                     }
 
-                    if (count($both_types->getAtomicTypes()) > 0) {
+                    if (count($both_types->getAtomicTypes()) > 0 && $has_truthy_or_falsy_exclusive_type) {
                         $both_types = $both_types->freeze();
                         IssueBuffer::maybeAdd(
                             new RiskyTruthyFalsyComparison(

diff --git a/tests/TypeReconciliation/ConditionalTest.php b/tests/TypeReconciliation/ConditionalTest.php
@@ -3153,6 +3153,25 @@ function foo( $a, $b ) {
                     '$existing' => 'null|stdClass',
                 ],
             ],
+            'nonStrictConditionWithoutExclusiveTruthyFalsyFuncCallNegated' => [
+                'code' => '<?php
+                    /**
+                     * @param array|null $arg
+                     * @return void
+                     */
+                    function foo($arg) {
+                        if (!bar($arg)) {
+                        }
+                    }
+
+                    /**
+                     * @param mixed $arg
+                     * @return float|int
+                     */
+                    function bar($arg) {}',
+            ],
+            'assertions' => [],
+            'ignored_issues' => ['InvalidReturnType'],
         ];
     }