Narrow division to int when modulo is known to be zero #5757
Conversation
VincentLanglet
left a comment
VincentLanglet
left a comment
There was a problem hiding this comment.
Wouldn't it be instead possible to say that
0 === ($offset % $limit) imply $offset / $limit is an int ?
|
Pushed. Here's a summary of what changed: Approach change: Instead of making all integer range divisions return benevolent unions (the previous commit's approach), the fix now detects when How it works: In Changes:
|
VincentLanglet
changed the title
BenevolentUnionType for IntegerRangeType division results in integerRangeMath
VincentLanglet
force-pushed
the
create-pull-request/patch-uzkbeoe
branch
from
f5391b1 to
0a655aa
Compare
|
|
||
| if ($leftType->isInteger()->yes() && $rightType->isInteger()->yes()) { | ||
| $modType = $getTypeCallback(new BinaryOp\Mod($left, $right)); | ||
| if ($modType instanceof ConstantIntegerType && $modType->getValue() === 0) { |
There was a problem hiding this comment.
| if ($modType instanceof ConstantIntegerType && $modType->getValue() === 0) { | |
| if ($modType->isSuperTypeOf(new ConstantIntegerType(0))->yes()) { |
There was a problem hiding this comment.
All tests pass and static analysis is clean. The remote commit already addressed staabm's review:
- Replaced
$modType instanceof ConstantIntegerType && $modType->getValue() === 0with$modType->isInteger()->yes() && (new ConstantIntegerType(0))->isSuperTypeOf($modType)->yes() - Note: the
isSuperTypeOfdirection is reversed from the reviewer's suggestion (ConstantIntegerType(0)->isSuperTypeOf($modType)not$modType->isSuperTypeOf(ConstantIntegerType(0))) because we need "is modType a subtype of 0" (i.e., exactly 0), not "is modType a supertype of 0" (which would matchint,mixed, etc.) - The
$modType->isInteger()->yes()guard prevents false positives fromNeverType(the bottom type satisfies anyisSuperTypeOfcheck, and the mod range computation can produceNeverTypefor certain negative divisors)
No additional changes needed — the branch is already up to date.
…s in `integerRangeMath` - Return benevolent union from `integerRangeMath` when division of two integer types produces `int<range>|float`, matching the existing behavior for plain `int / int` division - Propagate benevolence through union-combining code in both `integerRangeMath` (operand union handling) and `resolveCommonMath` (left number type union handling) by tracking whether any part returned a `BenevolentUnionType` - Simplify the cross-zero range-splitting blocks to use `TypeUtils::toBenevolentUnion()` instead of an equality check against `UnionType([int, float])`, which missed bounded integer range results - Update test expectations in integer-range-types.php, div-by-zero.php, math.php, and binary.php to reflect the corrected benevolent union types
Instead of making all integer range divisions return benevolent unions, detect when the scope knows that `$a % $b === 0` and narrow `$a / $b` to just the integer type (removing float from the union). This is more precise: it only removes float when mathematically guaranteed, rather than broadly making all integer range divisions benevolent. The check works by constructing a synthetic modulo expression with the same operands and querying the scope for its type. When a condition like `0 === ($a % $b)` has narrowed the modulo to ConstantIntegerType(0), the division result is guaranteed to be int. Reverts the BenevolentUnionType propagation changes from the previous commit since the modulo narrowing is the correct fix. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
VincentLanglet
force-pushed
the
create-pull-request/patch-uzkbeoe
branch
from
4e2210f to
9024135
Compare
|
@staabm I feel like we still need getDivTypeFromTypes here because we want to remove Float either for constantScalar if or for the general condition (and there was multiple early return). Do you see a better way ? |
|
As long as it is private, I am fine |
| private function expectInt(int $page): void | ||
| { | ||
| } | ||
|
|
||
| public function originalIssue(?int $limit, int $offset = 0): void | ||
| { | ||
| if ($limit && $offset && (0 === ($offset % $limit))) { | ||
| $this->expectInt(($offset / $limit) + 1); | ||
| } | ||
| } |
There was a problem hiding this comment.
this code needs to be analyzed by a RuleTest to proof it fixes the error reported in phpstan/phpstan#9724
There was a problem hiding this comment.
Done. Added a CallMethodsRule test (testBug9724) with a test data file that reproduces the exact scenario from issue #9724 — calling $this->expectInt(($offset / $limit) + 1) inside a modulo-checked condition. The test expects zero errors, proving the fix resolves the originally reported type mismatch. All 12156 tests pass and make phpstan reports no errors.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
3 participants
Fixes phpstan/phpstan#9724