Match expression does not handle remaining value: class-string

[Dgame]

Bug report

PHPStan does not recognise that all match arms are handled when you use a class union type.

Code snippet that reproduces the problem

https://phpstan.org/r/51d5cd6f-2479-4db6-ae2b-cfae97da918d

Expected output

Since all match arms are covered, no error should happen.

Did PHPStan help you today? Did it make you happy in any way?

Every day!

[Dgame]

Funny thing: get_class and ::class behave differently (at least the error is different):
https://phpstan.org/r/cd8e962a-e0a2-481b-be6f-e4ddad56fd85

But the error is wrong since it cannot be both at the same time 🤔

Here is the proof that the code works and does not generate an error: https://3v4l.org/XPrpC

[Dgame]

Might be related to #7593

[ondrejmirtes]

This problem isn't exclusive to the match expression: https://phpstan.org/r/add9ced8-da2d-4e95-8830-fff43659695c

GenericClassStringType should understand better final classes. It should probably be fixed in GenericClassStringType::tryRemove() method.

[Dgame]

I don't know if I can follow. I would expect the class-string<A>|class-string<B> output since it is a union type. What should it be instead? And the return should have no effect, or am I wrong?

[ondrejmirtes]

Compare the class-string (GenericClassStringType) example with ObjectType (https://apiref.phpstan.org/1.8.x/PHPStan.Type.ObjectType.html) that behaves correctly: https://phpstan.org/r/047f530a-ffe1-4e72-9490-d969bd46b9bc

[Dgame]

Ah, I see, so it should be made clear to phpstan as soon as it "sees" the if condition.

[Dgame]

I had a look at the tryRemove from ObjectType but I can't figure out how to implement/call it for this case, sorry. :/

[staabm]

tryRemove is called by the type analysis because of the code example

if ($class === B::class) { // should be always true
	return;
}

this code will try to remove B::class from $class's type (in the ELSE case; which happens because of the early-return underneath the IF block).

I think ondrey meant you need to look into implementing a GenericClassStringType->tryRemove() method which works with a given class-string type. As is GenericClassStringType inherits a generic tryRemove logic from its parent type, which does not work for your code example.

[Dgame]

@staabm I tried implementing the tryRemove like this:

$className = $this->type->describe(VerbosityLevel::value());
$objectType = new ObjectType($className);

return $objectType->tryRemove(...);

but it did not solve the problem (or I did it wrong). I believe the problem is the union type + the generic type: phpstan is not following that all cases are handled.

[staabm]

please open a PR on what you have, then we can further inspect/look into it

[Dgame]

Just did that: Dgame/phpstan-src@c4cafb5

[Dgame]

@staabm @ondrejmirtes It seems this is not just related to GenericClassString but also related to Objects / Instances: https://phpstan.org/r/ea16571e-54a2-44d1-97d9-93ab02c6f3c8

Does that mean that tryRemove is not working in this case for ObjectType? I'm still convinced that the issue is within the MatchExpressionRule.

[ondrejmirtes]

PHPStan cannot understand those match arms are exhaustive.

[Dgame]

PHPStan cannot understand those match arms are exhaustive.

Yes I think so too. Wouldn't my suggested PR fix this?

[ondrejmirtes]

It's not expressable on the typesystem level, I don't think we want or need to implement it.

[Dgame]

Well, at least it shouldn't throw a linting error where there isn't one. It is absolutely safe code: all possible match arms are handled. So a warning in these cases seems wrong. psalm and phan give no warning at all (even if match arms are missing). I would therefore prefer phpstan to either handle this correctly or ignore it completely.

[ondrejmirtes]

Your code isn't even safe, you can have class C extends Base that returns null for both methods.

[Dgame]

Your code isn't even safe, you can have class C extends Base that returns null for both methods.

True, it was just another quick-made example. 🙂 If I would introduce another class C, phpstan would be right to trigger a warning. But either way the first example I showed with the class-names is pretty much safe and easy to analyze and should not trigger a warning.

[Dgame]

@ondrejmirtes Here is a safe example: https://phpstan.org/r/e4476215-99ea-434f-adae-3564ef93a5a5
We have a union type of A|B|C, phpstan recognises this and displays it correctly. But the MatchExpressionRule does not handle this information accordingly.

[phpstan-bot]

@Dgame After the latest push in 1.8.x, PHPStan now reports different result with your code snippet:

@@ @@
-PHP 8.1 – 8.2 (2 errors)
+PHP 8.1 – 8.2 (1 error)
 ==========
 
-12: Match expression does not handle remaining values: class-string<A>|class-string<B>
 17: Match expression does not handle remaining value: class-string
 
 PHP 8.0 (1 error)

Full report

PHP 8.1 – 8.2 (1 error)

Line	Error
17	Match expression does not handle remaining value: class-string

PHP 8.0 (1 error)

Line	Error
7	Syntax error, unexpected T_STRING, expecting T_VARIABLE on line 7

PHP 7.1 – 7.4 (3 errors)

Line	Error
7	Syntax error, unexpected T_STRING, expecting T_VARIABLE on line 7
13	Syntax error, unexpected T_DOUBLE_ARROW on line 13
14	Syntax error, unexpected T_DOUBLE_ARROW on line 14

[phpstan-bot]

@ondrejmirtes After the latest push in 1.8.x, PHPStan now reports different result with your code snippet:

@@ @@
 ==========
 
 13: Dumped type: class-string<A>|class-string<B>
-19: Dumped type: class-string<A>|class-string<B>
-25: Dumped type: class-string<A>|class-string<B>
+19: Dumped type: class-string<B>
+25: Dumped type: *NEVER*
 
 PHP 7.1 – 8.0 (1 error)
 ==========
 
  8: Syntax error, unexpected T_STRING, expecting T_VARIABLE on line 8

Full report

PHP 8.1 – 8.2 (3 errors)

Line	Error
13	`Dumped type: class-string
19	Dumped type: class-string<B>
25	Dumped type: *NEVER*

PHP 7.1 – 8.0 (1 error)

Line	Error
8	Syntax error, unexpected T_STRING, expecting T_VARIABLE on line 8

[Dgame]

@Dgame After the latest push in 1.8.x, PHPStan now reports different result with your code snippet:

@@ @@
-PHP 8.1 – 8.2 (2 errors)
+PHP 8.1 – 8.2 (1 error)
 ==========
 
-12: Match expression does not handle remaining values: class-string<A>|class-string<B>
 17: Match expression does not handle remaining value: class-string
 
 PHP 8.0 (1 error)

Full report

I guess that is part of #7746 yes? 🙂

[ondrejmirtes]

Yes :)

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.