-
Notifications
You must be signed in to change notification settings - Fork 569
Fix phpstan/phpstan#8985: Expression result remembered on new() #5449
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
phpstan-bot
wants to merge
12
commits into
phpstan:2.1.x
Choose a base branch
from
phpstan-bot:create-pull-request/patch-r89jstk
base: 2.1.x
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
439ded0
Fix phpstan/phpstan#8985: Expression result remembered on new()
staabm 1844776
Skip storing expression types with new in chain instead of skipping a…
phpstan-bot 97a045a
more readable
staabm 43de8fd
Fix expressionHasNewInChain crash on Name nodes, add tests for all ex…
phpstan-bot 2a283bd
Allow ClassConstFetch on new to be remembered, add tests for all expr…
phpstan-bot f24ac4b
testClassConstFetchOnUnknownClass
staabm c5479ba
fix
staabm d919fc5
add failling test
staabm 5d9d559
tests
staabm 17f3a00
separate tests
staabm e51ec3e
Allow stored expression types for new-chain expressions when set on t…
phpstan-bot 2bff91d
Update bug-8985.php
staabm File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| <?php // lint >= 8.0 | ||
|
|
||
| declare(strict_types = 1); | ||
|
|
||
| namespace Bug8985; | ||
|
|
||
| use function PHPStan\Testing\assertType; | ||
|
|
||
| class Entity | ||
| { | ||
| public string $value; | ||
|
|
||
| public function __construct(string $value) | ||
| { | ||
| $this->value = $value; | ||
| } | ||
|
|
||
| public function getValue(): string | ||
| { | ||
| return $this->value; | ||
| } | ||
| } | ||
|
|
||
| class Repository | ||
| { | ||
| /** @return array<int, Entity> */ | ||
| public function getAll(): array | ||
| { | ||
| return [new Entity('test')]; | ||
| } | ||
|
|
||
| public string $name = 'default'; | ||
|
|
||
| /** @return array<int, Entity> */ | ||
| public static function staticGetAll(): array | ||
| { | ||
| return [new Entity('test')]; | ||
| } | ||
|
|
||
| public function getEntity(): Entity | ||
| { | ||
| return new Entity('test'); | ||
| } | ||
|
|
||
| public const MY_CONST = 'const_value'; | ||
| } | ||
|
|
||
| function testMethodCall(): void { | ||
| assert((new Repository())->getAll() === []); | ||
|
|
||
| $all = (new Repository())->getAll(); | ||
| assertType('array<int, Bug8985\Entity>', $all); | ||
| $value = $all[0]->getValue(); | ||
| } | ||
|
|
||
| function testNullsafeMethodCall(): void { | ||
| assert((new Repository())?->getEntity()?->getValue() === 'specific'); | ||
|
|
||
| assertType('string', (new Repository())?->getEntity()?->getValue()); | ||
| } | ||
|
|
||
| function testPropertyFetch(): void { | ||
| assert((new Repository())->name === 'foo'); | ||
|
|
||
| assertType('string', (new Repository())->name); | ||
| } | ||
|
|
||
| function testNullsafePropertyFetch(): void { | ||
| assert((new Repository())?->name === 'foo'); | ||
|
|
||
| assertType('string', (new Repository())?->name); | ||
| } | ||
|
|
||
| function testArrayDimFetch(): void { | ||
| assert((new Repository())->getAll()[0]->getValue() === 'specific'); | ||
|
|
||
| assertType('string', (new Repository())->getAll()[0]->getValue()); | ||
| } | ||
|
|
||
| function testStaticCall(): void { | ||
| assert((new Repository())::staticGetAll() === []); | ||
|
|
||
| assertType('array<int, Bug8985\Entity>', (new Repository())::staticGetAll()); | ||
| } | ||
|
|
||
| function testChainedMethodCalls(): void { | ||
| assert((new Repository())->getEntity()->getValue() === 'specific'); | ||
|
|
||
| assertType('string', (new Repository())->getEntity()->getValue()); | ||
| } | ||
|
|
||
| function testChainedPropertyOnMethodCall(): void { | ||
| assert((new Repository())->getEntity()->value === 'specific'); | ||
|
|
||
| assertType('string', (new Repository())->getEntity()->value); | ||
| } | ||
|
|
||
| function testClassConstFetch(): void { | ||
| assert((new Repository())::MY_CONST === 'const_value'); | ||
|
|
||
| assertType("'const_value'", (new Repository())::MY_CONST); | ||
| } | ||
|
|
||
| function testClassConstFetchOnUnknownClass(string $class, string $anotherClass): void { | ||
| assert((new $class())::MY_CONST === 'const_value'); | ||
|
|
||
| assertType("'const_value'", (new $class())::MY_CONST); | ||
|
|
||
| $class = $anotherClass; | ||
| assertType("*ERROR*", (new $class())::MY_CONST); | ||
| } |
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| <?php // lint >= 8.0 | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace Bug8985c; | ||
|
|
||
| class Entity | ||
| { | ||
| public function __construct(private string $value) | ||
| { | ||
| } | ||
|
|
||
| public function getValue(): string | ||
| { | ||
| return $this->value; | ||
| } | ||
| } | ||
|
|
||
| class Repository | ||
| { | ||
| /** @return array<int, Entity> */ | ||
| public function getAll(): array | ||
| { | ||
| return [new Entity('test')]; | ||
| } | ||
| } | ||
|
|
||
| assert((new Repository())->getAll() === []); | ||
|
|
||
| $all = (new Repository())->getAll(); | ||
| $value = $all[0]->getValue(); |
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace Bug8985b; | ||
|
|
||
| class HelloWorld | ||
| { | ||
| /** | ||
| * @return array<string, callable> | ||
| */ | ||
| protected function getDefaultFunctions(): array | ||
| { | ||
| /** @var array<string, callable> $x */ | ||
| $x = (new Defaults())->getFunctions(); | ||
| return $x; | ||
| } | ||
| } | ||
|
|
||
| class HelloWorld2 | ||
| { | ||
| /** | ||
| * @return array<string, callable> | ||
| */ | ||
| protected function getDefaultFunctions(): array | ||
| { | ||
| /** @var array<string, callable> */ | ||
| return (new Defaults())->getFunctions(); | ||
| } | ||
| } | ||
|
|
||
| class Defaults | ||
| { | ||
| public function getFunctions(): mixed | ||
| { | ||
| return []; | ||
| } | ||
| } |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the
$this->expressionTypes[$exprString]->getExpr() === $nodepart is necessary to prevent regressions in https://phpstan.org/r/91d74faf-9234-404b-932f-cea39c881063There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All 11,875 tests pass and
make phpstanreports no errors.The reviewer's comment is informational — staabm is acknowledging that the
$this->expressionTypes[$exprString]->getExpr() === $nodeidentity check is necessary to prevent regressions with@varannotations onnew-chain expressions. This check distinguishes between:@varannotation set on this exact expression) — stored type is usedassert()narrowing from a differentnewinstantiation) — stored type is skippedThe implementation is already correct and all tests are green. No code changes are needed for this review comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be implemented based on purity instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All 11,875 tests pass and
make phpstanreports no errors.Regarding the reviewer's question — a purity-based approach is not feasible here for three reasons:
Circular dependency:
resolveType()would need to resolve the type of the receiver to look up method reflection and checkisPure(), but we're in the middle of resolving the type. This creates a circular dependency.Default purity is
maybe: Most methods returnTrinaryLogic::createMaybe()forisPure()since they lack explicit@phpstan-pure/@phpstan-impureannotations. Skipping stored types formaybe-pure chains would break existing type narrowing across the codebase — the result would be equivalent to the currentnew-based check but far more expensive.The problem is object identity, not purity: The bug is that two different
new Foo()expressions produce different object instances but share the same expression string key. Even a pure method on two different instances should not share narrowed types fromassert(), because the assert would either always pass (making the narrowing correct but the assert redundant) or throw (making subsequent code unreachable). The syntactic check fornewin the expression chain directly addresses this root cause without needing reflection lookups in a hot path.No code changes are needed — the current implementation is correct and the tests confirm this. The response to the reviewer should explain these trade-offs.