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.

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

Fix make_simplified_union for instances with last_known_value #10373

Merged
merged 1 commit into from
Apr 30, 2021

Conversation

freundTech
Copy link
Contributor

@freundTech freundTech commented Apr 26, 2021

Description

This fixes make_simplified_union if multiple instances of the same type but with different last_known_values are passed to it.
Previously it would falsely keep the last_known_value of the first occurrence:

make_simplified_union([Literal[1]?, Literal[2]?])   # Results in Literal[1]?

I'm not sure if this can currently actually occur at runtime, but I ran into this problem while developing #10191 and decided to submit it as a separate PR, as it's not directly related to #10191.

More specifically I need it for match statements such as

match m:
    case 1 | 2 as num:
        reveal_type(num)  # Union[Literal[1]?, Literal[2]?]

Note that I need to use instances with last known value and can't use LiteralType as that would prevent changing the value of num.

With this change the last_known_value is taken into account before a type is removed from the union.

make_simplified_union([Literal[1]?, Literal[2]?])  # Union[Literal[1]?, Literal[2]?]
make_simplified_union([Literal[1]?, int])  # builtins.int
make_simplified_union([Literal[1]?, <nothing>])  # Literal[1]?

Test Plan

mypy current doesn't have tests for make_simplified_union, but the behavior is implicitly tested by other code using it.

This change causes a minor regression in one test, however that test was only working by pure chance before, as it used the fact that make_simplified_union([Literal[3]?, int]) returned Literal[3]?, which is obviously incorrect.

Copy link
Collaborator

@JukkaL JukkaL 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 the PR! Looks good.

It would be nice to have some unit tests for make_simplified_union. We have existing unit tests for some type operations in mypy.test.testtypes, for example. Would you be up to writing some unit tests for this new functionality as a separate PR?

@JukkaL JukkaL merged commit 7189a23 into python:master Apr 30, 2021
@freundTech
Copy link
Contributor Author

It would be nice to have some unit tests for make_simplified_union. We have existing unit tests for some type operations in mypy.test.testtypes, for example. Would you be up to writing some unit tests for this new functionality as a separate PR?

Done. See #10397

JukkaL pushed a commit that referenced this pull request May 5, 2021
This commit adds tests for make_simplified_union, including the new functionality 
added in #10373.
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.

2 participants