Don't create dynamic classes for non-dynamic managers

We don't need to create custom managers when the manager isn't
dynamically created. This changes the logic to just set the type of the
manager on the class.

There's a small regression/limitations with this approach, as
demonstrated by the
custom_manager_without_typevar_returns_any_model_types test case. If the
manager is _not_ typed with generics (ie. `MyManager(Manager)`
vs `MyManager(Manager[T])` the manager will return querysets with `Any`
as the model type instead because of implicit generics.

That can be solved in one of two ways: either through
"reparametrization" as implemented in typeddjango#1030 possibly or through setting
the methods as attributes with type `Any` as we do for dynamic managers

Author: ljodal

mypy_django_plugin/lib/helpers.py

@@ -1,5 +1,5 @@
 from collections import OrderedDict
-from typing import TYPE_CHECKING, Any, Dict, Iterable, Iterator, List, Optional, Set, Tuple, Union
+from typing import TYPE_CHECKING, Any, Dict, Iterable, Iterator, List, Optional, Set, Union
 
 from django.db.models.fields import Field
 from django.db.models.fields.related import RelatedField
@@ -10,11 +10,9 @@
 from mypy.nodes import (
     GDEF,
     MDEF,
-    Argument,
     Block,
     ClassDef,
     Expression,
-    FuncDef,
     MemberExpr,
     MypyFile,
     NameExpr,
@@ -34,11 +32,10 @@
     MethodContext,
     SemanticAnalyzerPluginInterface,
 )
-from mypy.plugins.common import add_method_to_class
 from mypy.semanal import SemanticAnalyzer
-from mypy.types import AnyType, CallableType, Instance, NoneTyp, TupleType
+from mypy.types import AnyType, Instance, NoneTyp, TupleType
 from mypy.types import Type as MypyType
-from mypy.types import TypedDictType, TypeOfAny, UnboundType, UnionType
+from mypy.types import TypedDictType, TypeOfAny, UnionType
 
 from mypy_django_plugin.lib import fullnames
 from mypy_django_plugin.lib.fullnames import WITH_ANNOTATIONS_FULLNAME
@@ -350,92 +347,6 @@ def add_new_sym_for_info(info: TypeInfo, *, name: str, sym_type: MypyType, no_se
     info.names[name] = SymbolTableNode(MDEF, var, plugin_generated=True, no_serialize=no_serialize)
 
 
-def build_unannotated_method_args(method_node: FuncDef) -> Tuple[List[Argument], MypyType]:
-    prepared_arguments = []
-    try:
-        arguments = method_node.arguments[1:]
-    except AttributeError:
-        arguments = []
-    for argument in arguments:
-        argument.type_annotation = AnyType(TypeOfAny.unannotated)
-        prepared_arguments.append(argument)
-    return_type = AnyType(TypeOfAny.unannotated)
-    return prepared_arguments, return_type
-
-
-def bind_or_analyze_type(t: MypyType, api: SemanticAnalyzer, module_name: Optional[str] = None) -> Optional[MypyType]:
-    """Analyze a type. If an unbound type, try to look it up in the given module name.
-
-    That should hopefully give a bound type."""
-    if isinstance(t, UnboundType) and module_name is not None:
-        node = api.lookup_fully_qualified_or_none(module_name + "." + t.name)
-        if node is not None and node.type is not None:
-            return node.type
-
-    return api.anal_type(t)
-
-
-def copy_method_to_another_class(
-    ctx: ClassDefContext,
-    self_type: Instance,
-    new_method_name: str,
-    method_node: FuncDef,
-    return_type: Optional[MypyType] = None,
-    original_module_name: Optional[str] = None,
-) -> None:
-    semanal_api = get_semanal_api(ctx)
-    if method_node.type is None:
-        if not semanal_api.final_iteration:
-            semanal_api.defer()
-            return
-
-        arguments, return_type = build_unannotated_method_args(method_node)
-        add_method_to_class(
-            semanal_api, ctx.cls, new_method_name, args=arguments, return_type=return_type, self_type=self_type
-        )
-        return
-
-    method_type = method_node.type
-    if not isinstance(method_type, CallableType):
-        if not semanal_api.final_iteration:
-            semanal_api.defer()
-        return
-
-    if return_type is None:
-        return_type = bind_or_analyze_type(method_type.ret_type, semanal_api, original_module_name)
-    if return_type is None:
-        return
-
-    # We build the arguments from the method signature (`CallableType`), because if we were to
-    # use the arguments from the method node (`FuncDef.arguments`) we're not compatible with
-    # a method loaded from cache. As mypy doesn't serialize `FuncDef.arguments` when caching
-    arguments = []
-    # Note that the first argument is excluded, as that's `self`
-    for pos, (arg_type, arg_kind, arg_name) in enumerate(
-        zip(method_type.arg_types[1:], method_type.arg_kinds[1:], method_type.arg_names[1:]),
-        start=1,
-    ):
-        bound_arg_type = bind_or_analyze_type(arg_type, semanal_api, original_module_name)
-        if bound_arg_type is None:
-            return
-        if arg_name is None and hasattr(method_node, "arguments"):
-            arg_name = method_node.arguments[pos].variable.name
-        arguments.append(
-            Argument(
-                # Positional only arguments can have name as `None`, if we can't find a name, we just invent one..
-                variable=Var(name=arg_name if arg_name is not None else str(pos), type=arg_type),
-                type_annotation=bound_arg_type,
-                initializer=None,
-                kind=arg_kind,
-                pos_only=arg_name is None,
-            )
-        )
-
-    add_method_to_class(
-        semanal_api, ctx.cls, new_method_name, args=arguments, return_type=return_type, self_type=self_type
-    )
-
-
 def add_new_manager_base(api: SemanticAnalyzerPluginInterface, fullname: str) -> None:
     sym = api.lookup_fully_qualified_or_none(fullnames.MANAGER_CLASS_FULLNAME)
     if sym is not None and isinstance(sym.node, TypeInfo):

mypy_django_plugin/transformers/managers.py

@@ -182,6 +182,9 @@ def create_new_manager_class_from_from_queryset_method(ctx: DynamicClassDefConte
     """
     semanal_api = helpers.get_semanal_api(ctx)
 
+    if semanal_api.is_class_scope():
+        return
+
     # Don't redeclare the manager class if we've already defined it.
     manager_node = semanal_api.lookup_current_scope(ctx.name)
     if manager_node and isinstance(manager_node.node, TypeInfo):

mypy_django_plugin/transformers/models.py

@@ -5,7 +5,7 @@
 from django.db.models.fields.related import ForeignKey
 from django.db.models.fields.reverse_related import ManyToManyRel, ManyToOneRel, OneToOneRel
 from mypy.checker import TypeChecker
-from mypy.nodes import ARG_STAR2, Argument, AssignmentStmt, Context, FuncDef, NameExpr, TypeInfo, Var
+from mypy.nodes import ARG_STAR2, Argument, AssignmentStmt, Context, NameExpr, TypeInfo, Var
 from mypy.plugin import AnalyzeTypeContext, AttributeContext, CheckerPluginInterface, ClassDefContext
 from mypy.plugins import common
 from mypy.semanal import SemanticAnalyzer
@@ -185,52 +185,6 @@ def run_with_model_cls(self, model_cls: Type[Model]) -> None:
 
 
 class AddManagers(ModelClassInitializer):
-    def has_any_parametrized_manager_as_base(self, info: TypeInfo) -> bool:
-        for base in helpers.iter_bases(info):
-            if self.is_any_parametrized_manager(base):
-                return True
-        return False
-
-    def is_any_parametrized_manager(self, typ: Instance) -> bool:
-        return typ.type.fullname in fullnames.MANAGER_CLASSES and isinstance(typ.args[0], AnyType)
-
-    def create_new_model_parametrized_manager(self, name: str, base_manager_info: TypeInfo) -> Instance:
-        bases = []
-        for original_base in base_manager_info.bases:
-            if self.is_any_parametrized_manager(original_base):
-                if original_base.type is None:
-                    raise helpers.IncompleteDefnException()
-
-                original_base = helpers.reparametrize_instance(original_base, [Instance(self.model_classdef.info, [])])
-            bases.append(original_base)
-
-        new_manager_info = self.add_new_class_for_current_module(name, bases)
-        # copy fields to a new manager
-        new_cls_def_context = ClassDefContext(cls=new_manager_info.defn, reason=self.ctx.reason, api=self.api)
-        custom_manager_type = Instance(new_manager_info, [Instance(self.model_classdef.info, [])])
-
-        for name, sym in base_manager_info.names.items():
-            # replace self type with new class, if copying method
-            if isinstance(sym.node, FuncDef):
-                helpers.copy_method_to_another_class(
-                    new_cls_def_context,
-                    self_type=custom_manager_type,
-                    new_method_name=name,
-                    method_node=sym.node,
-                    original_module_name=base_manager_info.module_name,
-                )
-                continue
-
-            new_sym = sym.copy()
-            if isinstance(new_sym.node, Var):
-                new_var = Var(name, type=sym.type)
-                new_var.info = new_manager_info
-                new_var._fullname = new_manager_info.fullname + "." + name
-                new_sym.node = new_var
-            new_manager_info.names[name] = new_sym
-
-        return custom_manager_type
-
     def run_with_model_cls(self, model_cls: Type[Model]) -> None:
         manager_info: Optional[TypeInfo]
 
@@ -254,40 +208,30 @@ def run_with_model_cls(self, model_cls: Type[Model]) -> None:
                 if manager_info is None:
                     continue
 
-            is_dynamically_generated = manager_info.metadata.get("django", {}).get("from_queryset_manager") is not None
-            if manager_name not in self.model_classdef.info.names or is_dynamically_generated:
-                manager_type = Instance(manager_info, [Instance(self.model_classdef.info, [])])
-                self.add_new_node_to_model_class(manager_name, manager_type)
-            elif self.has_any_parametrized_manager_as_base(manager_info):
-                # Ending up here could for instance be due to having a custom _Manager_
-                # that is not built from a custom QuerySet. Another example is a
-                # related manager.
-                custom_model_manager_name = manager.model.__name__ + "_" + manager_class_name
-                try:
-                    custom_manager_type = self.create_new_model_parametrized_manager(
-                        custom_model_manager_name, base_manager_info=manager_info
-                    )
-                except helpers.IncompleteDefnException:
-                    continue
+            manager_node = self.model_classdef.info.names.get(manager_name, None)
+            if manager_node and manager_node.type is not None:
+                continue
 
-                self.add_new_node_to_model_class(manager_name, custom_manager_type)
+            manager_type = Instance(manager_info, [Instance(self.model_classdef.info, [])])
+            self.add_new_node_to_model_class(manager_name, manager_type)
+
+        if incomplete_manager_defs:
+            #  Unless we're on the final round, see if another round could
+            #  figure out all manager types
+            if not self.api.final_iteration:
+                raise helpers.IncompleteDefnException()
 
-        if incomplete_manager_defs and not self.api.final_iteration:
-            #  Unless we're on the final round, see if another round could figure out all manager types
-            raise helpers.IncompleteDefnException()
-        elif self.api.final_iteration:
             for manager_name in incomplete_manager_defs:
                 # We act graceful and set the type as the bare minimum we know of
                 # (Django's default) before finishing. And emit an error, to allow for
                 # ignoring a more specialised manager not being resolved while still
                 # setting _some_ type
                 django_manager_info = self.lookup_typeinfo(fullnames.MANAGER_CLASS_FULLNAME)
-                assert (
-                    django_manager_info is not None
-                ), f"Type info for Django's {fullnames.MANAGER_CLASS_FULLNAME} missing"
+                assert django_manager_info, f"Type info for {fullnames.MANAGER_CLASS_FULLNAME} missing"
                 self.add_new_node_to_model_class(
                     manager_name, Instance(django_manager_info, [Instance(self.model_classdef.info, [])])
                 )
+
                 # Find expression for e.g. `objects = SomeManager()`
                 manager_expr = [
                     expr

tests/typecheck/fields/test_related.yml

@@ -680,7 +680,7 @@
 
                 # Note, that we cannot resolve dynamic calls for custom managers:
                 class Transaction(models.Model):
-                    objects = BaseManager.from_queryset(TransactionQuerySet)
+                    objects = BaseManager.from_queryset(TransactionQuerySet)()
                     def test(self) -> None:
                         reveal_type(self.transactionlog_set)
                         # We use a fallback Any type:
@@ -689,8 +689,10 @@
                 class TransactionLog(models.Model):
                     transaction = models.ForeignKey(Transaction, on_delete=models.CASCADE)
     out: |
+        myapp/models:9: error: Could not resolve manager type for "myapp.models.Transaction.objects"
         myapp/models:9: error: `.from_queryset` called from inside model class body
         myapp/models:11: note: Revealed type is "django.db.models.manager.RelatedManager[myapp.models.TransactionLog]"
+        myapp/models:13: error: "Manager[Transaction]" has no attribute "custom"
         myapp/models:13: note: Revealed type is "Any"
 
 

tests/typecheck/managers/test_managers.yml

@@ -329,17 +329,49 @@
 #                class MyUser(MyBaseUser):
 #                    objects = MyManager()
 
--   case: custom_manager_returns_proper_model_types
+-   case: custom_manager_without_typevar_returns_any_model_types
     main: |
         from myapp.models import User
-        reveal_type(User.objects)  # N: Revealed type is "myapp.models.User_MyManager2[myapp.models.User]"
+        reveal_type(User.objects)  # N: Revealed type is "myapp.models.MyManager[myapp.models.User]"
+        reveal_type(User.objects.select_related())  # N: Revealed type is "django.db.models.query._QuerySet[Any, Any]"
+        reveal_type(User.objects.get())  # N: Revealed type is "Any"
+        reveal_type(User.objects.get_instance())  # N: Revealed type is "builtins.int"
+        reveal_type(User.objects.get_instance_untyped('hello'))  # N: Revealed type is "Any"
+
+        from myapp.models import ChildUser
+        reveal_type(ChildUser.objects)  # N: Revealed type is "myapp.models.MyManager[myapp.models.ChildUser]"
+        reveal_type(ChildUser.objects.select_related())  # N: Revealed type is "django.db.models.query._QuerySet[Any, Any]"
+        reveal_type(ChildUser.objects.get())  # N: Revealed type is "Any"
+        reveal_type(ChildUser.objects.get_instance())  # N: Revealed type is "builtins.int"
+        reveal_type(ChildUser.objects.get_instance_untyped('hello'))  # N: Revealed type is "Any"
+    installed_apps:
+        - myapp
+    files:
+        -   path: myapp/__init__.py
+        -   path: myapp/models.py
+            content: |
+                from django.db import models
+                class MyManager(models.Manager):
+                    def get_instance(self) -> int:
+                        pass
+                    def get_instance_untyped(self, name):
+                        pass
+                class User(models.Model):
+                    objects = MyManager()
+                class ChildUser(models.Model):
+                    objects = MyManager()
+
+-   case: custom_manager_with_typevars_returns_proper_model_types
+    main: |
+        from myapp.models import User
+        reveal_type(User.objects)  # N: Revealed type is "myapp.models.MyManager[myapp.models.User]"
         reveal_type(User.objects.select_related())  # N: Revealed type is "django.db.models.query._QuerySet[myapp.models.User, myapp.models.User]"
         reveal_type(User.objects.get())  # N: Revealed type is "myapp.models.User"
         reveal_type(User.objects.get_instance())  # N: Revealed type is "builtins.int"
         reveal_type(User.objects.get_instance_untyped('hello'))  # N: Revealed type is "Any"
 
         from myapp.models import ChildUser
-        reveal_type(ChildUser.objects)  # N: Revealed type is "myapp.models.ChildUser_MyManager2[myapp.models.ChildUser]"
+        reveal_type(ChildUser.objects)  # N: Revealed type is "myapp.models.MyManager[myapp.models.ChildUser]"
         reveal_type(ChildUser.objects.select_related())  # N: Revealed type is "django.db.models.query._QuerySet[myapp.models.ChildUser, myapp.models.ChildUser]"
         reveal_type(ChildUser.objects.get())  # N: Revealed type is "myapp.models.ChildUser"
         reveal_type(ChildUser.objects.get_instance())  # N: Revealed type is "builtins.int"
@@ -350,8 +382,10 @@
         -   path: myapp/__init__.py
         -   path: myapp/models.py
             content: |
+                from typing import TypeVar
                 from django.db import models
-                class MyManager(models.Manager):
+                T = TypeVar("T", bound=models.Model)
+                class MyManager(models.Manager[T]):
                     def get_instance(self) -> int:
                         pass
                     def get_instance_untyped(self, name):
@@ -364,7 +398,7 @@
 -   case: custom_manager_annotate_method_before_type_declaration
     main: |
         from myapp.models import ModelA, ModelB, ManagerA
-        reveal_type(ModelA.objects)  # N: Revealed type is "myapp.models.ModelA_ManagerA1[myapp.models.ModelA]"
+        reveal_type(ModelA.objects)  # N: Revealed type is "myapp.models.ManagerA[myapp.models.ModelA]"
         reveal_type(ModelA.objects.do_something)  # N: Revealed type is "def (other_obj: myapp.models.ModelB) -> builtins.str"
     installed_apps:
         - myapp
@@ -383,7 +417,7 @@
                     movie = models.TextField()
 
 
--   case: override_manager_create1
+-   case: override_manager_create_missing_generic
     main: |
         from myapp.models import MyModel
         MyModel.objects.create()
@@ -397,14 +431,14 @@
                 class MyModelManager(models.Manager):
 
                     def create(self, **kwargs) -> 'MyModel':
-                          return super().create(**kwargs)
+                        return super().create(**kwargs)
 
 
                 class MyModel(models.Model):
 
                    objects = MyModelManager()
 
--   case: override_manager_create2
+-   case: override_manager_create_with_generic
     main: |
         from myapp.models import MyModel
         MyModel.objects.create()
@@ -427,7 +461,7 @@
 -   case: regression_manager_scope_foreign
     main: |
         from myapp.models import MyModel
-        reveal_type(MyModel.on_site)  # N: Revealed type is "myapp.models.MyModel_CurrentSiteManager[myapp.models.MyModel]"
+        reveal_type(MyModel.on_site)  # N: Revealed type is "django.contrib.sites.managers.CurrentSiteManager[myapp.models.MyModel]"
     installed_apps:
         - myapp
         - django.contrib.sites