Skip to content
Open
22 changes: 22 additions & 0 deletions src/Analyser/MutatingScope.php
Original file line number Diff line number Diff line change
Expand Up @@ -974,6 +974,7 @@ private function resolveType(string $exprString, Expr $node): Type
&& !$node instanceof Expr\Closure
&& !$node instanceof Expr\ArrowFunction
&& $this->hasExpressionType($node)->yes()
&& (!$this->expressionHasNewInChain($node) || $this->expressionTypes[$exprString]->getExpr() === $node)
Copy link
Copy Markdown
Contributor

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() === $node part is necessary to prevent regressions in https://phpstan.org/r/91d74faf-9234-404b-932f-cea39c881063

Copy link
Copy Markdown
Collaborator Author

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 phpstan reports no errors.

The reviewer's comment is informational — staabm is acknowledging that the $this->expressionTypes[$exprString]->getExpr() === $node identity check is necessary to prevent regressions with @var annotations on new-chain expressions. This check distinguishes between:

  • Same AST node (e.g., @var annotation set on this exact expression) — stored type is used
  • Different AST node with same string key (e.g., assert() narrowing from a different new instantiation) — stored type is skipped

The implementation is already correct and all tests are green. No code changes are needed for this review comment.

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Collaborator Author

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 phpstan reports no errors.

Regarding the reviewer's question — a purity-based approach is not feasible here for three reasons:

  1. Circular dependency: resolveType() would need to resolve the type of the receiver to look up method reflection and check isPure(), but we're in the middle of resolving the type. This creates a circular dependency.

  2. Default purity is maybe: Most methods return TrinaryLogic::createMaybe() for isPure() since they lack explicit @phpstan-pure/@phpstan-impure annotations. Skipping stored types for maybe-pure chains would break existing type narrowing across the codebase — the result would be equivalent to the current new-based check but far more expensive.

  3. 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 from assert(), because the assert would either always pass (making the narrowing correct but the assert redundant) or throw (making subsequent code unreachable). The syntactic check for new in 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.

) {
return $this->expressionTypes[$exprString]->getType();
}
Expand All @@ -990,6 +991,27 @@ private function resolveType(string $exprString, Expr $node): Type
return new MixedType();
}

private function expressionHasNewInChain(Expr $expr): bool
{
if (
$expr instanceof MethodCall ||
$expr instanceof Expr\NullsafeMethodCall ||
$expr instanceof Expr\ArrayDimFetch ||
$expr instanceof PropertyFetch ||
$expr instanceof Expr\NullsafePropertyFetch
) {
return $expr->var instanceof Expr\New_ || $this->expressionHasNewInChain($expr->var);
}
if (
$expr instanceof Expr\StaticCall
|| $expr instanceof Expr\StaticPropertyFetch
) {
return $expr->class instanceof Expr\New_ || ($expr->class instanceof Expr && $this->expressionHasNewInChain($expr->class));
}
Comment thread
staabm marked this conversation as resolved.

return false;
}

/**
* @param callable(Type): ?bool $typeCallback
*/
Expand Down
111 changes: 111 additions & 0 deletions tests/PHPStan/Analyser/nsrt/bug-8985.php
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);
}
Original file line number Diff line number Diff line change
Expand Up @@ -1270,6 +1270,13 @@ public function testBug13773(): void
]);
}

public function testBug8985(): void
{
$this->reportPossiblyNonexistentConstantArrayOffset = true;

$this->analyse([__DIR__ . '/data/bug-8985.php'], []);
}

public function testBug14308(): void
{
$this->reportPossiblyNonexistentConstantArrayOffset = true;
Expand Down
31 changes: 31 additions & 0 deletions tests/PHPStan/Rules/Arrays/data/bug-8985.php
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();
6 changes: 6 additions & 0 deletions tests/PHPStan/Rules/Methods/ReturnTypeRuleTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -1326,6 +1326,12 @@ public function testBug10924(): void
$this->analyse([__DIR__ . '/data/bug-10924.php'], []);
}

public function testBug8985(): void
{
$this->checkExplicitMixed = true;
$this->analyse([__DIR__ . '/data/bug-8985.php'], []);
}

public function testBug11430(): void
{
$this->analyse([__DIR__ . '/../../Analyser/nsrt/bug-11430.php'], []);
Expand Down
38 changes: 38 additions & 0 deletions tests/PHPStan/Rules/Methods/data/bug-8985.php
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 [];
}
}
Loading