python / mypy

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

Decorated function makes type-check fail if done inside of method from the class #9266

Open fabioz opened 4 years ago

fabioz commented 4 years ago

I believe this is a bug (explanation below).

I expected to be able to statically check a case by checking whether self matches a given protocol (with the check_implements below).

This seems to work, but if the method is decorated (with the implements as shown in the code below) it doesn't validate properly that a method return type is wrong.

i.e.: in the code below I expected that the _: IFoo = check_implements(self) line gives an error because the protocol declaration expects a bool return, whereas the method implementation says it returns a str.

I think this is a bug because the same type-check later on says that the implementation doesn't conform to the protocol if done outside of the class -- as _: IFoo = check_implements(Foo()), so, it does something on __init__ and a different thing if out of the __init__. Besides, it works if the function is not decorated.

Note: even if I do _: IFoo = check_implements(Foo()) in the __init__ it doesn't validate properly (so, it doesn't seem related to self -- and it works as expected if the function is not decorated).

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

F = TypeVar('F', bound=Callable[..., Any])

def implements(method: Any) -> Callable[[F], F]:
    """
    Meant to be used as

    class B:
        @implements(IExpectedProtocol.method)
        def method(self):
            pass

    Useful for knowing whether a name still exists in the interface and document
    what's being implemented.

    In runtime validates that the name passed is the same name implemented.
    """
    @functools.wraps(method)
    def wrapper(func):
        if func.__name__ != method.__name__:
            msg = "Wrong @implements: %r expected, but implementing %r."
            msg = msg % (func.__name__, method.__name__)
            raise AssertionError(msg)

        return func

    return wrapper

T = TypeVar("T")

def check_implements(x: T) -> T:
    """
    This is to be used in the constructor of a class which is expected to
    implement some protocol. It can be used as:

    def __init__(self):
        _: IExpectedProtocol = check_implements(self)

    Mypy should complain if `self` is not implementing the IExpectedProtocol.
    """
    return x

class IFoo(Protocol):

    def some_method(self) -> bool:
        pass

class Foo(object):

    def __init__(self) -> None:  # give type
        _: IFoo = check_implements(self)  # Issue: Mypy does not report error here

    @implements(IFoo.some_method)  # Note: if this implements is removed it reports the error at the __init__!
    def some_method(self) -> str:  # Note: return value does not match protocol!
        pass

_: IFoo = check_implements(Foo())  # Mypy properly reports error here (with or without the implements decorator).
gvanrossum commented 4 years ago

I didn't follow everything yet (it takes me a while) but does the same thing happen if you use a digger ale instead of '_'? IIRC there's a special case for that somewhere. (But I may very well be wrong, sorry.)

fabioz commented 4 years ago

Changing _ to any other name yields the same results.

Akuli commented 4 years ago

minimal example:

from typing import TypeVar, Protocol, Callable

F = TypeVar('F')
foo: Callable[[F], F] = lambda x: x

class IFoo(Protocol):
    def some_method(self) -> bool: ...

class Foo(object):

    def __init__(self) -> None:
        blah: IFoo = self

    #@foo
    def some_method(self) -> str:
        pass

uncommenting @foo removes error and it's not supposed to do that

gvanrossum commented 4 years ago

@Akuli Thanks for the translation. :-)

This feels like it’s because mypy doesn’t have the proper type for a decorated method when it checks the init body.

Definitely a bug, not simple to fix, so use the workaround of putting the check after the class.

Akuli commented 4 years ago

putting the check after the class requires creating an instance of the class just for the check

fabioz commented 4 years ago

Actually, I ended up doing a different approach (using a class decorator to check the type instead of using the __init__), with the code below...

So, while the issue persists, I already found another workaround where things work for me.

Just for reference, I'm using the check_implements decorator as below:

from typing import TypeVar, Protocol, Callable

F = TypeVar('F')
foo: Callable[[F], F] = lambda x: x

