Do not use cached expression types for member access chains rooted in new/clone#5497
Closed
phpstan-bot wants to merge 1 commit intophpstan:2.1.xfrom
Closed
Do not use cached expression types for member access chains rooted in new/clone#5497phpstan-bot wants to merge 1 commit intophpstan:2.1.xfrom
new/clone#5497phpstan-bot wants to merge 1 commit intophpstan:2.1.xfrom
Conversation
… `new`/`clone` - Add `isExpressionBasedOnNew()` helper in `MutatingScope` that detects expressions whose root object is a `new`/`clone` (e.g. `(new Foo())->method()`) - Skip the `expressionTypes` cache in `resolveType()` for such expressions, because each `new`/`clone` creates a fresh object and type narrowing from a previous evaluation (e.g. via `assert()`) must not carry over - Only skip cache for chains on `new`/`clone` (method calls, property fetches, array dim fetches, etc.), not for bare `new Foo()` itself, so `@var` annotations on `new` expressions continue to work - Covers analogous cases: property access, nullsafe method calls, chained method calls, array dim fetch, and clone expressions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When
assert()(or any type-narrowing construct) was applied to an expression rooted innew— e.g.assert((new Repository())->getAll() === [])— the narrowed type was cached inexpressionTypesunder the expression's string key. A subsequent occurrence of the same expression(new Repository())->getAll()would look up the cached type and incorrectly return the narrowed type (array{}), even thoughnewcreates a completely different object each time.Changes
isExpressionBasedOnNew()private method tosrc/Analyser/MutatingScope.phpthat walks the receiver chain of an expression (MethodCall, PropertyFetch, NullsafeMethodCall, NullsafePropertyFetch, ArrayDimFetch, StaticCall, StaticPropertyFetch, ClassConstFetch) to detect whether the root is anExpr\New_orExpr\Clone_resolveType()inMutatingScopeto skip theexpressionTypescache lookup whenisExpressionBasedOnNew()returns truenew Foo()orclone $objat the top level — only chains built on top of them — so that@varannotations onnewexpressions (like/** @var Foo<string> */ return new Foo()) continue to work correctlyAnalogous cases probed and fixed
All of these were confirmed broken without the fix and passing with it:
new:(new Foo())->method()— the original bug reportnew:(new Foo())->propertynew:(new Foo())->method()?->other()new:(new Factory())->create()->build()new:(new Foo())->getItems()(via variable assignment + assert)(clone $obj)->method()Cases probed and found already correct
new Foo()with@varannotation — correctly uses cache (tested via existingtestBug7218)newas the class expressionRoot cause
MutatingScope::resolveType()unconditionally used theexpressionTypescache for non-Variable, non-Closure expressions. Sincenewcreates a fresh object on every evaluation, two textually identical expressions like(new Repository())->getAll()refer to method calls on different objects. But the cache keyed them identically, causing type narrowing from one evaluation to leak into the next.Test
tests/PHPStan/Analyser/nsrt/bug-8985.php— regression test with 7 test functions covering the original bug and all analogous cases listed aboveFixes phpstan/phpstan#8985