From 6be89a638f9bf0d0447048ef08d89eec45f4bb6b Mon Sep 17 00:00:00 2001 From: kkmuffme <11071985+kkmuffme@users.noreply.github.com> Date: Fri, 15 Mar 2024 21:01:41 +0100 Subject: [PATCH 1/9] Fix https://github.com/vimeo/psalm/issues/10833 --- .../Expression/BinaryOpAnalyzer.php | 71 ++++++++++++++++--- tests/BinaryOperationTest.php | 56 +++++++++++++++ 2 files changed, 117 insertions(+), 10 deletions(-) diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php index 2b7b8b607ae..2d9ff6d0fd5 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php @@ -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; @@ -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; + } + + // 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 diff --git a/tests/BinaryOperationTest.php b/tests/BinaryOperationTest.php index e2dafc18724..2a87b35b104 100644 --- a/tests/BinaryOperationTest.php +++ b/tests/BinaryOperationTest.php @@ -1218,6 +1218,62 @@ function foo(string $s1, string $s2): string { }', 'error_message' => 'LessSpecificReturnStatement', ], + 'greaterEqualRedundant' => [ + 'code' => ' PHP_INT_MAX) { + echo "yes"; + }', + 'error_message' => 'PossiblyInvalidOperand', + ], + 'lessEqualThanImpossible' => [ + 'code' => ' 'PossiblyInvalidOperand', + ], + 'greaterRisky' => [ + 'code' => ' 0 ? array() : rand(); + if ($a > 0) { + echo "yes"; + }', + 'error_message' => 'PossiblyInvalidOperand', + ], + 'greaterEqualRisky' => [ + 'code' => ' 0 ? false : rand(); + if ($a >= 0) { + echo "yes"; + }', + 'error_message' => 'PossiblyInvalidOperand', + ], + 'lessEqualRisky' => [ + 'code' => ' 0 ? false : rand(); + if ($a <= 0) { + echo "yes"; + }', + 'error_message' => 'PossiblyInvalidOperand', + ], + 'greaterObjectNotice' => [ + 'code' => ' 0) { + echo "yes"; + }', + 'error_message' => 'PossiblyInvalidOperand', + ], + 'greaterEqualObjectNotice' => [ + 'code' => ' 0 ? rand() : new stdClass(); + if ($a >= 0) { + echo "yes"; + }', + 'error_message' => 'PossiblyInvalidOperand', + ], ]; } } From a1c530a61b98e05ca37516aa33075259c39ccda5 Mon Sep 17 00:00:00 2001 From: kkmuffme <11071985+kkmuffme@users.noreply.github.com> Date: Sat, 16 Mar 2024 08:37:12 +0100 Subject: [PATCH 2/9] allow DateTimeInterface --- .../Statements/Expression/BinaryOpAnalyzer.php | 17 +++++++++++++++++ tests/BinaryOperationTest.php | 17 +++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php index 2d9ff6d0fd5..a9b358c5106 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php @@ -17,6 +17,7 @@ use Psalm\Internal\Codebase\VariableUseGraph; use Psalm\Internal\DataFlow\DataFlowNode; use Psalm\Internal\MethodIdentifier; +use Psalm\Internal\Type\Comparator\AtomicTypeComparator; use Psalm\Issue\DocblockTypeContradiction; use Psalm\Issue\ImpureMethodCall; use Psalm\Issue\InvalidOperand; @@ -267,6 +268,14 @@ public static function analyze( continue; } + if (AtomicTypeComparator::isContainedBy( + $statements_analyzer->getCodebase(), + $atomic_type, + new TNamedObject('DateTimeInterface'), + )) { + continue; + } + // 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 @@ -292,6 +301,14 @@ public static function analyze( continue; } + if (AtomicTypeComparator::isContainedBy( + $statements_analyzer->getCodebase(), + $atomic_type, + new TNamedObject('DateTimeInterface'), + )) { + continue; + } + IssueBuffer::maybeAdd( new PossiblyInvalidOperand( 'Greater/Less than comparisons with type' diff --git a/tests/BinaryOperationTest.php b/tests/BinaryOperationTest.php index 2a87b35b104..7d9b8fefb55 100644 --- a/tests/BinaryOperationTest.php +++ b/tests/BinaryOperationTest.php @@ -1083,6 +1083,14 @@ function toPositiveInt(int $i): int ], 'ignored_issues' => ['InvalidOperand'], ], + 'lessDateTimeInterface' => [ + 'code' => ' 0 ? "now" : "yesterday"); + if ($a < $b) { + echo "yes"; + }', + ], ]; } @@ -1274,6 +1282,15 @@ function foo(string $s1, string $s2): string { }', 'error_message' => 'PossiblyInvalidOperand', ], + 'lessDateTimeInterfaceInvalid' => [ + 'code' => ' 'PossiblyInvalidOperand', + ], ]; } } From 77f19496b962aceb0b91c40bb6e595949c69577a Mon Sep 17 00:00:00 2001 From: kkmuffme <11071985+kkmuffme@users.noreply.github.com> Date: Sat, 16 Mar 2024 08:42:38 +0100 Subject: [PATCH 3/9] allow array-key (int|string) --- .../Analyzer/Statements/Expression/BinaryOpAnalyzer.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php index a9b358c5106..0ee85a7a9c1 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php @@ -28,6 +28,7 @@ use Psalm\IssueBuffer; use Psalm\Plugin\EventHandler\Event\AddRemoveTaintsEvent; use Psalm\Type; +use Psalm\Type\Atomic\TArrayKey; use Psalm\Type\Atomic\TFloat; use Psalm\Type\Atomic\TInt; use Psalm\Type\Atomic\TLiteralInt; @@ -263,6 +264,7 @@ public static function analyze( foreach ($stmt_left_type->getAtomicTypes() as $atomic_type) { if ($atomic_type instanceof TString || $atomic_type instanceof TInt + || $atomic_type instanceof TArrayKey || $atomic_type instanceof TFloat || $atomic_type instanceof TResource) { continue; @@ -296,6 +298,7 @@ public static function analyze( foreach ($stmt_right_type->getAtomicTypes() as $atomic_type) { if ($atomic_type instanceof TString || $atomic_type instanceof TInt + || $atomic_type instanceof TArrayKey || $atomic_type instanceof TFloat || $atomic_type instanceof TResource) { continue; From 83b6c9870fbf2d08d22c56d4bc785cca08d88b4a Mon Sep 17 00:00:00 2001 From: kkmuffme <11071985+kkmuffme@users.noreply.github.com> Date: Sat, 16 Mar 2024 09:58:18 +0100 Subject: [PATCH 4/9] no error for int|false > 0 --- .../Expression/BinaryOpAnalyzer.php | 32 +++++++++++++++++ tests/BinaryOperationTest.php | 36 +++++++++++++++++++ 2 files changed, 68 insertions(+) diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php index 0ee85a7a9c1..a6cd67142cb 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php @@ -29,11 +29,13 @@ use Psalm\Plugin\EventHandler\Event\AddRemoveTaintsEvent; use Psalm\Type; use Psalm\Type\Atomic\TArrayKey; +use Psalm\Type\Atomic\TFalse; 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\TNull; use Psalm\Type\Atomic\TResource; use Psalm\Type\Atomic\TString; use Psalm\Type\Union; @@ -278,6 +280,21 @@ public static function analyze( continue; } + if (($atomic_type instanceof TNull || $atomic_type instanceof TFalse) + && !$stmt_left_type->isSingle() + && $stmt_right_type->isSingleIntLiteral() + && (($stmt instanceof PhpParser\Node\Expr\BinaryOp\Greater + && $stmt_right_type->getSingleIntLiteral()->value >= 0) + || ($stmt instanceof PhpParser\Node\Expr\BinaryOp\GreaterOrEqual + && $stmt_right_type->getSingleIntLiteral()->value > 0) + || ($stmt instanceof PhpParser\Node\Expr\BinaryOp\Smaller + && $stmt_right_type->getSingleIntLiteral()->value <= 0) + || ($stmt instanceof PhpParser\Node\Expr\BinaryOp\SmallerOrEqual + && $stmt_right_type->getSingleIntLiteral()->value < 0)) + ) { + continue; + } + // 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 @@ -312,6 +329,21 @@ public static function analyze( continue; } + if (($atomic_type instanceof TNull || $atomic_type instanceof TFalse) + && !$stmt_right_type->isSingle() + && $stmt_left_type->isSingleIntLiteral() + && (($stmt instanceof PhpParser\Node\Expr\BinaryOp\Greater + && $stmt_left_type->getSingleIntLiteral()->value >= 0) + || ($stmt instanceof PhpParser\Node\Expr\BinaryOp\GreaterOrEqual + && $stmt_left_type->getSingleIntLiteral()->value > 0) + || ($stmt instanceof PhpParser\Node\Expr\BinaryOp\Smaller + && $stmt_left_type->getSingleIntLiteral()->value <= 0) + || ($stmt instanceof PhpParser\Node\Expr\BinaryOp\SmallerOrEqual + && $stmt_left_type->getSingleIntLiteral()->value < 0)) + ) { + continue; + } + IssueBuffer::maybeAdd( new PossiblyInvalidOperand( 'Greater/Less than comparisons with type' diff --git a/tests/BinaryOperationTest.php b/tests/BinaryOperationTest.php index 7d9b8fefb55..04dab2945f8 100644 --- a/tests/BinaryOperationTest.php +++ b/tests/BinaryOperationTest.php @@ -1091,6 +1091,34 @@ function toPositiveInt(int $i): int echo "yes"; }', ], + 'greaterThanZeroFalse' => [ + 'code' => ' 0 ? rand() : false; + if ($a > 0) { + echo "yes"; + }', + ], + 'greaterThanOneFalse' => [ + 'code' => ' 0 ? rand() : false; + if ($a > 1) { + echo "yes"; + }', + ], + 'greaterEqualOneFalse' => [ + 'code' => ' 0 ? rand() : false; + if ($a >= 1) { + echo "yes"; + }', + ], + 'smallerThanZeroFalse' => [ + 'code' => ' 0 ? rand() : false; + if ($a < 0) { + echo "yes"; + }', + ], ]; } @@ -1291,6 +1319,14 @@ function foo(string $s1, string $s2): string { }', 'error_message' => 'PossiblyInvalidOperand', ], + 'greaterEqualMinusOneFalse' => [ + 'code' => ' 0 ? rand() : false; + if ($a > -1) { + echo "yes"; + }', + 'error_message' => 'PossiblyInvalidOperand', + ], ]; } } From 28d80dffe5df4dfa376868654cf0240b544c4ba3 Mon Sep 17 00:00:00 2001 From: kkmuffme <11071985+kkmuffme@users.noreply.github.com> Date: Sat, 16 Mar 2024 09:58:51 +0100 Subject: [PATCH 5/9] add tests for null error case --- tests/BinaryOperationTest.php | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/BinaryOperationTest.php b/tests/BinaryOperationTest.php index 04dab2945f8..f8b372c943d 100644 --- a/tests/BinaryOperationTest.php +++ b/tests/BinaryOperationTest.php @@ -1327,6 +1327,22 @@ function foo(string $s1, string $s2): string { }', 'error_message' => 'PossiblyInvalidOperand', ], + 'greaterEqualIntNull' => [ + 'code' => ' 0 ? rand(0, 1000) : null; + if ($a >= 0) { + echo "can be null"; + }', + 'error_message' => 'PossiblyInvalidOperand', + ], + 'smallerIntNull' => [ + 'code' => ' 0 ? rand(0, 1000) : null; + if ($a < rand(0, 1000)) { + echo "can be null"; + }', + 'error_message' => 'PossiblyInvalidOperand', + ], ]; } } From 04698c131566f0317a27c008ebbb057f98fa84a3 Mon Sep 17 00:00:00 2001 From: kkmuffme <11071985+kkmuffme@users.noreply.github.com> Date: Sat, 16 Mar 2024 11:47:48 +0100 Subject: [PATCH 6/9] support for int ranges and floats --- .../Expression/BinaryOpAnalyzer.php | 74 ++++++++++++++++--- tests/BinaryOperationTest.php | 17 +++++ 2 files changed, 81 insertions(+), 10 deletions(-) diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php index a6cd67142cb..cf5802eb6a0 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php @@ -42,6 +42,8 @@ use UnexpectedValueException; use function in_array; +use function max; +use function min; use function strlen; /** @@ -263,6 +265,9 @@ public static function analyze( $statements_analyzer->getSuppressedIssues(), ); } else { + $left_literal_int_floats = self::getMinMaxLiteralIntFloat($stmt_left_type); + $right_literal_int_floats = self::getMinMaxLiteralIntFloat($stmt_right_type); + foreach ($stmt_left_type->getAtomicTypes() as $atomic_type) { if ($atomic_type instanceof TString || $atomic_type instanceof TInt @@ -282,19 +287,20 @@ public static function analyze( if (($atomic_type instanceof TNull || $atomic_type instanceof TFalse) && !$stmt_left_type->isSingle() - && $stmt_right_type->isSingleIntLiteral() + && $right_literal_int_floats !== [] && (($stmt instanceof PhpParser\Node\Expr\BinaryOp\Greater - && $stmt_right_type->getSingleIntLiteral()->value >= 0) + && min($right_literal_int_floats) >= 0) || ($stmt instanceof PhpParser\Node\Expr\BinaryOp\GreaterOrEqual - && $stmt_right_type->getSingleIntLiteral()->value > 0) + && min($right_literal_int_floats) > 0) || ($stmt instanceof PhpParser\Node\Expr\BinaryOp\Smaller - && $stmt_right_type->getSingleIntLiteral()->value <= 0) + && max($right_literal_int_floats) <= 0) || ($stmt instanceof PhpParser\Node\Expr\BinaryOp\SmallerOrEqual - && $stmt_right_type->getSingleIntLiteral()->value < 0)) + && max($right_literal_int_floats) < 0)) ) { continue; } + // @todo for cases like int|null >= int report RiskyTruthyFalsyComparison instead? // 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 @@ -331,15 +337,15 @@ public static function analyze( if (($atomic_type instanceof TNull || $atomic_type instanceof TFalse) && !$stmt_right_type->isSingle() - && $stmt_left_type->isSingleIntLiteral() + && $left_literal_int_floats !== [] && (($stmt instanceof PhpParser\Node\Expr\BinaryOp\Greater - && $stmt_left_type->getSingleIntLiteral()->value >= 0) + && min($left_literal_int_floats) >= 0) || ($stmt instanceof PhpParser\Node\Expr\BinaryOp\GreaterOrEqual - && $stmt_left_type->getSingleIntLiteral()->value > 0) + && min($left_literal_int_floats) > 0) || ($stmt instanceof PhpParser\Node\Expr\BinaryOp\Smaller - && $stmt_left_type->getSingleIntLiteral()->value <= 0) + && max($left_literal_int_floats) <= 0) || ($stmt instanceof PhpParser\Node\Expr\BinaryOp\SmallerOrEqual - && $stmt_left_type->getSingleIntLiteral()->value < 0)) + && max($left_literal_int_floats) < 0)) ) { continue; } @@ -471,6 +477,54 @@ public static function analyze( return true; } + /** + * @return list + */ + private static function getMinMaxLiteralIntFloat(Union $union): array + { + if ($union->hasArrayKey() + || $union->hasScalar() + || $union->hasNumeric() + || (isset($union->getAtomicTypes()['int']) + && $union->getLiteralInts() === array()) + || (isset($union->getAtomicTypes()['float']) + && $union->getLiteralFloats() === array())) { + return []; + } + + $literal_int_float_values = []; + foreach ($union->getLiteralInts() as $literal_int) { + $literal_int_float_values[] = $literal_int->value; + } + + foreach ($union->getRangeInts() as $int_range) { + if ($int_range->isNegative()) { + $literal_int_float_values[] = -1; + continue; + } + + if ($int_range->isNegativeOrZero()) { + $literal_int_float_values[] = 0; + $literal_int_float_values[] = -1; + continue; + } + + if ($int_range->isPositive()) { + $literal_int_float_values[] = 1; + continue; + } + + $literal_int_float_values[] = 0; + $literal_int_float_values[] = 1; + } + + foreach ($union->getLiteralFloats() as $literal_float) { + $literal_int_float_values[] = $literal_float->value; + } + + return $literal_int_float_values; + } + public static function addDataFlow( StatementsAnalyzer $statements_analyzer, PhpParser\Node\Expr $stmt, diff --git a/tests/BinaryOperationTest.php b/tests/BinaryOperationTest.php index f8b372c943d..54931d748e7 100644 --- a/tests/BinaryOperationTest.php +++ b/tests/BinaryOperationTest.php @@ -1119,6 +1119,14 @@ function toPositiveInt(int $i): int echo "yes"; }', ], + 'positiveRangeIntNull' => [ + 'code' => ' 0 ? rand() : null; + $b = rand(1, PHP_INT_MAX); + if ($a >= $b) { + echo "yes"; + }', + ], ]; } @@ -1343,6 +1351,15 @@ function foo(string $s1, string $s2): string { }', 'error_message' => 'PossiblyInvalidOperand', ], + 'rangeIntNull' => [ + 'code' => ' 0 ? rand() : null; + $b = rand(0, PHP_INT_MAX); + if ($a >= $b) { + echo "yes"; + }', + 'error_message' => 'PossiblyInvalidOperand', + ], ]; } } From 8e8debccc990d8c84f1c9a009bb3f2e425c3f04c Mon Sep 17 00:00:00 2001 From: kkmuffme <11071985+kkmuffme@users.noreply.github.com> Date: Sat, 16 Mar 2024 10:24:22 +0100 Subject: [PATCH 7/9] fix tests and shepherd --- .../Internal/Analyzer/MethodComparator.php | 4 +++- .../BinaryOp/ArithmeticOpAnalyzer.php | 4 ++-- .../PhpVisitor/PartialParserVisitor.php | 3 ++- .../Reflector/ClassLikeNodeScanner.php | 5 +++++ .../Internal/Provider/ParserCacheProvider.php | 5 +++-- .../Comparator/IntegerRangeComparator.php | 21 +++++++++---------- .../Type/SimpleAssertionReconciler.php | 3 ++- src/Psalm/Storage/FunctionLikeStorage.php | 2 +- src/Psalm/Type/Atomic/TIntRange.php | 6 ++---- tests/SwitchTypeTest.php | 3 ++- tests/TypeReconciliation/ConditionalTest.php | 8 +++++++ 11 files changed, 40 insertions(+), 24 deletions(-) diff --git a/src/Psalm/Internal/Analyzer/MethodComparator.php b/src/Psalm/Internal/Analyzer/MethodComparator.php index 61726c5adbe..e4760b89830 100644 --- a/src/Psalm/Internal/Analyzer/MethodComparator.php +++ b/src/Psalm/Internal/Analyzer/MethodComparator.php @@ -164,7 +164,9 @@ public static function compare( foreach ($guide_method_storage->params as $i => $guide_param) { if (!isset($implementer_method_storage->params[$i])) { - if (!$prevent_abstract_override && $i >= $guide_method_storage->required_param_count) { + if (!$prevent_abstract_override + && ($guide_method_storage->required_param_count === null + || $i >= $guide_method_storage->required_param_count)) { continue; } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOp/ArithmeticOpAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOp/ArithmeticOpAnalyzer.php index f4ec365b722..a4efe9b19c4 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOp/ArithmeticOpAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOp/ArithmeticOpAnalyzer.php @@ -1188,7 +1188,7 @@ private static function analyzeMulBetweenIntRange( $min_value = $calculated_min_type !== null ? $calculated_min_type->getSingleIntLiteral()->value : null; $max_value = $calculated_max_type !== null ? $calculated_max_type->getSingleIntLiteral()->value : null; - if ($min_value > $max_value) { + if ($max_value === null || ($min_value !== null && $min_value > $max_value)) { [$min_value, $max_value] = [$max_value, $min_value]; } @@ -1226,7 +1226,7 @@ private static function analyzeMulBetweenIntRange( $min_value = $calculated_min_type !== null ? $calculated_min_type->getSingleIntLiteral()->value : null; $max_value = $calculated_max_type !== null ? $calculated_max_type->getSingleIntLiteral()->value : null; - if ($min_value > $max_value) { + if ($max_value === null || ($min_value !== null && $min_value > $max_value)) { [$min_value, $max_value] = [$max_value, $min_value]; } diff --git a/src/Psalm/Internal/PhpVisitor/PartialParserVisitor.php b/src/Psalm/Internal/PhpVisitor/PartialParserVisitor.php index 59343c14c1b..d89d27c9761 100644 --- a/src/Psalm/Internal/PhpVisitor/PartialParserVisitor.php +++ b/src/Psalm/Internal/PhpVisitor/PartialParserVisitor.php @@ -315,7 +315,8 @@ public function enterNode(PhpParser\Node $node, bool &$traverseChildren = true) ) + 1; if ($stmt_inner_end_pos < $this->a_file_contents_length) { - $stmt_inner_end_pos = strpos($this->a_file_contents, '}', $stmt_inner_end_pos + 1); + $new_pos = strpos($this->a_file_contents, '}', $stmt_inner_end_pos + 1); + $stmt_inner_end_pos = $new_pos !== false ? $new_pos : $stmt_inner_end_pos; } } diff --git a/src/Psalm/Internal/PhpVisitor/Reflector/ClassLikeNodeScanner.php b/src/Psalm/Internal/PhpVisitor/Reflector/ClassLikeNodeScanner.php index 325fd4e77a5..31dca817e47 100644 --- a/src/Psalm/Internal/PhpVisitor/Reflector/ClassLikeNodeScanner.php +++ b/src/Psalm/Internal/PhpVisitor/Reflector/ClassLikeNodeScanner.php @@ -420,6 +420,11 @@ public function start(PhpParser\Node\Stmt\ClassLike $node): ?bool if ($docblock_info->templates) { $storage->template_types = []; + /** + * not sure what exactly is checked here, suppress it instead of changing it + * + * @psalm-suppress PossiblyInvalidOperand + */ usort( $docblock_info->templates, static fn(array $l, array $r): int => $l[4] > $r[4] ? 1 : -1, diff --git a/src/Psalm/Internal/Provider/ParserCacheProvider.php b/src/Psalm/Internal/Provider/ParserCacheProvider.php index 55aa13e03f7..fd1f33cdc7c 100644 --- a/src/Psalm/Internal/Provider/ParserCacheProvider.php +++ b/src/Psalm/Internal/Provider/ParserCacheProvider.php @@ -86,7 +86,7 @@ public function loadStatementsFromCache( if (isset($file_content_hashes[$file_cache_key]) && $file_content_hash === $file_content_hashes[$file_cache_key] && is_readable($cache_location) - && filemtime($cache_location) > $file_modified_time + && (int) filemtime($cache_location) > $file_modified_time ) { $stmts = $this->cache->getItem($cache_location); @@ -317,7 +317,8 @@ public function deleteOldParserCaches(float $time_before): int continue; } - if (filemtime($full_path) < $time_before) { + $filemtime = filemtime($full_path); + if ($filemtime === false || $filemtime < $time_before) { $this->cache->deleteItem($full_path); ++$removed_count; } diff --git a/src/Psalm/Internal/Type/Comparator/IntegerRangeComparator.php b/src/Psalm/Internal/Type/Comparator/IntegerRangeComparator.php index 3263efdb78b..3a1e2e8303e 100644 --- a/src/Psalm/Internal/Type/Comparator/IntegerRangeComparator.php +++ b/src/Psalm/Internal/Type/Comparator/IntegerRangeComparator.php @@ -25,19 +25,18 @@ public static function isContainedBy( TIntRange $input_type_part, TIntRange $container_type_part ): bool { - $is_input_min = $input_type_part->min_bound === null; - $is_input_max = $input_type_part->max_bound === null; - $is_container_min = $container_type_part->min_bound === null; - $is_container_max = $container_type_part->max_bound === null; - $is_input_min_in_container = ( - $is_container_min || - (!$is_input_min && $container_type_part->min_bound <= $input_type_part->min_bound) - ); + $container_type_part->min_bound === null + || (!($input_type_part->min_bound === null) + && $container_type_part->min_bound <= $input_type_part->min_bound) + ); + $is_input_max_in_container = ( - $is_container_max || - (!$is_input_max && $container_type_part->max_bound >= $input_type_part->max_bound) - ); + $container_type_part->max_bound === null + || (!($input_type_part->max_bound === null) + && $container_type_part->max_bound >= $input_type_part->max_bound) + ); + return $is_input_min_in_container && $is_input_max_in_container; } diff --git a/src/Psalm/Internal/Type/SimpleAssertionReconciler.php b/src/Psalm/Internal/Type/SimpleAssertionReconciler.php index ed40b070143..f1260c7e149 100644 --- a/src/Psalm/Internal/Type/SimpleAssertionReconciler.php +++ b/src/Psalm/Internal/Type/SimpleAssertionReconciler.php @@ -636,7 +636,8 @@ private static function reconcileNonEmptyCountable( if ($array_atomic_type instanceof TArray) { if (!$array_atomic_type instanceof TNonEmptyArray || ($assertion instanceof HasAtLeastCount - && $array_atomic_type->min_count < $assertion->count) + && ($array_atomic_type->min_count === null + || $array_atomic_type->min_count < $assertion->count)) ) { if ($array_atomic_type->isEmptyArray()) { $existing_var_type->removeType('array'); diff --git a/src/Psalm/Storage/FunctionLikeStorage.php b/src/Psalm/Storage/FunctionLikeStorage.php index ee6128e787f..d42da88c96e 100644 --- a/src/Psalm/Storage/FunctionLikeStorage.php +++ b/src/Psalm/Storage/FunctionLikeStorage.php @@ -91,7 +91,7 @@ abstract class FunctionLikeStorage implements HasAttributesInterface public $returns_by_ref = false; /** - * @var ?int + * @var ?int<0, max> */ public $required_param_count; diff --git a/src/Psalm/Type/Atomic/TIntRange.php b/src/Psalm/Type/Atomic/TIntRange.php index dd0c9fa829c..2bc29d808ef 100644 --- a/src/Psalm/Type/Atomic/TIntRange.php +++ b/src/Psalm/Type/Atomic/TIntRange.php @@ -86,10 +86,8 @@ public function isNegativeOrZero(): bool public function contains(int $i): bool { return - ($this->min_bound === null && $this->max_bound === null) || - ($this->min_bound === null && $this->max_bound >= $i) || - ($this->max_bound === null && $this->min_bound <= $i) || - ($this->min_bound <= $i && $this->max_bound >= $i); + ($this->min_bound === null || $this->min_bound <= $i) + && ($this->max_bound === null || $this->max_bound >= $i); } /** diff --git a/tests/SwitchTypeTest.php b/tests/SwitchTypeTest.php index 1560504d266..736ad114eea 100644 --- a/tests/SwitchTypeTest.php +++ b/tests/SwitchTypeTest.php @@ -244,7 +244,8 @@ function foo() : void { case 1: $x = rand() % 4; case 2: - if (isset($x) && $x > 2) { + // @todo type should be correctly inferred as int instead of mixed + if (isset($x) && is_int($x) && $x > 2) { echo "$x is large"; } break; diff --git a/tests/TypeReconciliation/ConditionalTest.php b/tests/TypeReconciliation/ConditionalTest.php index 321832df7c5..23e978c2b50 100644 --- a/tests/TypeReconciliation/ConditionalTest.php +++ b/tests/TypeReconciliation/ConditionalTest.php @@ -2817,6 +2817,7 @@ function getIntOrNull(): ?int{return null;} echo $a + 3; } + /** @psalm-suppress PossiblyInvalidOperand */ if ($a <= 0) { /** @psalm-suppress PossiblyNullOperand */ echo $a + 3; @@ -2826,6 +2827,7 @@ function getIntOrNull(): ?int{return null;} echo $a + 3; } + /** @psalm-suppress PossiblyInvalidOperand */ if ($a >= 0) { /** @psalm-suppress PossiblyNullOperand */ echo $a + 3; @@ -2835,6 +2837,7 @@ function getIntOrNull(): ?int{return null;} echo $a + 3; } + /** @psalm-suppress PossiblyInvalidOperand */ if (0 <= $a) { /** @psalm-suppress PossiblyNullOperand */ echo $a + 3; @@ -2844,6 +2847,7 @@ function getIntOrNull(): ?int{return null;} echo $a + 3; } + /** @psalm-suppress PossiblyInvalidOperand */ if (0 >= $a) { /** @psalm-suppress PossiblyNullOperand */ echo $a + 3; @@ -2860,6 +2864,7 @@ function getIntOrFalse() {return false;} echo $a + 3; } + /** @psalm-suppress PossiblyInvalidOperand */ if ($a <= 0) { /** @psalm-suppress PossiblyFalseOperand */ echo $a + 3; @@ -2869,6 +2874,7 @@ function getIntOrFalse() {return false;} echo $a + 3; } + /** @psalm-suppress PossiblyInvalidOperand */ if ($a >= 0) { /** @psalm-suppress PossiblyFalseOperand */ echo $a + 3; @@ -2878,6 +2884,7 @@ function getIntOrFalse() {return false;} echo $a + 3; } + /** @psalm-suppress PossiblyInvalidOperand */ if (0 <= $a) { /** @psalm-suppress PossiblyFalseOperand */ echo $a + 3; @@ -2887,6 +2894,7 @@ function getIntOrFalse() {return false;} echo $a + 3; } + /** @psalm-suppress PossiblyInvalidOperand */ if (0 >= $a) { /** @psalm-suppress PossiblyFalseOperand */ echo $a + 3; From 55846730e2e0df64dea0c890d855f28f83553c40 Mon Sep 17 00:00:00 2001 From: kkmuffme <11071985+kkmuffme@users.noreply.github.com> Date: Mon, 18 Mar 2024 23:16:54 +0100 Subject: [PATCH 8/9] fix reversed inequality relation used same condition as normal inequality relation leading to missing errors --- .../Analyzer/Statements/Expression/BinaryOpAnalyzer.php | 8 ++++---- tests/BinaryOperationTest.php | 8 ++++++++ 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php index cf5802eb6a0..30810f3885c 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php @@ -339,13 +339,13 @@ public static function analyze( && !$stmt_right_type->isSingle() && $left_literal_int_floats !== [] && (($stmt instanceof PhpParser\Node\Expr\BinaryOp\Greater - && min($left_literal_int_floats) >= 0) + && min($left_literal_int_floats) <= 0) || ($stmt instanceof PhpParser\Node\Expr\BinaryOp\GreaterOrEqual - && min($left_literal_int_floats) > 0) + && min($left_literal_int_floats) < 0) || ($stmt instanceof PhpParser\Node\Expr\BinaryOp\Smaller - && max($left_literal_int_floats) <= 0) + && max($left_literal_int_floats) >= 0) || ($stmt instanceof PhpParser\Node\Expr\BinaryOp\SmallerOrEqual - && max($left_literal_int_floats) < 0)) + && max($left_literal_int_floats) > 0)) ) { continue; } diff --git a/tests/BinaryOperationTest.php b/tests/BinaryOperationTest.php index 54931d748e7..0401f265cb7 100644 --- a/tests/BinaryOperationTest.php +++ b/tests/BinaryOperationTest.php @@ -1335,6 +1335,14 @@ function foo(string $s1, string $s2): string { }', 'error_message' => 'PossiblyInvalidOperand', ], + 'reverseGreaterEqualMinusOneFalse' => [ + 'code' => ' 0 ? rand() : false; + if (-1 < $a) { + echo "yes"; + }', + 'error_message' => 'PossiblyInvalidOperand', + ], 'greaterEqualIntNull' => [ 'code' => ' 0 ? rand(0, 1000) : null; From 6a11be090320414ec8bc80dc32753aff8ee8288a Mon Sep 17 00:00:00 2001 From: kkmuffme <11071985+kkmuffme@users.noreply.github.com> Date: Sat, 23 Mar 2024 00:59:19 +0100 Subject: [PATCH 9/9] allow GMP and restrict DateTimeInterface and DateTimeZone to comparison with same object type --- .../Expression/BinaryOpAnalyzer.php | 199 ++++++++++-------- tests/BinaryOperationTest.php | 15 ++ 2 files changed, 129 insertions(+), 85 deletions(-) diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php index 30810f3885c..9ed6da26dd6 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php @@ -265,101 +265,130 @@ public static function analyze( $statements_analyzer->getSuppressedIssues(), ); } else { - $left_literal_int_floats = self::getMinMaxLiteralIntFloat($stmt_left_type); - $right_literal_int_floats = self::getMinMaxLiteralIntFloat($stmt_right_type); - - foreach ($stmt_left_type->getAtomicTypes() as $atomic_type) { - if ($atomic_type instanceof TString - || $atomic_type instanceof TInt - || $atomic_type instanceof TArrayKey - || $atomic_type instanceof TFloat - || $atomic_type instanceof TResource) { - continue; + $is_valid_object_comparison = false; + if ($stmt_left_type->isSingle() + && $stmt_right_type->isSingle() + && $stmt_left_type->isObjectType() + && $stmt_right_type->isObjectType()) { + $comparable_objects = [ + // DateTime/DateTimeImmutable + 'DateTimeInterface', + 'DateTimeZone', + ]; + + foreach ($comparable_objects as $name) { + if (AtomicTypeComparator::isContainedBy( + $statements_analyzer->getCodebase(), + $stmt_left_type->getSingleAtomic(), + new TNamedObject($name), + ) && AtomicTypeComparator::isContainedBy( + $statements_analyzer->getCodebase(), + $stmt_right_type->getSingleAtomic(), + new TNamedObject($name), + )) { + $is_valid_object_comparison = true; + break; + } } + } - if (AtomicTypeComparator::isContainedBy( - $statements_analyzer->getCodebase(), - $atomic_type, - new TNamedObject('DateTimeInterface'), - )) { - continue; - } + if (!$is_valid_object_comparison) { + $left_literal_int_floats = self::getMinMaxLiteralIntFloat($stmt_left_type); + $right_literal_int_floats = self::getMinMaxLiteralIntFloat($stmt_right_type); + foreach ($stmt_left_type->getAtomicTypes() as $atomic_type) { + if ($atomic_type instanceof TString + || $atomic_type instanceof TInt + || $atomic_type instanceof TArrayKey + || $atomic_type instanceof TFloat + || $atomic_type instanceof TResource) { + continue; + } - if (($atomic_type instanceof TNull || $atomic_type instanceof TFalse) - && !$stmt_left_type->isSingle() - && $right_literal_int_floats !== [] - && (($stmt instanceof PhpParser\Node\Expr\BinaryOp\Greater - && min($right_literal_int_floats) >= 0) - || ($stmt instanceof PhpParser\Node\Expr\BinaryOp\GreaterOrEqual - && min($right_literal_int_floats) > 0) - || ($stmt instanceof PhpParser\Node\Expr\BinaryOp\Smaller - && max($right_literal_int_floats) <= 0) - || ($stmt instanceof PhpParser\Node\Expr\BinaryOp\SmallerOrEqual - && max($right_literal_int_floats) < 0)) - ) { - continue; - } + if (AtomicTypeComparator::isContainedBy( + $statements_analyzer->getCodebase(), + $atomic_type, + new TNamedObject('GMP'), + )) { + continue; + } - // @todo for cases like int|null >= int report RiskyTruthyFalsyComparison instead? - // 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(), - ); + if (($atomic_type instanceof TNull || $atomic_type instanceof TFalse) + && !$stmt_left_type->isSingle() + && $right_literal_int_floats !== [] + && (($stmt instanceof PhpParser\Node\Expr\BinaryOp\Greater + && min($right_literal_int_floats) >= 0) + || ($stmt instanceof PhpParser\Node\Expr\BinaryOp\GreaterOrEqual + && min($right_literal_int_floats) > 0) + || ($stmt instanceof PhpParser\Node\Expr\BinaryOp\Smaller + && max($right_literal_int_floats) <= 0) + || ($stmt instanceof PhpParser\Node\Expr\BinaryOp\SmallerOrEqual + && max($right_literal_int_floats) < 0)) + ) { + continue; + } - break; - } + // @todo for cases like int|null >= int report RiskyTruthyFalsyComparison instead? + // 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(), + ); - foreach ($stmt_right_type->getAtomicTypes() as $atomic_type) { - if ($atomic_type instanceof TString - || $atomic_type instanceof TInt - || $atomic_type instanceof TArrayKey - || $atomic_type instanceof TFloat - || $atomic_type instanceof TResource) { - continue; + break; } - if (AtomicTypeComparator::isContainedBy( - $statements_analyzer->getCodebase(), - $atomic_type, - new TNamedObject('DateTimeInterface'), - )) { - continue; - } + foreach ($stmt_right_type->getAtomicTypes() as $atomic_type) { + if ($atomic_type instanceof TString + || $atomic_type instanceof TInt + || $atomic_type instanceof TArrayKey + || $atomic_type instanceof TFloat + || $atomic_type instanceof TResource) { + continue; + } - if (($atomic_type instanceof TNull || $atomic_type instanceof TFalse) - && !$stmt_right_type->isSingle() - && $left_literal_int_floats !== [] - && (($stmt instanceof PhpParser\Node\Expr\BinaryOp\Greater - && min($left_literal_int_floats) <= 0) - || ($stmt instanceof PhpParser\Node\Expr\BinaryOp\GreaterOrEqual - && min($left_literal_int_floats) < 0) - || ($stmt instanceof PhpParser\Node\Expr\BinaryOp\Smaller - && max($left_literal_int_floats) >= 0) - || ($stmt instanceof PhpParser\Node\Expr\BinaryOp\SmallerOrEqual - && max($left_literal_int_floats) > 0)) - ) { - continue; - } + if (AtomicTypeComparator::isContainedBy( + $statements_analyzer->getCodebase(), + $atomic_type, + new TNamedObject('GMP'), + )) { + 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 (($atomic_type instanceof TNull || $atomic_type instanceof TFalse) + && !$stmt_right_type->isSingle() + && $left_literal_int_floats !== [] + && (($stmt instanceof PhpParser\Node\Expr\BinaryOp\Greater + && min($left_literal_int_floats) <= 0) + || ($stmt instanceof PhpParser\Node\Expr\BinaryOp\GreaterOrEqual + && min($left_literal_int_floats) < 0) + || ($stmt instanceof PhpParser\Node\Expr\BinaryOp\Smaller + && max($left_literal_int_floats) >= 0) + || ($stmt instanceof PhpParser\Node\Expr\BinaryOp\SmallerOrEqual + && max($left_literal_int_floats) > 0)) + ) { + 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; + } } } } diff --git a/tests/BinaryOperationTest.php b/tests/BinaryOperationTest.php index 0401f265cb7..a585664198b 100644 --- a/tests/BinaryOperationTest.php +++ b/tests/BinaryOperationTest.php @@ -1091,6 +1091,13 @@ function toPositiveInt(int $i): int echo "yes"; }', ], + 'greaterThanGmp' => [ + 'code' => ' 0) { + echo "yes"; + }', + ], 'greaterThanZeroFalse' => [ 'code' => ' 0 ? rand() : false; @@ -1327,6 +1334,14 @@ function foo(string $s1, string $s2): string { }', 'error_message' => 'PossiblyInvalidOperand', ], + 'greaterDateTimeInterfaceInvalid' => [ + 'code' => ' 0) { + echo "yes"; + }', + 'error_message' => 'PossiblyInvalidOperand', + ], 'greaterEqualMinusOneFalse' => [ 'code' => ' 0 ? rand() : false;