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

Improve open overloads when mode is a literal union #7428

Merged
merged 1 commit into from
Mar 6, 2022

Conversation

hauntsaninja
Copy link
Collaborator

@hauntsaninja hauntsaninja commented Mar 2, 2022

As pointed out by @gvanrossum in python/typing#1096

Improves type inference in cases when we know that mode is
OpenBinaryMode, but don't know anything more specific:

def my_open(name: str, write: bool):
    mode: Literal['rb', 'wb'] = 'wb' if write else 'rb'
    with open(name, mode) as f:
        reveal_type(f)  # previously typing.IO[Any], now typing.BinaryIO

You may be tempted into thinking this is some limitation of type
checkers. mypy does in fact have logic for detecting if we match
multiple overloads and union-ing up the return types of matched
overloads. The problem is the last overload interferes with this logic.
That is, if you remove the fallback overload (prior to this PR), you'd get
Union[io.BufferedReader, io.BufferedWriter] in the above example.

As pointed out by @gvanrossum in python/typing#1096

Improves type inference in cases when we know that mode is
OpenBinaryMode, but don't know anything more specific:
```
def my_open(name: str, write: bool):
    mode: Literal['rb', 'wb'] = 'wb' if write else 'rb'
    with open(name, mode) as f:
        reveal_type(f)  # previously typing.IO[Any], now typing.BinaryIO
```

You may be tempted into thinking this is some limitation of type
checkers. mypy does in fact have logic for detecting if we match
multiple overloads and union-ing up the return types of matched
overloads. The problem is the last overload interferes with this logic.
That is, if you remove the fallback overload (prior to this PR), you'd get
"Union[io.BufferedReader, io.BufferedWriter]" in the above example.
@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2022

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@gvanrossum
Copy link
Member

Oh, the previous overloads try to do clever things based on specific buffering values. I hadn't noticed that when I reported this. :-(

@hauntsaninja
Copy link
Collaborator Author

Most of the previous overloads still take default values for buffering, so their cleverness can mostly be ignored in the case where you don't pass anything in. The fix is still correct (afaict); the tricky part for me was realising why mypy's usual "union up the matching overloads" logic wasn't working.

@gvanrossum
Copy link
Member

I guess there are several cases:

  • No buffering specified
  • buffering is one of the literals in one of the previous overloads
  • buffering is an int

To validate correctness of the fix I recommend manually creating calls that exercise all of the above, combined with various values for mode, and make sure that the inferred result type is still the same.

Then repeat with various unions.

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Good catch

@JelleZijlstra JelleZijlstra merged commit 9796b9e into python:master Mar 6, 2022
@hauntsaninja hauntsaninja deleted the litopen branch March 21, 2022 21:31
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.

3 participants