Skip to content

Narrow array key type when removing HasOffsetType via ArrayType::tryRemove#5756

Open
phpstan-bot wants to merge 1 commit into
phpstan:2.2.xfrom
phpstan-bot:create-pull-request/patch-rvs2ifu
Open

Narrow array key type when removing HasOffsetType via ArrayType::tryRemove#5756
phpstan-bot wants to merge 1 commit into
phpstan:2.2.xfrom
phpstan-bot:create-pull-request/patch-rvs2ifu

Conversation

@phpstan-bot
Copy link
Copy Markdown
Collaborator

Summary

When array_key_exists(0, $arr) returned false, the array key type was not narrowed — e.g., array<0|string, mixed> remained unchanged instead of becoming array<string, mixed>. This affected the falsy branch of array_key_exists() and key_exists() checks.

Changes

  • Added HasOffsetType and HasOffsetValueType handling to ArrayType::tryRemove() in src/Type/ArrayType.php, which calls unsetOffset() to narrow the key type when an offset is known not to exist.
  • This mirrors the existing behavior in ConstantArrayType::tryRemove() which already handled both types.
  • Added regression test in tests/PHPStan/Analyser/nsrt/bug-9461.php covering:
    • Integer key removal from union key type (array<0|string, mixed>array<string, mixed>)
    • String key removal from union key type (array<'foo'|int, mixed>array<int, mixed>)
    • Generic integer key narrowing (array<int, mixed>array<int<min, -1>|int<1, max>, mixed>)
    • key_exists() alias (same behavior as array_key_exists)
    • Full flow from the original issue (check + unset + return)

Root cause

ArrayType::tryRemove() did not handle HasOffsetType removal. When array_key_exists() returned false, the ArrayKeyExistsFunctionTypeSpecifyingExtension specified HasOffsetType($keyType) as a sureNotType (type to remove). TypeCombinator::remove() delegated to ArrayType::tryRemove(), which returned null (no narrowing possible), so the array type was returned unchanged.

The extension also had a second narrowing path using ArrayType(MixedType, MixedType)->unsetOffset($keyType) intersected with the existing type, but this was ineffective because getIterableKeyType() normalizes MixedType (including subtracted variants like mixed~0) to BenevolentUnionType(int|string), causing ArrayType::isSuperTypeOf() to lose the subtraction information.

The fix adds HasOffsetType/HasOffsetValueType handling to ArrayType::tryRemove(), which calls unsetOffset() to properly narrow the key type. This approach naturally works for both PHPDoc and native types because MutatingScope::removeTypeFromExpression() calls TypeCombinator::remove() separately on each.

Analogous cases probed

  • ConstantArrayType::tryRemove(): Already handles HasOffsetType and HasOffsetValueType — no fix needed.
  • IntersectionType::tryRemove(): Delegates TypeCombinator::remove() to each component type — works correctly via the ArrayType fix.
  • UnionType::tryRemove(): Delegates to each union member — works correctly.
  • isset / ?? / empty: Semantically different from array_key_exists (they also check for null/falsy values), so key type narrowing via HasOffsetType removal is not applicable.
  • key_exists: Same extension as array_key_exists — already handled.

Test

Regression test tests/PHPStan/Analyser/nsrt/bug-9461.php covers integer key removal, string key removal, generic key narrowing, and the original issue's full code flow.

Fixes phpstan/phpstan#9461

Comment thread src/Type/ArrayType.php Outdated
Comment thread src/Type/ArrayType.php Outdated
@VincentLanglet VincentLanglet self-assigned this May 25, 2026
Comment thread src/Type/Constant/ConstantArrayType.php Outdated
Copy link
Copy Markdown
Contributor

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to reproduce the HasOffsetValueType bug with nsrt test ?

@phpstan-bot
Copy link
Copy Markdown
Collaborator Author

All tests pass (12160 tests, 79841 assertions) and make phpstan reports no errors.

Now for the reviewer's question: "Is there a way to reproduce the HasOffsetValueType bug with nsrt test?"

After thorough investigation, the answer is no. Here's why:

The only code path that creates HasOffsetValueType as a sureNotType (type to remove) is the count() comparison in TypeSpecifier::specifyTypesForCountFuncCall() at line 1473:

$sureNotTypes[$argExprString] = [$argExpr, new HasOffsetValueType(new ConstantIntegerType($sizeMax), new MixedType())];

This always uses MixedType() as the value type. Since MixedType is a supertype of everything, the value type guard ($typeToRemove->getValueType()->isSuperTypeOf(...)->yes()) always passes. No other type specifying extension creates HasOffsetValueType as a sureNotType — array_key_exists uses HasOffsetType (without value), and all other HasOffsetValueType usages are for type construction (sureTypes/intersections), not removal.

The guard is tested via TypeCombinatorTest::dataRemove() unit tests which directly test TypeCombinator::remove() with specific HasOffsetValueType instances including the case where the value type doesn't match (the third test case: HasOffsetValueType(0, string) on array<0|string, mixed> → no change).

There are no code changes needed for this review comment since no nsrt test can demonstrate the guard behavior. The existing unit tests are the appropriate verification level.

Comment thread tests/PHPStan/Analyser/nsrt/bug-9461.php
Comment thread tests/PHPStan/Analyser/nsrt/bug-9461.php
Comment thread src/Type/Constant/ConstantArrayType.php Outdated

if ($typeToRemove instanceof HasOffsetType || $typeToRemove instanceof HasOffsetValueType) {
if ($typeToRemove instanceof HasOffsetValueType
&& !$typeToRemove->getValueType()->isSuperTypeOf($this->getOffsetValueType($typeToRemove->getOffsetType()))->yes()) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about maybe() ? Should we make the key optional ? Should we tryRemove the intersection ?

@VincentLanglet VincentLanglet force-pushed the create-pull-request/patch-rvs2ifu branch 2 times, most recently from 4fb71da to 8026862 Compare May 27, 2026 16:33
@VincentLanglet VincentLanglet changed the base branch from 2.1.x to 2.2.x May 27, 2026 16:34
…ryRemove`

- Add `HasOffsetType` and `HasOffsetValueType` handling to `ArrayType::tryRemove`,
  calling `unsetOffset()` to narrow the key type when an offset is known to not exist
- This mirrors the existing `ConstantArrayType::tryRemove` behavior which already
  handles both `HasOffsetType` and `HasOffsetValueType`
- Fixes `array_key_exists()` falsy branch not narrowing key types on general
  `ArrayType` (e.g. `array<0|string, mixed>` → `array<string, mixed>`)
- Works for both PHP >= 8.0 and PHP < 8.0 code paths, and propagates correctly
  through `IntersectionType` and `UnionType` via their `tryRemove` delegation
@VincentLanglet VincentLanglet force-pushed the create-pull-request/patch-rvs2ifu branch from 8026862 to 4cdc600 Compare May 27, 2026 16:36
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.

3 participants