python / typing

Python static typing home. Hosts the documentation and a user help forum.
https://typing.readthedocs.io/
Other
1.6k stars 234 forks source link

Allow subclassing without supertyping #241

Open dmoisset opened 8 years ago

dmoisset commented 8 years ago

This issue comes from python/mypy#1237. I'll try to make a summary of the discussion here

Some one reported an issue with an artificial example (https://github.com/python/mypy/issues/1237#issue-136162181):

class Foo:
    def factory(self) -> str:
        return 'Hello'

class Bar(Foo):
    def factory(self) -> int:
        return 10

and then with the following stub files (https://github.com/python/mypy/issues/1237#issuecomment-188710925):

class QPixmap(QPaintDevice):
    def swap(self, other: 'QPixmap') -> None: ...

class QBitmap(QPixmap):
    def swap(self, other: 'QBitmap') -> None: ...

Which mypy currently reports as erroneous: Argument 1 of "swap" incompatible with supertype "QPixmap"

These were initially was argued to be a correct error because they violate Liskov Substitution Principle (the same case by a change of return type, the second is a covariant argument change). The problem in this scenario is that the actual classes are not meant to be substituted (the first example is actually a wrapper for a non-virtual C++ function so they're not substitutable, and the second aren't supposed to be mixed together, just reuse part of the code). (see https://github.com/python/mypy/issues/1237#issuecomment-189490903)

There was also a suggestion that allow covariant args explicitly (in the same way that Eiffel allows it and adds a runtime check) with a decorator

class QBitmap(QPixmap):
    @covariant_args
    def swap(self, other: 'QBitmap') -> None: ...

My proposal instead was add what some dialects of Eiffel call "non-conforming inheritance" ("conforms" is the Eiffel word for what PEP-483 calls "is-consistent-with"). It is essentially a mechanism to use subclassing just as a way to get the implementation from the superclass without creating a subtyping relation between them. See https://github.com/python/mypy/issues/1237#issuecomment-231199419 for a detailed explanation.

The proposal is to haven Implementation in typing so you can write:

class QBitmap(Implementation[QPixmap]):
    def swap(self, other: 'QBitmap') -> None: ...

which just defines a QBitmap class with all the mothods copied from QPixmap, but without making one a subtype of the other. In runtime we can just make Implementation[QPixmap] == QPixmap

danmou commented 5 years ago

I've come across a simple case which hasn't been mentioned yet. Variadic arguments can be overridden without (technically) violating the Liskov principle. For example:

class A:
    def foo(self, *args: Any, **kwargs: Any) -> int:
        ...

class B(A):
    def foo(self, *args: Any, bar: int, **kwargs: Any) -> int:
        ...

is (to my understanding) in principle valid, but still gives an incompatible with supertype error with mypy.

The reason I want to do this is: Suppose a library defines the following classes

class A:
    def foo(self, *args: Any, **kwargs: Any) -> int:
        raise NotImplementedError

class B:
    def __init__(self, obj: A) -> None:
        self.obj = obj
    def get_foo(self) -> int:
        return obj.foo()

And user code could be something like the following:

class MyA(A):
    def foo(self, *args: Any, bar: int, **kwargs: Any) -> int:
        return bar

class MyB(B):
    def __init__(self, obj: MyA) -> None:
        super().__init__(obj)
    def get_foo(self) -> int:
        return obj.foo(bar=2)

Importantly, MyA and MyB could both be defined in different third-party libraries. The original library just defines the general interface and basic functionality. Should this example be accepted by the type-checker? If not, how could it be type annotated?

JelleZijlstra commented 5 years ago

Your example is actually unsafe, because a call to x.foo(bar="string") where x: A would be invalid if x is actually an instance of B.

danmou commented 5 years ago

@JelleZijlstra that's true. I guess then a better solution would be using Callable without specifying the arguments in the base class, e.g.:

class A:
    foo: Callable[..., int]

But of course this still means mypy won't flag something like A().foo() as invalid.

ziima commented 5 years ago

@gvanrossum

Maybe this pattern helps? This seems to be about the best you can do given that you don't want the base class to specify the method signatures.

Declaring the methods/attributes in mixins is fine if there are just a few of them, but you don't really want to do that when there are a lot of them, for example a mixins for TestCase.

Having

class MyTestMixin:
    assertEqual: Callable
    assertRaises: Callable
    ... # 10 other asserts I used in the mixin

is not really a way I want to go.

leonardoramirezr commented 4 years ago

Try with:

from typing import Type, TYPE_CHECKING, TypeVar

T = TypeVar('T')

def with_typehint(baseclass: Type[T]) -> Type[T]:
    """
    Useful function to make mixins with baseclass typehint
class ReadonlyMixin(with_typehint(BaseAdmin))):
    ...
```
"""
if TYPE_CHECKING:
    return baseclass
return object
Example tested in Pyright:
```python
class ReadOnlyInlineMixin(with_typehint(BaseModelAdmin)):
    def get_readonly_fields(self,
                            request: WSGIRequest,
                            obj: Optional[Model] = None) -> List[str]:

        if self.readonly_fields is None:
            readonly_fields = []
        else:
            readonly_fields = self.readonly_fields # self get is typed by baseclass

        return self._get_readonly_fields(request, obj) + list(readonly_fields)

    def has_change_permission(self,
                              request: WSGIRequest,
                              obj: Optional[Model] = None) -> bool:
        return (
            request.method in ['GET', 'HEAD']
            and super().has_change_permission(request, obj) # super is typed by baseclass
        )

>>> ReadOnlyAdminMixin.__mro__
(<class 'custom.django.admin.mixins.ReadOnlyAdminMixin'>, <class 'object'>)
minrk commented 2 years ago

A pattern I've been struggling with is async subclasses of a concrete base class, following the pattern:

class Thing:
    def count(self) -> int: ...
    def name(self): -> str: ...

class AsyncThing(Thing):
    def count(self): -> Awaitable[int]: ...
    def name(self): -> Awaitable[str]: ...

where several methods on the subclass return a wrapper of the same type as the parent (in this case, Awaitable). This feels like it should be easy. It certainly is easy to implement, but I don't understand enough yet. In particular, it seems like Awaitable should be something like a TypeVar, e.g.

WT = TypeVar("T") # 'wrapper' type, may be Null for no wrapping
class Thing(Generic[WT]):
    def __init__(self: Thing[Null]): # default: no wrapper
    def count(self) -> WT[int]: ... # actually int
    def name(self): -> WT[str]: ...

    def not_wrapped(self): -> int # always int, not wrapped

    def derivative_method(self) -> WT[str]:
        "Not overridden in subclass, but should match return type of name"
        return self.name()

class AsyncThing(Thing[Awaitable]):
    def count(self): -> Awaitable[int]: ...
    def name(self): -> Awaitable[str]: ...

but you can't use getitem on TypeVars, so it seems they can't actually be used in this way. Derivative methods that didn't previously need implementations, now also need empty implementations, just to get the updated types.

Is there any way to accomplish something like this, or is it better to just forego types altogether if one follows this kind of pattern?

JelleZijlstra commented 2 years ago

@minrk that pattern is unsafe, because you're violating the Liskov principle. (If you do thing.count() where thing is of type Thing, you may get an Awaitable if thing is actually an AsyncThing.)

I'd suggest not using inheritance and making AsyncThing instead an independent class that wraps Thing. You could use __getattr__ in that case to implement it, though for type checking purposes you'd probably need to explicitly implement every method.

An alternative is to just # type: ignore everything. That's what we do in typeshed for the redis library, which has a similar pattern where the pipeline class inherits from the normal client but wraps everything in a pipeline.

minrk commented 2 years ago

you're violating the Liskov principle

Sorry for misunderstanding, I thought that was the point of this Issue: subclassing without preserving Liskov is valid (and in my experience, hugely valuable).

I'd suggest not using inheritance and making AsyncThing instead an independent class that wraps Thing

Thanks for the suggestion. Using a HasA wrapper would be significantly more complex because, as you say, I'd have to provide empty implementations of all methods, just to declare the type, when the only concrete benefit would be satisfying mypy. So far, type: ignore seems to be the best compromise, so I'll go with that.