Narrow the falsey branch of isset($a['x']['y']) only when the intermediate offset is known to exist - #6110
Open
phpstan-bot wants to merge 1 commit into
Open
Conversation
…mediate offset is known to exist
- `IssetHandler::specifyTypes()` no longer removes types from `$issetExpr->var`
in the falsey branch when that expression itself may be undefined. Removing a
type from `$a['x']` writes `hasOffsetValue('x', ...)` back into `$a` via
`MutatingScope::specifyExpressionType()`, which wrongly asserted that the
intermediate offset exists for the rest of the scope.
- Added `IssetHandler::isAlwaysSet()` which walks the array-dim-fetch chain and
reports whether every offset in it definitely exists. Undefined *variables*
keep the previous behaviour because their certainty is separately degraded to
"maybe" through `IssetExpr`.
- When every possible value of the intermediate expression has the checked
offset set, `isset()` can only be false because the intermediate offset itself
is missing, so the enclosing array is now specified with that offset unset
(`Type::unsetOffset()`). This turns `array{K?: array{Port: int}}` into
`array{}` and `array<int, array{Port: int}>` into
`array<int<min, -1>|int<1, max>, array{Port: int}>` in the falsey branch
instead of `*NEVER*`. The specification is skipped when it would collapse to
`never` (list types cannot express a missing offset 0).
- Same fix covers integer offsets, list offsets, property-fetch bases
(`isset($this->arr['K']['Port'])`), deeper chains (`$r['A']['B']['Port']`) and
multi-var `isset()` (rewritten to an and-chain), all of which leaked the same way.
- Probed and found already correct: `empty()` (delegates to `!isset() || !expr`),
`??`, `unset()`, non-constant offsets, `ArrayAccess` objects, and
`array_key_exists()` (which evaluates its array argument, so the intermediate
offset's existence really is implied there).
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
isset()on a nested array offset made PHPStan treat the intermediate offset as definitely existing for the rest of the enclosing scope.Because the falsey branch was narrowed to
never, merging it back with the truthy branch dropped the "offset may not exist" information, which producednullCoalesce.offsetandnullCoalesce.unnecessaryfalse positives on every$a['x']['y'] ?? nullsitting after such anisset().This PR makes the falsey-branch narrowing conditional on the intermediate offset being known to exist, and replaces the unsound narrowing with a precise "the intermediate offset is unset" specification where that is expressible.
Changes
src/Analyser/ExprHandler/IssetHandler.php:isAlwaysSet(), which walks an array-dim-fetch chain and answers whether every offset in it definitely exists (Type::hasOffsetValueType()->yes()at each level, plusScope::hasVariableType()->yes()at the root variable).$issetExpr->varisisAlwaysSet(), or when it is a plainVariable— variables keep the previous behaviour because their certainty is separately degraded to "maybe" viaIssetExpr.never, the only way forisset()to be false is the intermediate offset not existing. That fact is now specified on the enclosing array viaType::unsetOffset()instead of being (wrongly) written onto the intermediate expression.never, which happens forlist<T>—array<int<1, max>, T> & list<T>isneverin PHPStan even though the empty list satisfies both.Tests:
tests/PHPStan/Analyser/nsrt/bug-15005.php(new)tests/PHPStan/Rules/Variables/data/bug-15005.php(new) +testBug15005()intests/PHPStan/Rules/Variables/NullCoalesceRuleTest.phpAnalogous cases also fixed
All of these leaked in exactly the same way and are covered by the new NSRT file:
isset($r[0]['Port'])onarray<int, array{Port: int}>isset($r[0]['Port'])onlist<array{Port: int}>(previously narrowed the whole$rto*NEVER*)array{K?: array{Port: int}}now narrows toarray{}in the falsey branch (previously*NEVER*for the whole array)isset($this->arr['K']['Port'])isset($r['A']['B']['Port']), which leakedhasOffsetValue()two levels upisset($r['K']['Port'], $r['K']['Secure']), which is rewritten to an and-chain of single-varisset()sProbed and found already correct
empty($r['K']['Port'])—EmptyHandlerrewrites to!isset(...) || !expr, so it inherits the fix; the truthy branch was already correct.$r['K']['Port'] ?? null—CoalesceHandlernever performed this narrowing.unset($r['K']['Port'])— already fixed for Unsetting a nested array key makes PHPStan believe the parent array exists phpstan#7292 and unaffected.isset($r[$k]['Port'])) andArrayAccessobjects — the narrowing does not apply.array_key_exists('Port', $r['K'])— structurally similar, but not the same bug: unlikeisset(), it evaluates$r['K'], so the intermediate offset's existence really is implied by the call. Left as-is.Root cause
MutatingScope::specifyExpressionType()propagates a narrowed offset type back into the enclosing array: specifying$a['x']intersects$awithHasOffsetValueType('x', <type>). That write-back is correct when the narrowing implies the offset exists (assignment, the truthy branch ofisset()), butisset()'s falsey branch does not imply it —isset($a['x']['y'])is also false when$a['x']is entirely missing.The 2.1.40 falsey-branch narrowing removed the matching constant arrays from
$a['x']unconditionally. Witharray<string, array{Port: int, ...}>every value hasPortset, so the removal producednever, and the write-back stampedhasOffsetValue('K', *NEVER*)onto$a— simultaneously claiming "K exists" (which leaks out of the branch on merge) and "its value is impossible". The single-variable case in the same method already guarded against this by degrading the variable's certainty to "maybe" throughIssetExpr, but there is no such certainty slot for a nested offset, so the guard has to be an existence precondition instead.The fix therefore splits the two situations: narrow the intermediate expression only when it is known to exist, and otherwise record what is actually known — that the intermediate offset is missing — on the enclosing array.
Test
tests/PHPStan/Analyser/nsrt/bug-15005.phpreproduces the issue's playground sample (nestedIssetLeak,alsoAfterPlainIf,branches) plus the analogous cases listed above and the cases that must keep working (definitelySetIntermediate,empty(),??). Without the fix, 9 of itsassertType()calls fail (*NEVER*in the falsey branch, missing|nullafter theif,hasOffsetValue(...)leaking into$r).tests/PHPStan/Rules/Variables/NullCoalesceRuleTest::testBug15005()asserts that the reportednullCoalesce.unnecessaryandnullCoalesce.offsetfalse positives are gone; without the fix it reports both.make tests,make phpstanandmake csare green.Fixes phpstan/phpstan#15005