[red-knot] Fix MRO inference for protocol classes; allow inheritance from subscripted Generic[]; forbid subclassing unsubscripted Generic
#17452
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
AlexWaygood
force-pushed
the
alex/protocol-mros
branch
from
e4083ae to
a118287
Compare
AlexWaygood
changed the title
Generic[]Generic[]; forbid subclassing unsubscripted Generic
CodSpeed Performance ReportMerging #17452 will improve performances by 12.46%Comparing Summary
Benchmarks breakdown
|
Member
Author
hah... I think that might be entirely an accidental side effect of a temporary hack I added to reduce false positives when subscripting generic classes? But sure! |
Member
Author
|
I'm not sure we can land this with the number of |
carljm
reviewed
Apr 17, 2025
crates/red_knot_python_semantic/resources/mdtest/subscript/tuple.md
Outdated
Show resolved
Hide resolved
AlexWaygood
force-pushed
the
alex/protocol-mros
branch
from
d761b22 to
ee1d2b6
Compare
AlexWaygood
force-pushed
the
alex/protocol-mros
branch
from
ee1d2b6 to
ada203b
Compare
AlexWaygood
force-pushed
the
alex/protocol-mros
branch
from
ada203b to
0dc04bf
Compare
Member
Author
|
Here's a minimal repro for the import abc
class Foo(metaclass=abc.ABCMeta): ...
# error: No arguments provided for required parameters `bases`, `namespace` of bound method `__new__` (lint:missing-argument) [Ln 5, Col 1]
# error: Argument to this function is incorrect: Expected `str`, found `Literal[Foo]` (lint:invalid-argument-type) [Ln 5, Col 1]
Foo() We apparently think that the Something I don't yet understand though is why we're seeing errors like this on
The It appears that we already understand that the metaclass of cc. @sharkdp -- this may be the cause of many of the |
dcreager
reviewed
Apr 18, 2025
Member
dcreager
left a comment
dcreager
left a comment
There was a problem hiding this comment.
This looks reasonable to me. I hope it won't conflict too much with @dcreager 's legacy-generics PR. I do think we need to at least understand the new ecosystem diagnostics in order to make a decision to land this.
No worries about this — I think it will actually help that PR quite a bit, since I hadn't done anything yet related to Generic!
Comment on lines
186
to
199
| ClassBase::Protocol => Either::Left(Either::Left( | ||
| [self, ClassBase::Generic, ClassBase::object(db)].into_iter(), | ||
| )), | ||
| ClassBase::Dynamic(DynamicType::SubscriptedProtocol) => Either::Left(Either::Left( | ||
| [ | ||
| self, | ||
| ClassBase::Dynamic(DynamicType::SubscriptedGeneric), | ||
| ClassBase::object(db), | ||
| ] | ||
| .into_iter(), | ||
| )), | ||
| ClassBase::Dynamic(_) | ClassBase::Generic => { | ||
| Either::Left(Either::Right([self, ClassBase::object(db)].into_iter())) | ||
| } |
Member
There was a problem hiding this comment.
That's annoying that the different array lengths produce different types and therefore different Either wrappers. There's probably a way to simplify this down to a single type/wrapper using slices instead of arrays...but it's also probably not worth the effort! (So no recommended change here, just me blathering)
Member
Author
There was a problem hiding this comment.
I can't see a way of doing it with slices while keeping the borrow checker happy. But I think having to use nested Either types is reason enough to bite the bullet and write a custom Iterator type 😄 it's not that much boilerplate, and it makes it easier to read at the callsite.
Member
There was a problem hiding this comment.
I actually prefer the nested Either that you had before, tbh. I don't think it will have any performance implications. And I think it is too much boilerplate for what it's achieving, especially since you're still returning impl Iterator.
That said, if others disagree I'm happy to go along with the consensus.
Contributor
There was a problem hiding this comment.
FWIW I don't have strong feelings either way. Both versions seem kind of overwrought for what it's actually doing, but that's just life with the borrow checker sometimes :) The dedicated iterator is definitely more boilerplate overall, but also makes this method (which has the meaningful type logic in it) less painful to read (so it encapsulates the pain a little better). I approved because I'm fine with either.
Member
Author
There was a problem hiding this comment.
Yeah, I agree that both versions seem annoyingly verbose/overwrought! I weakly prefer the version I have now, though, basically for the reasons Carl mentions. I also don't have strong feelings any which way, though.
Contributor
Maybe this is because our current understanding of its MRO has I had wondered on the |
Contributor
AlexWaygood
commented
Apr 18, 2025
Comment on lines
+776
to
+781
| // HACK: we should implement some more general logic here that supports arbitrary custom | ||
| // metaclasses, not just `type` and `ABCMeta`. | ||
| if matches!( | ||
| class.known(db), | ||
| Some(KnownClass::Type | KnownClass::ABCMeta) | ||
| ) && policy.meta_class_no_type_fallback() |
Member
Author
There was a problem hiding this comment.
I added this as a short-term hack to avoid the __new__ false positives for classes that have ABCMeta as their metaclasses. Judging from the mypy_primer report, it looks extremely effective!
carljm
approved these changes
Apr 18, 2025
Comment on lines
+776
to
+781
| // HACK: we should implement some more general logic here that supports arbitrary custom | ||
| // metaclasses, not just `type` and `ABCMeta`. | ||
| if matches!( | ||
| class.known(db), | ||
| Some(KnownClass::Type | KnownClass::ABCMeta) | ||
| ) && policy.meta_class_no_type_fallback() |
Contributor
There was a problem hiding this comment.
You're robbing @sharkdp of the satisfaction of removing all these ecosystem false positives!
I guess it's also satisfying to remove hacks in the codebase...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
KnownInstanceType::Genericvariant to represent the type of the symboltyping(_extensions).GenericClassBase::ProtocolandClassBase::Genericvariants, to represent the fact that classes can have these symbols in their MRO at runtime, even though these are not represented as classes in typeshed.SubclassOfInnervariant to represent the inner data that atype[]type holds. With the introduction of the newClassBase::ProtocolandClassBase::Genericvariants, theClassBaseenum is no longer suitable for this purpose, sincetype[Protocol]andtype[Generic]are both meaningless.Generic[]GenericThis appears to get rid of a lot of
invalid-basefalse-positive errors and greatly improves the accuracy of our MRO inference for classes that inherit fromGenericand/orProtocol... unfortunately it seems to be introducing many new false positives regarding__new__in the mypy_primer report, which isn't something I expected. Possibly these were latent bugs that are now exposed due to the fact that we understand a lot of MROs a lot more precisely...?Test Plan
Mdtests updated and added.