ksindi / implements

:snake: Pythonic interfaces using decorators
http://implements.readthedocs.io/
Apache License 2.0
33 stars 4 forks source link

Interface Type Annotations (typing.Protocol?) #28

Closed KyleKing closed 2 years ago

KyleKing commented 3 years ago

What is your recommendation for type annotations for an Interface? For the below trivial example, I want to annotate that quackery() returns classes of type Quackable

I believe one could create a global variable QUACKABLE_TYPE = Union[Duck, ...] and def quackery(sound: str) -> QUACKABLE_TYPE:. Is that the recommended approach? Or is there an alternative?

# Simplified Implements Example
from implements import implements, Interface

class Quackable(Interface):
    def quack(self) -> str:
        ...

@implements(Quackable)
class Duck():
    def __init__(self, sound: str) -> None:
        self.sound = sound

    def quack(self) -> str:
        return self.sound

# Type Annotation Use Case
# (Note: this will fail type checking because type checkers do not know that Duck implements Quackable)
def quackery(sound: str) -> Quackable:
    return Duck(sound)

duck = quackery('Quack')

Example using Protocol

from typing import Protocol, runtime_checkable

@runtime_checkable
class QuackableProtocol(Protocol):
    def quack(self) -> str:
        ...

# (See notes below)
@implements(QuackableProtocol)
class Duck():
    def quack(self) -> str:
        return 'sound'

The above example will fail because some Protocol methods lack signatures, but more generally because of the missing methods. See below output:

