Skip to content

Commit

Permalink
Merge pull request #10733 from kkmuffme/fix-false-positive-riskytruth…
Browse files Browse the repository at this point in the history 
…yfalsycomparison
  • Loading branch information
@weirdan
weirdan authored Feb 21, 2024
2 parents 503ccd8 + e482c07 commit c401490
Show file tree
Hide file tree
Showing 5 changed files with 86 additions and 61 deletions.
4 changes: 2 additions & 2 deletions docs/running_psalm/issues/RiskyTruthyFalsyComparison.md
View file
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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

Expand Down
4 changes: 3 additions & 1 deletion src/Psalm/Internal/Analyzer/Statements/Block/IfConditionalAnalyzer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
4 changes: 3 additions & 1 deletion src/Psalm/Internal/Analyzer/Statements/Expression/BooleanNotAnalyzer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
4 changes: 3 additions & 1 deletion src/Psalm/Internal/Analyzer/Statements/Expression/EmptyAnalyzer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
131 changes: 75 additions & 56 deletions tests/TypeReconciliation/ConditionalTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,28 +41,28 @@ function foo($a): void {
'nonStrictConditionTruthyFalsyNoOverlap' => [
'code' => '<?php
/**
* @param non-empty-array|null $arg
* @return void
*/
function foo($arg) {
if ($arg) {
}
* @param non-empty-array|null $arg
* @return void
*/
function foo($arg) {
if ($arg) {
}
if (!$arg) {
}
if (!$arg) {
}
if (bar($arg)) {
}
if (bar($arg)) {
}
if (!bar($arg)) {
}
}
if (!bar($arg)) {
}
}
/**
* @param mixed $arg
* @return non-empty-array|null
*/
function bar($arg) {}',
/**
* @param mixed $arg
* @return non-empty-array|null
*/
function bar($arg) {}',
],
'typeResolutionFromDocblock' => [
'code' => '<?php
Expand Down Expand Up @@ -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'],
],
];
}

Expand Down Expand Up @@ -3539,61 +3558,61 @@ public function fluent(): self
'nonStrictConditionTruthyFalsy' => [
'code' => '<?php
/**
* @param array|null $arg
* @return void
*/
function foo($arg) {
if ($arg) {
}
}',
* @param array|null $arg
* @return void
*/
function foo($arg) {
if ($arg) {
}
}',
'error_message' => 'RiskyTruthyFalsyComparison',
],
'nonStrictConditionTruthyFalsyNegated' => [
'code' => '<?php
/**
* @param array|null $arg
* @return void
*/
function foo($arg) {
if (!$arg) {
}
}',
* @param array|null $arg
* @return void
*/
function foo($arg) {
if (!$arg) {
}
}',
'error_message' => 'RiskyTruthyFalsyComparison',
],
'nonStrictConditionTruthyFalsyFuncCall' => [
'code' => '<?php
/**
* @param array|null $arg
* @return void
*/
function foo($arg) {
if (bar($arg)) {
}
}
* @param array|null $arg
* @return void
*/
function foo($arg) {
if (bar($arg)) {
}
}
/**
* @param mixed $arg
* @return array|null
*/
function bar($arg) {}',
/**
* @param mixed $arg
* @return array|null
*/
function bar($arg) {}',
'error_message' => 'RiskyTruthyFalsyComparison',
],
'nonStrictConditionTruthyFalsyFuncCallNegated' => [
'code' => '<?php
/**
* @param array|null $arg
* @return void
*/
function foo($arg) {
if (!bar($arg)) {
}
}
* @param array|null $arg
* @return void
*/
function foo($arg) {
if (!bar($arg)) {
}
}
/**
* @param mixed $arg
* @return array|null
*/
function bar($arg) {}',
/**
* @param mixed $arg
* @return array|null
*/
function bar($arg) {}',
'error_message' => 'RiskyTruthyFalsyComparison',
],
'redundantConditionForNonEmptyString' => [
Expand Down

0 comments on commit c401490

Please sign in to comment.