Skip __toString purity check in ImplicitToStringCallHelper when type is not an object#5511
Skip __toString purity check in ImplicitToStringCallHelper when type is not an object#5511phpstan-bot wants to merge 5 commits intophpstan:2.1.xfrom
__toString purity check in ImplicitToStringCallHelper when type is not an object#5511Conversation
…ype is not an object - Add early return in `ImplicitToStringCallHelper::processImplicitToStringCall()` when `$exprType->isObject()->no()`, since string cast on non-object types (int, float, bool, string) is a built-in operation that does not call `__toString()` - For intersection types like `int&T` (where T is a template type), `isObject()` correctly returns No because int is not an object, preventing a false "possibly impure call to method stdClass::__toString()" error - This also prevents unnecessary throw points from being generated for `__toString` on non-object types - Verified that the general method call case on intersection types (where both types have the method) is already handled correctly by `IntersectionTypeMethodReflection::isPure()` using `TrinaryLogic::lazyMaxMin` - Added regression tests for both PureFunctionRule and PureMethodRule covering string cast, string concatenation, and multiple scalar types (int, float, bool, string) intersected with template types
VincentLanglet
left a comment
There was a problem hiding this comment.
Should the fix be on the IntersectionType instead ?
…od() Instead of guarding against non-object types in ImplicitToStringCallHelper, fix the root cause in IntersectionType::hasMethod(). When any type in the intersection is definitively not an object and definitively doesn't have the method, the intersection should return No. This prevents MixedType::hasMethod() (which returns Yes for all methods) from dominating via TrinaryLogic::lazyMaxMin's early-return-on-Yes behavior when intersected with concrete non-object types like int. The check is targeted: class-string types return Maybe (not No) from hasMethod() for methods that might exist on the referenced class, so the guard doesn't trigger for class-string intersections. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pushed. Here's a summary of the change: What changed: Moved the fix from The root cause: The fix: In Why this is targeted enough: |
VincentLanglet
left a comment
There was a problem hiding this comment.
@template T of int|Stringable already works fine and is_int check refine the type to
T of int (function test2(), argument)
So why for T of mixed, the type is not refined and stay int&T (function test2(), argument) ?
|
The branch is already up to date with no new changes needed. The reviewer's comment is a question about type narrowing, not a request for a code change. Here's my analysis for the PR response: Why the types differ: For For Why not fix it in TypeCombinator: Changing TypeCombinator to produce narrowed template types ( Why the |
VincentLanglet
left a comment
There was a problem hiding this comment.
Are they similar issue for other method of the IntersectionType ?
…taticProperty, hasConstant The same lazyMaxMin early-return issue from hasMethod() affects these methods: MixedType returns Yes for all of them, which dominates a non-object type's No via the early return in lazyMaxMin. This causes incorrect Yes results for intersection types like int&T where T is a template bounded by mixed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pushed. Here's the analysis: Yes, the same issue exists for other member-existence methods in What was fixed: Applied the same guard pattern from
Methods NOT changed (and why):
|
VincentLanglet
left a comment
There was a problem hiding this comment.
This duplicates call like hasProperty in both the foreach and the intersectResults.
Is there a smarter way to compute this ? Should we introduce a new private method ?
…calls The non-object guard and intersectResults were calling hasMethod/hasProperty/etc. twice per type. A single-pass private method now computes each result once, checks the guard, and feeds pre-computed results to TrinaryLogic::maxMin. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pushed. The refactoring extracts a new |
VincentLanglet
left a comment
There was a problem hiding this comment.
I feel like we're basically looking for a hacky fix because we're playing with kinda int&mixed type.
List other ideas
- Should MixedType be skipped in those method ?
- Should int&T be resolved differently ?
- Do you have suggestions ?
|
The token doesn't have permissions to comment on the PR directly. The instructions mention results will be posted in a separate step. Here's my analysis for the PR response: Analysis of reviewer's suggested alternativesI thoroughly investigated all three suggestions. No code changes are needed — the current 1. "Should MixedType be skipped in those methods?"Changing 2. "Should int&T be resolved differently?"For 3. Why not a simpler
|
VincentLanglet
left a comment
There was a problem hiding this comment.
Revert the changes inside IntersectionType and modify TypeCombinator::intersect to narrow the TemplateMixedType instead
int&T
will become T of int.
Update the needed test to show how big is the impact
Revert IntersectionType changes and instead narrow TemplateMixedType to a concrete template type (e.g. TemplateIntegerType) when intersected with a scalar type in TypeCombinator::intersect(). For `int & T` where T is TemplateMixedType, the result is now `T of int` (TemplateIntegerType) instead of an IntersectionType. Since TemplateIntegerType delegates hasMethod() to IntegerType (which returns No for __toString), the false positive purity error is avoided. The narrowing is restricted to scalar types only. Narrowing object or array types would cause information loss during template resolution (e.g. `T&MockObject` becoming just the resolved type without MockObject). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pushed. Here's a summary of the changes: What changed: Reverted all How it works: When Why scalar-only: I initially tried narrowing for all types but found it causes information loss during template resolution:
Scalar types don't have this problem because they have no methods/properties/constants that would be lost, and the original bug is specifically about scalar types ( Test impact (3 files):
|
VincentLanglet
left a comment
There was a problem hiding this comment.
Why ->isScalar()->yes() check is needed ?
|
All 11880 tests pass and static analysis reports no errors. The reviewer's comment is a question, not a code change request. No changes are needed. Here's the analysis for the PR response: Why Removing the scalar guard causes 10 test failures across two categories: 1. Object intersection identity loss (8 failures): 2. Unresolvable type detection failure (2 failures): For Why scalars are safe to narrow: Scalar types (int, string, float, bool) cannot form meaningful intersections — you can't call methods, access properties, or further refine them with |
Summary
When a value has an intersection type like
int&T(whereTis a template type bounded bymixed), casting it to string with(string)was incorrectly reported as "Possibly impure call to method stdClass::__toString()" inside@phpstan-purefunctions. The string cast on non-object types likeintis a built-in PHP operation that does not invoke__toString(), so no impure point should be generated.Changes
src/Analyser/ExprHandler/Helper/ImplicitToStringCallHelper.phpwhen$exprType->isObject()->no(). Since__toString()can only be called on objects, non-object types (scalars, null) use PHP's built-in string conversion which is always pure.int&T,isObject()correctly evaluates toNoviaTrinaryLogic::lazyMaxMin(int says No, template-mixed says Maybe → result is No), so the check is skipped.ImplicitToStringCallHelper: string casts ((string)), string concatenation (.),echo,print, interpolated strings ("$x"), and.=assignment.Analogous cases probed and found correct
PureInterface&ImpureInterface):IntersectionTypeMethodReflection::isPure()already usesTrinaryLogic::lazyMaxMinwhich returns Yes if any member says Yes. No fix needed.__clone,__invoke): Not handled viaImplicitToStringCallHelper, and their handlers don't have the same issue.Root cause
ImplicitToStringCallHelper::processImplicitToStringCall()unconditionally looked for a__toString()method on the expression type, even when the type was definitively not an object. Forint&T,IntegerTypedoesn't have__toString(usesNonObjectTypeTrait), butTemplateMixedTypeinheritsMixedType::hasMethod()which returns Yes for all methods. The intersection'shasMethod('__toString')returned Yes, so onlyT's (impure) method reflection was found, causing the false positive.Test
tests/PHPStan/Rules/Pure/data/bug-14511.php— regression test forPureFunctionRulecovering string cast onint&T, string concatenation onint&T, and string casts onfloat&T,bool&T,string&Ttests/PHPStan/Rules/Pure/data/bug-14511-method.php— regression test forPureMethodRulecovering the same patterns in a method contextFixes phpstan/phpstan#14511