python / mypy

Optional static typing for Python
https://www.mypy-lang.org/
Other
18.22k stars 2.78k forks source link

callables are inferred to be descriptors when they shouldn't always be, protocols are inferred to not be descriptors when maybe they should, and mypy converts between them with no error #15189

Open glyph opened 1 year ago

glyph commented 1 year ago

Bug Report

Callable implicitly has a __get__ method which makes it a bindable function when used at class scope.

Protocols with a __call__ method, on the other hand, are not assumed to have a __get__ method, and thus, not to be functions.

However, implicit conversion is allowed between the two, leading to confusing behavior in higher-order applications, such as decorators which wish to apply in similar fashion to either methods or free functions. For example, consider a decorator which returns a __call__able Protocol, in an effort to capture the nuances of named and default arguments (since mypy_extensions.NamedArg et. al. are clearly deprecated). It needs to manually re-specify the behavior of __get__ in order to be treated the same way as an equivalent Callable would.

To Reproduce

Consider the errors and non-errors in this program, with particular emphasis on the incredibly subtle distinction between be_a_typevar and be_a_callable, which behave differently:

from __future__ import annotations

from typing import Protocol, Callable, TypeVar, ParamSpec, Any

class HelloMethod(Protocol):
    def __call__(_, self: HelloSayer) -> None:
        ...

def be_a_protocol(method: HelloMethod) -> HelloMethod:
    return method

class HelloSelfless(Protocol):
    def __call__(self) -> None:
        ...

def be_selfless(method: HelloSelfless) -> HelloSelfless:
    return method

HelloVar = TypeVar("HelloVar", bound=Callable[..., Any])

def be_a_typevar(method: HelloVar) -> HelloVar:
    return method

Params = ParamSpec("Params")
Result = TypeVar("Result")

def be_a_callable(method: Callable[Params, Result]) -> Callable[Params, Result]:
    return method

class HelloSayer:

    def hello_regular(self) -> None:
        print("hello.")

    @be_a_protocol
    def hello_method(self) -> None:
        print("hello?")

    @be_selfless
    def hello_selfless(self) -> None:
        print("hello??")

    @be_a_typevar
    @be_a_protocol
    def hello_typevar(self) -> None:
        print("hello!")

    @be_a_callable
    @be_a_protocol
    def hello_callable(self) -> None:
        print("HELLO")

h = HelloSayer()
h.hello_regular()
h.hello_selfless()
h.hello_method()
h.hello_typevar()
h.hello_callable()

Proposed Solution

I think there really needs to be a FunctionType, and it should behave more or less like Callable does today with respect to the descriptor protocol. It should also have all the attributes that functions have, so that metaprogramming with __code__ and __name__ and soforth doesn't need to be littered with type:ignores.

In strict mode, I also think that this should become an error:

class Abstract(Protocol):
    def stuff(self):
        pass

class Concrete:
    var = Abstract()

abstract = Concrete().var  # Should be: Abstract has no attribute __get__

Any concrete type can trace its implementation of __get__ up through its hierarchy to object, but Protocol might have any implementation, or no implementation. It can't really be treated as if we can guess what it's going to return.

In general Callable and descriptors and all the adjacent special cases are kind of a big mess and this keeps getting reported in different ways. See also:

And this one is only tangentially related, but it makes it more annoying to write the __get__ for the descriptor protocol directly to work around it:

tmke8 commented 1 year ago

I think there really needs to be a FunctionType

I've come to the same conclusion based on the problems discussed in this typeshed PR: https://github.com/python/typeshed/pull/9834

Note that this problem affects all type checkers though, so maybe there needs to be a new PEP to specify the behavior.

KotlinIsland commented 8 months ago
KotlinIsland commented 8 months ago

@glyph Your point about Protocols breaking subtypes could be summed up as:

class Abstract(Protocol):
    def stuff(self):
        pass

class Impl(Abstract):
    def __get__(self, instance: object, owner: object) -> str:
        return "imposter"
    def stuff(self):
        print("stuff")

