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

Make overloads respect keyword-only args #4912

Conversation

Michael0x2a
Copy link
Collaborator

@Michael0x2a Michael0x2a commented Apr 14, 2018

This pull request resolves #1907.

Specifically, it turned out that support for non-positional args in overload was never implemented to begin with. Thankfully, it also turned out the bulk of the logic we wanted was already implemented within mypy.subtypes.is_callable_subtype. Rather then re-implementing that code, this pull request refactors that function to support any kind of check instead of specifically subtype checks.

This, as a side-effect, ended up making some partial progress towards #4159 -- this is because unlike the existing checks, mypy.subtypes.is_callable_subtype doesn't erase types and has better support for typevars in general.

The reason this commit doesn't fully remove type erasure from overload checks is because the new implementation still calls mypy.meet.is_overlapping_types which does perform erasure. But fixing that seemed out-of-scope for this commit, so I stopped here.

Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

A general comment, I would rather recommend spending some time in writing a strict specification of how overloads should work, otherwise these PRs feel a bit unsystematic. Also unfortunately I think we will not be able to review this before 0.600 is released.

from foo import *
[file foo.pyi]
from typing import overload, TypeVar
AnyStr = TypeVar('AnyStr', bytes, str)

@overload
def f(x: AnyStr) -> None: pass # E: Overloaded function signatures 1 and 2 overlap with incompatible return types
def f(x: AnyStr) -> None: pass
Copy link
Member

Choose a reason for hiding this comment

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

This example is not very useful, I would rather check what happens for (AnyStr) -> AnyStr.

@ilevkivskyi ilevkivskyi self-assigned this Apr 22, 2018
@Michael0x2a
Copy link
Collaborator Author

@ilevkivskyi -- I updated the test, fyi.

A general comment, I would rather recommend spending some time in writing a strict specification of how overloads should work, otherwise these PRs feel a bit unsystematic.

I think my strategy was to stick to making changes that improve the status quo, avoid making drastic changes, and steadily iterate before trying to nail down a specification. (E.g. regardless of how we handle overloads, it'd be nice if they understood keyword-only arguments; partially removing the erasure thing was just a nice side-effect that also improves the status quo).

That said, I don't mind trying to write a specification and pushing for more systemic changes if you + the mypy team think it's a good idea -- I just wasn't sure if anybody had the bandwidth to discuss/review any proposed changes.

But if you think I should go for it anyways, let me know and I can give it a shot :)

@ilevkivskyi
Copy link
Member

Specifying how exactly overloads should work is a long standing issue, I was going to work on this myself anyway, also I think other will agree it is an important thing to fix before PEP 484 is no more provisional, see python/typing#253

Concerning the bandwidth, discussing the specification will take less energy (at least for me) than reviewing code without specification. Also I think @JukkaL mentioned some time ago that specification and implementation should be developed in parallel, so that we don't specify something that is hard to implement. In this view you can use your PRs to test various aspects of the future proposed spec.

@Michael0x2a Michael0x2a force-pushed the make-overload-handle-args-and-kwargs branch from ab76984 to 75dae80 Compare April 23, 2018 22:13
This commit resolves python#1907.

Specifically, it turned out that support for non-positional args in
overload was never implemented to begin with. Thankfully, it also turned
out the bulk of the logic we wanted was already implemented within
`mypy.subtypes.is_callable_subtype`. Rather then re-implementing that
code, this commit refactors that method to support any kind of check,
instead of specifically subtype checks.

This, as a side-effect, ended up making some partial progress towards
python#4159 -- this is because unlike
the existing checks, `mypy.subtypes.is_callable_subtype` *doesn't* erase
types and has better support for typevars in general.

The reason this commit does not fully remove type erasure from overload
checks is because the new implementation still calls
`mypy.meet.is_overlapping_types` which *does* perform erasure. But
fixing that seemed out-of-scope for this commit, so I stopped here.
@Michael0x2a
Copy link
Collaborator Author

Superseded by #5084.

@Michael0x2a Michael0x2a deleted the make-overload-handle-args-and-kwargs branch July 9, 2018 04:18
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.

Wrong checking of overload for keyword-only args
2 participants