Narrow value type instead of unsetting key in ConstantArrayType::tryRemove for HasOffsetValueType

[phpstan-bot]

Summary

ConstantArrayType::tryRemove treated HasOffsetValueType the same as HasOffsetType by unsetting the key entirely. This was incorrect because HasOffsetValueType carries a value type constraint — when only part of the value type overlaps, the key should be preserved with a narrowed value type rather than removed.

Changes

• src/Type/Constant/ConstantArrayType.php: Split the HasOffsetValueType handling out of the combined HasOffsetType || HasOffsetValueType branch. The new logic checks valueTypeToRemove->isSuperTypeOf(currentValueType):

  • yes → unset the key (same as before, same as HasOffsetType)
  • no → return null (no impact)
  • maybe → narrow the value type at that key via TypeCombinator::remove, preserving the key's original optionality

• src/Type/Accessory/HasOffsetValueType.php: Added tryRemove() method (replacing NonRemoveableTypeTrait) to handle removing another HasOffsetValueType with the same offset by narrowing the value type. This fixes the analogous case for IntersectionTypes containing HasOffsetValueType.

• tests/PHPStan/Type/TypeCombinatorTest.php: Added 6 test cases to dataRemove:

  • ConstantArrayType with partial value overlap (mandatory key)
  • ConstantArrayType with partial value overlap (optional key)
  • ConstantArrayType with full value overlap on optional key
  • ConstantArrayType with partial overlap on multi-key array
  • HasOffsetValueType removing another HasOffsetValueType (standalone)
  • IntersectionType with HasOffsetValueType narrowing through intersection

Root cause

The root cause was ConstantArrayType::tryRemove treating HasOffsetValueType and HasOffsetType identically with a single instanceof check. Both were routed to unsetOffset(), which removes the key entirely. For HasOffsetType this is correct (removing "has key X" means the key is absent). For HasOffsetValueType this is wrong when the value type only partially overlaps — the key should remain with its value type narrowed.

Probed analogous cases

• UnionType::tryRemove — delegates to TypeCombinator::remove per inner type, no fix needed
• IntersectionType::tryRemove — delegates via intersectTypes, now benefits from the HasOffsetValueType::tryRemove fix
• HasOffsetType::tryRemove — uses NonRemoveableTypeTrait, correct (no value type to narrow)
• ObjectShapeType::tryRemove with HasPropertyType — no HasPropertyValueType exists, not analogous

Test

Added 6 test cases to TypeCombinatorTest::dataRemove covering mandatory keys, optional keys, full overlap, partial overlap, multi-key arrays, standalone HasOffsetValueType, and IntersectionType narrowing. All 3 failing cases now pass with the fix.

Fixes phpstan/phpstan#14711

[staabm]

should/can these tests better be expressed as "nsrt" tests?
that way it would get easier to read and real world use-cases would be clearer

[VincentLanglet]

In another PR it was already asked, and there was no possible nsrt test since we never remove HasOffsetValueType yet

[phpstan-bot]

I investigated the reviewer's suggestion thoroughly and concluded that no change is warranted — the tests cannot meaningfully be expressed as nsrt tests. Here's the evidence.

What I checked

The reviewer asked whether the 6 TypeCombinatorTest::dataRemove cases could become nsrt (assertType) tests so real-world use-cases are clearer. The crux: are these code paths in ConstantArrayType::tryRemove reachable from real PHP analysis?

To find out, I temporarily instrumented the partial-overlap (maybe) branch — the branch that is the actual subject of this fix — with a hit-counter, then ran:

1. Hand-written reproducers covering every plausible narrowing construct: is_int($arr['a'])/else, $arr['a'] === 'x'/else, in_array(..., true), match($arr['a']), discriminated-union shapes, list offsets, and unions of array shapes. → 0 hits
2. The entire analyser suite — NodeScopeResolverTest (runs all nsrt files) + AnalyserIntegrationTest, 1804 tests. → 0 hits
3. TypeCombinatorTest (sanity check the marker works). → 3 hits = exactly the 3 partial-overlap unit cases.

I also diffed several reproducers' output against the pre-fix ConstantArrayType and they were identical — confirming the visible narrowing comes from elsewhere.

Why nsrt tests don't fit here

Real-world array-offset value narrowing (the array{a: string|int} → array{a: string} effect from the issue) routes through setOffsetValueType during type-specification of $arr['a'], not through tryRemove. The HasOffsetValueType removal path in ConstantArrayType::tryRemove is only reached via the internal TypeCombinator::remove / Type::tryRemove API.

Consequences for the reviewer's suggestion:

• An nsrt test exercising this code path can't be written — no PHP snippet routes here.
• An nsrt test showing the user-visible narrowing (e.g. the is_int else branch) would pass with or without this fix, so it would be a misleading regression test that doesn't guard the changed code.

The TypeCombinatorTest::dataRemove unit tests directly assert the internal tryRemove contract — which is the only level at which this behavior is observable — and they sit alongside the sibling cases for HasOffsetType, NonEmptyArrayType, and plain ConstantArrayType removal already tested in that same data provider. That's the correct and consistent home.

State

No source or test changes; instrumentation removed; working tree clean. TypeCombinatorTest passes (1149 tests). Nothing to commit or push.

I'll relay this reasoning as the reply to staabm's comment in the follow-up step.

[phpstan-bot]

The working tree is clean and there is nothing to change here.

Assessment

VincentLanglet's comment isn't a change request — it's a confirmation of the conclusion already reached on this thread:

In another PR it was already asked, and there was no possible nsrt test since we never remove HasOffsetValueType yet

This directly corroborates the investigation posted earlier on staabm's "should these be nsrt tests?" comment: the HasOffsetValueType removal path in ConstantArrayType::tryRemove is not reachable from real PHP analysis — instrumenting the partial-overlap branch produced 0 hits across the entire analyser suite (NodeScopeResolverTest + AnalyserIntegrationTest, 1804 tests) and 0 hits on hand-written narrowing reproducers. PHPStan never removes a HasOffsetValueType during normal type-specification today; the only caller is the internal TypeCombinator::remove / Type::tryRemove API.

VincentLanglet adds the corroborating fact that this same question came up in another PR with the same answer. So both reviewers now independently agree there's no PHP snippet that would route through this code path, which means:

• An nsrt test exercising this path can't be written.
• An nsrt test showing the user-visible narrowing would pass with or without the fix, making it a non-guarding (misleading) regression test.

The TypeCombinatorTest::dataRemove unit cases assert the internal tryRemove contract directly — the only level at which this behavior is observable — alongside the sibling HasOffsetType / NonEmptyArrayType / ConstantArrayType removal cases in the same provider. That remains the correct home.

Action taken

No source or test changes are warranted — the comment confirms the existing approach rather than requesting a change. Working tree is clean (last commit e9ded3856 already restored the comment staabm flagged), and TypeCombinatorTest + make phpstan were green on that commit. Nothing to commit or push.

I'll relay this acknowledgement as the reply to VincentLanglet in the follow-up step.

[staabm]

since we never remove HasOffsetValueType yet

is this something planned for the future? or how did you find this problem in the first place? :)

[phpstan-bot]

The working tree is clean and there's nothing to change — this review comment is a question, not a change request, so there's no code to modify (and therefore nothing new to test).

Here's the answer I'll relay to staabm:

---

Good question! To be transparent about provenance:

How it was found: This didn't come from a real-world reproducer — it came from issue phpstan/phpstan#14711, which is itself an audit-style observation about the type system rather than a bug report with a failing snippet. The issue notes that ConstantArrayType::tryRemove routed HasOffsetValueType through the exact same unsetOffset() path as HasOffsetType, ignoring the value-type constraint that HasOffsetValueType carries. That's a latent correctness gap in the internal Type::tryRemove / TypeCombinator::remove contract: subtracting hasOffsetValue('a', int) from array{a: string|int} should yield array{a: string}, but the old code unset the key entirely.