class IFoo(Protocol):

    def some_method(self) -> bool: ...

def check_implements(_:F) -> Callable[[F], F]:

    def method(cls):
        return cls

    return method

@check_implements(IFoo)  # Properly complains here.
class Foo(object):

    @foo
    def some_method(self) -> str:
        pass
fabioz commented 4 years ago

Actually, sorry, that doesn't work as I expected... I'm still checking for a usable workaround.

gvanrossum commented 4 years ago

You could put the check after the class inside if typing.TYPE_CHECKING:, which prevents it from working at runtime.

Or you could add a dummy method at the end of the class with the check on self.

fabioz commented 4 years ago

I did another simpler approach with class decorators which seems to work:

def typecheck_ifoo(foo: Type[IFoo]):
    return foo

@typecheck_ifoo
class Foo(object):
    ....

it requires to define one method for each interface, but I think that's a bit better than having it at the end or out of the class.

gvanrossum commented 4 years ago

But if class decorators were implemented properly that would give Foo the type Any.

fabioz commented 4 years ago

But if class decorators were implemented properly that would give Foo the type Any.

I agree it's not ideal... do you see any other way to have such a typecheck as a class decorator (with the current set of features from Mypy)?

-- if possible I'd really like to have this close to the start of the class (as a class decorator or __init__ and due to this bug the __init__ isn't an option).

gvanrossum commented 4 years ago

Is there a reason your class can't just inherit from the protocol? IIUC PEP 554 says that should be possible.

fabioz commented 4 years ago

Is there a reason your class can't just inherit from the protocol? IIUC PEP 554 says that should be possible.

I could, but it doesn't give me type errors if I forget to implement something.

i.e.:

class IFoo(Protocol):

    def method(self) -> str:...

    def method2(self) -> str:...

class Foo(IFoo):

    def method(self) -> str:...

    # method2 not there, but it doesn't complain
gvanrossum commented 4 years ago

It would complain if the methods were abstract in the Protocol class.

fabioz commented 4 years ago

I still couldn't get MyPy to complain with the code below... It only seems to complain if I call some function expecting IFoo where the typing is not satisfied, not just from a wrong class declaration (which is what I'm attempting to do).

from abc import abstractmethod
from typing import Protocol

class IFoo(Protocol):

    @abstractmethod
    def method(self) -> str:...

    @abstractmethod
    def method2(self) -> str:...

class Foo(IFoo):

    def method(self) -> str:...

    # method2 not there, but it doesn't complain
Akuli commented 4 years ago

try adding Foo() to the end of that file to actually create an instance, it will error

fabioz commented 4 years ago

Yes, I know it'll error if I try to instance or try to call some method, but the whole point is that I want to type-check some class to conform to some interface without having to create an instance or do some declaration at the end of the class (like what would happen when you implement some java interface).

Akuli commented 4 years ago

I'm confused. Why would you create classes without ever instantiating them (and getting the error)?

fabioz commented 4 years ago

Oh, I would instance it and get the error somewhere, but the error would likely only appear in some test case (if I'm doing a library for others) or some other place where I'm using that class (which may be very far apart from the class implementation), whereas I'd like to have such an error close to the class declaration/name (near the actual class keyword)...

Is this such an odd case? (Personally, I find it a bit odd to have protocols and not being able to know if a class implementation conforms to it in the class implementation itself, but maybe my views on how typing should work are too biased from java interfaces)

Akuli commented 4 years ago

I would say practicality beats purity. You still get the name of the class in the error message and even a list of missing methods.

fabioz commented 4 years ago

But then it's not much better than doing it in runtime if I have to look somewhere else -- just to note, I actually do have such a decorator that checks the class during runtime and gives the error at the class declaration: https://github.com/fabioz/pyvmmonitor-core/blob/master/pyvmmonitor_core/interface.py#L220, or I could use ABCs, but I thought type-checking would allow me to get the error earlier... it's definitely one of the selling points for me ;).