gh-106102: Fix MRO resolution for nested generic classes by sobolevn · Pull Request #108415 · python/cpython

sobolevn · 2023-08-24T11:56:03Z

I went with Generic in getattr(...), because not all types are safe with issubclass, some of them might raise TypeError there.

Issue: cannot create consistent MRO for multiple generic inheritance #106102

sanderr · 2023-08-24T13:15:36Z

If the use of issubclass is unsafe, wouldn't that be an issue here as well?

AlexWaygood · 2023-08-24T20:29:40Z

+                if (
+                    (isinstance(b, _BaseGenericAlias)
+                        or Generic in getattr(b, '__mro__', ()))
+                    and b is not self
+                ):


This is no safer than using isinstance(b, type) and issubclass(b, Generic), which would be much more idiomatic. It's true that an issubclass(x, y) call could potentially call type(y).__subclasscheck__, which could potentially do anything, including raising a TypeError. But in this case, we know what y is -- it's Generic. So calling issubclass(b, Generic) is safe, as we know that type(Generic) is type, and we know that type.__subclasscheck__ never raises TypeError.

If the call was issubclass(Generic, b), then it would be a different question.

Also, I think this conditional is just a bit too complex to be readable:

Suggested change

if (

(isinstance(b, _BaseGenericAlias)

or Generic in getattr(b, '__mro__', ()))

and b is not self

):

if b is self:

continue

if isinstance(b, _BaseGenericAlias):

return ()

if isinstance(b, type) and issubclass(b, Generic):

return ()

Hmm, looks like I broke a test with this suggestion, but I can't really say I understand how.

This happens because in a case of class L2(Generic[S], L[S]): ... we would get b as L[S] which is a GenericAlias, it is not a type. So, your condition would be False. But, it still has __mro__, so the original one will work correctly.

We can also add

if isinstance(b, GenericAlias): return ()

But, it does not feel safe to me.

Could you elaborate on that? As far as I can see L2 is currently defined as inheriting from List, which is a typing._GenericAlias. Conversely, the built-in list is a types.GenericAlias. And both are actually a type as far as I can tell.

I played around with it a bit on Python 3.11.3 (not this branch):

>>> import typing >>> T = typing.TypeVar("T") >>> class LowercaseList(list[T]): pass ... >>> class UppercaseList(typing.List[T]): pass ... >>> LowercaseList.__mro__ (<class '__main__.LowercaseList'>, <class 'list'>, <class 'object'>) >>> UppercaseList.__mro__ (<class '__main__.UppercaseList'>, <class 'list'>, <class 'typing.Generic'>, <class 'object'>) >>> issubclass(LowercaseList, typing.Generic) False >>> issubclass(UppercaseList, typing.Generic) True >>> isinstance(LowercaseList, type) True >>> isinstance(UppercaseList, type) True >>> import types >>> isinstance(types.GenericAlias, type) True

That said, I did notice that the built-in list does not have Generic in its bases (see LowercaseList.__mro__ above). So I don't see why we would want to return () for them in the first place as that would just result in the absence of Generic in the MRO. What happens when you just use my initial suggestion? Does it actually raise a TypeError? For which types?

>>> from typing import TypeVar >>> T = TypeVar('T') >>> class L1(list[T]): ... ... >>> from typing import List >>> class L2(List[T]): ... ... >>> isinstance(L1[int], type) False >>> isinstance(L2[int], type) False

Indeed, I see. types.GenericAlias is not a subclass of type but it is supported in issubclass so that adding the isinstance(b, type) check changes the behavior.

Do I understand correctly that the patch as I initially proposed it doesn't actually result in a TypeError (except where the stable implementation does as well) because it doesn't really change when issubclass is called, and that your change is more a precaution against something that affects the current stable Python as well, i.e. more or less disjunct from #106102?

How about just trying the issubclass and catching the TypeError? It is generic in the sense that it doesn't assume anything about which objects are allowed in an issubclass check, unlike the isinstance(b, type) guard.

