-
-
Notifications
You must be signed in to change notification settings - Fork 34.5k
gh-106102: Fix MRO resolution for nested generic classes #108415
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
base: main
Are you sure you want to change the base?
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 | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1393,7 +1393,11 @@ def __mro_entries__(self, bases): | |||||||||||||
| return () | ||||||||||||||
| i = bases.index(self) | ||||||||||||||
| for b in bases[i+1:]: | ||||||||||||||
| if isinstance(b, _BaseGenericAlias) and b is not self: | ||||||||||||||
| if b is self: | ||||||||||||||
| continue | ||||||||||||||
| if isinstance(b, _BaseGenericAlias): | ||||||||||||||
|
Member
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. Both of these diffs would make all tests pass:
Suggested change
Suggested change
But, they can't both be right ;) This indicates that we're missing some tests for some edge cases. 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. The first breaks when you use The second might be correct, I can't say off the top of my head. Not to say that I disagree on your conclusion that this indicates one or more test cases are missing. |
||||||||||||||
| return () | ||||||||||||||
| if Generic in getattr(b, '__mro__', ()): | ||||||||||||||
|
Member
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 still don't think this is the proper solution here. Exactly which dunder attributes
Member
Author
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 can say even more: I don't yet understand what ideal |
||||||||||||||
| return () | ||||||||||||||
|
AlexWaygood marked this conversation as resolved.
|
||||||||||||||
| return (self.__origin__,) | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Fix mro resolution for nested generic classes. |
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.
This feels wrong: Generic is before L in the bases, so it should be earlier in the MRO.
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.
But
LhasGenericin its own bases, so it has to come first. Either that or reject it.