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.

Sign up for GitHub

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

Improve loose comparison on IntegerRange containing zero #3764

Merged
merged 1 commit into from
Jan 4, 2025
Merged

Improve loose comparison on IntegerRange containing zero #3764

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
55 changes: 54 additions & 1 deletion src/Type/IntegerRangeType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,16 @@ public function isSmallerThan(Type $otherType, PhpVersion $phpVersion): TrinaryL
$maxIsSmaller = (new ConstantIntegerType($this->max))->isSmallerThan($otherType, $phpVersion);
}

// 0 can have different results in contrast to the interval edges, see https://3v4l.org/iGoti
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is expected, at least for me, better to document the -1 > null case.

I belive -1 > null is somethings that can be even fixed in php-src, as I would expect null to be compared as 0.

Copy link
Contributor Author

@staabm staabm Jan 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feel free to open a php-src bug report. I guess they won't fix it because of BC concerns.

as it stands right now in PHPStan we should reflect php-src behaviour and don't invent our own rules for edge cases

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That might be a good reason to report >, >=, <, <= operator with nullable integer in phpstan-strict-rule no ?
https://phpstan.org/r/ed5e293c-721b-4b14-b9ea-aa42876f8a2c

$zeroInt = new ConstantIntegerType(0);
if (!$zeroInt->isSuperTypeOf($this)->no()) {
return TrinaryLogic::extremeIdentity(
$zeroInt->isSmallerThan($otherType, $phpVersion),
$minIsSmaller,
$maxIsSmaller,
);
}

return TrinaryLogic::extremeIdentity($minIsSmaller, $maxIsSmaller);
}

Expand All @@ -332,6 +342,16 @@ public function isSmallerThanOrEqual(Type $otherType, PhpVersion $phpVersion): T
$maxIsSmaller = (new ConstantIntegerType($this->max))->isSmallerThanOrEqual($otherType, $phpVersion);
}

// 0 can have different results in contrast to the interval edges, see https://3v4l.org/iGoti
$zeroInt = new ConstantIntegerType(0);
if (!$zeroInt->isSuperTypeOf($this)->no()) {
return TrinaryLogic::extremeIdentity(
$zeroInt->isSmallerThanOrEqual($otherType, $phpVersion),
$minIsSmaller,
$maxIsSmaller,
);
}

return TrinaryLogic::extremeIdentity($minIsSmaller, $maxIsSmaller);
}

Expand All @@ -349,6 +369,16 @@ public function isGreaterThan(Type $otherType, PhpVersion $phpVersion): TrinaryL
$maxIsSmaller = $otherType->isSmallerThan((new ConstantIntegerType($this->max)), $phpVersion);
}

// 0 can have different results in contrast to the interval edges, see https://3v4l.org/iGoti
$zeroInt = new ConstantIntegerType(0);
if (!$zeroInt->isSuperTypeOf($this)->no()) {
return TrinaryLogic::extremeIdentity(
$otherType->isSmallerThan($zeroInt, $phpVersion),
$minIsSmaller,
$maxIsSmaller,
);
}

return TrinaryLogic::extremeIdentity($minIsSmaller, $maxIsSmaller);
}

Expand All @@ -366,6 +396,16 @@ public function isGreaterThanOrEqual(Type $otherType, PhpVersion $phpVersion): T
$maxIsSmaller = $otherType->isSmallerThanOrEqual((new ConstantIntegerType($this->max)), $phpVersion);
}

// 0 can have different results in contrast to the interval edges, see https://3v4l.org/iGoti
$zeroInt = new ConstantIntegerType(0);
if (!$zeroInt->isSuperTypeOf($this)->no()) {
return TrinaryLogic::extremeIdentity(
$otherType->isSmallerThanOrEqual($zeroInt, $phpVersion),
$minIsSmaller,
$maxIsSmaller,
);
}

return TrinaryLogic::extremeIdentity($minIsSmaller, $maxIsSmaller);
}

Expand Down Expand Up @@ -694,7 +734,20 @@ public function toPhpDocNode(): TypeNode

