[red-knot] Allow classes with __call__ method to be a subtype of Callable

[MatthewMckee4]

Summary

Partially fixes #16953

I am aware this maybe doesn't align with some patterns, so i am very open to criticism.

Test Plan

Updated is_subtype_of.md

[github-actions]

mypy_primer results

Changes were detected when running on open source projects

rich (https://github.com/Textualize/rich)
- error[lint:invalid-argument-type] /tmp/mypy_primer/projects/rich/tests/test_progress.py:292:60: Object of type `MockClock` cannot be assigned to parameter `get_time` of function `track`; expected type `(() -> int | float) | None`
- Found 582 diagnostics
+ Found 581 diagnostics

[MatthewMckee4]

Perhaps implementing a method like
https://github.com/python/mypy/blob/b1be379f88e1e3735e04931f8253bbde4b387602/mypy/typeops.py#L366

would be more useful in the long run?

[carljm]

Thanks!

[carljm]

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

[MatthewMckee4]

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 guess since type of self is not yet dealt with, we cant do this.

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 Class instance and return Self or Type

[carljm]

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 class_member to look up __call__, we should just use .member() on the instance type itself, and then call .signature() on the resulting type. In the typical case the .member() call will give us back a Type::Callable(BoundMethod(_)), and Type::signature already does the right thing for a bound method, including issuing a diagnostic in the case where self has a wrong type annotation.

I do think we'll probably want to also issue a diagnostic at the definition site of a method with a bad self annotation, but that's really separate and should be handled separately. Notice that pyright issues diagnostics in both places (both the definition site of __call__, and the site where it is "used"). We are just worrying about the latter in this PR.

[MatthewMckee4]

(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?

[carljm]

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 .signature() on the type (no matter what it is) and construct a general callable type from that signature, and check if that's a subtype of target. Type::signature already takes care of correctly handling the self parameter. The resulting code should be quite a bit simpler and more general than what you pasted above. (And as a bonus it will handle all kinds of edge cases better where __call__ is something other than a normal method, too).

[carljm]

(We also don't need the separate if test for is_unbound(), that can just fall into the default case of the match, since it's not Symbol::Type.)

[carljm]

I just realized this should go one step further: Type::signatures already has handling for looking up __call__ on a Type::Instance, so we don't even have to do that here, we can just call .signatures(db) directly on the Instance type.

There is some complexity about the fact that GeneralCallableType only holds a single Signature, but if you get a more complex Signatures back from the instance type, you can just punt on that with a TODO for now.

[MatthewMckee4]

            // 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

[carljm]

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 bound_type to the CallableSignature, and then when we call it we know to bind that bound_type to the first parameter, and bind the provided arguments to the rest of the parameters. I think this is the right approach, as it lets us check type assignability for self.

The problem you're hitting here is that we set bound_type on the CallableSignature (which represents potentially a set of overloads), but GeneralCallableType only wraps a single Signature, which doesn't carry that bound_type.

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 bound_type, and we don't have that yet. Probably ultimately GeneralCallableType should wrap a CallableSignature, not just a single Signature. So I guess proper resolution of this issue is a bigger task than we'd realized. It may be best to just hold off on it for now.

[MatthewMckee4]

Okay sounds good. Thank you for the help anyway, i really appreciate it.

[carljm]

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 class_member to look up __call__, we should just use .member() on the instance type itself, and then call .signature() on the resulting type. In the typical case the .member() call will give us back a Type::Callable(BoundMethod(_)), and Type::signature already does the right thing for a bound method, including issuing a diagnostic in the case where self has a wrong type annotation.

I do think we'll probably want to also issue a diagnostic at the definition site of a method with a bad self annotation, but that's really separate and should be handled separately. Notice that pyright issues diagnostics in both places (both the definition site of __call__, and the site where it is "used"). We are just worrying about the latter in this PR.