Reparametrize managers without explicit type parameters #1169

ljodal · 2022-09-27T15:23:37Z

This extracts the reparametrization logic from #1030 in addition to removing the codepath that copied methods from querysets to managers. That code path seems to not be needed with this change.

This should properly fix #1022 as that keeps popping up from time to time when managers without type parameters are used.

ljodal · 2022-09-27T15:29:19Z

In addition to new type errors being discovered in our codebase, I've seen one new side-effect of this, which I'm unsure if we should consider a problem or not. Say you have a manager defined like this:

from typing import TypeVar
from django.db import models

T = TypeVar("T", bound="Model")

class MyManager(models.Manager[T]):
    pass

class MyModel(models.Model):
    pass

If you subclass this without specifying generics, like this:

class SecondManager(MyManager):
    pass

you'll get an error saying Type argument "T" of "MyManager" must be a subtype of "MyModel" [type-var].

But as I'm writing this I realize I might be able to make the logic smart enough to pick the "last" parameter in the MRO stack, if that sounds reasonable? Then SecondManager should be interpreted as SecondManager[T: MyModel] rather than SecondManager[T: Model]. I'll take a stab and see :)

ljodal · 2022-09-27T18:10:09Z

tests/typecheck/managers/test_managers.yml

+    out: |
+        myapp/models:4: error: Return type "MyModel" of "create" incompatible with return type "_T" in supertype "BaseManager"
+        myapp/models:5: error: Incompatible return value type (got "_T", expected "MyModel")


This is the failing test I mentioned in the PR description. I decided to fix it like this, basically saying this is an error. I think it should be fine to raise an error if you've restricted the type of an overridden method but the type vars of to the manager class

ljodal · 2022-09-27T18:11:13Z

[...]
But as I'm writing this I realize I might be able to make the logic smart enough to pick the "last" parameter in the MRO stack, if that sounds reasonable? Then SecondManager should be interpreted as SecondManager[T: MyModel] rather than SecondManager[T: Model]. I'll take a stab and see :)

I've implemented this fix now, so I think this should be good to go. It does not cause any new issues that I've seen in our codebase other than surfacing more type errors that were previously hidden behind implicit Anys

sobolevn

Can you please rebase your branch?
I've just merged a similar PR, I want to be sure that it is fine.

mypy_django_plugin/transformers/managers.py

sobolevn

Oh, you just did :)

sobolevn · 2022-09-29T13:14:10Z

@flaeppe any comments?

flaeppe

I only have one thought, I'm a bit unsure about it though

flaeppe · 2022-09-29T13:18:56Z

mypy_django_plugin/transformers/managers.py

+    is_missing_params = (
+        len(parent_manager.args) == 1
+        and isinstance(parent_manager.args[0], AnyType)
+        and parent_manager.args[0].type_of_any is not TypeOfAny.explicit


There's also a TypeOfAny.from_omitted_generics. Not sure what we should use here, but we might perhaps be a bit more narrow if we do TypeOfAny.from_omitted_generics?

If it even works, might be cases I'm forgetting..

Otherwise we could overwrite something like TypeOfAny.from_error etc etc

Aight, I'll take a look and see what type of any we actually get in these cases and make it specific to that if possible :)

This will trigger an error if disallow_any_generics = True is set.
I am not sure we want this.

I'm not following @sobolevn, you mean as the code is written now or if using TypeOfAny.from_omitted_generics?

I don't think that hits the new hook here, as that's (as far as I know) only called for class MyClass: statements in code. Related managers are handled elsewhere I think?

Sorry for causing so much confusion :)

I was thinking about cases where our users have disallow_any_generics = True set, current implementation looks like it can still cause this error. Let's add a test case with disallow_any_generics = True set. I am pretty sure that we won't be able to fix / suppress this error, because it happens too early: https://github.com/python/mypy/blob/ddd917767aa8135f3b1aeef47b0bb0616a4b63fb/mypy/typeanal.py#L1462

But, we can at least be sure: what happens and what to expect.
Maybe even mention this in the docs 🤔

Ah, okay :) I'll add a test case!

I don't think we should try to suppress the error message if possible though, if the user has enabled that flag I'd expect them to want a warning. At least in our codebase the implicit generics we add are less specific than what you'd manually add, so I consider this a sort of in-between solution until all of the project is fully typed (we're working in a fairly large codebase, gradually adding types)

Hmm, it seems like this interacts a bit weirdly with disallow_any_generics = True. You'll get the expected warning from mypy and it seems like whatever we do in the hook is overwritten and the return types are changed back to Any 🤔 I added a test that demonstrates this behaviour

Ah, I dug a bit deeper and when that option is enabled the Any type is changed to TypeOfAny.from_error, so the hook doesn't do anything. I think that's fair enough if you've enabled that option?

flaeppe · 2022-09-29T13:25:00Z

mypy_django_plugin/lib/helpers.py

