Add __class_getitem__ to support runtime type parameters for more classes

[Brady Kieffer]

We use the library django-stubs pretty extensively. Recently, they introduced a patch treating more types as Generics. To handle this on our side we either need to work mypy in a decently annoying manner. Much like issue #31223, I propose we add a __class_getitem__ to the following classes:

• BaseModelAdmin
• Options
• Lookup
• Field

Much like before __class_getitem__ would purely be for mypy type checks and a no-op for the django runtime. Only other thing to note, for us the most important class to update would be BaseModelAdmin, so if there are reasons not to add __class_getitem__ to the other classes, I can forgo that change.

Related patch - Fixed #31223: ​https://github.com/django/django/pull/12405
django-stubs: ​https://github.com/typeddjango/django-stubs

[Carlton Gibson]

Hi Brady. Thanks for the input.

I'm a bit sceptical about this. #31223 was fixed on the basis that:

1. It'll change our life
2. It's all we need.

No 2 now (already) seems false.

More though, this is more than a bit boilerplatey...

    @classmethod
    def __class_getitem__(cls, *args, **kwargs):
        return cls

Question: why isn't this mypy's issue? i.e. why can't we demand better type inference in this case?
(Type annotations cool. Unnecessary boilerplate, not so much…)
What do the mypy people say about that type of case?

Thanks for helping me to understand!

[Brady Kieffer]

Hi Carlton,

Thanks for the speedy response. I guess I just want to preface everything here by saying I pretty squarely fall into the bucket of being a user of both Django + django-stubs. I definitely agree that the code is a bit boilerplate-y. I don't actually think this is a mypy issue, it's more about how we choose to reconcile the true implementation of Django with third party libraries attempting to type it in a simple manner. So, when it comes to:

@classmethod
def __class_getitem__(cls, *args, **kwargs):
    return cls

I tend to agree that it's pretty annoying that we have to modify Django to make it usable with django-stubs. In defense of mypy and typing in general, there's a more idiomatic approach to this problem by defining a class as a Generic (​docs for Generics here). I _believe_ the reason that adding Generic as a base for QuerySet + Manager was avoided because it could have an impact downstream but, I don't really know why this was decided. So, mypy would recommend we use Generic, which we likely will not, and it sort of leaves open the question of what should we reasonably expect to do with Django if we want to support third party typing libraries. I'm not super opinionated, but I would definitely rather not have to monkeypatch every codebase that uses django-stubs if it can be avoided. If we do go down this path though, I would agree that your second point will likely never be satisfied.

Type checking is really cool, and it's saved us from some very weird bugs. But, it definitely feels like we're in a weird middle ground where there isn't "the one perfect way" of doing it.

[Carlton Gibson]

Hi Brady. Thanks for the follow-up.

In defense of mypy and typing in general, there's a more idiomatic approach to this problem by defining a class as a Generic (​docs for Generics here).

First: Not criticising you here! :)

This thought seems to me to have lost the wood for the trees. Translated it reads: Add a lot of noise to your code in order to avoid adding a lot of (more boilerplate) noise to your code. Typing was meant to be an optional add-on to Python. I can't (yet) see that it's not an issue with mypy that we have to add this stuff when (surely) cls could be inferred as the default for __class_getitem__. (Truly this baffles me.)

Don't suppose you know the best place to make this kind of point to the mypy maintainers do you? (I'll have to look around.)

I'm going to have to close this as needsinfo. I can't see that accepting this here is compatible with the previous Technical Board decision. I think we need to go back to the DevelopersMailingList to ask for review.

[Carlton Gibson]

Hi Brady.

I was following up with mypy folks here, trying to workout if there really is no option beyond the trivial __class_getitem__ implementation.

It was asked whether from __future__ import annotations ​as per PEP 563 would stop the interpreter error.
Can you comment on that?
Thanks.

[Brady Kieffer]

Hi Carlton,

