Make ReverseManyToOneDescriptor generic over a model (#2227)

This allows us to remove some plugin code, as we now can fall back to the default related manager on the descriptor instead of having to implement the fallback behaviour manually.

Author: flaeppe

django-stubs/db/models/fields/related_descriptors.pyi

@@ -73,7 +73,7 @@ class ReverseOneToOneDescriptor(Generic[_From, _To]):
     def __set__(self, instance: _From, value: _To | None) -> None: ...
     def __reduce__(self) -> tuple[Callable[..., Any], tuple[type[_To], str]]: ...
 
-class ReverseManyToOneDescriptor:
+class ReverseManyToOneDescriptor(Generic[_To]):
     """
     In the example::
 
@@ -84,29 +84,29 @@ class ReverseManyToOneDescriptor:
     """
 
     rel: ManyToOneRel
-    field: ForeignKey
+    field: ForeignKey[_To, _To]
     def __init__(self, rel: ManyToOneRel) -> None: ...
     @cached_property
-    def related_manager_cls(self) -> type[RelatedManager[Any]]: ...
+    def related_manager_cls(self) -> type[RelatedManager[_To]]: ...
     @overload
     def __get__(self, instance: None, cls: Any = ...) -> Self: ...
     @overload
-    def __get__(self, instance: Model, cls: Any = ...) -> RelatedManager[Any]: ...
+    def __get__(self, instance: Model, cls: Any = ...) -> RelatedManager[_To]: ...
     def __set__(self, instance: Any, value: Any) -> NoReturn: ...
 
 # Fake class, Django defines 'RelatedManager' inside a function body
 @type_check_only
-class RelatedManager(Manager[_M], Generic[_M]):
+class RelatedManager(Manager[_To], Generic[_To]):
     related_val: tuple[int, ...]
-    def add(self, *objs: _M | int, bulk: bool = ...) -> None: ...
-    async def aadd(self, *objs: _M | int, bulk: bool = ...) -> None: ...
-    def remove(self, *objs: _M | int, bulk: bool = ...) -> None: ...
-    async def aremove(self, *objs: _M | int, bulk: bool = ...) -> None: ...
-    def set(self, objs: QuerySet[_M] | Iterable[_M | int], *, bulk: bool = ..., clear: bool = ...) -> None: ...
-    async def aset(self, objs: QuerySet[_M] | Iterable[_M | int], *, bulk: bool = ..., clear: bool = ...) -> None: ...
+    def add(self, *objs: _To | int, bulk: bool = ...) -> None: ...
+    async def aadd(self, *objs: _To | int, bulk: bool = ...) -> None: ...
+    def remove(self, *objs: _To | int, bulk: bool = ...) -> None: ...
+    async def aremove(self, *objs: _To | int, bulk: bool = ...) -> None: ...
+    def set(self, objs: QuerySet[_To] | Iterable[_To | int], *, bulk: bool = ..., clear: bool = ...) -> None: ...
+    async def aset(self, objs: QuerySet[_To] | Iterable[_To | int], *, bulk: bool = ..., clear: bool = ...) -> None: ...
     def clear(self) -> None: ...
     async def aclear(self) -> None: ...
-    def __call__(self, *, manager: str) -> RelatedManager[_M]: ...
+    def __call__(self, *, manager: str) -> RelatedManager[_To]: ...
 
 def create_reverse_many_to_one_manager(
     superclass: type[BaseManager[_M]], rel: ManyToOneRel

ext/django_stubs_ext/patch.py

@@ -12,6 +12,7 @@
 from django.db.models.expressions import ExpressionWrapper
 from django.db.models.fields import Field
 from django.db.models.fields.related import ForeignKey
+from django.db.models.fields.related_descriptors import ReverseManyToOneDescriptor
 from django.db.models.lookups import Lookup
 from django.db.models.manager import BaseManager
 from django.db.models.query import QuerySet
@@ -71,6 +72,7 @@ def __repr__(self) -> str:
     MPGeneric(Lookup),
     MPGeneric(BaseConnectionHandler),
     MPGeneric(ExpressionWrapper),
+    MPGeneric(ReverseManyToOneDescriptor),
     # These types do have native `__class_getitem__` method since django 3.1:
     MPGeneric(QuerySet, (3, 1)),
     MPGeneric(BaseManager, (3, 1)),

mypy_django_plugin/lib/fullnames.py

@@ -34,6 +34,7 @@
 }
 
 REVERSE_ONE_TO_ONE_DESCRIPTOR = "django.db.models.fields.related_descriptors.ReverseOneToOneDescriptor"
+REVERSE_MANY_TO_ONE_DESCRIPTOR = "django.db.models.fields.related_descriptors.ReverseManyToOneDescriptor"
 MANY_TO_MANY_DESCRIPTOR = "django.db.models.fields.related_descriptors.ManyToManyDescriptor"
 MANY_RELATED_MANAGER = "django.db.models.fields.related_descriptors.ManyRelatedManager"
 RELATED_FIELDS_CLASSES = frozenset(

mypy_django_plugin/main.py

@@ -24,7 +24,17 @@
 from mypy_django_plugin.django.context import DjangoContext
 from mypy_django_plugin.exceptions import UnregisteredModelError
 from mypy_django_plugin.lib import fullnames, helpers
-from mypy_django_plugin.transformers import fields, forms, init_create, manytomany, meta, querysets, request, settings
+from mypy_django_plugin.transformers import (
+    fields,
+    forms,
+    init_create,
+    manytomany,
+    manytoone,
+    meta,
+    querysets,
+    request,
+    settings,
+)
 from mypy_django_plugin.transformers.functional import resolve_str_promise_attribute
 from mypy_django_plugin.transformers.managers import (
     create_new_manager_class_from_as_manager_method,
@@ -190,11 +200,13 @@ def get_method_hook(self, fullname: str) -> Optional[Callable[[MethodContext], M
             if info and info.has_base(fullnames.FORM_MIXIN_CLASS_FULLNAME):
                 return forms.extract_proper_type_for_get_form
 
-        elif method_name == "__get__" and class_fullname in {
-            fullnames.MANYTOMANY_FIELD_FULLNAME,
-            fullnames.MANY_TO_MANY_DESCRIPTOR,
-        }:
-            return manytomany.refine_many_to_many_related_manager
+        elif method_name == "__get__":
+            hooks = {
+                fullnames.MANYTOMANY_FIELD_FULLNAME: manytomany.refine_many_to_many_related_manager,
+                fullnames.MANY_TO_MANY_DESCRIPTOR: manytomany.refine_many_to_many_related_manager,
+                fullnames.REVERSE_MANY_TO_ONE_DESCRIPTOR: manytoone.refine_many_to_one_related_manager,
+            }
+            return hooks.get(class_fullname)
 
         manager_classes = self._get_current_manager_bases()
 

mypy_django_plugin/transformers/manytoone.py

@@ -0,0 +1,52 @@
+from typing import Optional
+
+from mypy.plugin import MethodContext
+from mypy.types import Instance
+from mypy.types import Type as MypyType
+
+from mypy_django_plugin.lib import fullnames, helpers
+
+
+def get_model_of_related_manager(ctx: MethodContext) -> Optional[Instance]:
+    """
+    Returns the type parameter (_To) instance of a `RelatedManager` instance when it's a
+    model. Otherwise `None` is returned.
+
+    For example: if given a `RelatedManager[A]` where `A` is a model then `A` is
+    returned.
+    """
+    if (
+        isinstance(ctx.default_return_type, Instance)
+        and ctx.default_return_type.type.fullname == fullnames.RELATED_MANAGER_CLASS
+    ):
+        # This is a call to '__get__' overload with a model instance of
+        # 'ReverseManyToOneDescriptor'. Returning a 'RelatedManager'. Which we want to,
+        # just like Django, build from the default manager of the related model.
+        related_manager = ctx.default_return_type
+        # Require first type argument of 'RelatedManager' to be a model
+        if (
+            len(related_manager.args) >= 1
+            and isinstance(related_manager.args[0], Instance)
+            and helpers.is_model_type(related_manager.args[0].type)
+        ):
+            return related_manager.args[0]
+
+    return None
+
+
+def refine_many_to_one_related_manager(ctx: MethodContext) -> MypyType:
+    """
+    Updates the 'RelatedManager' returned by e.g. 'ReverseManyToOneDescriptor' to be a subclass
+    of 'RelatedManager' and the related model's default manager.
+    """
+    to_model_instance = get_model_of_related_manager(ctx)
+    if to_model_instance is None:
+        return ctx.default_return_type
+
+    checker = helpers.get_typechecker_api(ctx)
+    related_manager_info = helpers.get_reverse_manager_info(
+        checker, to_model_instance.type, derived_from="_default_manager"
+    )
+    if related_manager_info is None:
+        return ctx.default_return_type
+    return Instance(related_manager_info, [])

mypy_django_plugin/transformers/models.py

@@ -114,7 +114,7 @@ def get_generated_manager_info(self, manager_fullname: str, base_manager_fullnam
         # Not a generated manager
         return None
 
-    def get_or_create_manager_with_any_fallback(self, related_manager: bool = False) -> Optional[TypeInfo]:
+    def get_or_create_manager_with_any_fallback(self) -> Optional[TypeInfo]:
         """
         Create a Manager subclass with fallback to Any for unknown attributes
         and methods. This is used for unresolved managers, where we don't know
@@ -123,7 +123,7 @@ def get_or_create_manager_with_any_fallback(self, related_manager: bool = False)
         The created class is reused if multiple unknown managers are encountered.
         """
 
-        name = "UnknownManager" if not related_manager else "UnknownRelatedManager"
+        name = "UnknownManager"
 
         # Check if we've already created a fallback manager class for this
         # module, and if so reuse that.
@@ -134,9 +134,7 @@ def get_or_create_manager_with_any_fallback(self, related_manager: bool = False)
         fallback_queryset = self.get_or_create_queryset_with_any_fallback()
         if fallback_queryset is None:
             return None
-        base_manager_fullname = (
-            fullnames.MANAGER_CLASS_FULLNAME if not related_manager else fullnames.RELATED_MANAGER_CLASS
-        )
+        base_manager_fullname = fullnames.MANAGER_CLASS_FULLNAME
         base_manager_info = self.lookup_typeinfo(base_manager_fullname)
         if base_manager_info is None:
             return None
@@ -455,6 +453,10 @@ class AddReverseLookups(ModelClassInitializer):
     def reverse_one_to_one_descriptor(self) -> TypeInfo:
         return self.lookup_typeinfo_or_incomplete_defn_error(fullnames.REVERSE_ONE_TO_ONE_DESCRIPTOR)
 
+    @cached_property
+    def reverse_many_to_one_descriptor(self) -> TypeInfo:
+        return self.lookup_typeinfo_or_incomplete_defn_error(fullnames.REVERSE_MANY_TO_ONE_DESCRIPTOR)
+
     @cached_property
     def many_to_many_descriptor(self) -> TypeInfo:
         return self.lookup_typeinfo_or_incomplete_defn_error(fullnames.MANY_TO_MANY_DESCRIPTOR)
@@ -466,23 +468,21 @@ def process_relation(self, relation: ForeignObjectRel) -> None:
             # explicitly declared(i.e. non-inferred) reverse accessors alone
             return
 
-        related_model_cls = self.django_context.get_field_related_model_cls(relation)
-        related_model_info = self.lookup_class_typeinfo_or_incomplete_defn_error(related_model_cls)
+        to_model_cls = self.django_context.get_field_related_model_cls(relation)
+        to_model_info = self.lookup_class_typeinfo_or_incomplete_defn_error(to_model_cls)
 
         if isinstance(relation, OneToOneRel):
             self.add_new_node_to_model_class(
                 attname,
                 Instance(
                     self.reverse_one_to_one_descriptor,
-                    [Instance(self.model_classdef.info, []), Instance(related_model_info, [])],
+                    [Instance(self.model_classdef.info, []), Instance(to_model_info, [])],
                 ),
             )
             return
 
         elif isinstance(relation, ManyToManyRel):
             # TODO: 'relation' should be based on `TypeInfo` instead of Django runtime.
-            to_fullname = helpers.get_class_fullname(relation.remote_field.model)
-            to_model_info = self.lookup_typeinfo_or_incomplete_defn_error(to_fullname)
             assert relation.through is not None
             through_fullname = helpers.get_class_fullname(relation.through)
             through_model_info = self.lookup_typeinfo_or_incomplete_defn_error(through_fullname)
@@ -492,79 +492,65 @@ def process_relation(self, relation: ForeignObjectRel) -> None:
             )
             return
 
-        related_manager_info = None
-        try:
-            related_manager_info = self.lookup_typeinfo_or_incomplete_defn_error(fullnames.RELATED_MANAGER_CLASS)
-            default_manager = related_model_info.names.get("_default_manager")
-            if not default_manager:
-                raise helpers.IncompleteDefnException()
-        except helpers.IncompleteDefnException as exc:
-            if not self.api.final_iteration:
-                raise exc
-
-            # If a django model has a Manager class that cannot be
-            # resolved statically (if it is generated in a way where we
-            # cannot import it, like `objects = my_manager_factory()`),
-            # we fallback to the default related manager, so you at
-            # least get a base level of working type checking.
-            #
-            # See https://github.com/typeddjango/django-stubs/pull/993
-            # for more information on when this error can occur.
-            fallback_manager = self.get_or_create_manager_with_any_fallback(related_manager=True)
-            if fallback_manager is not None:
-                self.add_new_node_to_model_class(
-                    attname, Instance(fallback_manager, [Instance(related_model_info, [])])
-                )
-            related_model_fullname = related_model_cls.__module__ + "." + related_model_cls.__name__
-            self.ctx.api.fail(
-                (
-                    "Couldn't resolve related manager for relation "
-                    f"{relation.name!r} (from {related_model_fullname}."
-                    f"{relation.field})."
-                ),
-                self.ctx.cls,
-                code=MANAGER_MISSING,
-            )
-            return
-
-        # Check if the related model has a related manager subclassed from the default manager
+        related_manager_info = self.lookup_typeinfo_or_incomplete_defn_error(fullnames.RELATED_MANAGER_CLASS)
         # TODO: Support other reverse managers than `_default_manager`
-        default_reverse_manager_info = helpers.get_reverse_manager_info(
-            self.api, model_info=related_model_info, derived_from="_default_manager"
-        )
-        if default_reverse_manager_info:
-            self.add_new_node_to_model_class(attname, Instance(default_reverse_manager_info, []))
-            return
+        default_manager = to_model_info.names.get("_default_manager")
+        if default_manager is None and not self.api.final_iteration:
+            raise helpers.IncompleteDefnException()
+        elif (
+            # When we get no default manager we can't customize the reverse manager any
+            # further and will just fall back to the manager declared on the descriptor
+            default_manager is None
+            # '_default_manager' attribute is a node type we can't process
+            or not isinstance(default_manager.type, Instance)
+            # Already has a related manager subclassed from the default manager
+            or helpers.get_reverse_manager_info(self.api, model_info=to_model_info, derived_from="_default_manager")
+            is not None
+            # When the default manager isn't custom there's no need to create a new type
+            # as `RelatedManager` has `models.Manager` as base
+            or default_manager.type.type.fullname == fullnames.MANAGER_CLASS_FULLNAME
+        ):
+            if default_manager is None and self.api.final_iteration:
+                # If a django model has a Manager class that cannot be
+                # resolved statically (if it is generated in a way where we
+                # cannot import it, like `objects = my_manager_factory()`),
+                #
+                # See https://github.com/typeddjango/django-stubs/pull/993
+                # for more information on when this error can occur.
+                self.ctx.api.fail(
+                    (
+                        f"Couldn't resolve related manager {attname!r} for relation "
+                        f"'{to_model_info.fullname}.{relation.field.name}'."
+                    ),
+                    self.ctx.cls,
+                    code=MANAGER_MISSING,
+                )
 
-        # The reverse manager we're looking for doesn't exist. So we
-        # create it. The (default) reverse manager type is built from a
-        # RelatedManager and the default manager on the related model
-        parametrized_related_manager_type = Instance(related_manager_info, [Instance(related_model_info, [])])
-        default_manager_type = default_manager.type
-        assert default_manager_type is not None
-        assert isinstance(default_manager_type, Instance)
-        # When the default manager isn't custom there's no need to create a new type
-        # as `RelatedManager` has `models.Manager` as base
-        if default_manager_type.type.fullname == fullnames.MANAGER_CLASS_FULLNAME:
-            self.add_new_node_to_model_class(attname, parametrized_related_manager_type)
+            self.add_new_node_to_model_class(
+                attname, Instance(self.reverse_many_to_one_descriptor, [Instance(to_model_info, [])])
+            )
             return
 
+        # Create a reverse manager subclassed from the default manager of the related
+        # model and 'RelatedManager'
+        related_manager = Instance(related_manager_info, [Instance(to_model_info, [])])
         # The reverse manager is based on the related model's manager, so it makes most sense to add the new
         # related manager in that module
         new_related_manager_info = helpers.add_new_class_for_module(
-            module=self.api.modules[related_model_info.module_name],
-            name=f"{related_model_cls.__name__}_RelatedManager",
-            bases=[parametrized_related_manager_type, default_manager_type],
+            module=self.api.modules[to_model_info.module_name],
+            name=f"{to_model_info.name}_RelatedManager",
+            bases=[related_manager, default_manager.type],
         )
-        new_related_manager_info.metadata["django"] = {"related_manager_to_model": related_model_info.fullname}
         # Stash the new reverse manager type fullname on the related model, so we don't duplicate
         # or have to create it again for other reverse relations
         helpers.set_reverse_manager_info(
-            related_model_info,
+            to_model_info,
             derived_from="_default_manager",
             fullname=new_related_manager_info.fullname,
         )
-        self.add_new_node_to_model_class(attname, Instance(new_related_manager_info, []))
+        self.add_new_node_to_model_class(
+            attname, Instance(self.reverse_many_to_one_descriptor, [Instance(to_model_info, [])])
+        )
 
     def run_with_model_cls(self, model_cls: Type[Model]) -> None:
         # add related managers etc.

scripts/stubtest/allowlist_todo.txt

@@ -140,7 +140,6 @@ django.contrib.auth.models.User.is_staff
 django.contrib.auth.models.User.is_superuser
 django.contrib.auth.models.User.last_login
 django.contrib.auth.models.User.last_name
-django.contrib.auth.models.User.logentry_set
 django.contrib.auth.models.User.password
 django.contrib.auth.models.User.user_permissions
 django.contrib.auth.models.User.username
@@ -169,9 +168,7 @@ django.contrib.contenttypes.management.inject_rename_contenttypes_operations
 django.contrib.contenttypes.models.ContentType.app_label
 django.contrib.contenttypes.models.ContentType.app_labeled_name
 django.contrib.contenttypes.models.ContentType.id
-django.contrib.contenttypes.models.ContentType.logentry_set
 django.contrib.contenttypes.models.ContentType.model
-django.contrib.contenttypes.models.ContentType.permission_set
 django.contrib.contenttypes.models.ContentTypeManager.__init__
 django.contrib.contenttypes.models.ContentTypeManager.__slotnames__
 django.contrib.flatpages.admin.FlatPageAdmin
@@ -515,7 +512,6 @@ django.contrib.sites.models.Site.domain
 django.contrib.sites.models.Site.flatpage_set
 django.contrib.sites.models.Site.id
 django.contrib.sites.models.Site.name
-django.contrib.sites.models.Site.redirect_set
 django.contrib.sites.models.SiteManager.__slotnames__
 django.contrib.staticfiles.finders.BaseStorageFinder.storage
 django.contrib.staticfiles.finders.DefaultStorageFinder.storage