Skip to content

Commit 2f03f56

Browse files
committed
Fix match exhaustiveness for class-string with union of final classes
- GenericClassStringType::tryRemove() only handled class-string with a single generic type parameter, failing for unions like class-string<A|B|C> - Added handling for union generic types: when removing a constant class-string of a final class, remove the corresponding ObjectType from the inner union - New regression tests in both rule test and nsrt for bug #9534
1 parent 8a619a5 commit 2f03f56

File tree

5 files changed

+140
-3
lines changed

5 files changed

+140
-3
lines changed

CLAUDE.md

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -279,6 +279,14 @@ Loops are a frequent source of false positives because PHPStan must reason about
279279

280280
Match expressions in `NodeScopeResolver` (around line 4154) process each arm and merge the resulting scopes. The critical pattern for variable certainty is: when a match is exhaustive (has a `default` arm or an always-true condition), arm body scopes should be merged only with each other (not with the original pre-match scope). This mirrors how if/else merging works — `$finalScope` starts as `null`, and each branch's scope is merged via `$branchScope->mergeWith($finalScope)`. When the match is NOT exhaustive, the original scope must also participate in the merge (via `$scope->mergeWith($armBodyFinalScope)`) because execution may skip all arms and throw `UnhandledMatchError`. The `mergeVariableHolders()` method in `MutatingScope` uses `ExpressionTypeHolder::createMaybe()` for variables present in only one scope, so merging an arm scope that defines `$x` with the original scope that lacks `$x` degrades certainty to "maybe" — this is the root cause of false "might not be defined" reports for exhaustive match expressions.
281281

282+
### GenericClassStringType narrowing and tryRemove
283+
284+
`GenericClassStringType` represents `class-string<T>` where `T` is the generic object type. When the generic type is a union (e.g., `class-string<Car|Bike|Boat>`), it's a single `GenericClassStringType` with an inner `UnionType`. This is distinct from `class-string<Car>|class-string<Bike>|class-string<Boat>` which is a `UnionType` of individual `GenericClassStringType`s.
285+
286+
The `tryRemove()` method handles removing a `ConstantStringType` (e.g., `'Car'`) from the class-string type. It must check whether the class is final — only for final classes can exact class-string removal be performed, since non-final classes could have subclasses whose class-strings would still be valid values. When the inner generic type is a union, `TypeCombinator::remove()` is used to remove the corresponding `ObjectType` from the inner union.
287+
288+
This affects match expression exhaustiveness: `class-string<FinalA|FinalB>` matched against `FinalA::class` and `FinalB::class` is exhaustive only because both classes are final.
289+
282290
### PHPDoc inheritance
283291

284292
PHPDoc types (`@return`, `@param`, `@throws`, `@property`) are inherited through class hierarchies. Bugs arise when:

src/Type/Generic/GenericClassStringType.php

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -211,10 +211,27 @@ public function tryRemove(Type $typeToRemove): ?Type
211211
$generic = $this->getGenericType();
212212

