Skip to content

Fix overload handling with union types in signatures - #3300

Merged
JukkaL merged 6 commits into
masterfrom
union-overloading
Sep 6, 2017
Merged

Fix overload handling with union types in signatures#3300
JukkaL merged 6 commits into
masterfrom
union-overloading

Conversation

@JukkaL

@JukkaL JukkaL commented May 2, 2017

Copy link
Copy Markdown
Collaborator

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.

@JukkaL
Fix overload handling with union types in signatures
ae3accb 
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.
ilevkivskyi
ilevkivskyi reviewed May 2, 2017
View reviewed changes
Comment thread mypy/subtypes.py Outdated
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)

@ilevkivskyi ilevkivskyi May 2, 2017

Copy link
Copy Markdown
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)

@gvanrossum gvanrossum May 2, 2017

Copy link
Copy Markdown
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.

@JukkaL JukkaL Jun 28, 2017

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Updated.

@gvanrossum

gvanrossum commented May 2, 2017

Copy link
Copy Markdown
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

gvanrossum commented May 10, 2017

Copy link
Copy Markdown
Member

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

@gvanrossum

gvanrossum commented Jun 10, 2017

Copy link
Copy Markdown
Member

@JukkaL, ping?

@JukkaL

JukkaL commented Jun 28, 2017

Copy link
Copy Markdown
Collaborator Author

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.

ilevkivskyi
ilevkivskyi approved these changes Jun 28, 2017
View reviewed changes

@ilevkivskyi ilevkivskyi left a comment

Copy link
Copy Markdown
Member

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

JukkaL commented Jun 28, 2017

Copy link
Copy Markdown
Collaborator Author

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

@JukkaL

JukkaL commented Jul 5, 2017

Copy link
Copy Markdown
Collaborator Author

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 mentioned this pull request Jul 5, 2017
Closed
@JukkaL JukkaL mentioned this pull request Sep 4, 2017
Closed
@ilevkivskyi ilevkivskyi mentioned this pull request Sep 5, 2017
Merged
@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

Reviewers

@gvanrossum gvanrossum gvanrossum left review comments
@ilevkivskyi ilevkivskyi ilevkivskyi approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@JukkaL @gvanrossum @ilevkivskyi