Skip to content
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.

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

Improve Psalm's understanding of match exhaustiveness with union types? #8715

Open
Ocramius opened this issue Nov 17, 2022 · 8 comments
Open
Labels
enhancement match Issues related to the match expression

Comments

@Ocramius
Copy link
Contributor

Ocramius commented Nov 17, 2022

I was looking at #5835 and discussing with @DavideBicego about how psalm could analyze union type exhaustiveness in switch () conditions.

I don't know if this is idiomatic PHP, but take this example:

https://psalm.dev/r/33d7f97784

<?php

final class A { function foo(): void {} }
final class B { function bar(): void {} }
final class C { function baz(): void {} }

/** @var A|B|C $result */

return match (true) {
    $result instanceof A => 'branch for A',
    $result instanceof B => 'branch for B',
};
Psalm output (using commit 12f33fa): 

No issues!

Psalm cannot really determine whether this match () is exhaustive.

In the example above, I'd like the developer to be forced to declare a $result instanceof C => 'branch for C',, or get an error.

match () is the best candidate for "control structure that requires exhaustiveness": other ways/workarounds for writing this exist, but they may be confused as redundant code.

Therefore I raise 2 questions:

  • can psalm already handle match (true) { $a instanceof A => ... } expressions, and am I doing something wrong in my example?
  • if not, where can this be improved? I'd gladly try to throw some time at this problem.
@psalm-github-bot
Copy link

psalm-github-bot bot commented Nov 17, 2022

I found these snippets:

https://psalm.dev/r/33d7f97784
<?php

final class A { function foo(): void {} }
final class B { function bar(): void {} }
final class C { function baz(): void {} }

/** @var A|B|C $result */

return match (true) {
    $result instanceof A => 'branch for A',
    $result instanceof B => 'branch for B',
};
Psalm output (using commit 12f33fa):

No issues!

@weirdan
Copy link
Collaborator

weirdan commented Nov 18, 2022

can psalm already handle match (true) { $a instanceof A => ... } expressions, and am I doing something wrong in my example?

Yes it can, as can be shown here: https://psalm.dev/r/4a97cdba7f .

From PHP standpoint, the only requirement is that at least one match arm should match. So this one is valid: https://psalm.dev/r/554f4b7d4e even though it does not cover all possible values for $a. This means we either need to generalize the exhaustiveness logic to those more complex cases (and I'm not sure how), or we have to guess the author's intent (which I would rather avoid).

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/4a97cdba7f
<?php

final class A { function foo(): void {} }
final class B { function bar(): void {} }
final class C { function baz(): void {} }

/** @var A|B|C $result */

match (true) {
    $result instanceof A => $result->foo(),
    $result instanceof B => $result->bar(),
    default => $result->bar()
};
Psalm output (using commit 12f33fa):

ERROR: UndefinedMethod - 12:25 - Method C::bar does not exist
https://psalm.dev/r/554f4b7d4e
<?php

final class A { function foo(): void {} }
final class B { function bar(): void {} }
final class C { function baz(): void {} }

/** @param int<10,max> $b */
function f(A|B|C $a, int $b): int {
    assert($b > 10);
    return match (true) {
        $a instanceof A || $a instanceof C => 1,
        $b > 100 => -5,
        $b >= 11 => 0
    };
}
Psalm output (using commit 12f33fa):

No issues!

@Ocramius
Copy link
Contributor Author

Yeah, that's why I was wondering about the approach, since I don't know if this is kosher:

match (true) {
   $input instanceof A => 'a',
   $input instanceof B => 'b',
}

It's hard to frame a pattern into a "teach the SA tooling to use it", when the pattern is not well defined.

@weirdan
Copy link
Collaborator

weirdan commented Nov 18, 2022

Something like this could be easier to teach Psalm about: https://psalm.dev/r/e5fb607dc6, but it won't work with inheritance.

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/e5fb607dc6
<?php

final class A { function foo(): void {} }
final class B { function bar(): void {} }
final class C { function baz(): void {} }

/** @var A|B|C $result */

match ($result::class) {
    A::class => $result->foo(),
    B::class => $result->bar(),
    default => $result->baz()
};
Psalm output (using commit 9d4c718):

ERROR: PossiblyUndefinedMethod - 12:25 - Method A::baz does not exist

@Ocramius
Copy link
Contributor Author

Yeah, I don't think match () on a class name is a good idea anyway :D

@weirdan
Copy link
Collaborator

weirdan commented Nov 20, 2022

Related: #4525

@weirdan weirdan added enhancement match Issues related to the match expression labels Aug 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement match Issues related to the match expression
Projects
None yet
Development

No branches or pull requests

2 participants