Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix RiskyTruthyFalsyComparison irrelevant errors when there is no explicit truthy/falsy type #10733

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/running_psalm/issues/RiskyTruthyFalsyComparison.md
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
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
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
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
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
Loading