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

Remove and refactor old overload selection logic #5321

Merged

Conversation

Michael0x2a
Copy link
Collaborator

This commit removes a few remnants of the old overload selection algorithm in checkexpr.

Specifically, this commit:

  1. Modifies erased_signature_similarity so it returns a bool instead of an int.

  2. Simplifies how erased_signature_similarity handles types like Type[X].

    Note: although this change relaxes and loosens the precision of erased_signature_similarity, it will not impact correctness: we now use this method exclusively as a heuristic to help us determine which overloads the user might have meant to select in our error messages.

    As a consequence, loosening this function actually led to a more helpful error message in one or two cases.

  3. Remove the match_signature_types method. It didn't appear like anybody was calling it.

This commit removes a few remnants of the old overload selection
algorithm in checkexpr.

Specifically, this commit:

1.  Modifies `erased_signature_similarity` so it returns a bool
    instead of an int.

2.  Simplifies how `erased_signature_similarity` handles types like
    `Type[X]`.

    Note: although this change relaxes and loosens the precision of
    `erased_signature_similarity`, it will not impact correctness:
    we now use this method exclusively as a heuristic to help us
    determine which overloads the user might have meant to select in
    our error messages.

    As a consequence, loosening this function actually led to a more
    helpful error message in one case.

3.  Remove the `match_signature_types` method. It didn't appear as if
    anybody was calling it.
@Michael0x2a Michael0x2a force-pushed the remove-old-overload-selection-algo branch from 3b88e61 to b8845ac Compare August 4, 2018 13:57
@Michael0x2a
Copy link
Collaborator Author

As a note, this PR is completely independent and unrelated to my other overload-related PRs.

It's also a pretty low-priority cleanup -- I don't think it's particularly important that we get this merged any time soon.

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.

Thanks! I have few minor comments.

(I know you said this is low priority PR, but it is already a month old, so I went ahead, it looks quite straightforward.)

def is_typetype_like(typ: Type) -> bool:
return (isinstance(typ, TypeType)
or (isinstance(typ, FunctionLike) and typ.is_type_obj())
or (isinstance(typ, Instance) and typ.type.fullname() == "builtins.object"))
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't expect object to be Type[...]-ish. Maybe this should be builtins.type? Or anything that is a metaclass instance (will have builtins.type in MRO)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, good point. I changed that to builtins.type.

main:5: note: Possible overload variants:
main:5: note: def __get__(self, inst: None, own: Type[Base]) -> D
main:5: note: def __get__(self, inst: Base, own: Type[Base]) -> str
main:5: error: Revealed type is 'd.D'
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure this is a good idea to have d.D instead of Any. In case of errors we typically return Any to avoid other (potentially bogus) errors.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think in this case, what's happening is that there's actually only one plausible matching overload -- when we do A.f and f is a descriptor, we always end up calling the overload def __get__(self, inst: None, own: Type[Base]) -> D.

So, since only one variant is a plausible match, we just infer that variant's return type. (And regarding your comment below -- this is also why we don't bother showing the possible overload variants here).

I think this behavior is consistent with how we handle bad calls to regular functions. E.g. mypy will infer the return type instead of Any in this program:

def foo(x: int) -> int: 
    return x

a = foo("asdf")   # Error
reveal_type(a)  # Revealed type is 'builtins.int', not Any

Granted, I don't think it's obvious that only one variant will be selected here, so I added another test case for this behavior.

Copy link
Member

Choose a reason for hiding this comment

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

OK.

main:5: note: def __get__(self, inst: None, own: Type[Base]) -> D
main:5: note: def __get__(self, inst: Base, own: Type[Base]) -> str
main:5: error: Revealed type is 'd.D'
main:5: error: Argument 2 to "__get__" of "D" has incompatible type "Type[A]"; expected "Type[Base]"
Copy link
Member

Choose a reason for hiding this comment

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

Why don't we have the same nicer error for the case right below? (This is very low priority.)

@Michael0x2a
Copy link
Collaborator Author

@ilevkivskyi -- this PR is also now ready for a second look whenever!

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.

LGTM

@ilevkivskyi ilevkivskyi merged commit 4e6d753 into python:master Aug 27, 2018
@Michael0x2a Michael0x2a deleted the remove-old-overload-selection-algo branch August 27, 2018 15:33
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.

2 participants