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

Jump to bottom

Greater > and Less Than < comparisons with non-numeric, non-string, non-resource types should report error #10834

Open
wants to merge 9 commits into
base: 5.x
Choose a base branch
from
Open

Greater > and Less Than < comparisons with non-numeric, non-string, non-resource types should report error #10834

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6be89a6
Fix https://github.com/vimeo/psalm/issues/10833
kkmuffme Mar 15, 2024
a1c530a
allow DateTimeInterface
kkmuffme Mar 16, 2024
77f1949
allow array-key (int|string)
kkmuffme Mar 16, 2024
83b6c98
no error for int|false > 0
kkmuffme Mar 16, 2024
28d80df
add tests for null error case
kkmuffme Mar 16, 2024
04698c1
support for int ranges and floats
kkmuffme Mar 16, 2024
8e8debc
fix tests and shepherd
kkmuffme Mar 16, 2024
5584673
fix reversed inequality relation used same condition as normal inequa…
kkmuffme Mar 18, 2024
6a11be0
allow GMP and restrict DateTimeInterface and DateTimeZone to comparis…
kkmuffme Mar 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
71 changes: 61 additions & 10 deletions src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,20 @@
use Psalm\Issue\DocblockTypeContradiction;
use Psalm\Issue\ImpureMethodCall;
use Psalm\Issue\InvalidOperand;
use Psalm\Issue\PossiblyInvalidOperand;
use Psalm\Issue\RedundantCondition;
use Psalm\Issue\RedundantConditionGivenDocblockType;
use Psalm\Issue\TypeDoesNotContainType;
use Psalm\IssueBuffer;
use Psalm\Plugin\EventHandler\Event\AddRemoveTaintsEvent;
use Psalm\Type;
use Psalm\Type\Atomic\TFloat;
use Psalm\Type\Atomic\TInt;
use Psalm\Type\Atomic\TLiteralInt;
use Psalm\Type\Atomic\TLiteralString;
use Psalm\Type\Atomic\TNamedObject;
use Psalm\Type\Atomic\TResource;
use Psalm\Type\Atomic\TString;
use Psalm\Type\Union;
use UnexpectedValueException;

Expand Down Expand Up @@ -240,19 +245,65 @@ public static function analyze(
|| $stmt instanceof PhpParser\Node\Expr\BinaryOp\GreaterOrEqual
|| $stmt instanceof PhpParser\Node\Expr\BinaryOp\Smaller
|| $stmt instanceof PhpParser\Node\Expr\BinaryOp\SmallerOrEqual)
&& $statements_analyzer->getCodebase()->config->strict_binary_operands
&& $stmt_left_type
&& $stmt_right_type
&& (($stmt_left_type->isSingle() && $stmt_left_type->hasBool())
|| ($stmt_right_type->isSingle() && $stmt_right_type->hasBool()))
) {
IssueBuffer::maybeAdd(
new InvalidOperand(
'Cannot compare ' . $stmt_left_type->getId() . ' to ' . $stmt_right_type->getId(),
new CodeLocation($statements_analyzer, $stmt),
),
$statements_analyzer->getSuppressedIssues(),
);
if ($statements_analyzer->getCodebase()->config->strict_binary_operands
&& (($stmt_left_type->isSingle() && $stmt_left_type->hasBool())
|| ($stmt_right_type->isSingle() && $stmt_right_type->hasBool()))) {
IssueBuffer::maybeAdd(
new InvalidOperand(
'Cannot compare ' . $stmt_left_type->getId() . ' to ' . $stmt_right_type->getId(),
new CodeLocation($statements_analyzer, $stmt),
),
$statements_analyzer->getSuppressedIssues(),
);
} else {
foreach ($stmt_left_type->getAtomicTypes() as $atomic_type) {
if ($atomic_type instanceof TString
|| $atomic_type instanceof TInt
|| $atomic_type instanceof TFloat
|| $atomic_type instanceof TResource) {
continue;
}
kkmuffme marked this conversation as resolved.
Show resolved Hide resolved

// array and object behave extremely unexpectedly and might accidentally end up in a comparison
// this can be further improved upon to reduce false positives, e.g. for keyed arrays
// however these will mostly be fringe cases
// https://www.php.net/manual/en/language.operators.comparison.php#language.operators.comparison.types
IssueBuffer::maybeAdd(
new PossiblyInvalidOperand(
'Greater/Less than comparisons with type'
. ' ' . $stmt_left_type->getId()
. ' can behave unexpectedly.',
new CodeLocation($statements_analyzer, $stmt),
),
$statements_analyzer->getSuppressedIssues(),
);

break;
}

foreach ($stmt_right_type->getAtomicTypes() as $atomic_type) {
if ($atomic_type instanceof TString
|| $atomic_type instanceof TInt
|| $atomic_type instanceof TFloat
|| $atomic_type instanceof TResource) {
continue;
}

IssueBuffer::maybeAdd(
new PossiblyInvalidOperand(
'Greater/Less than comparisons with type'
. ' ' . $stmt_right_type->getId()
. ' can behave unexpectedly.',
new CodeLocation($statements_analyzer, $stmt),
),
$statements_analyzer->getSuppressedIssues(),
);
break;
}
}
}

if (($stmt instanceof PhpParser\Node\Expr\BinaryOp\Equal
Expand Down
56 changes: 56 additions & 0 deletions tests/BinaryOperationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1218,6 +1218,62 @@ function foo(string $s1, string $s2): string {
}',
'error_message' => 'LessSpecificReturnStatement',
],
'greaterEqualRedundant' => [
'code' => '<?php
$a = array();
if ($a > PHP_INT_MAX) {
echo "yes";
}',
'error_message' => 'PossiblyInvalidOperand',
kkmuffme marked this conversation as resolved.
Show resolved Hide resolved
],
'lessEqualThanImpossible' => [
'code' => '<?php
$a = array();
if ($a < PHP_INT_MAX) {
echo "yes";
}',
'error_message' => 'PossiblyInvalidOperand',
],
'greaterRisky' => [
'code' => '<?php
$a = rand(0, 1) > 0 ? array() : rand();
if ($a > 0) {
echo "yes";
}',
'error_message' => 'PossiblyInvalidOperand',
],
'greaterEqualRisky' => [
'code' => '<?php
$a = rand(0, 1) > 0 ? false : rand();
if ($a >= 0) {
echo "yes";
}',
'error_message' => 'PossiblyInvalidOperand',
],
'lessEqualRisky' => [
'code' => '<?php
$a = rand(0, 1) > 0 ? false : rand();
if ($a <= 0) {
echo "yes";
}',
'error_message' => 'PossiblyInvalidOperand',
],
'greaterObjectNotice' => [
'code' => '<?php
$a = new stdClass();
if ($a > 0) {
echo "yes";
}',
'error_message' => 'PossiblyInvalidOperand',
],
'greaterEqualObjectNotice' => [
'code' => '<?php
$a = rand(0, 1) > 0 ? rand() : new stdClass();
if ($a >= 0) {
echo "yes";
}',
'error_message' => 'PossiblyInvalidOperand',
],
];
}
}