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.

Sign up for GitHub

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

Jump to bottom

Recombine complete union of enum literals into original type (#9063) #9097

Merged
merged 1 commit into from
Mar 11, 2021
Merged

Recombine complete union of enum literals into original type (#9063) #9097

merged 1 commit into from
Mar 11, 2021

Conversation

ethan-leba
Copy link
Contributor

@ethan-leba ethan-leba commented Jul 5, 2020 •
edited
Loading

Closes #9063

@ethan-leba ethan-leba force-pushed the contract-union branch 2 times, most recently from 74be4c5 to 9ce8a55 Compare July 5, 2020 16:23
@ethan-leba ethan-leba marked this pull request as ready for review July 5, 2020 16:59
@ethan-leba ethan-leba mentioned this pull request Jul 5, 2020
Open
sobolevn
sobolevn reviewed Jul 8, 2020
View reviewed changes
Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome feature! 👍

@ethan-leba ethan-leba mentioned this pull request Aug 8, 2020
Closed
@ethan-leba
Copy link
Contributor Author

@JukkaL Is there any chance I could receive a review on this PR?

@TH3CHARLie TH3CHARLie self-requested a review March 2, 2021 23:46
TH3CHARLie
TH3CHARLie reviewed Mar 4, 2021
View reviewed changes
Copy link
Collaborator

@TH3CHARLie TH3CHARLie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! I like this cleaner message output. Left a few minor comments and/or uncertainties.

test-data/unit/check-enum.test Outdated
@@ -632,6 +632,7 @@ elif x is Foo.C:
reveal_type(x) # N: Revealed type is 'Literal[__main__.Foo.C]'
else:
reveal_type(x) # No output here: this branch is unreachable
reveal_type(y) # N: Revealed type is '__main__.Foo'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't make much sense to check the type of y here

test-data/unit/check-enum.test Outdated
@@ -641,6 +642,7 @@ elif Foo.C is x:
reveal_type(x) # N: Revealed type is 'Literal[__main__.Foo.C]'
else:
reveal_type(x) # No output here: this branch is unreachable
reveal_type(y) # N: Revealed type is '__main__.Foo'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

test-data/unit/check-enum.test Outdated
@@ -877,7 +889,7 @@ def func(x: Union[int, None, Empty] = _empty) -> int:
# E: Unsupported left operand type for + ("Empty") \
# N: Left operand is of type "Union[int, None, Empty]"
if x is _empty:
reveal_type(x) # N: Revealed type is 'Literal[__main__.Empty.token]'
reveal_type(x) # N: Revealed type is '__main__.Empty'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether should we display the original type when there's only one enum item. Displaying the only enum item seems like an acceptable result here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a fair point, I can add a special case in the logic for 1-item enums 👍

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool!

@ethan-leba ethan-leba requested a review from TH3CHARLie March 6, 2021 02:10
TH3CHARLie
TH3CHARLie approved these changes Mar 8, 2021
View reviewed changes
Copy link
Collaborator

@TH3CHARLie TH3CHARLie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@TH3CHARLie TH3CHARLie merged commit a503132 into python:master Mar 11, 2021
JukkaL added a commit that referenced this pull request Apr 30, 2021
@JukkaL
Make enum type compatible with union of all enum item literals
7a2798c 
For example, consider this enum:

```
class E(Enum):
    A = 1
    B = 1
```

This PR makes `E` compatible with `Literal[E.A, E.B]`.

Also fix mutation of the argument list in
`try_contracting_literals_in_union`.

This fixes some regressions introduced in #9097.
@JukkaL JukkaL mentioned this pull request Apr 30, 2021
Merged
JukkaL added a commit that referenced this pull request Apr 30, 2021
@JukkaL
Make enum type compatible with union of all enum item literals (#10388)
322ef60 
For example, consider this enum:

```
class E(Enum):
    A = 1
    B = 1
```

This PR makes `E` compatible with `Literal[E.A, E.B]`.

Also fix mutation of the argument list in
`try_contracting_literals_in_union`.

This fixes some regressions introduced in #9097.
@JukkaL JukkaL mentioned this pull request May 4, 2021
Closed
@hauntsaninja hauntsaninja mentioned this pull request Jul 5, 2021
Closed
@97littleleaf11 97littleleaf11 mentioned this pull request Oct 5, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sobolevn sobolevn sobolevn left review comments

@TH3CHARLie TH3CHARLie TH3CHARLie approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature request] Recombine complete union of enum literals into original type
3 participants
@ethan-leba @sobolevn @TH3CHARLie