Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
phpstan-bot wants to merge 3 commits into
base: 2.2.x
Choose a base branch
from
+123 −3
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1e8dae9
Narrow value type instead of unsetting key in `ConstantArrayType::try…
VincentLanglet May 27, 2026
e9ded38
Restore list-subtraction comment in ConstantArrayType::tryRemove
phpstan-bot May 30, 2026
4ec2815
Drop dead value-type guard in HasOffsetValueType::tryRemove
phpstan-bot May 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions src/Type/Accessory/HasOffsetValueType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
use PHPStan\Type\Traits\MaybeStringTypeTrait;
use PHPStan\Type\Traits\NonGeneralizableTypeTrait;
use PHPStan\Type\Traits\NonGenericTypeTrait;
use PHPStan\Type\Traits\NonRemoveableTypeTrait;
use PHPStan\Type\Traits\TruthyBooleanTypeTrait;
use PHPStan\Type\Traits\UndecidedComparisonCompoundTypeTrait;
use PHPStan\Type\Type;
Expand All @@ -57,7 +56,6 @@ class HasOffsetValueType implements CompoundType, AccessoryType
use TruthyBooleanTypeTrait;
use NonGenericTypeTrait;
use UndecidedComparisonCompoundTypeTrait;
use NonRemoveableTypeTrait;
use NonGeneralizableTypeTrait;

public function __construct(private ConstantStringType|ConstantIntegerType $offsetType, private Type $valueType)
Expand Down Expand Up @@ -213,6 +211,22 @@ public function unsetOffset(Type $offsetType): Type
return $this;
}

public function tryRemove(Type $typeToRemove): ?Type
{
// Only reachable through TypeCombinator::remove(), which short-circuits the
// full-overlap case to NeverType and the disjoint case to the unchanged type
// before delegating here. So when we get here the value types always partially
// overlap and removing $typeToRemove's value type genuinely narrows ours.
if ($typeToRemove instanceof self && $this->offsetType->equals($typeToRemove->getOffsetType())) {
return new self(
$this->offsetType,
TypeCombinator::remove($this->valueType, $typeToRemove->getValueType()),
);
}

return null;
}

public function getKeysArrayFiltered(Type $filterValueType, TrinaryLogic $strict): Type
{
return $this->getKeysArray();
Expand Down
47 changes: 46 additions & 1 deletion src/Type/Constant/ConstantArrayType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2629,7 +2629,52 @@
return new ConstantArrayType([], []);
}

if ($typeToRemove instanceof HasOffsetType || $typeToRemove instanceof HasOffsetValueType) {
if ($typeToRemove instanceof HasOffsetValueType) {
$offsetType = $typeToRemove->getOffsetType();
$valueTypeToRemove = $typeToRemove->getValueType();

foreach ($this->keyTypes as $i => $keyType) {
Comment thread
VincentLanglet marked this conversation as resolved.
if ($keyType->getValue() !== $offsetType->getValue()) {
continue;
}

$currentValueType = $this->valueTypes[$i];
$valueIsSuperType = $valueTypeToRemove->isSuperTypeOf($currentValueType);

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

if ($valueIsSuperType->yes()) {
$unsetResult = $this->unsetOffset($offsetType, true);
// When the source was definitely a list but the post-unset shape
// definitely isn't (e.g. unsetting a non-optional leading key
// creates a hole), no value of $this could have lacked the
// removed key — the subtraction yields the empty set.
if ($this->isList->yes() && $unsetResult->isList()->no()) {

Check warning on line 2654 in src/Type/Constant/ConstantArrayType.php

View workflow job for this annotation

GitHub Actions / Mutation Testing (8.4, ubuntu-latest) 

Escaped Mutant for Mutator "PHPStan\Infection\TrinaryLogicMutator":

@@ @@
 					// definitely isn't (e.g. unsetting a non-optional leading key
 					// creates a hole), no value of $this could have lacked the
 					// removed key — the subtraction yields the empty set.
-					if ($this->isList->yes() && $unsetResult->isList()->no()) {
+					if ($this->isList->yes() && !$unsetResult->isList()->yes()) {
 						return new NeverType();
 					}
 					return $unsetResult;

Check warning on line 2654 in src/Type/Constant/ConstantArrayType.php

View workflow job for this annotation

GitHub Actions / Mutation Testing (8.4, ubuntu-latest) 

Escaped Mutant for Mutator "PHPStan\Infection\TrinaryLogicMutator":

@@ @@
 					// definitely isn't (e.g. unsetting a non-optional leading key
 					// creates a hole), no value of $this could have lacked the
 					// removed key — the subtraction yields the empty set.
-					if ($this->isList->yes() && $unsetResult->isList()->no()) {
+					if (!$this->isList->no() && $unsetResult->isList()->no()) {
 						return new NeverType();
 					}
 					return $unsetResult;

Check warning on line 2654 in src/Type/Constant/ConstantArrayType.php

View workflow job for this annotation

GitHub Actions / Mutation Testing (8.3, ubuntu-latest) 

Escaped Mutant for Mutator "PHPStan\Infection\TrinaryLogicMutator":

@@ @@
 					// definitely isn't (e.g. unsetting a non-optional leading key
 					// creates a hole), no value of $this could have lacked the
 					// removed key — the subtraction yields the empty set.
-					if ($this->isList->yes() && $unsetResult->isList()->no()) {
+					if (!$this->isList->no() && $unsetResult->isList()->no()) {
 						return new NeverType();
 					}
 					return $unsetResult;

Check warning on line 2654 in src/Type/Constant/ConstantArrayType.php

View workflow job for this annotation

GitHub Actions / Mutation Testing (8.3, ubuntu-latest) 

Escaped Mutant for Mutator "PHPStan\Infection\TrinaryLogicMutator":

@@ @@
 					// definitely isn't (e.g. unsetting a non-optional leading key
 					// creates a hole), no value of $this could have lacked the
 					// removed key — the subtraction yields the empty set.
-					if ($this->isList->yes() && $unsetResult->isList()->no()) {
+					if ($this->isList->yes() && !$unsetResult->isList()->yes()) {
 						return new NeverType();
 					}
 					return $unsetResult;
return new NeverType();
}
return $unsetResult;
}

$newValueType = TypeCombinator::remove($currentValueType, $valueTypeToRemove);
$valueTypes = $this->valueTypes;
$valueTypes[$i] = $newValueType;

return $this->recreate(
$this->keyTypes,
$valueTypes,
$this->nextAutoIndexes,
$this->optionalKeys,
$this->isList,
$this->unsealed,
);
}

return null;
}

if ($typeToRemove instanceof HasOffsetType) {
$unsetResult = $this->unsetOffset($typeToRemove->getOffsetType(), true);
// When the source was definitely a list but the post-unset shape
// definitely isn't (e.g. unsetting a non-optional leading key
Expand Down
61 changes: 61 additions & 0 deletions tests/PHPStan/Type/TypeCombinatorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6185,6 +6185,67 @@ public static function dataRemove(): array
ArrayType::class,
'array<0|string, mixed>',
],
// HasOffsetValueType with partial value overlap — narrow value, keep key mandatory
Copy link
Copy Markdown
Contributor

@staabm staabm May 30, 2026

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@VincentLanglet VincentLanglet May 30, 2026

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Collaborator Author

@phpstan-bot phpstan-bot May 30, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator Author

@phpstan-bot phpstan-bot May 30, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@staabm staabm May 30, 2026

Choose a reason for hiding this comment

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

since we never remove HasOffsetValueType yet

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

Copy link
Copy Markdown
Collaborator Author

@phpstan-bot phpstan-bot May 30, 2026

Choose a reason for hiding this comment

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

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.

[
new ConstantArrayType(
[new ConstantStringType('a')],
[new UnionType([new StringType(), new IntegerType()])],
),
new HasOffsetValueType(new ConstantStringType('a'), new IntegerType()),
ConstantArrayType::class,
'array{a: string}',
],
// HasOffsetValueType with partial value overlap — narrow value, keep key optional
[
new ConstantArrayType(
[new ConstantStringType('a')],
[new UnionType([new StringType(), new IntegerType()])],
[0],
[0],
),
new HasOffsetValueType(new ConstantStringType('a'), new IntegerType()),
ConstantArrayType::class,
'array{a?: string}',
],
// HasOffsetValueType with full value overlap on optional key — remove key entirely
[
new ConstantArrayType(
[new ConstantStringType('a')],
[new IntegerType()],
[0],
[0],
),
new HasOffsetValueType(new ConstantStringType('a'), new IntegerType()),
ConstantArrayType::class,
'array{}',
],
// HasOffsetValueType with partial value overlap — multi-key array
[
new ConstantArrayType(
[new ConstantStringType('a'), new ConstantStringType('b')],
[new UnionType([new StringType(), new IntegerType()]), new StringType()],
),
new HasOffsetValueType(new ConstantStringType('a'), new IntegerType()),
ConstantArrayType::class,
'array{a: string, b: string}',
],
// HasOffsetValueType removing another HasOffsetValueType with same offset
[
new HasOffsetValueType(new ConstantStringType('a'), new UnionType([new StringType(), new IntegerType()])),
new HasOffsetValueType(new ConstantStringType('a'), new IntegerType()),
HasOffsetValueType::class,
'hasOffsetValue(\'a\', string)',
],
// HasOffsetValueType on IntersectionType — narrow value type through intersection
[
new IntersectionType([
new ArrayType(new MixedType(), new MixedType()),
new HasOffsetValueType(new ConstantStringType('a'), new UnionType([new StringType(), new IntegerType()])),
]),
new HasOffsetValueType(new ConstantStringType('a'), new IntegerType()),
IntersectionType::class,
'non-empty-array&hasOffsetValue(\'a\', string)',
],
];
}

Expand Down
Loading