213213
$genericObjectClassNames = $generic->getObjectClassNames();
214+
$reflectionProvider = ReflectionProviderStaticAccessor::getInstance();
215+
214216
if (count($genericObjectClassNames) === 1) {
215-
$classReflection = ReflectionProviderStaticAccessor::getInstance()->getClass($genericObjectClassNames[0]);
216-
if ($classReflection->isFinal() && $genericObjectClassNames[0] === $typeToRemove->getValue()) {
217-
return new NeverType();
217+
if ($reflectionProvider->hasClass($genericObjectClassNames[0])) {
218+
$classReflection = $reflectionProvider->getClass($genericObjectClassNames[0]);
219+
if ($classReflection->isFinal() && $genericObjectClassNames[0] === $typeToRemove->getValue()) {
220+
return new NeverType();
221+
}
222+
}
223+
} elseif (count($genericObjectClassNames) > 1) {
224+
$objectTypeToRemove = new ObjectType($typeToRemove->getValue());
225+
if ($reflectionProvider->hasClass($typeToRemove->getValue())) {
226+
$classReflection = $reflectionProvider->getClass($typeToRemove->getValue());
227+
if ($classReflection->isFinal()) {
228+
$remainingType = TypeCombinator::remove($generic, $objectTypeToRemove);
229+
if ($remainingType instanceof NeverType) {
230+
return new NeverType();
231+
}
232+
233+
return new self($remainingType);
234+
}
218235
}
219236
}
220237
}
Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
<?php // lint >= 8.0
2+
3+
declare(strict_types = 1);
4+
5+
namespace Bug9534Nsrt;
6+
7+
use function PHPStan\Testing\assertType;
8+
9+
final class FinalCar {}
10+
final class FinalBike {}
11+
final class FinalBoat {}
12+
13+
/**
14+
* @param class-string<FinalCar|FinalBike|FinalBoat> $string
15+
*/
16+
function narrowing(string $string): void {
17+
assertType('class-string<Bug9534Nsrt\FinalBike|Bug9534Nsrt\FinalBoat|Bug9534Nsrt\FinalCar>', $string);
18+
19+
if ($string === FinalCar::class) {
20+
assertType("'Bug9534Nsrt\\\\FinalCar'", $string);
21+
return;
22+
}
23+
24+
assertType('class-string<Bug9534Nsrt\FinalBike|Bug9534Nsrt\FinalBoat>', $string);
25+
26+
if ($string === FinalBike::class) {
27+
assertType("'Bug9534Nsrt\\\\FinalBike'", $string);
28+
return;
29+
}
30+
31+
assertType("class-string<Bug9534Nsrt\\FinalBoat>", $string);
32+
}

tests/PHPStan/Rules/Comparison/MatchExpressionRuleTest.php

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -416,4 +416,23 @@ public function testBug13303(): void
416416
]);
417417
}
418418

419+
#[RequiresPhp('>= 8.0')]
420+
public function testBug9534(): void
421+
{
422+
$this->analyse([__DIR__ . '/data/bug-9534.php'], [
423+
[
424+
'Match expression does not handle remaining value: class-string<Bug9534\Bike|Bug9534\Boat|Bug9534\Car>',
425+
21,
426+
],
427+
[
428+
'Match expression does not handle remaining value: class-string<Bug9534\FinalBike|Bug9534\FinalBoat>',
429+
47,
430+
],
431+
[
432+
'Match expression does not handle remaining value: class-string<Bug9534\Bike|Bug9534\Car>',
433+
58,
434+
],
435+
]);
436+
}
437+
419438
}
Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
<?php // lint >= 8.0
2+
3+
declare(strict_types = 1);
4+
5+
namespace Bug9534;
6+
7+
class Car {}
8+
class Bike {}
9+
class Boat {}
10+
11+
final class FinalCar {}
12+
final class FinalBike {}
13+
final class FinalBoat {}
14+
15+
/**
16+
* Non-final classes: match can't be exhaustive because subclasses may exist
17+
*
18+
* @param class-string<Car|Bike|Boat> $string
19+
*/
20+
function accept_class(string $string): void {
21+
match ($string) { // error on this line
22+
Car::class => 'car',
23+
Bike::class => 'bike',
24+
Boat::class => 'boat',
25+
};
26+
}
27+
28+
/**
29+
* Final classes: match IS exhaustive because no subclasses can exist
30+
*
31+
* @param class-string<FinalCar|FinalBike|FinalBoat> $string
32+
*/
33+
function accept_final_class(string $string): void {
34+
match ($string) { // no error
35+
FinalCar::class => 'car',
36+
FinalBike::class => 'bike',
37+
FinalBoat::class => 'boat',
38+
};
39+
}
40+
41+
/**
42+
* Partial match with final classes: should report remaining value
43+
*
44+
* @param class-string<FinalCar|FinalBike|FinalBoat> $string
45+
*/
46+
function partial_final_match(string $string): void {
47+
match ($string) { // error on this line
48+
FinalCar::class => 'car',
49+
};
50+
}
51+
52+
/**
53+
* Partial match with non-final classes
54+
*
55+
* @param class-string<Car|Bike> $string
56+
*/
57+
function partial_match(string $string): void {
58+
match ($string) { // error on this line
59+
Car::class => 'car',
60+
};
61+
}

0 commit comments

Comments
 (0)