pylint-dev / pylint

It's not just a linter that annoys you!
https://pylint.readthedocs.io/en/latest/
GNU General Public License v2.0
5.18k stars 1.1k forks source link

Proposal - Check match case exhaustiveness for enums #8636

Open alonme opened 1 year ago

alonme commented 1 year ago

Current problem

I think that exhaustiveness checks may be possible for enums

from enum import Enum

class Color(Enum):
    RED = "red"
    GREEN = "green"
    BLUE = "blue"

def foo(color: Color):
    match color:
        case Color.RED:
            print(255,0,0)
        case color.GREEN:
            print(0,255,0)

Given that foo was written before BLUE was added to the enum, its very possible to encounter bugs, by forgetting to handle new enum values

Desired solution

I would like Pylint to detect a non-exhaustive match on an enum, and warn on that.

Additional context

  1. Ideally this should be extended to more types in the future
  2. The rust compiler has similar features for the rust match statement - https://doc.rust-lang.org/book/ch06-02-match.html#matches-are-exhaustive
alonme commented 1 year ago

If this feature makes sense to the maintainers (meaning you would accept it, and think its possible to achieve) - i would try and implement it

jacobtylerwalls commented 1 year ago

I'm interested in this proposal. Someone else requested it here.

Also, even if a "missing default case" check is introduced like eslint in the future, this would still be a nice complementary check. Folks could disable the more opinionated missing-default message and then rely on this check instead, like the above commenter.

Soft yes from me, but feel free to wait for another opinion before starting in earnest. Thanks for indicating an interest to contribute!

Pierre-Sassoulas commented 1 year ago

I also like this check. Thank you for opening the issue. I think this will require some specifications but this can be discussed around the functional tests during implementation.

DanielNoord commented 1 year ago

Doesn't assert_never with a type checker do this as well? It feels like we might want to defer to those tools since they have explicit syntax to indicate you want to check for it in a specific match statement and already have implemented the necessary logic.

Pierre-Sassoulas commented 1 year ago

I'm not familiar with assert_never, from what you said it seems it's something you have to opt-in to be warned ? Or are mypy/pyright already doing this ? (I don't know because we can't use match case when we had to be compatible with python 3.7 until very recentely

Trigger warning; extreme software engineer abuse or worst, python 3.5 on legacy system at work
😄).

DanielNoord commented 1 year ago
from enum import Enum
from typing_extensions import assert_never # or typing

class MyEnum(Enum):
    A = "A"
    B = "B"

def f(x: MyEnum):
    match x:
        case MyEnum.A:
            return True
        case _:
            assert_never(x)

Mypy and all other type checkers will warn that assert_never is not never since there is an actual case that might occur. See https://typing.readthedocs.io/en/latest/source/unreachable.html#assert-never-and-exhaustiveness-checking It also raises at runtime https://docs.python.org/3/library/typing.html#typing.assert_never.

jacobtylerwalls commented 1 year ago

Good find, I wasn't familiar with assert_never(). But I think we're mixing a couple things up. The point of pylint is to emit warnings for code smells. We can't say the same about remembering to add assert_never() where appropriate: that's adding to the cognitive burden of the reviewer, not subtracting from it.

Pierre-Sassoulas commented 1 year ago

Ok, it seems that this is something that you need to know and that pylint could warn/teach about. Feel like the "missing default case" could actually be a "missing assert_never case" check and become nicer, and that the check for exhaustiveness is still desirable.

DanielNoord commented 1 year ago

Good find, I wasn't familiar with assert_never(). But I think we're mixing a couple things up. The point of pylint is to emit warnings for code smells. We can't say the same about remembering to add assert_never() where appropriate: that's adding to the cognitive burden of the reviewer, not subtracting from it.

Agreed, and I would even like a (plugin) to suggest adding it. However, I think we might have trouble doing a good exhaustiveness check without also taking typing into account. Thus, I wonder if we can actually do this ourselves without investing a lot of time to make it working.

Pierre-Sassoulas commented 1 year ago

Maybe we could do the ""missing assert_never case" only and defer to type checker for exhaustiveness.

jacobtylerwalls commented 1 year ago

The OP's request was only for exhaustiveness with enums, not for a general exhaustivity check with mixed types and all.

alonme commented 1 year ago

Ideally i think this could also be great while checking for types, but i assumed that would be much more complex, so i thought that starting with Enums is a good case.

Regarding deferring this problem to a type checker - do you have any stats regarding how many users use Pylint without a type checker?

Also - aren't there many other warnings / errors in Pylint that could have been raised by mypy? (if these are older stuff, than it makes sense of course)

Pierre-Sassoulas commented 1 year ago

Regarding deferring this problem to a type checker - do you have any stats regarding how many users use Pylint without a type checker?

We do suggest to use a type checker in our doc alongside pylint. That being said pylint's inference is less useful with a type checker on a fully typed code base, so it's an argument in favor of doing the exhaustiveness check in pylint. (Because our users are more likely to be the one that do not have the possibility to activate a type checker yet.)

Also - aren't there many other warnings / errors in Pylint that could have been raised by mypy? (if these are older stuff, than it makes sense of course)

There are, in fact one of the worst offender is https://pylint.readthedocs.io/en/latest/user_guide/messages/warning/invalid-envvar-default.html, is a type check for one specific arg of one specific function. But as you said it's historical, we should not add this kind of check when a type checker is a thousand time more efficient than pylint. (And we might remove this kind of check for performance reason tbf).

Let's vote:

alonme commented 1 year ago

That being said pylint's inference is less useful with a type checker on a fully typed code base, so it's an argument in favor of doing the exhaustiveness check in pylint. (Because our users are more likely to be the one that do not have the possibility to activate a type checker yet.)

For me, this is the reason to implement the exhaustiveness check in pylint, for several projects i work on, pylint is used without a type checker.