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 overload impl checks correctly handle TypeVars and untyped impls #5236

Merged
merged 2 commits into from Jun 25, 2018

Conversation

Projects
None yet
2 participants
@Michael0x2a
Copy link
Collaborator

Michael0x2a commented Jun 19, 2018

This pull request fixes #4619 as well as an unrelated bug where if the overload implementation was untyped, we only check to see if there are unsafe overlaps between the first overload alternative and the rest.

Make overload impl checks correctly handle TypeVars and untyped impls
This pull request fixes #4619 as
well as an unrelated bug where if the overload implementation was untyped,
we only check to see if there are unsafe overlaps between the *first*
overload alternative and the rest.

@Michael0x2a Michael0x2a force-pushed the Michael0x2a:fix-overloads-impl-checks branch from 69b06a5 to 2d604fa Jun 21, 2018

@Michael0x2a Michael0x2a requested a review from ilevkivskyi Jun 21, 2018

@ilevkivskyi ilevkivskyi self-assigned this Jun 21, 2018

@ilevkivskyi
Copy link
Collaborator

ilevkivskyi left a comment

Thanks, looks good! I have only one comment.

is_compat=is_subtype,
ignore_return=True):
ignore_return=True,
unify_generics=False):

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 25, 2018

Collaborator

This function has already too many arguments. Is there a way to avoid unify_generics? Like if you already unified above, the unification in is_callable_compatible will be a no-op?

This comment has been minimized.

@Michael0x2a

Michael0x2a Jun 25, 2018

Author Collaborator

Huh, that's weird. I could have sworn we needed this parameter, but I tried removing it and everything still worked. Maybe it was a holdover from one of my previous attempts at tackling the bug?

Anyways, I updated the PR to remove this param. Regarding the number of arguments: I'm planning on refactoring and splitting up is_callable_compatible in the upcoming PR for improving error messages -- that should help us cut down on the number of params/ad-hoc special casing.

@ilevkivskyi
Copy link
Collaborator

ilevkivskyi left a comment

OK, thanks! I will merge after some more testing.

@ilevkivskyi ilevkivskyi merged commit c5de2fd into python:master Jun 25, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Michael0x2a Michael0x2a deleted the Michael0x2a:fix-overloads-impl-checks branch Jun 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.