-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[red-knot] Class literal __new__ function callable subtyping
#17533
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
[red-knot] Class literal __new__ function callable subtyping
#17533
Conversation
|
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.
Looks good, thank you! Just a couple comments.
crates/red_knot_python_semantic/resources/mdtest/type_properties/is_subtype_of.md
Show resolved
Hide resolved
Co-authored-by: Carl Meyer <carl@oddbird.net>
Co-authored-by: Carl Meyer <carl@oddbird.net>
crates/red_knot_python_semantic/resources/mdtest/type_properties/is_subtype_of.md
Show resolved
Hide resolved
dhruvmanila
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.
Thank you.
I think it would be useful to create a into_callable method on ClassLiteralType to avoid expanding the is_subtype_of and to keep the match arms simple.
| let new_function = new_function.into_bound_method_type(db, self); | ||
| return new_function.is_subtype_of(db, target); |
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.
nit:
| let new_function = new_function.into_bound_method_type(db, self); | |
| return new_function.is_subtype_of(db, target); | |
| return new_function.into_bound_method_type(db, self).is_subtype_of(db, target); |
| pub(crate) fn into_bound_method_type( | ||
| self, | ||
| db: &'db dyn Db, | ||
| self_instance: Type<'db>, |
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.
The parameter name is a bit misleading because in this PR above where this is being called it's not an instance of the class but the class itself. This is also the case for the field name on BoundMethodType which is also called self_instance. I'm not sure what to name it, maybe self_or_cls (and remove the _instance suffix).
Regardless, this shouldn't block this PR and something that could be done in a follow-up instead.
|
Sounds good |
| let self_ty = Type::from(self); | ||
| let metaclass_call_function_symbol = self_ty | ||
| .member_lookup_with_policy( | ||
| db, | ||
| "__call__".into(), | ||
| MemberLookupPolicy::NO_INSTANCE_FALLBACK | ||
| | MemberLookupPolicy::META_CLASS_NO_TYPE_FALLBACK, | ||
| ) | ||
| .symbol; |
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.
For some reason this wasnt working
let metaclass_call_function_symbol = self
.class_member(
db,
"__call__",
MemberLookupPolicy::NO_INSTANCE_FALLBACK
| MemberLookupPolicy::META_CLASS_NO_TYPE_FALLBACK,
)
.symbol;It couldn't find __call__
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.
I suspect it found it, but it wasn't a BoundMethod. That's because it's Type::member_lookup_with_policy that implements the descriptor protocol, which turns a FunctionType into a BoundMethod.
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.
Looks good, thank you!
| let self_ty = Type::from(self); | ||
| let metaclass_call_function_symbol = self_ty | ||
| .member_lookup_with_policy( | ||
| db, | ||
| "__call__".into(), | ||
| MemberLookupPolicy::NO_INSTANCE_FALLBACK | ||
| | MemberLookupPolicy::META_CLASS_NO_TYPE_FALLBACK, | ||
| ) | ||
| .symbol; |
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.
I suspect it found it, but it wasn't a BoundMethod. That's because it's Type::member_lookup_with_policy that implements the descriptor protocol, which turns a FunctionType into a BoundMethod.
* main: (28 commits) [red-knot] Make `BoundMethodType` a salsa interned (#17581) [red-knot] Emit a diagnostic if a non-protocol is passed to `get_protocol_members` (#17551) [red-knot] Add more tests for protocol members (#17550) [red-knot] Assignability for subclasses of `Any` and `Unknown` (#17557) [red-knot] mypy_primer: add strawberry, print compilation errors to stderr (#17578) [red-knot] GenericAlias instances as a base class (#17575) Remove redundant `type_to_visitor_function` entries (#17564) Fixes how the checker visits `typing.cast`/`typing.NewType` arguments (#17538) [red-knot] Class literal `__new__` function callable subtyping (#17533) [red-knot] Surround intersections with `()` in potentially ambiguous contexts (#17568) [minor] Delete outdated TODO comment (#17565) [red-knot] add regression test for fixed cycle panic (#17535) [red-knot] fix unions of literals, again (#17534) red_knot_python_semantic: remove last vestige of old diagnostics! red_knot_python_semantic: migrate `types` to new diagnostics red_knot_python_semantic: migrate `types/diagnostic` to new diagnostics red_knot_python_semantic: migrate `types/call/bind` to new diagnostics red_knot_python_semantic: migrate `types/string_annotation` to new diagnostics red_knot_python_semantic: migrate `types/infer` to new diagnostic model red_knot_python_semantic: migrate INVALID_ASSIGNMENT for inference ...
…var-instance * dcreager/generic-constructor: (29 commits) We don't need this [red-knot] Make `BoundMethodType` a salsa interned (#17581) [red-knot] Emit a diagnostic if a non-protocol is passed to `get_protocol_members` (#17551) [red-knot] Add more tests for protocol members (#17550) [red-knot] Assignability for subclasses of `Any` and `Unknown` (#17557) [red-knot] mypy_primer: add strawberry, print compilation errors to stderr (#17578) [red-knot] GenericAlias instances as a base class (#17575) Remove redundant `type_to_visitor_function` entries (#17564) Fixes how the checker visits `typing.cast`/`typing.NewType` arguments (#17538) [red-knot] Class literal `__new__` function callable subtyping (#17533) [red-knot] Surround intersections with `()` in potentially ambiguous contexts (#17568) [minor] Delete outdated TODO comment (#17565) [red-knot] add regression test for fixed cycle panic (#17535) [red-knot] fix unions of literals, again (#17534) red_knot_python_semantic: remove last vestige of old diagnostics! red_knot_python_semantic: migrate `types` to new diagnostics red_knot_python_semantic: migrate `types/diagnostic` to new diagnostics red_knot_python_semantic: migrate `types/call/bind` to new diagnostics red_knot_python_semantic: migrate `types/string_annotation` to new diagnostics red_knot_python_semantic: migrate `types/infer` to new diagnostic model ...
Summary
From https://typing.python.org/en/latest/spec/constructors.html#converting-a-constructor-to-callable
this covers step 2 and partially step 3 (always respecting the
__new__)Test Plan
Update is_subtype_of.md