Invalid self argument when overriding UpdateModelMixin.perform_update

[antonagestam]

Hi!

Since updating to 1.4.0 we see an odd issue when subclassing UpdateModelMixin: https://github.com/5monkeys/django-bananas/pull/61/checks?check_run_id=1855834150

bananas/drf/fencing.py:101: error: Invalid self argument
"FencedUpdateModelMixin" to attribute function "perform_update" with type
"Callable[[UsesQuerySet[_MT], BaseSerializer[_MT]], None]"  [misc]
            super().perform_update(serializer)

As a minimal reproduction, this produces the same error:

from rest_framework.mixins import UpdateModelMixin
from rest_framework.serializers import BaseSerializer

class Subclass(UpdateModelMixin):
    def perform_update(self, serializer: BaseSerializer) -> None:
        super().perform_update(serializer)

I suspect this is an unfortunate side-effect of this PR: https://github.com/typeddjango/djangorestframework-stubs/pull/131

[sobolevn]

Well, I am not sure that this is a bug actually.

Because, you change self type in mixin subclass and this means that you completely override its semantics. Can you please try to keep the correct self annotation here? What's the result?

[antonagestam]

@sobolevn Ah, hmm, I see your point. I'm losing the connection between self and the serializer argument. I think using advanced self types might not be a fit perfect here though, it kind of seems like mypy is interpreting it as "self is an instance of type", and not "self is an instance of the intersection between type(self) and the given type".

For instance, updating as you suggest to use the same semantics:

from typing import Protocol, TypeVar

from django.db.models import Model, QuerySet
from rest_framework.mixins import UpdateModelMixin
from rest_framework.serializers import BaseSerializer

_MT_co = TypeVar("_MT_co", bound=Model, covariant=True)

class UsesQuerySet(Protocol[_MT_co]):
    def get_queryset(self) -> QuerySet[_MT_co]:
        ...

class Subclass(UpdateModelMixin):
    def perform_update(
        self: UsesQuerySet[_MT_co], serializer: BaseSerializer[_MT_co]
    ) -> None:
        super().perform_update(serializer)

Gives the following, (I've noticed before that advanced self types work very poorly with super calls).

bananas/drf/reproduce.py:19: error: Argument 2 for "super" not an instance of argument 1 
[misc]
            super().perform_update(serializer)
            ^

Further, updating to reflect the real usecase and using a property on the subclass:

from typing import Protocol, TypeVar

from django.db.models import Model, QuerySet
from rest_framework.mixins import UpdateModelMixin
from rest_framework.serializers import BaseSerializer

_MT_co = TypeVar("_MT_co", bound=Model, covariant=True)

class UsesQuerySet(Protocol[_MT_co]):
    def get_queryset(self) -> QuerySet[_MT_co]:
        ...

class Subclass(UpdateModelMixin):
    attr: int

    def perform_update(
        self: UsesQuerySet[_MT_co], serializer: BaseSerializer[_MT_co]
    ) -> None:
        print(self.attr)
        super().perform_update(serializer)

Gives one more error, which is why I think mypy interprets as strictly UsesQuerySet, not Intersection[type(self), UsesQuerySet]:

bananas/drf/reproduce.py:21: error: "UsesQuerySet[_MT_co]" has no attribute "attr"  [attr-defined]
            print(self.attr)
                  ^

So the ergonomics aren't great. I noticed though that giving the subclass an abstract method for get_queryset() at least silences errors so it would be interesting to see if that could work as an alternate solution instead of using self types. This passes:

import abc
from typing import Generic, TypeVar

from django.db.models import Model, QuerySet
from rest_framework.mixins import UpdateModelMixin
from rest_framework.serializers import BaseSerializer

_MT_co = TypeVar("_MT_co", bound=Model, covariant=True)

class Subclass(UpdateModelMixin, Generic[_MT_co]):
    attr: int

    @abc.abstractmethod
    def get_queryset(self) -> QuerySet[_MT_co]:
        ...

    def perform_update(self, serializer: BaseSerializer[_MT_co]) -> None:
        print(self.attr)
        super().perform_update(serializer)

[sobolevn]

Great explanation! Mixins are ugly 😞

What solutions do you see?

[antonagestam]

@sobolevn Right, self types seem to bite me every time I think their going to save me ^^

Typing UpdateModelMixin (and siblings) with that abstract method might be a solution, but I guess it would have to be tested to make sure that _MT_co gets properly bound.

[antonagestam]

Turns out using an abstract method didn't work so well after all since that ends up overriding the implementation in the MRO and breaks when the concrete class defines queryset = ... instead of providing a get_queryset() method. So it's not a good workaround, but it might still be a viable solution for type hints.