-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Let the multiple inheritance checks consider callable objects as possible subtypes of usual functions (Fixes #14852). #14855
Changes from all commits
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 |
---|---|---|
|
@@ -668,3 +668,41 @@ class D1(B[str], C1): ... | |
class D2(B[Union[int, str]], C2): ... | ||
class D3(C2, B[str]): ... | ||
class D4(B[str], C2): ... # E: Definition of "foo" in base class "A" is incompatible with definition in base class "C2" | ||
|
||
|
||
[case testMultipleInheritanceOverridingOfFunctionsWithCallableInstances] | ||
from typing import Any, Callable | ||
|
||
def dec1(f: Callable[[Any, int], None]) -> Callable[[Any, int], None]: ... | ||
|
||
class F: | ||
def __call__(self, x: int) -> None: ... | ||
|
||
def dec2(f: Callable[[Any, int], None]) -> F: ... | ||
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 give it a more descriptive name, e.g. 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. Do you mean 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. Yeah, no strong opinion TBH. After I looked at this test for a few minutes, I got acquainted with "dec1", "dec2", "A", "B" etc. It's just that on first glance dec1 and dec2 seemed less than descriptive, but they'd be perfect names in Go :) 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. Never worked with Go. Maybe I should try it... 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. (https://go.dev/talks/2014/names.slide does a good job at describing what's considered good style there) 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.
|
||
|
||
class B1: | ||
def f(self, x: int) -> None: ... | ||
|
||
class B2: | ||
@dec1 | ||
def f(self, x: int) -> None: ... | ||
|
||
class B3: | ||
@dec2 | ||
def f(self, x: int) -> None: ... | ||
|
||
class B4: | ||
f = F() | ||
|
||
class C12(B1, B2): ... | ||
class C13(B1, B3): ... # E: Definition of "f" in base class "B1" is incompatible with definition in base class "B3" | ||
class C14(B1, B4): ... # E: Definition of "f" in base class "B1" is incompatible with definition in base class "B4" | ||
class C21(B2, B1): ... | ||
class C23(B2, B3): ... # E: Definition of "f" in base class "B2" is incompatible with definition in base class "B3" | ||
class C24(B2, B4): ... # E: Definition of "f" in base class "B2" is incompatible with definition in base class "B4" | ||
class C31(B3, B1): ... | ||
class C32(B3, B2): ... | ||
class C34(B3, B4): ... | ||
class C41(B4, B1): ... | ||
class C42(B4, B2): ... | ||
class C43(B4, B3): ... |
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 keep wondering if this could be somehow generalized, e.g. check compatibility between any callables the same way, with simple functions being "upgraded" to a synthetic Instance with a
__call__
member. I didn't check the feasibility of this too much, though.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.
Yes, there are possibly more places where similar checks are performed (or should be performed).
A quick search for
find_member("__call__",
: five hits that seem to serve the same or similar purposes besides my own one.However, there seem to be many small differences. So, refactoring it might not be simple.
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 had a second look, just searching for
"__call__"
. 51 hits. Many special cases. I have no idea how far a generalisation like the one you suggested would be helpful. And it would be too much effort for me, currently.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.
Yeah, totally understood. BTW I'm just trying to help with reviews, I'm not a maintainer.
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 see. Do you know how to proceed? Besides this pull request (which I really would like to land in Mypy 1.2), I have two older ones open (#14440 and #14390). I'm not sure who to ping when and how...