Skip to content
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

Fix implementation checks on generic interfaces #109

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,28 @@
------------------

* Added support for Python 3.10.
* #109: Non-generic implementers of generic interfaces are now correctly checked.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great changelog entry. 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* #109: Non-generic implementers of generic interfaces are now correctly checked.
* `#109 <https://github.com/ESSS/oop-ext/pull/109>`_: Non-generic implementers of generic interfaces are now correctly checked.


Previously checking if a non-generic class implements a generic interface always used to fail, as
the latter has a special method (``__class_getitem__``) that the former does't have, which led to
false negatives.

.. code-block:: python

class IFoo(Interface, typing.Generic[T]):
def Bar(self) -> T:
...


@ImplementsInterface(IFoo, no_check=True)
class Foo:
@Implements(IFoo.Bar)
def Bar(self) -> str:
return "baz"


AssertImplements(Foo, IFoo) # Ok now.
Comment on lines +13 to +25
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And without the no_check? Does the interface infra complain?

To appease mypy do we still have to declare TypeCheckingSupport?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And without the no_check? Does the interface infra complain?

Yes, it does complain.

To appease mypy do we still have to declare TypeCheckingSupport?

Yeah we still have to.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And without the no_check? Does the interface infra complain?

Yes, it does complain.

Why? How?



2.1.0 (2021-03-19)
------------------
Expand Down
2 changes: 1 addition & 1 deletion src/oop_ext/interface/_interface.py
Original file line number Diff line number Diff line change
Expand Up @@ -747,7 +747,7 @@ def AssertImplementsFullChecking(


# set of methods that might be declared in interfaces but should be not be required by implementations
_INTERFACE_METHODS_TO_IGNORE = {"__init_subclass__"}
_INTERFACE_METHODS_TO_IGNORE = {"__init_subclass__", "__class_getitem__"}
nicoddemus marked this conversation as resolved.
Show resolved Hide resolved


def _AssertImplementsFullChecking(
Expand Down
36 changes: 36 additions & 0 deletions src/oop_ext/interface/_tests/test_interface.py
Original file line number Diff line number Diff line change
Expand Up @@ -1077,3 +1077,39 @@ def AfterCaption(*args):

assert Foo.GetCaption() == "Foo"
assert Foo().GetValues("m") == [0.1, 10.0]


def testGenericInterface() -> None:
"""Generic interfaces need to support checking on both generic and non-generic implementers"""
from typing import FrozenSet, Generic, TypeVar

T = TypeVar("T", covariant=True)

class IFoo(Interface, Generic[T], TypeCheckingSupport):
# Class instance defined here as a workaround for this class to work in Python 3.6.
__abstractmethods__: FrozenSet = frozenset()
Comment on lines +1088 to +1090
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nicoddemus
Do you think we could drop python 3.6 support in oop-ext?

Copy link
Member

@nicoddemus nicoddemus Dec 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely, IMO we can do this for all of our OS projects, supporting >=3.7 or even >=3.10 only, if the former is too much work.

Comment on lines +1089 to +1090
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's drop Python 3.6 first.

Suggested change
# Class instance defined here as a workaround for this class to work in Python 3.6.
__abstractmethods__: FrozenSet = frozenset()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To drop Python 3.6:


def GetOutput(self) -> T: # type:ignore[empty-body]
...
Comment on lines +1092 to +1093
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To solve this new warning we are decorating interface methods with @abstractmethod, so mypy won't complain. Also let's make the example more comprehensive by also having an input parameter:

Suggested change
def GetOutput(self) -> T: # type:ignore[empty-body]
...
@abstractmethod
def GetOutput(self, v: T) -> T:
...


@ImplementsInterface(IFoo, no_check=True) # Will check later.
class GenericFoo(Generic[T]):
def __init__(self, output: T) -> None:
self.output = output

@Implements(IFoo.GetOutput)
def GetOutput(self) -> T:
return self.output

@ImplementsInterface(IFoo, no_check=True)
class NonGenericFoo:
@Implements(IFoo.GetOutput)
def GetOutput(self) -> int:
return 1

# This works out of the box.
AssertImplements(GenericFoo, IFoo)
AssertImplements(GenericFoo[str](output="foo"), IFoo)

# This only works if we skip the verification of `__class_getitem__` method.
AssertImplements(NonGenericFoo, IFoo)
Comment on lines +1111 to +1115
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a question.

What about AssertImplements(GenericFoo[str], IFoo)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes it raise an AssertionError with the following error:

E AssertionError: Method 'GetOutput' is missing in class '_GenericAlias' (required by interface 'IFoo')
E type object '_GenericAlias' has no attribute 'GetOutput'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we want to handle that?

AssertImplements(GenericFoo[str], IFoo[str]) also fail?

Loading