-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[red-knot] Allow classes with __call__ method to be a subtype of Callable #17005
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
34f6824
a343bb8
f2c748b
2309715
dbc5867
dc2a5b0
ba1e58f
eed5e1a
fa18160
8cd517c
04cb547
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 |
|---|---|---|
|
|
@@ -493,6 +493,19 @@ impl<'db> Parameters<'db> { | |
| .enumerate() | ||
| .rfind(|(_, parameter)| parameter.is_keyword_variadic()) | ||
| } | ||
|
|
||
| /// Return a new Parameters type with the first parameter. | ||
| pub(crate) fn without_self_parameter(self) -> Self { | ||
| if let Some(first_param) = self.get(0) { | ||
| return if first_param.name().map(ruff_python_ast::name::Name::as_str) == Some("self") { | ||
|
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. We should not check the name of the parameter, it doesn't matter. I think this method should return the removed parameter, because in the caller we should check if its annotated type (if any) is assignable from the instance type. That does matter. (We should add a test for the case where it isn't, e.g. class C:
def __call__(self: int) -> int:
return 1
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. How should that example be handled? It seems like this should be rejected before getting to this stage but i could be wrong. pylance says "Type of parameter "self" must be a supertype of its class "A"" I may need some more guidance on this anyway, what do you think of https://github.com/python/mypy/blob/b1be379f88e1e3735e04931f8253bbde4b387602/mypy/typeops.py#L366 and implementing a bind method (instead of into_callable_type_without_self_parameter which i think currently is not a very good function) to FunctionType that can take in the
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. Good call, thanks for pushing on whether this should be done in a better way. After looking at it, I think it should be done differently. We shouldn't need this function. Instead of using I do think we'll probably want to also issue a diagnostic at the definition site of a method with a bad
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. (Type::Instance(_), Type::Callable(_)) => {
let call_symbol = self.member(db, "__call__").symbol;
if call_symbol.is_unbound() {
false
} else {
match call_symbol {
Symbol::Type(
Type::Callable(CallableType::BoundMethod(call_function)),
_,
) => {
let callable_type = call_function.function(db).into_callable_type(db);
match callable_type {
Type::Callable(CallableType::General(_)) => {
callable_type.is_subtype_of(db, target)
}
_ => false,
}
}
_ => false,
}
}
}into_callable_type here still give a GeneralCallableType with the self parameter, it seems like maybe BoundMethodType should handle removing self from the parameters of its function?
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. No we should not be matching on the specific type of the symbol result like that and pulling out internal details of the type. Instead we should just call
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. (We also don't need the separate if test for
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. I just realized this should go one step further: There is some complexity about the fact that
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. // An instance type with `__call__` method can be a subtype of Callable
(Type::Instance(_), Type::Callable(_)) => {
let signatures = self.signatures(db);
let mut iter = signatures.iter();
match iter.next() {
Some(signature) => {
let mut overloads = signature.iter();
match overloads.next() {
Some(overload) => {
let callable_type = Type::Callable(CallableType::General(
GeneralCallableType::new(db, overload),
));
callable_type.is_subtype_of(db, target)
}
None => false,
}
}
None => false,
}
}I may be on the wrong track here, but callable_type here still has a self parameter
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. No, you're on the right track. It looks like the issue here is that the way we handle "binding self" for a bound method is not by removing the parameter, but by adding a The problem you're hitting here is that we set I think really we need to resolve this issue in the context of broader support for overloads; we need to add support for callable subtyping with overloads, and with
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. Okay sounds good. Thank you for the help anyway, i really appreciate it. |
||
| let new_parameters = self.as_slice()[1..].to_vec(); | ||
| Parameters::new(new_parameters) | ||
| } else { | ||
| self | ||
| }; | ||
| } | ||
| self | ||
| } | ||
| } | ||
|
|
||
| impl<'db, 'a> IntoIterator for &'a Parameters<'db> { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.