`(ManagerClass).from_queryset` is not creating a valid base class

[asottile-sentry]

Bug report

so I realize this is not the best of reports -- all attempts I've made to make a minimal case do not reproduce the bug so I suspect it is something to do with how our repository is set up somehow.

I have thrown together a reproducible docker image to show the problem:

FROM python:3.11-slim

RUN : \
    && apt-get update \
    && DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends dumb-init git \
    && apt-get clean \
    && rm -rf /var/lib/apt/lists/*

WORKDIR /src

RUN : \
    && git clone https://github.com/getsentry/sentry --branch master . \
    && git checkout 0bcc06037ce710ff3a97346f8d956766c4cb4787

ENV PATH=/src/.venv/bin:$PATH PIP_NO_CACHE_DIR=1
RUN : \
    && python3 -m venv .venv \
    && pip install -r requirements-dev-frozen.txt \
    # ensure we reproduce the problem without our fork
    && pip uninstall -y sentry-forked-django-stubs \
    && pip install django-stubs==5.0.2 \
    && python3 -m tools.fast_editable --path .

RUN sentry init

# apply the "patch" which should allow it to have queryset methods
RUN : \
    && sed  -i \
        's/class BaseManager.*$/_base = DjangoBaseManager.from_queryset(BaseQuerySet)\nclass BaseManager(_base[M]):/g' \
        src/sentry/db/models/manager/base.py \
    && echo 'from typing import TYPE_CHECKING' | tee -a src/sentry/db/models/manager/base.py \
    && echo 'if TYPE_CHECKING: reveal_type(BaseManager.using_replica)' | tee -a src/sentry/db/models/manager/base.py

ENTRYPOINT ["dumb-init", "--"]
CMD ["mypy", "src/sentry/db/models/manager/base.py"]

after the patch is applied this line here should look like this (the dockerfile does this automatically):

_base = DjangoBaseManager.from_queryset(BaseQuerySet)
class BaseManager(_base[M]):

and the end of the file should have:

from typing import TYPE_CHECKING
if TYPE_CHECKING: reveal_type(BaseManager.using_replica)

this is one of the custom methods added by our custom base QuerySet

we technically use a fork of django-stubs -- but I've removed that in the docker image above to demonstrate that this is not caused by our patches there (though there will be some additional output because we use django's cache with strings in a ~somewhat unsafe way).

I build and run the docker image via:

docker build -t mypy-repro .
docker run --rm -ti mypy-repro

What's wrong

the full output of the above is:

full output

```console $ docker run --rm -ti mypy-repro src/sentry/db/models/manager/base.py:48: error: Variable "sentry.db.models.manager.base._base" is not valid as a type [valid-type] src/sentry/db/models/manager/base.py:48: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases src/sentry/db/models/manager/base.py:48: error: Invalid base class "_base" [misc] src/sentry/db/models/manager/base.py:169: error: Item "None" of "Field[Any, Any] | None" has no attribute "name" [union-attr] src/sentry/db/models/manager/base.py:181: error: Argument "version" to "set" of "BaseCache" has incompatible type "str"; expected "int | None" [arg-type] src/sentry/db/models/manager/base.py:193: error: Argument "version" to "set" of "BaseCache" has incompatible type "str"; expected "int | None" [arg-type] src/sentry/db/models/manager/base.py:208: error: Argument "version" to "delete" of "BaseCache" has incompatible type "str"; expected "int | None" [arg-type] src/sentry/db/models/manager/base.py:219: error: Item "None" of "Field[Any, Any] | None" has no attribute "name" [union-attr] src/sentry/db/models/manager/base.py:226: error: Argument "version" to "delete" of "BaseCache" has incompatible type "str"; expected "int | None" [arg-type] src/sentry/db/models/manager/base.py:230: error: Argument "version" to "delete" of "BaseCache" has incompatible type "str"; expected "int | None" [arg-type] src/sentry/db/models/manager/base.py:287: error: Argument "version" to "delete" of "BaseCache" has incompatible type "str"; expected "int | None" [arg-type] src/sentry/db/models/manager/base.py:293: error: Argument "version" to "get" of "BaseCache" has incompatible type "str"; expected "int | None" [arg-type] src/sentry/db/models/manager/base.py:381: error: Argument "version" to "get_many" of "BaseCache" has incompatible type "str"; expected "int | None" [arg-type] src/sentry/db/models/manager/base.py:459: error: Argument "version" to "delete" of "BaseCache" has incompatible type "str"; expected "int | None" [arg-type] src/sentry/db/models/manager/base.py:575: note: Revealed type is "Any" Found 13 errors in 1 file (checked 1 source file) ```

the important bits of that are:

$ docker run --rm -ti mypy-repro
src/sentry/db/models/manager/base.py:48: error: Variable "sentry.db.models.manager.base._base" is not valid as a type  [valid-type]
src/sentry/db/models/manager/base.py:48: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
src/sentry/db/models/manager/base.py:48: error: Invalid base class "_base"  [misc]
...
src/sentry/db/models/manager/base.py:575: note: Revealed type is "Any"

showing that the result of the from_queryset manager isn't suitable as a base class (for whatever reason!)

I've done a bit of debugging and thrown some prints in mypy and django-stubs:

diff --git a/mypy/typeanal.py b/mypy/typeanal.py
index 3f4b86185..70181dbed 100644
--- a/mypy/typeanal.py
+++ b/mypy/typeanal.py
@@ -923,6 +923,7 @@ class TypeAnalyser(SyntheticTypeVisitor[Type], TypeAnalyzerPluginInterface):
             message = 'Cannot interpret reference "{}" as a type'
         if not defining_literal:
             # Literal check already gives a custom error. Avoid duplicating errors.
+            print(f'failing with error! {name}')
             self.fail(message.format(name), t, code=codes.VALID_TYPE)
             for note in notes:
                 self.note(note, t, code=codes.VALID_TYPE)

diff --git a/mypy_django_plugin/transformers/managers.py b/mypy_django_plugin/transformers/managers.py
index cbf80704..b4e3e531 100644
--- a/mypy_django_plugin/transformers/managers.py
+++ b/mypy_django_plugin/transformers/managers.py
@@ -312,16 +312,19 @@ def create_new_manager_class_from_from_queryset_method(ctx: DynamicClassDefConte
     manager_sym = semanal_api.lookup_current_scope(ctx.name)
     if manager_sym and isinstance(manager_sym.node, TypeInfo):
         # This is just a deferral run where our work is already finished
+        print('django-stubs: already defined')
         return

     new_manager_info = create_manager_info_from_from_queryset_call(semanal_api, ctx.call, ctx.name)
     if new_manager_info is None:
         if not ctx.api.final_iteration:
             ctx.api.defer()
+            print(f'django-stubs: deferred {ctx.name}')
         return

     # So that the plugin will reparameterize the manager when it is constructed inside of a Model definition
     helpers.add_new_manager_base(semanal_api, new_manager_info.fullname)
+    print(f'django-stubs: produced new manager class: {new_manager_info.fullname}')

 def register_dynamically_created_manager(fullname: str, manager_name: str, manager_base: TypeInfo) -> None:

this produces the following:

$ rm -rf .mypy_cache && mypy src/sentry/db/models/manager/base.py 
failing with error! builtins.NotImplemented
failing with error! builtins.NotImplemented
failing with error! builtins.NotImplemented
failing with error! builtins.NotImplemented
failing with error! builtins.NotImplemented
failing with error! builtins.NotImplemented
failing with error! builtins.NotImplemented
failing with error! _pytest.compat.LEGACY_PATH
failing with error! pydantic.types.T
failing with error! pydantic.types.T
failing with error! pydantic.types.T
failing with error! pydantic.types.T
failing with error! pydantic.types.T
failing with error! pydantic.types.T
failing with error! pydantic.types.T
failing with error! pydantic.types.T
failing with error! pydantic.types.T
failing with error! pydantic.types.T
failing with error! pydantic.types.T
failing with error! pydantic.types.T
failing with error! typing_extensions.TypedDict
failing with error! typing_extensions.TypedDict
failing with error! annotation
django-stubs: deferred _base
failing with error! sentry.db.models.manager.base._base
failing with error! rest_framework.serializers._MT
django-stubs: produced new manager class: sentry.db.models.manager.base.BaseManagerFromBaseQuerySet
failing with error! rest_framework.serializers._MT
failing with error! pipeline_cls
src/sentry/db/models/manager/base.py:48: error: Variable "sentry.db.models.manager.base._base" is not valid as a type  [valid-type]
...

to me this ~maybe points at a bug in mypy in that it's producing the error but with an extra pass it would have resolved the base class? maybe it's somehow a bug here in that we need to indicate the base class should be a higher priority? I don't know

How is that should be

hopefully clear from above, the class should be a suitable base class and the methods from the queryset should be usable.

in a minimal example everything annoyingly works as expected:

diff --git a/tests/typecheck/managers/querysets/test_from_queryset.yml b/tests/typecheck/managers/querysets/test_from_queryset.yml
index 2cfbe2af..8dbe1f1a 100644
--- a/tests/typecheck/managers/querysets/test_from_queryset.yml
+++ b/tests/typecheck/managers/querysets/test_from_queryset.yml
@@ -791,6 +791,39 @@

               StrCallable = BaseManager.from_queryset(ModelQuerySet, class_name=str(1))

+-   case: from_queryset_inheritable
+    main: |
+        from myapp.models import Concrete
+        reveal_type(Concrete.objects.qs_meth())
+        reveal_type(Concrete.objects.manager_meth())
+    installed_apps:
+        - myapp
+    files:
+        - path: myapp/__init__.py
+        - path: myapp/models.py
+          content: |
+            from typing import TypeVar, ClassVar, Self
+            from django.db import models
+            from django.db.models.manager import Manager
+
+            M = TypeVar("M", bound=models.Model, covariant=True)
+
+            class CustomQuerySet(models.QuerySet[M]):
+                def qs_meth(self) -> int:
+                    return 1
+
+            _base = Manager.from_queryset(CustomQuerySet)
+
+            class CustomBase(_base[M]):
+                def manager_meth(self) -> str:
+                    return 'hi'
+
+            class Concrete(models.Model):
+                objects = CustomBase()
+    out: |
+        main:2: note: Revealed type is "builtins.int"
+        main:3: note: Revealed type is "builtins.str"
+
 -   case: test_queryset_arg_as_unsupported_expressions
     main: |
         from typing import Union, Generic, TypeVar

System information

• OS:
• python version: 3.11.9
• django version: 5.0.6
• mypy version: 1.10.0
• django-stubs version: 5.0.2
• django-stubs-ext version: 5.0.2

[flaeppe]

If I read what you wrote correctly, here are some relevant references:

Some time ago I attempted to introduce support for inheriting .from_queryset generated managers via the plugin, but got stuck: #1699. It also links upstream to: https://github.com/python/mypy/issues/8897#issuecomment-634241682

(I don't think that it's too generic to understand this form of inheritance for django-stubs mypy plugin, but I don't seem to have found the right tools/place to extend mypy)

[asottile-sentry]

a little different from that -- it already works for simple cases (as shown in the testcase in my report) -- just not in our codebase for whatever reason

[asottile-sentry]

I think I may have reduced this problem to a bug (?) in mypy: https://github.com/python/mypy/issues/17402

[flaeppe]

Ah, nice find!

Just also want to mention that the plugin doesn't always behave when it comes to generic QuerySets. Not sure if it's affecting anything here though

[asottile-sentry]

had a bit of a breakthrough on the defer problem -- with at least a workaround that seems to get a lot further!

https://github.com/python/mypy/issues/17402#issuecomment-2181509517

[asottile]

I have a fix in #2228 though I'm struggling to write a test to demonstrate the problem -- if you know of some way to force a round of deferral through a cycle it'd probably help me write a test for this

[flaeppe]

I've tried to have a look but I have to say that I don't know. I was about to suggest to have a look at

• 2127 and

• 2142

To see if there's some inspiration for testing cycles there. But I realised that you were already involved in those.

The only idea I can come up with, while I'm writing this, is that there might be a case when setting up a myapp/managers.py and a myapp/models.py where a model from myapp/models.py is imported to myapp/managers.py and a manager from myapp/managers.py is imported to myapp/models.py. We should already have some tests laying around somewhere doing that, might be something cyclic that triggers a defer if you dissect those.

But I'm totally not sure.