From 8cf5d64f9a1f091ff9669cfffaed75015f1a3b43 Mon Sep 17 00:00:00 2001 From: VincentLanglet <9052536+VincentLanglet@users.noreply.github.com> Date: Mon, 6 Apr 2026 17:40:06 +0000 Subject: [PATCH 1/2] Fix phpstan/phpstan#13092: random_int false positive with correlated args - Skip reporting "maybe" errors in RandomIntParametersRule when both arguments share a variable reference - When $min and $max are derived from the same variable (e.g. random_int($x, $x * 3)), the ranges are correlated and max can never be less than min - Added sharesVariable/extractVariableNames helper methods to detect shared variables in argument expressions - New regression test in tests/PHPStan/Rules/Functions/data/bug-13092.php --- .../Functions/RandomIntParametersRule.php | 45 ++++++++++++++++++- .../Functions/RandomIntParametersRuleTest.php | 5 +++ .../Rules/Functions/data/bug-13092.php | 12 +++++ 3 files changed, 61 insertions(+), 1 deletion(-) create mode 100644 tests/PHPStan/Rules/Functions/data/bug-13092.php diff --git a/src/Rules/Functions/RandomIntParametersRule.php b/src/Rules/Functions/RandomIntParametersRule.php index 1205283002d..c9020d82d7e 100644 --- a/src/Rules/Functions/RandomIntParametersRule.php +++ b/src/Rules/Functions/RandomIntParametersRule.php @@ -15,6 +15,7 @@ use PHPStan\Type\IntegerRangeType; use PHPStan\Type\VerbosityLevel; use function count; +use function is_string; use function sprintf; /** @@ -65,7 +66,7 @@ public function processNode(Node $node, Scope $scope): array $isSmaller = $maxType->isSmallerThan($minType, $this->phpVersion); - if ($isSmaller->yes() || $isSmaller->maybe() && $this->reportMaybes) { + if ($isSmaller->yes() || $isSmaller->maybe() && $this->reportMaybes && !$this->sharesVariable($args[0]->value, $args[1]->value)) { $message = 'Parameter #1 $min (%s) of function random_int expects lower number than parameter #2 $max (%s).'; return [ RuleErrorBuilder::message(sprintf( @@ -79,4 +80,46 @@ public function processNode(Node $node, Scope $scope): array return []; } + private function sharesVariable(Node\Expr $expr1, Node\Expr $expr2): bool + { + $vars1 = $this->extractVariableNames($expr1); + if ($vars1 === []) { + return false; + } + + $vars2 = $this->extractVariableNames($expr2); + + foreach ($vars1 as $var => $_) { + if (isset($vars2[$var])) { + return true; + } + } + + return false; + } + + /** + * @return array + */ + private function extractVariableNames(Node\Expr $expr): array + { + $vars = []; + if ($expr instanceof Node\Expr\Variable && is_string($expr->name)) { + $vars[$expr->name] = true; + } + + foreach ($expr->getSubNodeNames() as $name) { + $subNode = $expr->{$name}; + if (!($subNode instanceof Node\Expr)) { + continue; + } + + foreach ($this->extractVariableNames($subNode) as $var => $_) { + $vars[$var] = true; + } + } + + return $vars; + } + } diff --git a/tests/PHPStan/Rules/Functions/RandomIntParametersRuleTest.php b/tests/PHPStan/Rules/Functions/RandomIntParametersRuleTest.php index 39160f054fc..03727000b1f 100644 --- a/tests/PHPStan/Rules/Functions/RandomIntParametersRuleTest.php +++ b/tests/PHPStan/Rules/Functions/RandomIntParametersRuleTest.php @@ -74,4 +74,9 @@ public function testBug6361(): void $this->analyse([__DIR__ . '/data/bug-6361.php'], []); } + public function testBug13092(): void + { + $this->analyse([__DIR__ . '/data/bug-13092.php'], []); + } + } diff --git a/tests/PHPStan/Rules/Functions/data/bug-13092.php b/tests/PHPStan/Rules/Functions/data/bug-13092.php new file mode 100644 index 00000000000..89822f07a37 --- /dev/null +++ b/tests/PHPStan/Rules/Functions/data/bug-13092.php @@ -0,0 +1,12 @@ + Date: Mon, 6 Apr 2026 17:58:57 +0000 Subject: [PATCH 2/2] Evaluate shared variable expressions at boundary values instead of blanket suppression Instead of suppressing all "maybe" errors when arguments share a variable, evaluate both expressions at the boundary values of shared variables and only suppress when max >= min for all combinations. This ensures cases like random_int($x, $x - 1) are still reported as errors. Co-Authored-By: Claude Opus 4.6 --- .../Functions/RandomIntParametersRule.php | 109 +++++++++++++++++- .../Functions/RandomIntParametersRuleTest.php | 7 +- .../Rules/Functions/data/bug-13092.php | 6 + 3 files changed, 115 insertions(+), 7 deletions(-) diff --git a/src/Rules/Functions/RandomIntParametersRule.php b/src/Rules/Functions/RandomIntParametersRule.php index c9020d82d7e..819129943b9 100644 --- a/src/Rules/Functions/RandomIntParametersRule.php +++ b/src/Rules/Functions/RandomIntParametersRule.php @@ -14,6 +14,7 @@ use PHPStan\Type\Constant\ConstantIntegerType; use PHPStan\Type\IntegerRangeType; use PHPStan\Type\VerbosityLevel; +use function array_unique; use function count; use function is_string; use function sprintf; @@ -66,7 +67,7 @@ public function processNode(Node $node, Scope $scope): array $isSmaller = $maxType->isSmallerThan($minType, $this->phpVersion); - if ($isSmaller->yes() || $isSmaller->maybe() && $this->reportMaybes && !$this->sharesVariable($args[0]->value, $args[1]->value)) { + if ($isSmaller->yes() || $isSmaller->maybe() && $this->reportMaybes && !$this->isAlwaysValidDueToSharedVariables($args[0]->value, $args[1]->value, $scope)) { $message = 'Parameter #1 $min (%s) of function random_int expects lower number than parameter #2 $max (%s).'; return [ RuleErrorBuilder::message(sprintf( @@ -80,22 +81,118 @@ public function processNode(Node $node, Scope $scope): array return []; } - private function sharesVariable(Node\Expr $expr1, Node\Expr $expr2): bool + private function isAlwaysValidDueToSharedVariables(Node\Expr $minExpr, Node\Expr $maxExpr, Scope $scope): bool { - $vars1 = $this->extractVariableNames($expr1); + $vars1 = $this->extractVariableNames($minExpr); if ($vars1 === []) { return false; } - $vars2 = $this->extractVariableNames($expr2); + $vars2 = $this->extractVariableNames($maxExpr); + $hasShared = false; foreach ($vars1 as $var => $_) { if (isset($vars2[$var])) { - return true; + $hasShared = true; + break; } } - return false; + if (!$hasShared) { + return false; + } + + // Get all variables from both expressions + $allVars = $vars1 + $vars2; + + // Get boundary values for each variable + $varBoundaries = []; + foreach ($allVars as $var => $_) { + $varType = $scope->getType(new Node\Expr\Variable($var))->toInteger(); + if ($varType instanceof ConstantIntegerType) { + $varBoundaries[$var] = [$varType->getValue()]; + } elseif ($varType instanceof IntegerRangeType) { + $min = $varType->getMin(); + $max = $varType->getMax(); + if ($min === null || $max === null) { + return false; + } + $varBoundaries[$var] = array_unique([$min, $max]); + } else { + return false; + } + } + + // Generate all combinations of boundary values + /** @var array> $combinations */ + $combinations = [[]]; + foreach ($varBoundaries as $var => $values) { + $newCombinations = []; + foreach ($combinations as $combo) { + foreach ($values as $value) { + $newCombo = $combo; + $newCombo[$var] = $value; + $newCombinations[] = $newCombo; + } + } + $combinations = $newCombinations; + } + + // For each combination, evaluate both expressions and check max >= min + foreach ($combinations as $combo) { + $minValue = $this->evaluateExpr($minExpr, $combo); + $maxValue = $this->evaluateExpr($maxExpr, $combo); + + if ($minValue === null || $maxValue === null) { + return false; + } + + if ($maxValue < $minValue) { + return false; + } + } + + return true; + } + + /** + * @param array $varValues + */ + private function evaluateExpr(Node\Expr $expr, array $varValues): ?int + { + if ($expr instanceof Node\Expr\Variable && is_string($expr->name)) { + return $varValues[$expr->name] ?? null; + } + + if ($expr instanceof Node\Scalar\Int_) { + return $expr->value; + } + + if ($expr instanceof Node\Expr\BinaryOp) { + $left = $this->evaluateExpr($expr->left, $varValues); + $right = $this->evaluateExpr($expr->right, $varValues); + if ($left === null || $right === null) { + return null; + } + + return match (true) { + $expr instanceof Node\Expr\BinaryOp\Plus => $left + $right, + $expr instanceof Node\Expr\BinaryOp\Minus => $left - $right, + $expr instanceof Node\Expr\BinaryOp\Mul => $left * $right, + default => null, + }; + } + + if ($expr instanceof Node\Expr\UnaryMinus) { + $val = $this->evaluateExpr($expr->expr, $varValues); + return $val !== null ? -$val : null; + } + + if ($expr instanceof Node\Expr\UnaryPlus) { + return $this->evaluateExpr($expr->expr, $varValues); + } + + return null; } /** diff --git a/tests/PHPStan/Rules/Functions/RandomIntParametersRuleTest.php b/tests/PHPStan/Rules/Functions/RandomIntParametersRuleTest.php index 03727000b1f..cc2ea31da6f 100644 --- a/tests/PHPStan/Rules/Functions/RandomIntParametersRuleTest.php +++ b/tests/PHPStan/Rules/Functions/RandomIntParametersRuleTest.php @@ -76,7 +76,12 @@ public function testBug6361(): void public function testBug13092(): void { - $this->analyse([__DIR__ . '/data/bug-13092.php'], []); + $this->analyse([__DIR__ . '/data/bug-13092.php'], [ + [ + 'Parameter #1 $min (int<1000, 10000>) of function random_int expects lower number than parameter #2 $max (int<999, 9999>).', + 16, + ], + ]); } } diff --git a/tests/PHPStan/Rules/Functions/data/bug-13092.php b/tests/PHPStan/Rules/Functions/data/bug-13092.php index 89822f07a37..d1f42c688fa 100644 --- a/tests/PHPStan/Rules/Functions/data/bug-13092.php +++ b/tests/PHPStan/Rules/Functions/data/bug-13092.php @@ -9,4 +9,10 @@ public function sayHello(): void $shoppers = \random_int(1000, 10000); $transactions = \random_int($shoppers, $shoppers * 3); } + + public function shouldStillReport(): void + { + $shoppers = \random_int(1000, 10000); + $transactions = \random_int($shoppers, $shoppers - 1); + } }