Regression with default match pattern for Never type on 1.1.296

[bellini666]

Take this simple example with reportUnnecessaryComparison set to warning

from typing import assert_never

def get_foo() -> int | str:
    ...

foo = get_foo()

match foo:
    case int():
        ...
    case str():
        ...
    case _:  # Pattern will never be matched for subject type "Never" (reportUnnecessaryComparison)
        assert_never(foo)

This was working fine on previous versions.

[erictraut]

This is working as designed. It's a new feature introduced in 1.1.296. The reportUnnecessaryComparison diagnostic check now applies to unnecessary case statements that will never be matched. In your code sample, the case _: is completely unnecessary because it will never be hit. There's also no need to use assert_never with match statements if you use reportMatchNotExhaustive.

If you don't like this behavior, you can disable reportUnnecessaryComparison.

[bellini666]

I understand and like the reportUnnecessaryComparison diagnostic for case statements that will never be matched (e.g. in the example code, if I had a case float() or case anything it would be useful).

But I don't think it should apply to case _, as it prevents that kind of defensive code. In that specific example, if get_foo happens to return something that is not an int or str, it would be nice to get a runtime error (before it existed I used to raise AssertionError at that line)

For 1.1.296 my only options are:

1. To disable reportUnnecessaryComparison, which I don't want to do because it provides me a lot of benefits that I don't want to lose
2. Add type: ignore on all those case _ lines, which is acceptable but really annoying
3. Rely on reportMatchNotExhaustive, but I would also have to add a if not isinstance(foo, (int, str)) before or after the match if I want to ensure the type at runtime, which would also raise another issue for reportUnnecessaryIsInstance

[erictraut]

The reportUnnecessaryComparison assumes that static types are correct and accurately reflect the runtime types. If you don't want to make that assumption and feel the need to add additional defensive runtime checks, then it probably makes sense to disable reportUnnecessaryComparison globally in your project.

[bellini666]

If you don't want to make that assumption and feel the need to add additional defensive runtime checks

For my own code I have almost all reports enabled and do my best to avoid having to rely on type: ignore specifically to be able to have that assumption.

Still, there's no way to be sure that that assumption is 100% correct. We have 3dy party libs that might do something wrong here and there, we have external data coming in, and even unexpected behaviours that may happen from time to time. That is also the reason why assert_never raises a runtime error instead of just serving as a marker for static type checkers, like assert_type

Also, this is the same as doing the following, which doesn't report anything:

if isinstance(foo, int):
    ...
elif isinstance(foo, str):
    ...
else:
    assert_never(foo)

Yes, I know it is because the else is not a comparison, but case _ is in fact an else, it should match "anything else that wasn't matched before"

[bellini666]

This kind of pattern is exampled in the assert_never documentation [1] and also suggested as well in the typing documentation [2]

[1] https://docs.python.org/3/library/typing.html#typing.assert_never
[2] https://typing.readthedocs.io/en/latest/source/unreachable.html#assert-never-and-exhaustiveness-checking

[erictraut]

Yes, it is important to do this if your type checker doesn't support exhaustion checking. I suppose I could conditionalize this diagnostic based on whether reportMatchNotExhaustive is enabled (i.e. don't report an unnecessary comparison for wildcards if reportMatchNotExhaustive is disabled). Do you enable reportMatchNotExhaustive in your project? If so, why do you also use a redundant runtime check?

[erictraut]

Another thing you could do if you are worried that you may be receiving incorrectly-typed values from libraries is to add an assert that validates your assumptions.

assert isinstance(foo, (int, str))

This way, you know that the runtime types and static types match, and all of subsequent static type analysis is valid. The wildcard case statement and assert_never is then unnecessary.

[bellini666]

Do you enable reportMatchNotExhaustive in your project? If so, why do you also use a redundant runtime check?

Yes I do. The redundant runtime check is for defensive purposes. Before assert_never I used to do a raise AssertionError(f"unexpected type {foo}") or something like that, for both match cases and also the if/elif/else example I have. The assert_never is just an easier way to do the same that also happens to do some static type checking.

Another thing you could do if you are worried that you may be receiving incorrectly-typed values from libraries is to add an assert that validates your assumptions.

Yes it would work, but not only that is an extra isinstance for something that theoretically should never happen (each if/elif/case will do one by themselves for each type), IMHO is unnecessary based on the fact that the case _ (or the else in the other example) could be used exactly for that.

