Generalize reachability checks to support enums

[Michael0x2a]

Fixes #1803

This diff adds support for performing reachability and narrowing analysis when doing certain enum checks.

For example, given the following enum:

class Foo(Enum):
    A = 1
    B = 2

...this pull request will make mypy do the following:

x: Foo
if x is Foo.A:
    reveal_type(x)  # type: Literal[Foo.A]
elif x is Foo.B:
    reveal_type(x)  # type: Literal[Foo.B]
else:
    reveal_type(x)  # No output: branch inferred as unreachable

This diff does not attempt to perform this same sort of narrowing for equality checks: I suspect implementing those will be harder due to their overridable nature. (E.g. you can define custom __eq__ methods within Enum subclasses).

This pull request also finally adds support for the enum behavior described in PEP 484 and also sort of partially addresses #6366

[bluetech]

Yay :) I've tested it with pip install git+https://github.com/Michael0x2a/mypy.git@refine-enum-branch-analysis on some real cases and it works as expected.

This diff does not attempt to perform this same sort of narrowing for equality checks

I suppose in/__contains__ is the same? (We often use if status in (Foo.a, Foo.b) as a shortcut, but changing them to is won't be too bad).

[Michael0x2a]

This diff does not attempt to perform this same sort of narrowing for equality checks

I suppose in/contains is the same? (We often use if status in (Foo.a, Foo.b) as a shortcut, but changing them to is won't be too bad).

Yeah -- I only tried handling the blah is Enum.Foo case.

I was maybe thinking of taking a swing at supporting the blah == Enum.Foo and blah in (...) cases later: I think narrowing would be safe to do in those cases so long as the user doesn't define custom __eq__, __hash__, and __contains__ methods. (But I haven't thought about it too much yet.)

[ilevkivskyi]

I was maybe thinking of taking a swing at supporting the blah == Enum.Foo and blah in (...) cases later: I think narrowing would be safe to do in those cases so long as the user doesn't define custom __eq__, __hash__, and __contains__ methods. (But I haven't thought about it too much yet.)

I think we should support == and in checks if there are no custom methods. Also I think it is better to make it a separate PR (just so it is easier to review).

[ilevkivskyi]

Thank you for working in this! Mostly looks good, I have few comments.

Also I think the PR description should be updated to mention that this fixes #1803 (I will update it now unless you disagree.)

[ilevkivskyi]

This looks like a mystery to me. What if one has if a is b is c or even more operands?

[Michael0x2a]

We've actually never properly handled this case, I think. The old if-check on line 3490-ish lets this code run only if there's exactly only a single operator; the new if-check I'm replacing that with continues to do the same thing. So, as a consequence, we can safely assume there'll be exactly two operands at this point.

I have a fix for this, but I decided it might be better to submit it as a separate PR. Once I combined this with the equality changes mentioned above, the changes ended up being much more intrusive.

[ilevkivskyi]

The old if-check on line 3490-ish lets this code run only if there's exactly only a single operator

Maybe I am missing something, but the code there looks like it is about completely different case, it is about isinstance() and issubclass() having other number of arguments than two.

(Also it is in a different if branch, so it will not affect this branch).

[Michael0x2a]

Oh, maybe the line numbers shifted after I merged. It's now line 3541-ish.

The old check used to do this:

is_not = node.operators == ['is not']
if any(is_literal_none(n) for n in node.operands) and (
                is_not or node.operators == ['is']):

And the new checks do this:

is_not = node.operators == ['is not']
if (is_not or node.operators == ['is']) and len(operand_types) == len(node.operands):

We also make the same assumption when handling the == and in operators as well -- those are:

elif node.operators == ['==']:

and:

elif node.operators in [['in'], ['not in']]:

[ilevkivskyi]

How hard would be it be to do exactly the same for ==? (Mostly so that example in #4223 will not give the false positive.)

[Michael0x2a]

It's slightly trickier -- the semantics of a is SomeEnum.Foo and a == SomeEnum.Foo are different, unfortunately.

If a is something like an int or some other unrelated type, we know the first expression will always be False. But for the second, we have no idea since a could have defined a custom __eq__ function. SomeEnum itself could also have defined/inherited a custom __eq__ method, which would further complicate things.

I'll submit a separate PR for this: it ended up being easier to make this change if I also added support for chained operator comparisons at the same time (see below).

[ilevkivskyi]

Thanks for the updates! There is only one remaining question.

[ilevkivskyi]

The old if-check on line 3490-ish lets this code run only if there's exactly only a single operator

Maybe I am missing something, but the code there looks like it is about completely different case, it is about isinstance() and issubclass() having other number of arguments than two.

(Also it is in a different if branch, so it will not affect this branch).

[ilevkivskyi]

OK, thanks for explanation!