-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Allow overloaded overrides if they don't extend domain #5505
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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: | ||
return tp.arg_types[0] | ||
return None | ||
elif isinstance(tp, Overloaded): | ||
raw_items = [self.get_op_other_domain(it) for it in tp.items()] | ||
items = [it for it in raw_items if it] | ||
if items: | ||
return UnionType.make_simplified_union(items) | ||
return None | ||
else: | ||
assert False, "Need to check all FunctionLike subtypes here" | ||
|
||
def check_override(self, override: FunctionLike, original: FunctionLike, | ||
name: str, name_in_super: str, supertype: str, | ||
original_class_or_static: bool, | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we rename this variable to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point! |
||
if not is_subtype(override, original, ignore_pos_arg_names=True): | ||
fail = True | ||
elif (not isinstance(original, Overloaded) and | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not 100% sure, but I think we don't need this for |
||
# Operator method overrides cannot extend the domain, as | ||
# this could be unsafe with reverse operator methods. | ||
fail = True | ||
|
||
original_domain = self.get_op_other_domain(original) | ||
override_domain = self.get_op_other_domain(override) | ||
if (original_domain and override_domain and | ||
not is_subtype(override_domain, original_domain)): | ||
fail = True | ||
dunder_note = True | ||
if isinstance(original, FunctionLike) and isinstance(override, FunctionLike): | ||
if original_class_or_static and not override_class_or_static: | ||
fail = True | ||
|
@@ -1427,6 +1444,9 @@ def erase_override(t: Type) -> Type: | |
# Fall back to generic incompatibility message. | ||
self.msg.signature_incompatible_with_supertype( | ||
name, name_in_super, supertype, node) | ||
if dunder_note: | ||
self.note("Overloaded operator methods can't have wider argument types" | ||
" in overrides", node) | ||
|
||
def visit_class_def(self, defn: ClassDef) -> None: | ||
"""Type check a class definition.""" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4125,6 +4125,59 @@ main:11: error: Argument 7 to "f" has incompatible type "Union[int, str]"; expec | |
main:11: error: Argument 8 to "f" has incompatible type "Union[int, str]"; expected "int" | ||
main:11: note: Not all union combinations were tried because there are too many unions | ||
|
||
[case testSafeDunderOverlapInSubclass] | ||
from typing import overload | ||
|
||
class A: | ||
def f(self, x : 'A') -> 'A': ... | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh wait, it looks like I used There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually this test gets updated in this PR, it was passing but now fails, see |
||
|
||
class B(A): | ||
@overload | ||
def f(self, x : 'B') -> 'B': ... | ||
@overload | ||
def f(self, x : 'A') -> 'A' : ... | ||
def f(self, x): | ||
pass | ||
[out] | ||
|
||
[case testUnsafeDunderOverlapInSubclass] | ||
from typing import overload | ||
|
||
class A: | ||
def __add__(self, x : 'A') -> 'A': | ||
if isinstance(x, A): | ||
return A() | ||
else: | ||
return NotImplemented | ||
|
||
# This is unsafe override because of the problem below | ||
class B(A): | ||
@overload # E: Signature of "__add__" incompatible with supertype "A" \ | ||
# N: Overloaded operator methods can't have wider argument types in overrides | ||
def __add__(self, x : 'Other') -> 'B' : ... | ||
@overload | ||
def __add__(self, x : 'A') -> 'A': ... | ||
def __add__(self, x): | ||
if isinstance(x, Other): | ||
return B() | ||
elif isinstance(x, A): | ||
return A() | ||
else: | ||
return NotImplemented | ||
|
||
class Other: | ||
def __radd__(self, x: 'A') -> 'Other': | ||
if isinstance(x, A): | ||
return Other() | ||
else: | ||
return NotImplemented | ||
|
||
actually_b: A = B() | ||
reveal_type(actually_b + Other()) # E: Revealed type is '__main__.Other' | ||
# Runtime type is B, this is why we report the error on overriding. | ||
[builtins fixtures/isinstance.pyi] | ||
[out] | ||
|
||
[case testOverloadErrorMessageManyMatches] | ||
from typing import overload | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.