Skip to content

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
phpstan:2.2.xfrom
phpstan-bot:create-pull-request/patch-d0ilk0b
Open

Narrow the falsey branch of isset($a['x']['y']) only when the intermediate offset is known to exist#6110
phpstan-bot wants to merge 1 commit into
phpstan:2.2.xfrom
phpstan-bot:create-pull-request/patch-d0ilk0b

Conversation

@phpstan-bot

Copy link
Copy Markdown
Collaborator

Summary

isset() on a nested array offset made PHPStan treat the intermediate offset as definitely existing for the rest of the enclosing scope.

/** @param array<string, array{Port: int, Secure: string|null}> $r */
function f(array $r): void
{
	if (isset($r['K']['Port'])) {
		// array{Port: int, Secure: string|null}
	} else {
		// *NEVER*  <- wrong, should be array{...}|null
	}
	// array{Port: int, Secure: string|null}  <- |null leaked away
}

Because the falsey branch was narrowed to never, merging it back with the truthy branch dropped the "offset may not exist" information, which produced nullCoalesce.offset and nullCoalesce.unnecessary false positives on every $a['x']['y'] ?? null sitting after such an isset().

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:

  • Added the private helper isAlwaysSet(), which walks an array-dim-fetch chain and answers whether every offset in it definitely exists (Type::hasOffsetValueType()->yes() at each level, plus Scope::hasVariableType()->yes() at the root variable).
  • The existing falsey-branch narrowing (introduced in 2.1.40 by phpstan-src#4983) now runs only when $issetExpr->var is isAlwaysSet(), or when it is a plain Variable — variables keep the previous behaviour because their certainty is separately degraded to "maybe" via IssetExpr.
  • Added a new, sound branch for the remaining case: when removing the candidate constant arrays leaves never, the only way for isset() to be false is the intermediate offset not existing. That fact is now specified on the enclosing array via Type::unsetOffset() instead of being (wrongly) written onto the intermediate expression.
  • That specification is skipped when intersecting it back with the original type collapses to never, which happens for list<T>array<int<1, max>, T> & list<T> is never in 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() in tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php

Analogous cases also fixed

All of these leaked in exactly the same way and are covered by the new NSRT file:

  • integer offsets — isset($r[0]['Port']) on array<int, array{Port: int}>
  • list offsets — isset($r[0]['Port']) on list<array{Port: int}> (previously narrowed the whole $r to *NEVER*)
  • optional intermediate keys — array{K?: array{Port: int}} now narrows to array{} in the falsey branch (previously *NEVER* for the whole array)
  • property-fetch bases — isset($this->arr['K']['Port'])
  • deeper chains — isset($r['A']['B']['Port']), which leaked hasOffsetValue() two levels up
  • multi-var isset($r['K']['Port'], $r['K']['Secure']), which is rewritten to an and-chain of single-var isset()s

Probed and found already correct

  • empty($r['K']['Port'])EmptyHandler rewrites to !isset(...) || !expr, so it inherits the fix; the truthy branch was already correct.
  • $r['K']['Port'] ?? nullCoalesceHandler never 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.
  • non-constant offsets (isset($r[$k]['Port'])) and ArrayAccess objects — the narrowing does not apply.
  • array_key_exists('Port', $r['K']) — structurally similar, but not the same bug: unlike isset(), 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 $a with HasOffsetValueType('x', <type>). That write-back is correct when the narrowing implies the offset exists (assignment, the truthy branch of isset()), but isset()'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. With array<string, array{Port: int, ...}> every value has Port set, so the removal produced never, and the write-back stamped hasOffsetValue('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" through IssetExpr, 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.php reproduces 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 its assertType() calls fail (*NEVER* in the falsey branch, missing |null after the if, hasOffsetValue(...) leaking into $r).
  • tests/PHPStan/Rules/Variables/NullCoalesceRuleTest::testBug15005() asserts that the reported nullCoalesce.unnecessary and nullCoalesce.offset false positives are gone; without the fix it reports both.
  • Full make tests, make phpstan and make cs are green.

Fixes phpstan/phpstan#15005

…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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

isset() on a nested array offset narrows the falsey branch to never, leaking the intermediate offset's existence (regression in 2.1.40)

1 participant