class Concrete:
    var: Abstract = Impl()

Concrete().var.stuff()  # runtime error: "imposter" detected

But would this also apply to any and all subtypes?

class A:
    val = 1

class B(A):
    def __get__(x, y, z):
        return "imposter"
class Z:
    a: A = B()
Z.a.val  # runtime error: "imposter" detected

Any thoughts?

glyph commented 8 months ago

But would this also apply to any and all subtypes?

It had not occurred to me that the normal checking of subtype compatibility for object.__get__ was suspended across all types. I fleshed out your example a bit, and if __get__ is explicitly specified on A, then this type of subtyping on B does trigger errors, as it does on a normal method with a signature conflict:

https://mypy-play.net/?mypy=latest&python=3.12&flags=strict&gist=0ecd67b6fce983b0d27c900c4ec4d603

I'd argue that object.__get__ is mis-specified to allow this.

glyph commented 8 months ago

(The thing that is special about Protocol is that it ought to be able to not have an opinion about __get__; since it is an abstract and not a concrete type, portions of the type which it doesn't mention should be allowed to be unspecified rather than assuming default behavior.)

KotlinIsland commented 8 months ago

@glyph Do you want to discuss this further? I am interested in your perspective.

Protocol is abstract, portions of the type which it doesn't mention should be allowed to be unspecified rather than assuming default behavior.

I don't quite understand what you mean by this. Do you mean that any Protocol can't be used as it is abstract?

protocol Foo:
    f: int
class A:
    foo: Foo
def f(a: A):
    a.foo  # error, foo is abstract

I you want we can discuss via email or a call or something.

glyph commented 8 months ago

I don't quite understand what you mean by this. Do you mean that any Protocol can't be used as it is abstract?

No, I mean the parts of a Protocol that aren't specified ought to be unspecified, not "specified to be the default behavior of object". There aren't a ton of behaviors that do anything interesting beyond __get__, but just for an example of one of the few others, __eq__ has some flexibility in its return value (enabling syntaxes like SQLAlchemy), but object.__eq__ returns a bool. Protocol.__eq__ should default to something more like typing.Never to indicate that if it is used, it's not obligated to return anything in particular, unless that specific Protocol specifies something.

glyph commented 8 months ago
class A:
    foo: Foo
def f(a: A):
    a.foo  # error, foo is abstract

Just to be clear about why this example doesn't make sense from my perspective: foo: Foo says that foo is an instance variable with type Foo. If, instead, you did this:

class A:
    foo: ClassVar[Foo]
def f(a: A):
    a.foo

then yes, we don't know what Foo.__get__ is, and this would be an error. In a perfect world, this would raise an error too:

class NonGettableFoo:
    # otherwise conforms to Foo
    def __get__(
        self,
        instance: object,
        owner: type | None = None,
    ) -> None:
        return None

class A:
    foo: Foo = NonGettableFoo()  # error, won't be Foo at instance level
KotlinIsland commented 8 months ago

I don't quite follow what you are saying in regards to instance vs class:

class D:
    def __get__(x, y, z):
        return None

class A:
    d = D()

print(A.d)  # None
print(A().d)  # None
KotlinIsland commented 8 months ago

Protocol.__eq__ should default to something more like typing.Never to indicate that if it is used, it's not obligated to return anything in particular, unless that specific Protocol specifies something.

if it was Never you would see:

protocol P:
    pass
p: P
p == 1
print("done")   # error, unreachable

Maybe object would be a better default for this?

KotlinIsland commented 8 months ago
glyph commented 8 months ago

Maybe object would be a better default for this?

Ah, fair enough. I suppose "unreachable" is not really the right error to report here.

glyph commented 8 months ago

I don't quite follow what you are saying in regards to instance vs class:

Right, right, I always forget about that nuance. It's not really "instance" vs. ClassVar, so much as it is that the class scope is just a bit of a mess. I suppose any assignment at class scope without a clear descriptor is potentially risky.