Also, It is not that I'm afraid that the runtime will not match the static type. 99% of times they will, but if something happens and they don't match, I prefer to abort the operation with an error that is easy to understand the cause than having it cause another error (or worst, don't cause any error).

2 examples using the example:

1. If I create a variable inside each of those case statements. When trying to use it after it will be there for sure, because I tested all possibilities. But if something goes wrong and nothing matches, I'll get an unbound variable error.

2. The worst kind of error: I do some operation inside each case and follow my code assuming the correct operation happened. The lack of the operation might not generate a runtime error, but will leave me later with possibly wrong data or generate other errors in the code chain that follows, which would make it hard to trace back to it, considering that from the static type checking "everything is fine"

The first example is not that bad as it is easy to understand what might have happened (even though having a better error message is nice). Now the second one is the one I want to prevent and be defensive about.

[erictraut]

The question is whether matching against a wildcard pattern should be flagged as an error when static analysis indicates that such a match will never occur. Such code is likely indicative of a bug, or so the thinking goes.

You're arguing that there's a legitimate use case for matching against a wildcard pattern (but presumably not other patterns?) even when static analysis indicates that such a match is impossible. This is because you can't trust that your static types match your runtime types, and you want to add extra logic to catch this case for defensive purposes. This argument strikes me as a bit weak. If your static types cannot be trusted, then defensive programming in a match statement is the least of your worries.

I prefer simple, straightforward rules without exceptions. If there is an exception to a rule, it had better have a good justification — and a principle to back it up. You're arguing here that a wildcard pattern should be treated as an exception for purposes of reportUnnecessaryComparison checking. I guess I'm not yet convinced that an exception here is merited based on your personal usage pattern. I could be convinced if enough other pyright users indicated that they were doing something similar, but I suspect that your usage is somewhat unusual (perhaps unique) here.

I think the options are:

1. Add special-case logic to exempt wildcard patterns from this check. (This will likely result in future questions and bug reports, as it introduces an inconsistency, so I'm somewhat reluctant to do it.)
2. Move the unnecessary case pattern matching to a separate diagnostic rule (reportUnnecessaryPatternMatch?) so you can disable it independently of the other diagnostics in reportUnnecessaryComparison.
3. Leave the behavior as is for now and see if we receive feedback from other pyright users. In the meantime, you'd need to decide how you want to work around the issue in your code: # type: ignore, removal of your defensive code (perhaps with the addition of an assert, as I suggested above), or disabling of reportUnnecessaryComparison.

The default answer is #3.

[bellini666]

You're arguing that there's a legitimate use case for matching against a wildcard pattern (but presumably not other patterns?) even when static analysis indicates that such a match is impossible. This is because you can't trust that your static types match your runtime types, and you want to add extra logic to catch this case for defensive purposes. This argument strikes me as a bit weak. If your static types cannot be trusted, then defensive programming in a match statement is the least of your worries.

That's not what I said. I can and I do trust my static typing, but IMO I think this extra layer of defensive programming is nice to have.

As I said, there are external libs that might misbehave, external data (e.g. those coming from external requests) that might also misbehave. My own code can also be trusted, but I don't know who is going to be developing/maintaining it in the future.. Someone might type: ignore or cast a call to a function that uses match and introduce an error by mistake, which might fall into the situation 2 that I mentioned.

I prefer simple, straightforward rules without exceptions. If there is an exception to a rule, it had better have a good justification — and a principle to back it up. You're arguing here that a wildcard pattern should be treated as an exception for purposes of reportUnnecessaryComparison checking. I guess I'm not yet convinced that an exception here is merited based on your personal usage pattern. I could be convinced if enough other pyright users indicated that they were doing something similar, but I suspect that your usage is somewhat unusual (perhaps unique) here.

I also prefer that, but I don't think that this is an exception. The match _ is basically a syntactic sugar for an else, which also has a code that would never be executed. Any other matches that is not part of the possible types should be reported and is probably an indication of a bug.

And I disagree that it is "my personal usage pattern". As I mentioned here, that pattern is described in the docs for assert_never and also in the official typing documentation exactly the same as in my example.

I think the options are:

I think 1 is the way to go, but I understand your reluctance. Although I really think it is the other way around (e.g. reason I opened this issue). I base that on the fact that the python docs suggests that pattern and people knowing about assert_never would probably use it exactly for that kind of code, but I might be wrong.

I don't think 2 is necessary as I can just do 3. I'm actually doing 3 right now, which is just an annoyance and not the end of the world.

[roman-g]

@erictraut
I also was a bit confused after getting this update.

I understand that reportMatchNotExhaustive does the job, but I believe assert_never is a widespread practice. Abandoning it could make potential switching to another type checker more dangerous (eg, as far as I know, mypy doesn't provide a behaviour like reportMatchNotExhaustive and suggests relying on assert_never), as well as it adds additional work when switching a large existing codebase to pyright. Disabling reportUnnecessaryComparison is also not an option, because it provides lots of benefits in other cases.

So, this pyright enhancement forces us to write code that's a sort of "incompatible" with other type checkers relying on default language tools.

[erictraut]

There is an open issue in the mypy issue tracker that calls for the addition of a disallow-non-exhaustive-match check. If this is something you'd like to see, please upvote it.

I think your argument is "pyright should not implement a check that will catch legitimate bugs because someone might hypothetically want to switch their code base from pyright to mypy at some point in the future, and mypy necessitates additional boilerplate code to detect match exhaustion because it's missing a feature that pyright includes". That argument doesn't sound very compelling to me.

That said, I think it's reasonable to move pyright's "unnecessary case statement" logic to a separate diagnostic rule (say, reportUnnecessaryCase) so you can selectively disable it if you don't want to catch this class of bugs and would prefer to retain the extra assert_never code for the benefit of mypy. Would that meet your needs?