Do not skip constant-array guards when creating conditional expressions for subtype-absorbed targets#6012
Open
phpstan-bot wants to merge 1 commit into
Open
Conversation
…ns for subtype-absorbed targets - Revert the constant-array guard skip in MutatingScope::createConditionalExpressions() that dropped the conditional expression linking a subtype-absorbed target (e.g. a `?string` re-narrowed to a literal in a branch) to a constant-array guard. - The skip assumed such a constant-array guard "is not re-asserted as a condition later", but it can be: narrowing a value derived from the guard (e.g. `if ($class === null) return;` where `$class` holds the derived constant array) re-selects the branch and re-narrows the dependent variable. - Because the skip only ever fired on `isConstantArray()->yes()` guards, this restores dependent-type re-narrowing for the whole family; non-array derived values were never affected.
Contributor
|
@ondrejmirtes this PR reverts the perf optimization from #6002 to fix a bug |
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 a variable is derived from another variable across mutually exclusive branches (e.g. an
if/elseif/elsechain or amatch), narrowing the derived value later should re-narrow the original variable back to the branch conditions. This regressed for the case where the derived value is a constant array: PHPStan inferredstring|nullinstead of'aa'|'bb'|'cc'.Changes
src/Analyser/MutatingScope.php: removed the special-case skip increateConditionalExpressions()that dropped conditional expressions for subtype-absorbed targets whenever the guard's type is a constant array.tests/PHPStan/Analyser/nsrt/bug-14926.php: regression test covering both theif/elseif/elseform and thematchform from the issue.Root cause
Commit "Skip constant-array guards when creating conditional expressions for subtype-absorbed targets" added a performance optimization that skipped creating a conditional expression whenever a subtype-absorbed target was paired with a constant-array guard. Its premise was that a constant-array guard "represents a unique literal value that is not re-asserted as a condition later", so the conditional expression could never re-narrow anything.
That premise is wrong: the constant array is held by a derived variable (
$classin the reproducer), and asserting a condition about that derived variable —if ($class === null) return;— re-selects the branch and must re-narrow the dependent variable ($x). Dropping the conditional expression removed exactly the link needed for this re-narrowing, so$xstayed at its widenedstring|nulltype.The skip only ever fired when the guard's type was a constant array (
isConstantArray()->yes()), which is why the bug disappeared when the derived value was not an array — reverting the skip therefore fixes the entire family at once. The#5876behavior of keeping subtype-absorbed variables as conditional-expression targets is preserved; only the incorrect constant-array narrowing of that behavior is removed.Test
tests/PHPStan/Analyser/nsrt/bug-14926.phpasserts$xis'aa'|'bb'|'cc'after thenullcheck, in both theif/elseif/elseandmatchforms. Both fail before the change (inferredstring|null) and pass after. The full test suite and PHPStan self-analysis remain green.Fixes phpstan/phpstan#14926