public function looseCompare(Type $type, PhpVersion $phpVersion): BooleanType
{
if ($this->isSmallerThan($type, $phpVersion)->yes() || $this->isGreaterThan($type, $phpVersion)->yes()) {
$zeroInt = new ConstantIntegerType(0);
if ($zeroInt->isSuperTypeOf($this)->no()) {
if ($type->isTrue()->yes()) {
return new ConstantBooleanType(true);
}
if ($type->isFalse()->yes()) {
return new ConstantBooleanType(false);
}
}

if (
$this->isSmallerThan($type, $phpVersion)->yes()
|| $this->isGreaterThan($type, $phpVersion)->yes()
) {
return new ConstantBooleanType(false);
}

Expand Down
60 changes: 57 additions & 3 deletions tests/PHPStan/Analyser/nsrt/loose-comparisons.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -712,7 +712,9 @@ public function sayEmptyStr(
* @param array{} $emptyArr
* @param 'php' $phpStr
* @param '' $emptyStr
* @param int<10, 20> $intRange
* @param int<10, 20> $positiveIntRange
* @param int<-20, -10> $negativeIntRange
* @param int<-10, 10> $minusTenToTen
*/
public function sayInt(
$true,
Expand All @@ -731,6 +733,9 @@ public function sayInt(
int $intRange,
string $emptyStr,
string $phpStr,
int $positiveIntRange,
int $negativeIntRange,
int $minusTenToTen,
): void
{
assertType('bool', $int == $true);
Expand All @@ -746,8 +751,57 @@ public function sayInt(
assertType('false', $int == $emptyArr);
assertType('false', $int == $array);

assertType('false', $intRange == $emptyArr);
assertType('false', $intRange == $array);
assertType('true', $positiveIntRange == $true);
assertType('false', $positiveIntRange == $false);
assertType('false', $positiveIntRange == $one);
assertType('false', $positiveIntRange == $zero);
assertType('false', $positiveIntRange == $minusOne);
assertType('false', $positiveIntRange == $oneStr);
assertType('false', $positiveIntRange == $zeroStr);
assertType('false', $positiveIntRange == $minusOneStr);
assertType('false', $positiveIntRange == $plusOneStr);
assertType('false', $positiveIntRange == $null);
assertType('false', $positiveIntRange == $emptyArr);
assertType('false', $positiveIntRange == $array);

assertType('true', $negativeIntRange == $true);
assertType('false', $negativeIntRange == $false);
assertType('false', $negativeIntRange == $one);
assertType('false', $negativeIntRange == $zero);
assertType('false', $negativeIntRange == $minusOne);
assertType('false', $negativeIntRange == $oneStr);
assertType('false', $negativeIntRange == $zeroStr);
assertType('false', $negativeIntRange == $minusOneStr);
assertType('false', $negativeIntRange == $plusOneStr);
assertType('false', $negativeIntRange == $null);
assertType('false', $negativeIntRange == $emptyArr);
assertType('false', $negativeIntRange == $array);

// see https://3v4l.org/VudDK
assertType('bool', $minusTenToTen == $true);
assertType('bool', $minusTenToTen == $false);
assertType('bool', $minusTenToTen == $one);
assertType('bool', $minusTenToTen == $zero);
assertType('bool', $minusTenToTen == $minusOne);
assertType('bool', $minusTenToTen == $oneStr);
assertType('bool', $minusTenToTen == $zeroStr);
assertType('bool', $minusTenToTen == $minusOneStr);
assertType('bool', $minusTenToTen == $plusOneStr);
assertType('bool', $minusTenToTen == $null);
assertType('false', $minusTenToTen == $emptyArr);
assertType('false', $minusTenToTen == $array);

// see https://3v4l.org/oJl3K
assertType('false', $minusTenToTen < $null);
assertType('bool', $minusTenToTen > $null);
assertType('bool', $minusTenToTen <= $null);
assertType('true', $minusTenToTen >= $null);

// see https://3v4l.org/oRSgU
assertType('bool', $null < $minusTenToTen);
assertType('false', $null > $minusTenToTen);
assertType('true', $null <= $minusTenToTen);
assertType('bool', $null >= $minusTenToTen);

assertType('false', 5 == $emptyArr);
assertType('false', $emptyArr == 5);
Expand Down
10 changes: 8 additions & 2 deletions tests/PHPStan/Rules/Comparison/ConstantLooseComparisonRuleTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,15 +210,21 @@ public function testBug11694(): void
39,
'Because the type is coming from a PHPDoc, you can turn off this check by setting <fg=cyan>treatPhpDocTypesAsCertain: false</> in your <fg=cyan>%configurationFile%</>.',
],
[
'Loose comparison using == between true and int<10, 20> will always evaluate to true.',
41,
],
[
'Loose comparison using == between int<10, 20> and true will always evaluate to true.',
42,
],
[
'Loose comparison using == between false and int<10, 20> will always evaluate to false.',
44,
'Because the type is coming from a PHPDoc, you can turn off this check by setting <fg=cyan>treatPhpDocTypesAsCertain: false</> in your <fg=cyan>%configurationFile%</>.',
],
[
'Loose comparison using == between int<10, 20> and false will always evaluate to false.',
45,
'Because the type is coming from a PHPDoc, you can turn off this check by setting <fg=cyan>treatPhpDocTypesAsCertain: false</> in your <fg=cyan>%configurationFile%</>.',
],
]);

Expand Down
Loading