try: if issubclass(b, Generic): return () except TypeError: continue

@sanderr I tried it, didn't work :)

====================================================================== ERROR: test_multiple_inheritance_special_multiple_layers (test.test_typing.GenericTests.test_multiple_inheritance_special_multiple_layers) ---------------------------------------------------------------------- Traceback (most recent call last): File "/Users/sobolev/Desktop/cpython/Lib/test/test_typing.py", line 4878, in test_multiple_inheritance_special_multiple_layers class L2(Generic[S], L[S]): ... TypeError: Cannot create a consistent method resolution order (MRO) for bases Generic, L ---------------------------------------------------------------------- Ran 595 tests in 0.193s FAILED (errors=1, skipped=1) test test_typing failed test_typing failed (1 error) == Tests result: FAILURE == 1 test failed: test_typing Total duration: 321 ms Tests result: FAILURE

Thanks for that. I played around with the tests myself a bit and I think I understand it better now: L[S] is a types.GenericAlias with Generic in its MRO. The current logic (or my suggested patch) doesn't account for that possibility: it assumes that any type that has Generic in its MRO also has has typing._GenericAlias as its metaclass. But the L[S] type doesn't satisfy that assumption. Could you confirm this is the root cause of the issue, and the reason you took the approach of checking __mro__ instead of issubclass?

I wonder to what extent it's worth addressing this issue, because it seems to me that the assumption I mention above holds in all valid scenarios (Generic, subscripted or not, implies the corresponding metaclass) except for the typing.List etc types that somehow (I haven't figured out how exactly yet) get the corresponding built-in's metaclass when subclassing. Since this is a very niche issue that already exists on main and stable, and is contained to these deprecated types, what is Python's policy on fixing it if it clutters the implementation?

As a sanity check, I tried with the non-deprecated list instead of List and only had to change the order for a few of the assertions, and I think all of these make sense. This is on 026430f, so without the try-catch I suggested earlier.

diff --git a/Lib/test/test_typing.py b/Lib/test/test_typing.py index 0e8bdf1df0..6eafbeefd3 100644 --- a/Lib/test/test_typing.py +++ b/Lib/test/test_typing.py @@ -4856,27 +4856,27 @@ class B5(FromGenericAlias[S], GenericMixin[S], Generic[S]): ... def test_multiple_inheritance_special(self): S = TypeVar('S') class B(Generic[S]): ... - class C(List[int], B): ... + class C(list[int], B): ... self.assertEqual(C.__mro__, (C, list, B, Generic, object)) def test_multiple_inheritance_special_multiple_layers(self): S = TypeVar('S') - class L(List[S]): ... + class L(list[S]): ... class M(Generic[S]): ... # Direct subclasses: - class D1(List[S], Generic[S]): ... + class D1(list[S], Generic[S]): ... self.assertEqual(D1.__mro__, (D1, list, Generic, object)) - class D2(Generic[S], List[S]): ... - self.assertEqual(D2.__mro__, (D2, list, Generic, object)) + class D2(Generic[S], list[S]): ... + self.assertEqual(D2.__mro__, (D2, Generic, list, object)) # Nested subclasses: class L1(L[S], Generic[S]): ... self.assertEqual(L1.__mro__, (L1, L, list, Generic, object)) class L2(Generic[S], L[S]): ... - self.assertEqual(L2.__mro__, (L2, L, list, Generic, object)) + self.assertEqual(L2.__mro__, (L2, Generic, L, list, object)) class L3(L[S], M[S], Generic[S]): ... self.assertEqual(L3.__mro__, (L3, L, list, M, Generic, object))

bedevere-bot · 2023-08-24T20:30:13Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

requested changes were made

AlexWaygood · 2023-08-28T13:04:50Z