I didn't think you were criticizing, I actually agree with your worry about the boilerplate :)

As for the from __future__ import annotations suggestion - that won't actually fix the issue in this case. The problem is that we're encountering a type error for the following snippet of code:

from django.contrb import admin
from myapp import models as db

class MyModelAdmin(admin.ModelAdmin):
    pass

admin.site.register(db.MyModel, MyModelAdmin)

In django-stubs 1.7.0 BaseModelAdmin was made generic to allow properly typing model admin methods that expect a model class of a certain type, ​link to this patch. Due to this change, when running mypy in strict mode, the above snippet won't work because admin.ModelAdmin needs a type parameter. So, we'd need to write the following code:

from django.contrb import admin
from myapp import models as db

class MyModelAdmin(admin.ModelAdmin[db.MyModel]):
    pass

admin.site.register(db.MyModel, MyModelAdmin)

which breaks because admin.ModelAdmin does not have the __class_getitem__ attribute. So, to fix this issue I think the paths forward would be one of the following:

1. Have BaseModelAdmin inherit directly from Generic, I think this is the most correct approach but I'm not 100% certain (getting input from the mypy devs here would be a good idea). This would be the least boilerplate-y and Generic was added in Python 3.5, so this should be fine for all supported versions of Django. The only downside I can think of with this approach is if there would be some weirdness with having Generic for all of these classes? I don't really know
2. Directly bolt on a no-op for __class_getitem__ to the BaseModelAdmin class (as I did in the proposed patch), which would allow me to write class MyModelAdmin(admin.ModelAdmin[db.MyModel]): ... . This directly solves the issue at hand but is boilerplate-y and doesn't address the underlying problem going forward.
3. Revert the changes in django-stubs to account for the fact that this functionality isn't yet supported in Django (I'm not a fan of this one). Would stop typechecking from breaking for us but we'd lose out on a lot of powerful typing functionality, something I think django-stubs has done very well.
4. Monkey-patch BaseModelAdmin directly. I'm not a fan of this approach either because it feels like we'd just be kicking the can down the road. Also, it'll be more work for my team to maintain divergent behavior. Although, the implementation really isn't difficult, it could just be done like so:

   from django.contrib.admin import options
   
   options.BaseModelAdmin.__class_getitem__ = lambda cls, *args, **kwargs: cls # type: ignore
   

I'm personally a fan of (1) because it avoids the issue of boilerplate entirely + leverages builtin types.

Refs:
django stubs: ​https://github.com/typeddjango/django-stubs
mypy generics: ​https://mypy.readthedocs.io/en/stable/generics.html#generics
django stubs issue related to this: ​https://github.com/typeddjango/django-stubs/issues/507

[Carlton Gibson]

Hi Brady. Thanks for the follow-up.

Given the previous discussion, this is a wontfix issue. The correct workflow to change that is to go via the DevelopersMailingList (see TicketClosingReasons/DontReopenTickets)

Of your options, I'll just say here, that:

• I worry about 1 — We're enforcing a structure that's never been there and never been needed. Typing is meant to serve the code, not the other way round. (I'm not convinced that ModelAdmin is best modelled as a statically typed generic...)
• 4 actually seems like a good option here. This stuff is still evolving. If we change Django now and find it goes a different way, we'll be stuck.

What do the core devs say on this kind of example? Typing is meant to be optional, an add-on. Surely the goal is to be able to work with code such as Django's without the requirement that we retrofit all this stuff?

The promise of stubs was to keep the typing stuff out of the code: it seems like a valid bug report on mypy or typing that "We can't implement X, Y, Z in django-stubs without requiring change A, B, C in Django." — If you make that kind of point, what response do you get?

Thanks again.

[Adam Johnson]

Just to add my 2c - I think option 4, having django-stubs monkey-patch django, is a good approach. "Kicking the can down the road" is probably a good approach whilst things are still evolving.