-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[ty] Do not carry the generic context of Protocol or Generic in the ClassBase enum
#17989
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
Conversation
bfcf1d3 to
b1c2b95
Compare
|
|
I haven't reviewed the code yet, but I like the idea! The reason I had put it there to begin with was because I thought I'd need it when determining that a class is generic via the legacy syntax. But we do that by looking at the explicit bases, not the MRO, which means that we're looking at a ruff/crates/ty_python_semantic/src/types/class.rs Lines 539 to 542 in 7a48477
So I don't foresee any problems with this vis-à-vis the generics machinery. |
|
I think I might need to emulate the logic that lives at https://github.com/python/cpython/blob/98e2c3af4794d6c6ebe47b20badbd31c542d944e/Lib/typing.py#L1214-L1244 at runtime to fix those Inheriting from |
…he `ClassBase` enum
b1c2b95 to
e1ae972
Compare
60334e9 to
ab145fa
Compare
|
that primer diff is starting to look a little better... still looks like there might be some bugs here... |
…GenericAlias.__mro_entries__`
0c7fe3f to
4fb2a7b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
ff7fb17 to
09cd8a5
Compare
mypy_primer analysis
These classes do succeed at runtime: >>> from typing import Protocol, TypeVar, Sized, Iterable
>>> T = TypeVar("T")
>>> class _SizedIterable(Protocol[T], Sized, Iterable[T]): ...
...
>>>but they wouldn't succeed if these bases actually had the MROs that typeshed says they have! >>> from typing import Protocol, TypeVar
>>> T = TypeVar("T")
>>> class Sized(Protocol): ...
...
>>> class Iterable(Protocol[T]): ...
...
>>> class _SizedIterable(Protocol[T], Sized, Iterable[T]): ...
...
Traceback (most recent call last):
File "<python-input-6>", line 1, in <module>
class _SizedIterable(Protocol[T], Sized, Iterable[T]): ...
File "/Users/alexw/.pyenv/versions/3.13.1/lib/python3.13/typing.py", line 2067, in __new__
return super().__new__(mcls, name, bases, namespace, **kwargs)
~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "<frozen abc>", line 106, in __new__
TypeError: Cannot create a consistent method resolution order (MRO) for bases Protocol, Sized, IterableThe two new hits in django-stubs look similar, as does the new one in anyio. There's an increase in the total number of diagnostics on mypy, but this looks like it's mostly because a bogus |
09cd8a5 to
2d6f535
Compare
|
Okay, this is finally ready for review! |
carljm
left a comment
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.
Very nice!
|
|
||
| class peekable(Generic[T], Iterator[T]): ... | ||
|
|
||
| # revealed: tuple[<class 'peekable[Unknown]'>, <class 'Iterator[T]'>, <class 'Iterable[T]'>, typing.Protocol, typing.Generic, <class 'object'>] |
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.
It's not clear to me why we see Iterator[T] and Iterable[T] in the MRO here, rather than Iterator[Unknown] and Iterable[Unknown]. I thought we propagated specialization up through the MRO? Do we fail to propagate the multiple layers of specialization ("the typevar used in Iterator in typeshed" -> "our T typevar here" -> Unknown)?
(This might be a @dcreager question, and doesn't need to block this PR.)
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, I agree that this still isn't quite right. The issue predates this PR, though — you can see it in lots of other MROs in our tests — so I'm going to punt on fixing it for now.
Summary
It doesn't seem to be necessary for our generics implementation to carry the
GenericContextin theClassBasevariants. Removing it simplifies the code, fixes many TODOs aboutGenericorProtocolappearing multiple times in MROs when each should only appear at most once, and allows us to more accurately detect runtime errors that occur due toGenericorProtocolappearing multiple times in a class's bases.In order to remove the
GenericContextfrom theClassBasevariant, it turns out to be necessary to emulatetyping._GenericAlias.__mro_entries__, or we end up with a large number of false-positiveinconsistent-mroerrors. This PR therefore also does that.Lastly, this PR fixes the inferred MROs of PEP-695 generic classes, which implicitly inherit from
Genericeven if they have no explicit bases.Test Plan
mdtests