-                if isinstance(b, _BaseGenericAlias) and b is not self:
+                if b is self:
+                    continue
+                if isinstance(b, _BaseGenericAlias):


Both of these diffs would make all tests pass:

Suggested change

if isinstance(b, _BaseGenericAlias):

if isinstance(b, (_BaseGenericAlias, types.GenericAlias)):

Suggested change

if isinstance(b, _BaseGenericAlias):

if isinstance(b, types.GenericAlias):

b = b.__origin__

if isinstance(b, _BaseGenericAlias):

But, they can't both be right ;)

This indicates that we're missing some tests for some edge cases.

The first breaks when you use list instead of List (see diff here) because it excludes Generic if any of the following base types is types.GenericAlias while we should only exclude it if it is both that and has Generic in its MRO.

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.

sobolevn · 2023-08-28T13:36:00Z

I cannot say that I fully understand G* tests I've added 🤔

sanderr · 2023-08-28T13:39:02Z

I cannot say that I fully understand G* tests I've added 🤔

To my (admittedly limited) understanding they look good: list[T] is not Generic, therefore the normal MRO resolution applies.

AlexWaygood · 2023-08-28T13:39:19Z

+                    continue
+                if isinstance(b, _BaseGenericAlias):
+                    return ()
+                if Generic in getattr(b, '__mro__', ()):


I still don't think this is the proper solution here. Exactly which dunder attributes types.GenericAlias delegates to the "underlying" class has changed in the past. It could change again in the future. This feels to me like it's a fragile solution that could easily break.

I can say even more: I don't yet understand what ideal __mro__ should look like for all the cases I've added in tests.

JelleZijlstra · 2023-09-07T03:54:57Z

+        self.assertEqual(L1.__mro__, (L1, L, list, Generic, object))
+
+        class L2(Generic[S], L[S]): ...
+        self.assertEqual(L2.__mro__, (L2, L, list, Generic, object))


This feels wrong: Generic is before L in the bases, so it should be earlier in the MRO.

But L has Generic in its own bases, so it has to come first. Either that or reject it.

github-actions · 2026-04-13T18:24:33Z

This PR is stale because it has been open for 30 days with no activity.

pythongh-106102: Fix MRO resolution for nested generic classes

b4ec3d9

sobolevn requested review from AlexWaygood, Fidget-Spinner, JelleZijlstra and gvanrossum as code owners August 24, 2023 11:56

bedevere-bot mentioned this pull request Aug 24, 2023

cannot create consistent MRO for multiple generic inheritance #106102

Open

bedevere-bot added the awaiting review label Aug 24, 2023

AlexWaygood previously requested changes Aug 24, 2023

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Aug 24, 2023

Update Lib/typing.py

5fcad86

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

AlexWaygood reviewed Aug 24, 2023

View reviewed changes

Comment thread Lib/typing.py

Update Lib/typing.py

026430f

bedevere-bot added awaiting review and removed awaiting changes labels Aug 24, 2023

AlexWaygood reviewed Aug 28, 2023

View reviewed changes

More tests

666664f

AlexWaygood reviewed Aug 28, 2023

View reviewed changes

AlexWaygood added the topic-typing label Aug 28, 2023

JelleZijlstra reviewed Sep 7, 2023

View reviewed changes

github-actions Bot added the stale Stale PR or inactive for long period of time. label Apr 13, 2026

	if isinstance(b, _BaseGenericAlias):
	if isinstance(b, (_BaseGenericAlias, types.GenericAlias)):

Uh oh!

Conversation

sobolevn commented Aug 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sanderr commented Aug 24, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanderr Aug 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Aug 24, 2023

Uh oh!

Uh oh!

AlexWaygood Aug 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sobolevn commented Aug 28, 2023

Uh oh!

sanderr commented Aug 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sobolevn commented Aug 24, 2023 •

edited

Loading

sanderr Aug 28, 2023 •

edited

Loading

AlexWaygood Aug 28, 2023 •

edited

Loading