-
Notifications
You must be signed in to change notification settings - Fork 1.8k
[red-knot] Fix constructor calls on classes with metaclasses #17662
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
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 | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -20,6 +20,7 @@ Since every class has `object` in it's MRO, the default implementations are `obj | |||||||||||||||||||||||||
| - If neither `__new__` nor `__init__` are defined anywhere in the MRO of class (except for `object`) | ||||||||||||||||||||||||||
| \- no arguments are accepted and `TypeError` is raised if any are passed. | ||||||||||||||||||||||||||
| - If `__new__` is defined, but `__init__` is not - `object.__init__` will allow arbitrary arguments! | ||||||||||||||||||||||||||
| - If `__init__` is defined, but `__new__` is not - `object.__new__` will allow arbitrary arguments! | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| As of today there are a number of behaviors that we do not support: | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
@@ -323,3 +324,23 @@ reveal_type(Foo(1)) # revealed: Foo | |||||||||||||||||||||||||
| # error: [too-many-positional-arguments] "Too many positional arguments to bound method `__init__`: expected 1, got 2" | ||||||||||||||||||||||||||
| reveal_type(Foo(1, 2)) # revealed: Foo | ||||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| ## `__new__` defined on a meta-class should not be used | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| 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 | ||||||||||||||||||||||||||
|
Contributor
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.
Suggested change
|
||||||||||||||||||||||||||
| a meta-class. | ||||||||||||||||||||||||||
|
Comment on lines
+330
to
+335
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.
Suggested change
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| ```py | ||||||||||||||||||||||||||
| class Meta(type): | ||||||||||||||||||||||||||
| def __new__(mcls, name, bases, namespace, /, **kwargs): | ||||||||||||||||||||||||||
| return super().__new__(mcls, name, bases, namespace) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| class Foo(metaclass=Meta): ... | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| # This should not raise an error | ||||||||||||||||||||||||||
| reveal_type(Foo()) # revealed: Foo | ||||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -201,6 +201,7 @@ bitflags! { | |||||
|
|
||||||
| /// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| /// | ||||||
| /// This is similar to no object fallback above | ||||||
| const META_CLASS_NO_TYPE_FALLBACK = 1 << 2; | ||||||
|
|
@@ -2641,12 +2642,28 @@ impl<'db> Type<'db> { | |||||
| qualifiers: meta_attr_qualifiers, | ||||||
| }, | ||||||
| meta_attr_kind, | ||||||
| ) = Self::try_call_dunder_get_on_attribute( | ||||||
| db, | ||||||
| self.class_member_with_policy(db, name.into(), member_policy), | ||||||
| ) = if matches!( | ||||||
| self, | ||||||
| self.to_meta_type(db), | ||||||
| ); | ||||||
| Type::ClassLiteral(_) | Type::GenericAlias(_) | Type::SubclassOf(_) | ||||||
|
Contributor
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. 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 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). |
||||||
| ) && member_policy.mro_no_object_fallback() | ||||||
| { | ||||||
| // When looking up the attribute on the meta-type with `NO_OBJECT_FALLBACK` policy, we | ||||||
| // imply that the looked up attribute exists on `object` even if we do not want to fall back | ||||||
| // back to it. This in turn means that descriptor protocol will not select same named | ||||||
| // data-descriptor on the meta-type - hence we return `SymbolAndQualifiers::default()` here, | ||||||
| // which is essentially a `Symbol::Unbound` with no qualifiers. | ||||||
| ( | ||||||
| SymbolAndQualifiers::default(), | ||||||
| AttributeKind::NormalOrNonDataDescriptor, | ||||||
| ) | ||||||
| } else { | ||||||
| Self::try_call_dunder_get_on_attribute( | ||||||
| db, | ||||||
| self.class_member_with_policy(db, name.into(), member_policy), | ||||||
| self, | ||||||
| self.to_meta_type(db), | ||||||
| ) | ||||||
| }; | ||||||
|
|
||||||
| let SymbolAndQualifiers { | ||||||
| symbol: fallback, | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -879,12 +879,15 @@ impl<'db> ClassLiteral<'db> { | |||||
| continue; | ||||||
| } | ||||||
|
|
||||||
| // 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() | ||||||
| // Some meta-class methods such as `__call__` need special treatment depending on | ||||||
|
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.
Suggested change
|
||||||
| // whether they are explicitly defined, or inherited from `type.__call__`. | ||||||
| // We use `MemberLookupPolicy::META_CLASS_NO_TYPE_FALLBACK` flag and it's getter | ||||||
| // `policy.meta_class_no_type_fallback` to allow downstream code to check whether | ||||||
| // a custom implementation of such symbol exists on a type. | ||||||
| // E.g. this is currently used in `ClassLiteral::into_callable` which passes this | ||||||
| // policy here through a chain of calls. | ||||||
| if matches!(class.known(db), Some(KnownClass::Type)) | ||||||
| && policy.meta_class_no_type_fallback() | ||||||
| { | ||||||
| continue; | ||||||
| } | ||||||
|
Comment on lines
+889
to
893
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. Could you explain why we still need to add some special casing for
Contributor
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. @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:
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 see -- I didn't realise that that note from your PR description was referring to this, since this method is called
Contributor
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'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! |
||||||
|
|
||||||
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.