-    return api.anal_type(t)
-
-
-def copy_method_to_another_class(


How great, we finally get rid of this one!

sobolevn · 2022-09-30T07:02:56Z

tests/typecheck/managers/test_managers.yml

    installed_apps:
        - myapp
+    out: |
+        myapp/models:4: error: Return type "MyModel" of "create" incompatible with return type "_T" in supertype "BaseManager"


This looks like a problem: BaseManager defines create() -> [T: Model], and our ModelManger(BaseManager) defines it as create() -> MyModel. Why is that an error?

Refs #1169 (comment)

Hmm, yeah, I can dig a bit deeper into this

The error here is exactly the same as you'd get from mypy if you did this:

from typing import TypeVar from django.db import models T = TypeVar("T", bound=models.Model) class MyManager(models.Manager[T]): def create(self, **kwargs) -> "MyModel": ... class MyModel(models.Model): ...

So it's expected based on what the plugin is doing, but I guess it might be surprising?

Yes, I now understand. The error is correct, but is very confusing.

Last question: can we somehow make it more clear? Or complete remove it (eventhough it iw correct)?

Hmm, I can see what I can do, but I'm not aware of any methods we have available to change mypy's warnings?

At the very least I can add a note about this in the readme. I might also be able to detect when this has happened and omit an additional error maybe, but I'm not sure if that helps much?

We can add a note saying that it probably should be class MyManager(models.Manager[MyModel]):, not class MyManager(models.Manager[T]):

How does that sound?

Or we can just ignore it for now :)
And wait for some complains first.

I don't think it's easy to detect these cases, given how many methods on Manager might be affected by this. I added a new section to the FAQ about this issue, so hopefully that should be enough.

This extracts the reparametrization logic from typeddjango#1030 in addition to removing the codepath that copied methods from querysets to managers. That code path seems to not be needed with this change.

This removes warnings when subclassing from something other than the base manager class, where the typevar has been restricted.

sobolevn

Thank you so much! Awesome feature.

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

ljodal commented Sep 27, 2022

View reviewed changes

ljodal force-pushed the fix/reparametrize-managers branch from 8824289 to 0ad212c Compare September 29, 2022 12:42

sobolevn reviewed Sep 29, 2022

View reviewed changes

mypy_django_plugin/transformers/managers.py Outdated Show resolved Hide resolved

mypy_django_plugin/transformers/managers.py Outdated Show resolved Hide resolved

sobolevn approved these changes Sep 29, 2022

View reviewed changes

flaeppe reviewed Sep 29, 2022

View reviewed changes

sobolevn reviewed Sep 30, 2022

View reviewed changes

ljodal added 8 commits September 30, 2022 21:53

Reparametrize managers without explicit type parameters

ffc48c0

This extracts the reparametrization logic from typeddjango#1030 in addition to removing the codepath that copied methods from querysets to managers. That code path seems to not be needed with this change.

Use typevars from parent instead of base

abff00e

Use typevars from parent manager instead of base manager

6bf8351

This removes warnings when subclassing from something other than the base manager class, where the typevar has been restricted.

Remove unused imports

f9e7c3b

Fix failed test

8643a92

Only reparametrize if generics are omitted

531da1c

Fix docstring

5b72d7e

Add test with disallow_any_generics=True

fc6406a

ljodal force-pushed the fix/reparametrize-managers branch from d0b95ef to fc6406a Compare September 30, 2022 19:53

Add an FAQ section and document disallow_any_generics behaviour

7079f33

ljodal force-pushed the fix/reparametrize-managers branch from de9416b to 7079f33 Compare October 3, 2022 17:16

sobolevn approved these changes Oct 3, 2022

View reviewed changes

sobolevn merged commit dde0f2f into typeddjango:master Oct 3, 2022

ljodal deleted the fix/reparametrize-managers branch October 3, 2022 19:21

This was referenced Jan 24, 2023

Upgrading to mypy 0.770 and Django-stubs 1.5, breaks mypy_django_plugin #344

Closed

mypy INTERNAL ERROR #459

Closed

AssertionError when extending from an abstract model that includes a manager #461

Closed

UnknownPlatypus mentioned this pull request Aug 21, 2025

Manager with generic QuerySet using TypeVar defaults #2776

Draft

Uh oh!

Reparametrize managers without explicit type parameters #1169

Reparametrize managers without explicit type parameters #1169

Uh oh!

Conversation

ljodal commented Sep 27, 2022

Uh oh!

ljodal commented Sep 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ljodal commented Sep 27, 2022

Uh oh!

sobolevn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sobolevn left a comment

Choose a reason for hiding this comment

Uh oh!

sobolevn commented Sep 29, 2022

Uh oh!

flaeppe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sobolevn Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sobolevn Oct 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sobolevn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sobolevn Sep 30, 2022 •

edited

Loading

sobolevn Oct 1, 2022 •

edited

Loading