<built-in method __init_subclass__ of type object at 0x7fcab96c4990> -- no signature found for builtin <built-in method __init_subclass__ of type object at 0x7fcab96c4990>
<built-in method __subclasshook__ of type object at 0x7fcab96c4990> -- no signature found for builtin <built-in method __subclasshook__ of type object at 0x7fcab96c4990>
Traceback (most recent call last):
  File "test_implements.py", line 36, in <module>
    class Duck():
  File "/usr/local/Caskroom/miniconda/base/lib/python3.8/site-packages/implements.py", line 53, in _decorator
    raise NotImplementedError(
NotImplementedError: Found 10 errors in implementation:
- 'Duck' must implement '__class_getitem__' as a classmethod as defined in interface 'Quackable'
- 'Duck' must implement method '__class_getitem__(params)' defined in interface 'Quackable'
- 'Duck' must implement method '__init__(self, *args, **kwargs)' defined in interface 'Quackable'
- 'Duck' must implement method '__new__(cls, *args, **kwds)' defined in interface 'Quackable'
- 'Duck' must have class attribute '__abstractmethods__' defined in interface 'Quackable'
- 'Duck' must have class attribute '_abc_impl' defined in interface 'Quackable'
- 'Duck' must have class attribute '__slots__' defined in interface 'Quackable'
- 'Duck' must have class attribute '_is_protocol' defined in interface 'Quackable'
- 'Duck' must have class attribute '__parameters__' defined in interface 'Quackable'
- 'Duck' must have class attribute '_is_runtime_protocol' defined in interface 'Quackable'
with <class '__main__.Duck'>

I was thinking about if there was a way to use implements (for verbose signature checking) and typing.Protocols (for type annotations). Would it be of interest to have a ProtocolInterface base class (i.e. class ProtocolInterface(Protocol)) and logic so that the Protocol base class is excluded in the implements check? I believe something like the below example might work (full working, but hacky example here: https://github.com/KyleKing/implements/tree/hack/28):

from typing import Protocol # Note: introduced in 3.8, but I think there are backports for 3.6 and above)
class ProtocolInterface(Protocol):
    ...

def verify_methods(interface_cls, cls):

    excluded_names = dir(ProtocolInterface)
    members = inspect.getmembers(interface_cls, methods_predicate)
    filtered_members = [(name, method) for name, method in members if name not in excluded_names]
    for name, method in filtered_members:
        ...

https://github.com/ksindi/implements/blob/781f5ee51093bdfc0bdda915c985ef6d2d2c0f46/implements.py#L137-L144

KyleKing commented 3 years ago

I pushed an implementation here that I have been using for a bit: https://github.com/KyleKing/implements/commit/075ceb5fe9e9034e105aa9886f33e4be0cd40c58

If interested, I can add add documentation and tests and cleanup the code, but I'm also open to better approaches!

pshirali commented 3 years ago

Hi @KyleKing. Thanks for raising this request and pushing an implementation for it. I'm sorry for the delay in response!

I'll need another week or two before I go over this.

pshirali commented 3 years ago

Hello @KyleKing . I'm sorry for the long delay in reviewing this issue.

I analysed your post in two parts:

  1. Using a Union type to indicate return value [which represents all classes which quack]
  2. Inclusion of typing.Protocol

[1] Using Union

Yes. This is one way to solve the problem. It's not the perfect solution, but it does work. The downside is that the Union has to be independently declared, with the planned classes defined within them. It does not solve the problem in general. One has to maintain the Union(s) with relationships independently.

The real issue is that inspect.signature comparison doesn't handle subclassing, while type checks do.

-- Example 1

In the code below, Sedan and Hatchback inherit Car. mypy is comfortable with summon(klass) -> Car

class Car:
    def summon(self) -> 'Car':
        raise NotImplementedError()

class Sedan(Car):               # <--- inherits Car
    def summon(self) -> 'Sedan':
        return Sedan()

def summon(klass) -> Car:
    return klass().summon()

summon(Sedan)

from inspect import signature
car_sig = signature(Car.summon)
sedan_sig = signature(Sedan.summon)
print(car_sig = sedan_sig)               # <------------ False: signatures don't match

-- Example 2

Here, we remove the inheritance, and introduce implements. mypy doesn't complain, however, the code fails trying to enforce signature checks.

from implements import implements

class Car:
    def summon(self) -> 'Car':
        raise NotImplementedError()

@implements(Car)
class Sedan:
    def summon(self) -> 'Sedan':
        return Sedan()

def summon(klass) -> Car:
    return klass().summon()

summon(Sedan)

Error:

'Sedan' must implement method 'summon(self) -> 'Car'' defined in interface 'Car'
with <class '__main__.Sedan'>

-- Example 3

Here, we'll utilise Union to mitigate the problem seen in Example 2. implements won't complain purely because the signature summon(self) -> CAR_TYPES matches between the interface and implementation.

Drawbacks:

  1. CAR_TYPES is added maintenance and can independently change [could be error prone]
  2. It works only for this specific case. It'd be hard to define a scenario where Sedan.summon should return Sedan, and Hatchback.summon should return Hatchback.
from implements import implements
from typing import Union

CAR_TYPES = Union['Sedan', 'Hatchback']

class Car:
    def summon(self) -> CAR_TYPES:
        raise NotImplementedError()

@implements(Car)
class Sedan:
    def summon(self) -> CAR_TYPES:
        return Sedan()

@implements(Car)
class Hatchback:
    def summon(self) -> CAR_TYPES:
        return Hatchback()

def summon(klass) -> Car:
    return klass().summon()

summon(Sedan)
summon(Hatchback)

[2] typing.Protocol

  1. In your Example using Protocol code snippet, QuackableProtocol.quack and Duck.quack have the same signature quack(self) -> str. From the examples above, I'd believe that the problem rests with signature mismatch, when trying to solve for both interface enforement and type-checking. In this scenario, implements wouldn't be complain since the signatures do match. Hence, I was unable to understand what typing.Protocol was meant to solve in this case.
  2. In the suggested fix, the line excluded_names = dir(ProtocolInterface) also includes attributes which are common between the dummy class and the _ProtocolInterface. Because of this, some implemented magic methods from the interface may not get enforced in the implementation. Example: If the interface implemented __eq__, it'd have been enforced earlier, but it'd get excluded now.

In verify_methods, the logic could be:

class_members = inspect.getmembers(interface_cls, methods_predicate)
proto_members = inspect.getmembers(_ProtocolInterface, methods_predicate)
proto_unique_names = set([n for n, v in proto_members])      # skip only those which are unique to _ProtocolInterface
filtered_members = [(n, m) for (n, m) in class_members if n not in proto_unique_names]

For pt.(1) above, could you please help me understand what I missed? Thank you :)

KyleKing commented 3 years ago

Thanks for the reply and detailed analysis! My filter for excluded_names was just a quick proof of concept and getting the set-difference is much better!

My use case is an MVC-like application which has functionality that can operate on any implementation of a type of View interface. Below is a very trivial example of a function that can display any class that implement the "PopUpView" interface. This example relies on a runtime type checker (beartype or Typeguard) and typing.Protocol and ultimately works to catch the signature error that Notification.show is incorrect.

My first comment was code that should work, but fails because of the inheritance from Protocol and the dunder classes that aren't part of the actual interface. The below example is one that would have been better if implements could have caught the error earlier. Not to mention, that the @implements(PopUpView) syntax much more clearly indicates interface implementation

from typing import Protocol, runtime_checkable
from beartype import beartype

@runtime_checkable
class PopUpView(Protocol):
    def show(self) -> None:
        ...

# @implements(PopUpView)
class LinuxNotification:
    def show(self, cat: str) -> None:
        ...

@beartype
def display_popup(view: PopUpView) -> None:
    view.show()

display_popup(LinuxNotification())
❯ poetry run python tmp.py
Traceback (most recent call last):
  File "~/tmp.py", line 18, in <module>
    display_popup(LinuxNotification())
    │             └ <class '__main__.LinuxNotification'>
    └ <function display_popup at 0x104b918b0>
  File "<string>", line 34, in display_popup
  File "~/tmp.py", line 16, in display_popup
    view.show()
    └ <__main__.LinuxNotification object at 0x104b9e0d0>
TypeError: show() missing 1 required positional argument: 'cat'

The other thing that implements provides over typing.Protocol is checking that the whole type signature matches. (i.e. if cat: str was in the Interface, cat: int would fail)

Let me know if these examples help!

KyleKing commented 2 years ago

Just following up. What do you think the next step would be? Would you be open to merging a code snippet like this? Do you have any follow-up questions, or do you know of a better way?

Thank you for your input so far!

KyleKing commented 2 years ago

I ended up relying on mypy for implementation enforcement. The below snippet is adapted from https://github.com/beartype/beartype/issues/117#issuecomment-1086875109

from beartype import beartype
from typing import runtime_checkable
from beartype.typing import Protocol as RuntimeProtocol

@runtime_checkable
class Test(RuntimeProtocol):
    something: int

    # When not commented:
    #   > beartype doesn't catch that the type signature is wrong
    #   > MyPy catches this error
    #   > pytype doesn't raise any errors
    #   > pyright always thinks TestImplementation won't work
    def bar(self, name: str) -> None:
        ...

class TestImplementation:
    # When commented out:
    #   > beartype prints a generic error
    #   > MyPy provide specifics
    #   > pytype doesn't raise any errors
    #   > pyright always thinks TestImplementation won't work
    something = 1

    def bar(self) -> None:
        ...

@beartype
def test(x: Test) -> None:
    print(x)

test(TestImplementation())