Fix phpstan/phpstan#11565: False positive: Conditional return type takes the wrong branch

[phpstan-bot]

Summary

When a variable is reassigned from a function with a conditional return type (e.g., $items = iteratorToList($items)), PHPStan incorrectly resolved the conditional return type to never instead of list<T>. This happened because the TypeSpecifier used the post-assignment scope to evaluate the conditional, causing the variable's new type (the function's return type) to be used as the input, creating a circular evaluation.

Changes

• src/Analyser/TypeSpecifier.php: In the null-context handling of Assign expressions, remove any specifiedTypes that target the assigned variable. This prevents the post-assignment type from incorrectly influencing the conditional return type evaluation.
• src/Analyser/SpecifiedTypes.php: Added removeExpr(string $exprString) method to allow filtering out entries from both sureTypes and sureNotTypes by expression key.
• tests/PHPStan/Analyser/nsrt/bug-11565.php: Regression test covering both the buggy case (same variable) and the working case (different variable).

Root cause

After processing $items = iteratorToList($items), the scope has $items typed as list<string>. The statement processing in NodeScopeResolver then calls specifyTypesInCondition with this post-assignment scope to extract additional type narrowing. For an Assign in null context, the TypeSpecifier recurses into the RHS function call. ParametersAcceptorSelector::selectFromArgs evaluates the argument $items using the post-assignment scope, getting list<string> instead of the original iterable<string, string>. Since list<string> IS a list, the conditional ($iterable is list ? never : list<T>) resolves to never, and the TypeSpecifier creates a "not list" narrowing for $items. When applied to list<string>, this produces *NEVER*.

The fix removes specifiedTypes targeting the assigned variable in null context, since after reassignment the variable no longer represents the function's input.

Test

Added tests/PHPStan/Analyser/nsrt/bug-11565.php which tests that $items = iteratorToList($items) correctly resolves to list<string> when $items starts as iterable<string, string>. Also verifies the existing working case with different variables.

Fixes phpstan/phpstan#11565

[VincentLanglet]

Is it the only place the removeExpr is needed ?

• Only if context is null ?
• Only for Assign (And not other type of assign like ??=)

[phpstan-bot]

Pushed. Here's what was done to address the review:

Changes made:

1. src/Analyser/TypeSpecifier.php: Replaced the variable-only removeExpr guard (instanceof Expr\Variable && is_string(...)) with $this->exprPrinter->printExpr($expr->var). This handles any LHS expression type (variables, property fetches, array dim fetches, etc.) and removes the conditional — it always runs in null context.

2. tests/PHPStan/Analyser/nsrt/bug-11565.php: Added a test case for same-variable reassignment inside an if condition (truthy context) to verify it works correctly.

Answers to the reviewer's questions:

• Only null context? Yes — in non-null (truthy/falsy) context, the code recurses into $expr->var (the LHS variable), not $expr->expr (the RHS expression). The conditional return type is never re-evaluated, so the bug can't occur.

• Only for Assign, not ??=? Yes — AssignOp\Coalesce (??=) is not handled in specifyTypesInCondition at all. In null context it falls through to returning empty SpecifiedTypes, so there's no recursion into the RHS where the bug could occur.

[VincentLanglet]

@staabm I remembered you said something about $this->exprPrinter->printExpr($expr->var) (about how it was wrong to use it and how to fix it) ; do you remember something about it ?

[staabm]

I remembered you said something about $this->exprPrinter->printExpr($expr->var) (about how it was wrong to use it and how to fix it) ; do you remember something about it ?

I can remember that too much expression printing can be slow.

[staabm]

why do we need this fix only for $context->null()?
shouldn't a assignment always override pre-existing types?

[phpstan-bot]

Pushed. Here's the summary:

Changes made:

Added code comments in src/Analyser/TypeSpecifier.php explaining why removeExpr is only needed in the $context->null() branch, and cleaned up unnecessary comments in the test file.

Answer to the reviewer's question:

The removeExpr is only needed for $context->null() because the two branches recurse into different sub-expressions:

• Null context (statement like $items = iteratorToList($items);): recurses into $expr->expr (the RHS). The RHS function call's conditional return type may produce specifiedTypes targeting the LHS variable, causing circular evaluation. removeExpr prevents this.
• Non-null context (condition like if ($items = iteratorToList($items))): recurses into $expr->var (the LHS variable) to narrow based on truthiness. It does not recurse into the RHS, so conditional return types are never re-evaluated and the bug cannot occur. Applying removeExpr here would be wrong — it would remove the truthiness narrowing for the assigned variable.

All 11,708 tests pass and make phpstan reports no errors.

[staabm]

this fix also affects assignments to expressions different than Variable. add a test which is affected by this change

[phpstan-bot]

Pushed. Added two test cases to tests/PHPStan/Analyser/nsrt/bug-11565.php that exercise the generalized removeExpr for non-Variable LHS expressions:

1. Property fetch ($holder->items = iteratorToList($holder->items)) — verifies the conditional return type resolves correctly when the LHS is a property fetch
2. Array dim fetch ($data['items'] = iteratorToList($data['items'])) — verifies the same for array dimension access

Both make phpstan and make tests pass with all 11708 tests green.