Implement GMP operator type specifying extension#5223
Implement GMP operator type specifying extension#5223Firehed wants to merge 3 commits intophpstan:2.1.xfrom
Conversation
|
You've opened the pull request against the latest branch 2.2.x. PHPStan 2.2 is not going to be released for months. If your code is relevant on 2.1.x and you want it to be released sooner, please rebase your pull request and change its target to 2.1.x. |
a7b2630 to
c75bac6
Compare
| $specifiedTypes = $this->operatorTypeSpecifyingExtensionRegistryProvider->getRegistry() | ||
| ->callOperatorTypeSpecifyingExtensions($expr, $leftType, $rightType); | ||
| if ($specifiedTypes !== null) { | ||
| return $specifiedTypes; | ||
| } |
There was a problem hiding this comment.
This become a duplicate with the check later
$specifiedTypes = $this->operatorTypeSpecifyingExtensionRegistryProvider->getRegistry()
->callOperatorTypeSpecifyingExtensions($expr, $leftType, $rightType);
if ($specifiedTypes !== null) {
return $specifiedTypes;
}
I would either:
- remove this and rely on the later call
- or just move the existing call first with condition on object.
| $leftType = $getTypeCallback($left); | ||
| $rightType = $getTypeCallback($right); | ||
|
|
||
| if ($leftType->isObject()->yes() || $rightType->isObject()->yes()) { |
There was a problem hiding this comment.
This shouldn't check for Object and be done unconditionally.
| $leftType = $getTypeCallback($left); | ||
| $rightType = $getTypeCallback($right); | ||
|
|
||
| if ($leftType->isObject()->yes() || $rightType->isObject()->yes()) { |
There was a problem hiding this comment.
This shouldn't check for Object and be done unconditionally.
| $leftType = $getTypeCallback($left); | ||
| $rightType = $getTypeCallback($right); | ||
|
|
||
| if ($leftType->isObject()->yes() || $rightType->isObject()->yes()) { |
There was a problem hiding this comment.
This shouldn't check for Object and be done unconditionally.
| // GMP supports unary minus and returns GMP | ||
| if ($type->isObject()->yes() && (new ObjectType('GMP'))->isSuperTypeOf($type)->yes()) { | ||
| return new ObjectType('GMP'); | ||
| } |
There was a problem hiding this comment.
Rather than an hardcoded check for this, I feel like it we should introduce an UnaryOperatorTypeSpecifyingExtension as explained here: #4980 (comment)
This is maybe better in a dedicated PR ?
| $exprType = $getTypeCallback($expr); | ||
|
|
||
| // GMP supports bitwise not and returns GMP | ||
| if ($exprType->isObject()->yes() && (new ObjectType('GMP'))->isSuperTypeOf($exprType)->yes()) { |
There was a problem hiding this comment.
Same for UnaryOperatorTypeSpecifyingExtension
|
Updated to target 2.1 instead of 2.2 per the bot's suggestion. The two failing-as-of-writing tests appear to be failing on other PRs so I don't think they're specific to this change. I'll check out the other feedback shortly. Thank you for the very quick review! |
|
Please introduce the changes to InitializerExprTypeResolver in a separate PR - this will force you to create synthetic extensions just for test purposes, and review these changes separately, which I want you to do. Without this, if we once removed the GMP extensions then this feature would become untested. After that is merged, then you can take advantage of it with new GMP extensions in a new PR. |
|
Can do. Thanks for the feedback! |
|
Split per feedback: #5226 contains the |
ade4b49 to
4442956
Compare
VincentLanglet
left a comment
There was a problem hiding this comment.
Nice.
Now please again, split this PR in two:
- One with the GmpOperatorTypeSpecifyingExtension
- One with the unary minus, unary plus and bitwise not operator
Because for the second one, I think it will be a better API to introduce an UnaryOperatorTypeSpecifyingExtension
|
Gotcha, will do. Missed your message before continuing. |
|
@VincentLanglet #5284 sets up the UnaryOperatorTypeSpecifyingExtension; once that's looking good I'll update the GMP code to build on it instead. |
It's merged, you can now rebase/update this PR :) |
This adds comprehensive type inference tests for GMP operations: - Arithmetic operators (+, -, *, /, %, **) with GMP on left and right - Bitwise operators (&, |, ^, ~, <<, >>) with GMP on left and right - Comparison operators (<, <=, >, >=, ==, !=, <=>) with GMP on left and right - Assignment operators (+=, -=, *=) - Corresponding gmp_* functions (gmp_add, gmp_sub, gmp_mul, etc.) These tests currently fail because PHPStan lacks a GmpOperatorTypeSpecifyingExtension to specify that GMP operations return GMP rather than int|float. Related: phpstan/phpstan#12123 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add GmpOperatorTypeSpecifyingExtension to properly infer return types for GMP operator overloads. GMP supports arithmetic (+, -, *, /, %, **), bitwise (&, |, ^, ~, <<, >>), and comparison (<, <=, >, >=, ==, !=, <=>) operators. The extension only claims support when both operands are GMP-compatible (GMP, int, or numeric-string). Operations with incompatible types like stdClass are left to the default type inference. Also update InitializerExprTypeResolver to call operator extensions early for object types in resolveCommonMath and bitwise methods, and add explicit GMP handling for unary operators (-$a, ~$a). Fixes phpstan/phpstan#14288 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Implements UnaryOperatorTypeSpecifyingExtension for GMP to handle unary minus (-), unary plus (+), and bitwise NOT (~) operators. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
aafd495 to
eaebe39
Compare
|
Alright @VincentLanglet I've done the rebase again, and now this is purely the GMP stuff leveraging the type overloading stuff from the previous two split-off PRs. It's still pretty much entirely Claude going at it, but it's now feeling pretty tightly-scoped from my perspective. LMK if there's anything else here :) |
| if (!$leftIsGmp && !$rightIsGmp) { | ||
| return false; | ||
| } | ||
|
|
||
| // The other side must be GMP-compatible (GMP, int, or numeric-string) | ||
| // GMP operations with incompatible types (like stdClass) will error at runtime | ||
| return $this->isGmpCompatible($leftSide, $gmpType) && $this->isGmpCompatible($rightSide, $gmpType); |
There was a problem hiding this comment.
Rather than not supporting when it's not compatible you could return an ErrorType.
I feel like you could do like BcMathNumberOperatorTypeSpecifyingExtension and
- Not having a isGmpCompatible method
- adding some checks in specifyType and returning an ErrorType
Except this feedback, this PR seems ok to me :)
Summary
GmpOperatorTypeSpecifyingExtensionto properly infer return types for GMP operator overloadsInitializerExprTypeResolverto call operator extensions early for object typesFixes phpstan/phpstan#14288
Test plan
tests/PHPStan/Analyser/nsrt/gmp-operators.phpgmp_*functions🤖 Generated with Claude Code