Skip to content

Force enum literals to simplify when inferring unions #7904

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

Closed

Conversation

Michael0x2a
Copy link
Collaborator

While working on overhauling #7169, I discovered that simply just "deconstructing" enums into unions leads to some false positives in some real-world code. This is an existing problem, but became more prominent as I worked on improving type inference in the above PR.

Here's a simplified example of one such error I ran into:

from enum import Enum

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

class Wrapper:
    def __init__(self, x: bool, y: Foo) -> None:
        if x:
            if y is Foo.A:
                # 'y' is of type Literal[Foo.A] here
                pass
            else:
                # ...and of type Literal[Foo.B] here
                pass
            # We join these two types after the if/else to end up with
            # Literal[Foo.A, Foo.B]
            self.y = y
        else:
            # ...and so this fails! 'Foo' is not considered a subtype of
            # 'Literal[Foo.A, Foo.B]'
            self.y = y

I considered three different ways of fixing this:

  1. Modify our various type comparison operations (is_same, is_subtype, is_proper_subtype, etc...) to consider Foo and Literal[Foo.A, Foo.B] equivalent.

  2. Modify the 'join' logic so that when we join enum literals, we check and see if we can merge them back into the original class, undoing the "deconstruction".

  3. Modify the make_simplified_union logic to do the reconstruction instead.

I rejected the first two options: the first approach is the most sound one, but seemed complicated to implement. We have a lot of different type comparison operations and attempting to modify them all seems error-prone. I also didn't really like the idea of having two equally valid representations of the same type, and would rather push mypy to always standardize on one, just from a usability point of view.

The second option seemed workable but limited to me. Modifying join would fix the specific example above, but I wasn't confident that was the only place we'd needed to patch.

So I went with modifying make_simplified_union instead.

The main disadvantage of this approach is that we still get false positives when working with Unions that come directly from the semantic analysis phase. For example, we still get an error with the following program:

x: Literal[Foo.A, Foo.B]
y: Foo

# Error, we still think 'x' is of type 'Literal[Foo.A, Foo.B]'
x = y

But I think this is an acceptable tradeoff for now: I can't imagine too many people running into this.

But if they do, we can always explore finding a way of simplifying unions after the semantic analysis phase or bite the bullet and implement approach 1.

While working on overhauling python#7169,
I discovered that simply just "deconstructing" enums into unions
leads to some false positives in some real-world code. This is an
existing problem, but became more prominent as I worked on improving
type inference in the above PR.

Here's a simplified example of one such problem I ran into:

```
from enum import Enum

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

class Wrapper:
    def __init__(self, x: bool, y: Foo) -> None:
        if x:
            if y is Foo.A:
                # 'y' is of type Literal[Foo.A] here
                pass
            else:
                # ...and of type Literal[Foo.B] here
                pass
            # We join these two types after the if/else to end up with
            # Literal[Foo.A, Foo.B]
            self.y = y
        else:
            # ...and so this fails! 'Foo' is not considered a subtype of
            # 'Literal[Foo.A, Foo.B]'
            self.y = y
```

I considered three different ways of fixing this:

1. Modify our various type comparison operations (`is_same`,
   `is_subtype`, `is_proper_subtype`, etc...) to consider
   `Foo` and `Literal[Foo.A, Foo.B]` equivalent.

2. Modify the 'join' logic so that when we join enum literals,
   we check and see if we can merge them back into the original
   class, undoing the "deconstruction".

3. Modify the `make_simplified_union` logic to do the reconstruction
   instead.

I rejected the first two options: the first approach is the most sound
one, but seemed complicated to implement. We have a lot of different
type comparison operations and attempting to modify them all seems
error-prone. I also didn't really like the idea of having two equally
valid representations of the same type, and would rather push mypy to
always standardize on one, just from a usability point of view.

The second option seemed workable but limited to me. Modifying join would
fix the specific example above, but I wasn't confident that was the only
place we'd needed to patch.

So I went with modifying `make_simplified_union` instead.

The main disadvantage of this approach is that we still get false
positives when working with Unions that come directly from the
semantic analysis phase. For example, we still get an error with
the following program:

    x: Literal[Foo.A, Foo.B]
    y: Foo

    # Error, we still think 'x' is of type 'Literal[Foo.A, Foo.B]'
    x = y

But I think this is an acceptable tradeoff for now: I can't imagine
too many people running into this.

But if they do, we can always explore finding a way of simplifying
unions after the semantic analysis phase or bite the bullet and
implement approach 1.
@Michael0x2a Michael0x2a force-pushed the force-enum-simplification branch from c68ddc9 to 68656e8 Compare November 8, 2019 21:39
@Michael0x2a
Copy link
Collaborator Author

Hmm, actually, now that I've had some more time to mull over this, I'm less convinced this is the correct approach.

Specifically, I was exploring some of the issues brought up in #4168 and came to the tentative conclusion that we should probably switch to making inferred union/literal types be an instance with a literal fallback instead of a literal. That would end up making this change much less useful, since joins would naturally work as expected.

I'll explore that approach first and close this PR if it seems that approach is viable. In the meantime, feel free to skip reviewing this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant