Allow overloaded overrides if they don't extend domain #5505
Conversation
| @@ -1341,6 +1341,20 @@ def check_method_override_for_base_with_name( | |||
| self.msg.signature_incompatible_with_supertype( | |||
| defn.name(), name, base.name(), context) | |||
|
|
|||
| def get_op_other_domain(self, tp: FunctionLike) -> Optional[Type]: | |||
| if isinstance(tp, CallableType): | |||
| if tp.arg_kinds and tp.arg_kinds[0] == ARG_POS: | |||
There was a problem hiding this comment.
Do we care what happens if the user does def __add__(self, *args: Foo) -> Foo or something?
It's technically not illegal, and I think we even have one or two tests existing tests somewhere that check for this sort of thing in operator methods.
There was a problem hiding this comment.
I think we don't care for now, I would say this kind of situations is already rare, and detecting a rare unsafety due to extended domain is essentially rare ** 2.
mypy/checker.py
Outdated
| @@ -1357,15 +1371,18 @@ def check_override(self, override: FunctionLike, original: FunctionLike, | |||
| """ | |||
| # Use boolean variable to clarify code. | |||
| fail = False | |||
| dunder_note = False | |||
There was a problem hiding this comment.
Can we rename this variable to op_method_wider_note or something else that makes it clear this is related to operator methods, not dunder methods in general? (E.g. we don't display this note for dunder methods like __len__ or __init__.)
| from typing import overload | ||
|
|
||
| class A: | ||
| def f(self, x : 'A') -> 'A': ... |
There was a problem hiding this comment.
Maybe we should also add a unit test to see what happens if the parent class is also defining an overloaded operator? Up to you.
There was a problem hiding this comment.
Oh wait, it looks like I used f instead of __add__, I will switch to __add__. Btw the test case you mention already exists in other file, so I will just keep this one.
There was a problem hiding this comment.
Actually this test gets updated in this PR, it was passing but now fails, see testOverloadedOperatorMethodOverrideWithNewItem.
| isinstance(override, Overloaded) and | ||
| self.is_forward_op_method(name)): | ||
| # Operator method overrides cannot introduce overloading, as | ||
| elif isinstance(override, Overloaded) and self.is_forward_op_method(name): |
There was a problem hiding this comment.
While we're at it, do you think we also need to perform this same check for reverse operator methods?
(I'm actually not sure myself. I spent a little bit of time trying to come up with an example but couldn't think of anything, but that's not a particularly convincing argument.)
There was a problem hiding this comment.
I am not 100% sure, but I think we don't need this for __rX__. It seems to me that we can check unsafe overlap only for one, and not extending the domain only for other. This however probably only applies for simple cases where the runtime logic is simple: first check __X__ if it doesn't work, go to __rX__. But then again, the rare ** 2 argument probably applies here.
|
@Michael0x2a Thanks for review! Are you OK with my responses to comments? |
|
Yup, looks good to me -- I'll go ahead and merge this in. |
Fixes #4985
Mypy previously disallowed introducing overloads for operator methods in overrides because this can interact unsafely with reverse methods. However it looks like this is safe for the situations where the domain of the override is not extended.
In fact the same unsafety exists for non-overloaded op-methods (see example in #4985 (comment)), but it looks like we are fine with this, I have found few existing tests explicitly testing for this.