Repository navigation
Remove and refactor old overload selection logic - #5321
Conversation
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.
3b88e61 to
b8845ac
Compare
|
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. |
ilevkivskyi
left a comment
ilevkivskyi
left a comment
There was a problem hiding this comment.
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")) |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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' |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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]" |
There was a problem hiding this comment.
Why don't we have the same nicer error for the case right below? (This is very low priority.)
|
@ilevkivskyi -- this PR is also now ready for a second look whenever! |
2 participants
This commit removes a few remnants of the old overload selection algorithm in checkexpr.
Specifically, this commit:
Modifies
erased_signature_similarityso it returns a bool instead of an int.Simplifies how
erased_signature_similarityhandles types likeType[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.
Remove the
match_signature_typesmethod. It didn't appear like anybody was calling it.