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 overload handling with union types in signatures #3300

Merged
merged 6 commits into from
Sep 6, 2017

Conversation

JukkaL
Copy link
Collaborator

@JukkaL JukkaL commented May 2, 2017

Previously None was not considered as more precise than
Optional[x], which was obviously incorrect. Fixed the
implementation of type precision checking to match the
description, now that proper subtype checking works.

Fixes the original example in #3295.

Previously `None` was not considered as more precise than
`Optional[x]`, which was obviously incorrect. Fixed the
implementation of type precision checking to match the
description, how that proper subtype checking works.

Fixes the original example in #3295.
mypy/subtypes.py Outdated
@@ -720,4 +720,4 @@ def is_more_precise(t: Type, s: Type) -> bool:
# Fall back to subclass check and ignore other properties of the callable.
return is_proper_subtype(t.fallback, s)
return is_proper_subtype(t, s)
return sametypes.is_same_type(t, s)
return is_proper_subtype(t, s)
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need a special case when t is CallableType and s is Instance? It looks like ProperSubtypeVisitor above does exactly the same. So that the whole function boils down to:

    if isinstance(s, AnyType):
        return True
    return is_proper_subtype(t, s)

Copy link
Member

Choose a reason for hiding this comment

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

Good point. I puzzled over this for a while and it really doesn't help that the visitor uses left/right while this function uses t/s.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated.

@gvanrossum
Copy link
Member

I had a similar testcase based on #3295 which still gives an error:

from typing import overload, Optional

@overload
def foo(x: None) -> None:
    pass
@overload
def foo(x: int) -> int:
    pass
def foo(*args):
    pass

def f(a: Optional[int]):
    reveal_type(foo(a))  # Line 13
    if isinstance(a, int):
        reveal_type(foo(a))
    else:
        reveal_type(foo(a))  # Error here

With --strict-optional, the output is unchanged from master:

__tmp__.py:13: error: Revealed type is 'Any'
__tmp__.py:15: error: Revealed type is 'builtins.int'
__tmp__.py:17: error: Revealed type is 'Any'
__tmp__.py:17: error: "foo" does not return a value

@gvanrossum
Copy link
Member

@JukkaL This seems stalled waiting for action/response on your end.

@gvanrossum
Copy link
Member

@JukkaL, ping?

@JukkaL
Copy link
Collaborator Author

JukkaL commented Jun 28, 2017

This PR only addresses the first example in #3295. The more general issue requires a much more involved fix, and I think that it can be fixed in a separate PR.

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.

This is fine for me as a quick fix. A broader solution for #3295 can be in a separate PR

@JukkaL
Copy link
Collaborator Author

JukkaL commented Jun 28, 2017

Don't merge yet -- I'll check this with Dropbox internal codebases first.

@JukkaL
Copy link
Collaborator Author

JukkaL commented Jul 5, 2017

This doesn't work quite right when run against Dropbox internal codebases. I won't merge this until I've resolved the issue.

@JukkaL JukkaL merged commit a423d67 into master Sep 6, 2017
@gvanrossum gvanrossum deleted the union-overloading branch September 29, 2017 21:32
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