[red-knot] Fix constructor calls on classes with metaclasses #17662
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
mishamsk
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
Contributor
|
Contributor
Author
|
mypy_primer fixes seem legit! |
AlexWaygood
reviewed
Apr 28, 2025
Comment on lines
+882
to
886
| if matches!(class.known(db), Some(KnownClass::Type)) | ||
| && policy.meta_class_no_type_fallback() | ||
| { | ||
| continue; | ||
| } |
Member
There was a problem hiding this comment.
Could you explain why we still need to add some special casing for type specifically here? I see some tests fail in is_subtype_of.md without this, but I don't understand why we need to continue if the class is type but not if it's some type subclass. Adding a comment here that explains this would be very helpful.
Contributor
Author
There was a problem hiding this comment.
@AlexWaygood yeah, I initially removed it and then stumbled on failing tests and figured out it is not possible (at least without refactoring into_callable logic). I've updated the policy bit flags enum docs and mentioned the reason in the description of the PR, but agree that adding additional comment at the use site would be helpful for future readers. I'll wait for other reviews and will add.
Here is my line from the PR for the underlying reason:
We can't get rid of META_CLASS_NO_TYPE_FALLBACK as it is necessary to figure out if a given meta-class has an explicit
__call__method defined, not inherited from type. This is used ininto_callableimplementation
Member
There was a problem hiding this comment.
Here is my line from the PR for the underlying reason:
I see -- I didn't realise that that note from your PR description was referring to this, since this method is called Type::class_member_from_mro rather than Type::into_callable :-)
Contributor
Author
There was a problem hiding this comment.
I'll do better with wording! Thanks for pointing this out. I've just pushed a commit with a comment, hopefully it is clear enough. Let me know, I can continue exercising my English if it doesn't!
AlexWaygood
changed the title
carljm
reviewed
Apr 29, 2025
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
Thanks for working on this! I'm not yet quite convinced by the correctness of the fix, though...
| We lookup `__new__` method using descriptor protocol, which technically allows it to be found on a | ||
| meta-class if `__new__` is not defined anywher in class MRO. At runtime this is never the case, | ||
| since all classes have `object` in their MRO, which has a `__new__` method. However, for the | ||
| purposes of type checking we use special lookup logic that ignores `object.__new__` as it's runtime |
Contributor
There was a problem hiding this comment.
Suggested change
| purposes of type checking we use special lookup logic that ignores `object.__new__` as it's runtime | |
| purposes of type checking we use special lookup logic that ignores `object.__new__` as its runtime |
| meta-class if `__new__` is not defined anywher in class MRO. At runtime this is never the case, | ||
| since all classes have `object` in their MRO, which has a `__new__` method. However, for the | ||
| purposes of type checking we use special lookup logic that ignores `object.__new__` as it's runtime | ||
| behavior is not expressible in typeshed. This not cause false-positives when `__new__` is defined on |
Contributor
There was a problem hiding this comment.
Suggested change
| behavior is not expressible in typeshed. This not cause false-positives when `__new__` is defined on | |
| behavior is not expressible in typeshed. This should not cause false-positives when `__new__` is defined on |
| self, | ||
| self.to_meta_type(db), | ||
| ); | ||
| Type::ClassLiteral(_) | Type::GenericAlias(_) | Type::SubclassOf(_) |
Contributor
There was a problem hiding this comment.
This match on these specific type variants here looks suspicious to me; I think I need a clearer statement of why this is correct? Tests fail without it, so it's clearly not redundant.
It seems like we are using "is self one of these three variants" as a proxy for "are we currently looking up an attribute on the meta type", but I don't think that's really a correct proxy? Or at least, if it's correct today, it's making assumptions that could be invalidated in future. (For instance, Instance(<builtins.type>) could definitely be a meta-type -- I can't remember if we normalize that to SubclassOf(<builtins.object>) currently.)
I'd have to spend more time digging into details to make this more concrete, but it feels like there should be some place higher up the call chain where we should know for certain that we are about to look up the attribute on the meta type, and that's the place where we should be making a decision about this case (and if necessary, passing some policy or context down to here to reflect that fact).
AlexWaygood
reviewed
Apr 29, 2025
Member
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
Some minor spelling and grammar nits :-)
Comment on lines
+330
to
+335
| We lookup `__new__` method using descriptor protocol, which technically allows it to be found on a | ||
| meta-class if `__new__` is not defined anywher in class MRO. At runtime this is never the case, | ||
| since all classes have `object` in their MRO, which has a `__new__` method. However, for the | ||
| purposes of type checking we use special lookup logic that ignores `object.__new__` as it's runtime | ||
| behavior is not expressible in typeshed. This not cause false-positives when `__new__` is defined on | ||
| a meta-class. |
Member
There was a problem hiding this comment.
Suggested change
| We lookup `__new__` method using descriptor protocol, which technically allows it to be found on a | |
| meta-class if `__new__` is not defined anywher in class MRO. At runtime this is never the case, | |
| since all classes have `object` in their MRO, which has a `__new__` method. However, for the | |
| purposes of type checking we use special lookup logic that ignores `object.__new__` as it's runtime | |
| behavior is not expressible in typeshed. This not cause false-positives when `__new__` is defined on | |
| a meta-class. | |
| We lookup `__new__` methods using the descriptor protocol, which technically allows it to be found on a | |
| metaclass if `__new__` is not defined anywhere in a class's MRO. At runtime this is never the case, | |
| since all classes have `object` in their MRO, and `object` has a `__new__` method. However, for the | |
| purposes of type checking we use special lookup logic that ignores `object.__new__` as its runtime | |
| behavior is not expressible in typeshed. This does not cause false-positives when `__new__` is defined on | |
| a metaclass. |
|
|
||
| /// When looking up an attribute on a class, we sometimes need to avoid | ||
| /// looking up attributes defined on `type` if this is the metaclass of the class. | ||
| /// For example, this is used to find if a meta-class implements a `__call__` method. |
Member
There was a problem hiding this comment.
Suggested change
| /// For example, this is used to find if a meta-class implements a `__call__` method. | |
| /// For example, this is used to find if a metaclass implements a `__call__` method. |
| class.known(db), | ||
| Some(KnownClass::Type | KnownClass::ABCMeta) | ||
| ) && policy.meta_class_no_type_fallback() | ||
| // Some meta-class methods such as `__call__` need special treatment depending on |
Member
There was a problem hiding this comment.
Suggested change
| // Some meta-class methods such as `__call__` need special treatment depending on | |
| // Some metaclass methods such as `__call__` need special treatment depending on |
Contributor
|
@mishamsk While reviewing this, I needed to dig deeper into how class construction work, and found some things that were problematic with the current way we handle things. There are two main things that we need to distinguish:
The two are not unrelated. A normal attribute lookup works like this:
For implicit dunder calls like For This explains why it's problematic to use |
Contributor
Author
|
@sharkdp yes, I agree with your assessment and I was thinking that Carl's comment basically implies that I should "fix" the root cause (improve descriptor protocol modeling) instead of doing a band-aid. But if you are already working on it, I'll wait. Although I wonder if there is going to be anything left from this PR after the change. Maybe tests & some comments... I'll see if it makes sense for me to do other follow-ups that I had without waiting. I need to tackle |
Contributor
The PR with the alternative approach is here: #17733. My intention was to write review feedback for your PR, but by the time I had understood what was going on, I had written so much new code for my analysis that it seemed easier to open a PR myself. Sorry for that! I think this PR was still valuable for exploration of possible options. For example, I agree with you that we still need the
I think this would be a great follow-up, yes! Especially since my PR now reveals a few new false positives on enum construction (see PR description).
Not exactly sure what that refers to, but reducing technical debt always sounds like a good idea. |
sharkdp
added a commit
that referenced
this pull request
Apr 30, 2025
## Summary Model the lookup of `__new__` without going through `Type::try_call_dunder`. The `__new__` method is only looked up on the constructed type itself, not on the meta-type. This now removes ~930 false positives across the ecosystem (vs 255 for #17662). It introduces 30 new false positives related to the construction of enums via something like `Color = enum.Enum("Color", ["RED", "GREEN"])`. This is expected, because we don't handle custom metaclass `__call__` methods. The fact that we previously didn't emit diagnostics there was a coincidence (we incorrectly called `EnumMeta.__new__`, and since we don't fully understand its signature, that happened to work with `str`, `list` arguments). closes #17462 ## Test Plan Regression test
Contributor
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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
Fixes #17462
Notes:
__call__method defined, not inherited from type. This is used ininto_callableimplementationNO_META_CLASS_FALLBACKpolicy flag, and instead hard-code the logic into descriptor protocol, that ifMRO_NO_OBJECT_FALLBACKis used, we should assume that the lookup is being done for a symbol that is always present onobjectand hence we also assume that it can never be looked up on a meta-classThe last point is not 100% correct, since data-descriptors should theoretically override lookup even for attributes present on
object. But as of today,MRO_NO_OBJECT_FALLBACKis only used for__new__which can't be overridden even by a data-descriptor on a meta-class (I've checked runtime behavior).Test Plan
Added the failing code example from #17462 to mdtest