Is the path reachable today? Not from normal analysis — which is exactly what VincentLanglet meant by "we never remove HasOffsetValueType yet." I verified this independently earlier on this thread: I instrumented the partial-overlap branch with a hit-counter and ran the full analyser suite (NodeScopeResolverTest + AnalyserIntegrationTest, 1804 tests) plus hand-written narrowing reproducers → 0 hits. User-visible array-offset value narrowing routes through setOffsetValueType during type-specification, not through tryRemove. So this is fixing the contract at the API level before anything depends on it.

Planned for the future? I don't have visibility into a specific roadmap item that will start removing HasOffsetValueType, so I won't overstate it. But the natural place it would become reachable is type subtraction on array offsets — e.g. extending TypeSpecifier so that an else branch of is_int($arr['a']) subtracts the value type at that offset rather than rebuilding the shape. When/if that lands, tryRemove needs to already behave correctly, which is what this PR ensures. That's also why the tests live in TypeCombinatorTest::dataRemove as unit assertions on the internal contract — there's no PHP snippet that routes here yet, so an nsrt test can't exercise it.

In short: found by inspecting the tryRemove logic via the issue, fixing a real-but-currently-dormant correctness gap, with the tests pinned at the only level where the behavior is observable today.

[staabm]

can we kill this mutant?

public function tryRemove(Type $typeToRemove): ?Type { if ($typeToRemove instanceof self && $this->offsetType->equals($typeToRemove->getOffsetType())) { 
- $valueIsSuperType = $typeToRemove->getValueType()->isSuperTypeOf($this->valueType); 
+ $valueIsSuperType = $this->valueType->isSuperTypeOf($typeToRemove->getValueType());

if ($valueIsSuperType->no()) { return null;

[phpstan-bot]

Done. Here's the reply I'll relay to staabm:

---

Good catch — that mutant is actually an equivalent mutant, which is why flipping the direction left every test green. Rather than paper over it with a test, I removed the dead line entirely.

Why the line is unobservable: HasOffsetValueType::tryRemove is only ever reached through TypeCombinator::remove (line 90), and IntersectionType::tryRemove routes through the same function. But TypeCombinator::remove guards the call at lines 75-81:

$isSuperType = $typeToRemove->isSuperTypeOf($fromType);
if ($isSuperType->yes()) { return new NeverType(); }   // full overlap — never reaches tryRemove
if ($isSuperType->no())  { return $fromType; }          // disjoint    — never reaches tryRemove
...
$removed = $fromType->tryRemove($typeToRemove);          // only the Maybe (partial-overlap) case

I instrumented this empirically:

this value	remove value	b.isSuperTypeOf(a)	reaches tryRemove?
string|int	int	Maybe	✅ → hasOffsetValue('a', string)
int	int<0,5>	Maybe	✅ → hasOffsetValue('a', int<min,-1>|int<6,max>)
int	int|string	Yes	❌ short-circuits to *NEVER*
string	int	No	❌ short-circuits to unchanged

So by the time we're inside tryRemove, the value types always partially overlap, meaning $valueIsSuperType->no() is never true. The return null branch was unreachable — flipping $a->isSuperTypeOf($b) vs $b->isSuperTypeOf($a) couldn't change behavior because neither was ever consulted for the no() decision in a reachable state.

The fix deletes the guard and keeps only the genuine narrowing:

public function tryRemove(Type $typeToRemove): ?Type
{
	if ($typeToRemove instanceof self && $this->offsetType->equals($typeToRemove->getOffsetType())) {
		return new self(
			$this->offsetType,
			TypeCombinator::remove($this->valueType, $typeToRemove->getValueType()),
		);
	}

	return null;
}

The mutant is now killed by deletion, and a comment documents the caller contract so the simplification doesn't read as an oversight. I checked the sibling ConstantArrayType::tryRemove — its three-way yes/no/maybe branches there are reachable (a constant array can match on one key while staying Maybe overall via other keys), so that direction is genuine and untouched.

vendor/bin/phpunit tests/PHPStan/Type/TypeCombinatorTest.php → OK (1149 tests), make tests → OK (12556 tests), make phpstan → No errors. Pushed as 4ec281591.