Manager with generic QuerySet using TypeVar defaults
#2776
Conversation
The `self.manager_info` can trigger a deferral pass, leaving an incomplete model TypeInfo. Splitting this in 2 makes it idempotent
I've done this in the same pr because now that we infer more correctly the queryset type, we were infering incomplete types more often
Need to comment on the related issue to ask for generic params to be provided
| @overload | ||
| def __get__(self, instance: None, cls: Any | None = None) -> Self: ... | ||
| def __get__(self, instance: None, cls: Any | None = None) -> ReverseManyToOneDescriptor[_To, _To_QS]: ... |
There was a problem hiding this comment.
I wanted to return Self[_To, _To_QS] but it's not supported. It's probably fine because I don't expect people to subclass this ?
| creation_counter: int | ||
| auto_created: bool | ||
| use_in_migrations: bool | ||
| name: str | ||
| model: type[_T] | ||
| _queryset_class: type[_QS] |
There was a problem hiding this comment.
I needed to expose this symbol because I use it in the plugin.
If we don't want that, I can also add a type-ignore in the plugin code but I think it's fine to have it because it's a core component of a manager and very unlikely to disappear
| def add_through_table_managers(self, through_model: TypeInfo) -> None: | ||
| """The `self.manager_info` lookup might trigger a deferral pass so this has to be idempotent""" |
There was a problem hiding this comment.
This solves an issue that was already present but happening less often I suppose. With the typevar default, it started to cause some tests to fail.
Sometimes, create_through_table_class would fail creating the objects symbol, leaving an incomplete table definition and later mypy passes were never filling the missing part
| def reparametrize_any_queryset_hook(ctx: ClassDefContext) -> None: | ||
| """ | ||
| Add implicit generics to queryset classes that are defined without generic. | ||
|
|
||
| Eg. | ||
|
|
||
| class MyQuerySet(models.QuerySet): ... | ||
|
|
||
| is interpreted as: | ||
|
|
||
| _T = TypeVar("_T", bound=Model, covariant=True) | ||
| class MyQuerySet(models.QuerySet[_T]): ... | ||
| """ | ||
| reparametrize_generic_class(ctx, fullnames.QUERYSET_CLASS_FULLNAME) |
There was a problem hiding this comment.
I've added the automatic parametrisation because without it, a bunch of places where it was before Queryset[MyModel] became MyQueryset[Any] for untyped custom queryset.
There are still a lot of untyped django package around so this makes them work with little additional effort
| # We have to define different typevars here otherwise it conflicts with the ones above | ||
| _T2 = TypeVar("_T2", bound=Model, covariant=True) | ||
| _QS2 = TypeVar("_QS2", bound=QuerySet[Any], covariant=True, default=QuerySet[_T2]) | ||
|
|
||
| class EmptyManager(Manager[_T2, _QS2]): | ||
| def __init__(self, model: type[_T2]) -> None: ... |
There was a problem hiding this comment.
I'm really not quite sure why but If I reuse the T and _QS typevar from above, some tests start to fail
For some reason, it was affecting Manager inference and the _T``1 queryset defaults where not all associated.
reveal_type(models.Manager[MyModel])
-note: Revealed type is "django.db.models.manager.Manager[django.db.models.base.Model, django.db.models.query.QuerySet[_T`1, _T`1]]"
+note: Revealed type is "django.db.models.manager.Manager[django.db.models.base.Model, django.db.models.query.QuerySet[django.db.models.base.Model, django.db.models.base.Model]]
UnknownPlatypus
changed the title
Manager with generic QuerySet using TypeVar defaults
I have made things!
This is redo for #1270 using
TypeVardefaults to implement the change in a non-breaking way.This is not completely ready, I opening it to get some feedback and because there are still a few issues to address.
The PR currently passes tests but I've tried it on my work codebase and some related issues have resurfaced (not caused by this PR, but by the fact that a lot of places previously ignored are now checked)
Most notably:
.annotate()in custom queryset method looses typeManager(cf Reparametrize managers without explicit type parameters #1169) which causes exactly the same kind of issue with overrides ofmodels.Manager.get_querysetwhich are rather common because documentedAlso Prefetch's to_attr raises "Model" has no attribute "prefetched_field" #795 was more present-- Fixed in Initial support forto_attrinferrence inPrefetchcalls #2779)I think this is step in the right direction but I'm a bit afraid it might cause some churn if we don't address the related issues first.
Related issues
Closes #863 (tracking Issue)
Closes #1270 (supersede prior pr addressing #863)
Also fixes a bunch of related issues, I've added regression tests for them:
Closes #2602
Closes #1067
Closes #1023
Closes #728
Closes #1918