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

Overloads defined for None and object get inferred as Any #3757

Closed
benkuhn opened this issue Jul 22, 2017 · 7 comments
Closed

Overloads defined for None and object get inferred as Any #3757

benkuhn opened this issue Jul 22, 2017 · 7 comments

Comments

@benkuhn
Copy link
Contributor

benkuhn commented Jul 22, 2017

(This came up while playing with descriptors, where the first parameter of __get__ can be either None or an arbitrary object and you want different return types in the two cases.)

Example reproduction using mypy --strict-optional on 0.520:

from typing import *

@overload
def a_or_1(obj: None) -> str:
    ...

@overload
def a_or_1(obj: object) -> int:
    # If you change the type of `obj` to e.g. AnyStr, the bug goes away
    ...

def a_or_1(obj):
    if obj is None:
        return 'a'
    return 1

reveal_type(a_or_1('a string'))   # correctly revealed as 'builtins.int'
reveal_type(a_or_1(None))         # revealed as type Any (should be 'builtins.str')
@ilinum
Copy link
Collaborator

ilinum commented Jul 22, 2017

Thanks for reporting the issue!

This is a duplicate of #3256.

@ilinum ilinum marked this as a duplicate of #3256 Jul 22, 2017
@ilinum ilinum closed this as completed Jul 22, 2017
@benkuhn
Copy link
Contributor Author

benkuhn commented Jul 22, 2017

Hmm, really? Without much knowledge of the underlying machinery, that seems surprising to me--#3256 is only a problem without strict-optional, and this test case only makes sense with strict-optional enabled. Also, this seems to have to do with some kind of special-casing of object, and object isn't mentioned at all in #3256. #3256 seems like a design problem for how to cope with None causing type overlaps, whereas this seems like it should straightforwardly just work (the testOverloadWithNone test case in the current unit tests fails if you replace the int annotation with object).

Sorry if there's some deeper reason I'm missing as to why they're actually the same! I'm interested in knowing because I was planning to try to fix this today :)

@benkuhn
Copy link
Contributor Author

benkuhn commented Jul 22, 2017

Hmm, ok, it appears to actually be intentional that None and object overlap in overloads. There's a test case testNoneMatchesObjectInOverload in check-optional.test, introduced in #1836 to solve #1766. The code that handles it is a special case in checkexpr.overload_arg_similarity:2653:

    if isinstance(actual, NoneTyp):
        if not experiments.STRICT_OPTIONAL:
            # NoneTyp matches anything if we're not doing strict Optional checking
            return 2
        else:
            # NoneType is a subtype of object
            if isinstance(formal, Instance) and formal.type.fullname() == "builtins.object":
                return 2

I made a branch changing the return 2 to return 1 (meaning the types are compatible using type promotions), which fixes this issue and passes all other tests. But maybe that's an abuse of the meaning of "similarity" there? Let me know if this, or something similar, seems like a good direction.

@benkuhn
Copy link
Contributor Author

benkuhn commented Jul 23, 2017

Hey @ilinum or anyone else, wondering whether you have feedback on whether this is truly a duplicate, or whether my proposal (or something like it) seems like a good resolution?

@ilinum
Copy link
Collaborator

ilinum commented Jul 24, 2017

Sorry, I was away for the weekend, so didn't see your comments until just now.

Yep, you're right this is not a duplicate of the #3256. I missed the fact that your example fails with --strict-optional. Sorry!

Your fix seems reasonable, I think it could work. It's probably worth sending a PR and seeing what other say.

@ilevkivskyi
Copy link
Member

Oops, wrong place, deleting the comment.

@Michael0x2a
Copy link
Collaborator

Update: The original example still won't typecheck (by design), but we recently added code to master that special-cases and allows this kind of overload for specifically descriptors -- see #5093 for examples.

So, I think we can probably close this now? Feel free to reopen if anybody disagrees.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants