Repository navigation
Require the union with the other branch's type to equal the consequent before rescuing a one-sided conditional holder - #6835
Open
phpstan-bot wants to merge 1 commit into
Conversation
…t before rescuing a one-sided conditional holder
- MutatingScope::preserveVacuousConditionalExpressions() kept a conditional
holder present in only one branch when the other branch's state was a
subtype of the holder's consequent. Without bleeding edge, array shapes are
unsealed in isSuperTypeOf(), so array{}|array{a?: T} counted as a supertype
of array{a?: T, b: T}. Applying that holder later dropped the 'b' key,
which caused false positives such as "Offset ... on left side of ?? does
not exist".
- Now also require TypeCombinator::union(consequent, other) to equal the
consequent, so the other branch's state adds nothing to it.
- Ported the same check to turbo-ext/src/MutatingScope.cpp.
- Probed the other isSuperTypeOf() uses in the conditional-holder machinery
(vacuous-guard check, ScopeOps guard matching, StatementsHandler loop
holders). They compare guards rather than check that a consequent still
holds, so they don't have this problem.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
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
Since 2.3.0, reading an array key that was assigned conditionally, after some unrelated
ifblocks, could be reported asOffset '...' on left side of ?? does not exist.The cause was a stale conditional expression holder for$postthat survived an if-merge it should not have survived. When the holder was applied later, it narrowed$postback to a shape without the newly assigned key.Changes
src/Analyser/MutatingScope.php(preserveVacuousConditionalExpressions()): before rescuing a one-sided holder because "the other branch's state already satisfies the consequent", also requireTypeCombinator::union($consequent, $otherState)->equals($consequent).turbo-ext/src/MutatingScope.cpp: the same check in the native mirror.isSuperTypeOf()uses in the conditional-holder machinery: the vacuous-guard check (->no()),ScopeOpsguard matching, and theStatementsHandlerloop holders. They compare guards rather than check that a consequent still holds, so none of them has this problem.Root cause
Without bleeding edge, array shapes are unsealed, so
array{}|array{field0?: T, field1?: T}counts as a supertype ofarray{field0?: T, field1?: T, other0?: T, company_address: T}. The subsumption rescue therefore keptif otherEnabled()=false then $post is array{}|array{field0?, field1?}across theisEnabled()block that assignedcompany_address. A laterif (isVisible() && !otherEnabled())applied that holder, and the merge recorded a wrongif isEnabled()=true then $post is …holder that droppedcompany_address. Checking that the union is unchanged expresses what the rescue actually needs: the merged state under the guard is still exactly the consequent. With bleeding edge, shapes are sealed andisSuperTypeOf()already answered correctly, which is why the normal test suite (always bleeding edge) did not catch this.Test
tests/PHPStan/Rules/Variables/NullCoalesceRuleNoBleedingEdgeTest.php+data/bug-15534.php: the issue's reproducer, run without bleeding edge, expects no errors.tests/PHPStan/Analyser/Bug15534Test.php+data/bug-15534.php: a type-inference test run without bleeding edge that asserts$postkeepscompany_address?after the later!otherEnabled()/isEnabled()checks.Both fail without the fix and pass with it.
Fixes phpstan/phpstan#15534
🤖 Generated with Claude Code