Keep never in MutatingScope::intersectButNotNever() instead of widening it to null - #6507
Merged
Merged
Conversation
…dening it to `null`
* `intersectButNotNever()` restores the native type's nullability onto the
intersection result. It relied on the old `TypeCombinator::addNull()`
behaviour, where `addNull(never)` returned `never`. Since `addNull()` was
improved it returns `null` for `never`, so a nullable native type combined
with an inferred `never` produced `null`.
* Return the intersection unchanged as soon as it is `never`, before the
nullability is added back.
* Fixes closure and arrow function return types resolved in
`ClosureTypeResolver` (`fn (): null => neverReturning()` and
`function (): ?int { throw ...; }` are `Closure(): never` again), including
the `static` variants and closures declared `: ?Generator` whose body only
throws.
* Fixes the same helper's other call sites - closure and arrow function
parameter types in `MutatingScope::enterAnonymousFunctionWithoutReflection()`
and `enterArrowFunctionWithoutReflection()`. A `?int` parameter in a
`callable(never): void` context (e.g. `array_map()` over `list<never>`) is
`never` inside the body again, for both the fixed and the variadic parameter
branch.
* Probed the other `TypeCombinator::addNull()` call sites that can receive a
`never` by instrumenting `addNull()` and running the whole test suite. The
nullsafe short-circuit sites (`MethodCallHandler`, `StaticCallHandler`,
`PropertyFetchHandler`, `StaticPropertyFetchHandler`, `ArrayDimFetchHandler`,
`MutatingScope::resolveScopeStateType()`) and
`ArrayFindFunctionReturnTypeExtension` are correct as they are: there `null`
really is the result of the short circuit / of not finding anything.
`TypehintHelper::decideType()` cannot reach its `addNull()` with a `never`,
because an explicit `@return never` returns early and an implicit one is
never picked as the result type.
(cherry picked from commit 7c9cd26)
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.
Cherry-pick of #6504 (merged into 2.3.x as 7c9cd26) onto 2.2.x.
The regression it fixes is on 2.2.x too: #6493 was merged into 2.2.x, so
fn (): null => $neverReturning()andfn (): ?int => throw ...are inferred asClosure(): nullinstead ofClosure(): neverthere as well.Verified on 2.2.x:
nsrt/bug-15285.phpandCallCallablesRuleTest::testBug15285fail without theMutatingScopechange (Closure(): null/static-Closure(): nullwhereneveris expected, plus theargument.typeerror) and pass with it.Parameter #2 of closure expects (Closure(): never)|null, Closure(): null given.errors going away.Refs phpstan/phpstan#15285
🤖 Generated with Claude Code
https://claude.ai/code/session_01Dhb4ssXuKVWcVBcNFpsdn7