Emit error and set fallback type for managers that can't be resolved (#999)

* Emit error and set fallback type for managers that can't be resolved

* fixup! Emit error and set fallback type for managers that can't be resolved

Author: flaeppe

mypy_django_plugin/errorcodes.py

@@ -1,4 +1,4 @@
 from mypy.errorcodes import ErrorCode
 
 MANAGER_UNTYPED = ErrorCode("django-manager", "Untyped manager disallowed", "Django")
-MANAGER_MISSING = ErrorCode("django-manager-missing", "Couldn't resolve related manager for model", "Django")
+MANAGER_MISSING = ErrorCode("django-manager-missing", "Couldn't resolve manager for model", "Django")

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, Context, FuncDef, TypeInfo, Var
+from mypy.nodes import ARG_STAR2, Argument, AssignmentStmt, Context, FuncDef, NameExpr, TypeInfo, Var
 from mypy.plugin import AnalyzeTypeContext, AttributeContext, CheckerPluginInterface, ClassDefContext
 from mypy.plugins import common
 from mypy.semanal import SemanticAnalyzer
@@ -234,7 +234,7 @@ def create_new_model_parametrized_manager(self, name: str, base_manager_info: Ty
     def run_with_model_cls(self, model_cls: Type[Model]) -> None:
         manager_info: Optional[TypeInfo]
 
-        encountered_incomplete_manager_def = False
+        incomplete_manager_defs = set()
         for manager_name, manager in model_cls._meta.managers_map.items():
             manager_class_name = manager.__class__.__name__
             manager_fullname = helpers.get_class_fullname(manager.__class__)
@@ -243,13 +243,11 @@ def run_with_model_cls(self, model_cls: Type[Model]) -> None:
             except helpers.IncompleteDefnException as exc:
                 # Check if manager is a generated (dynamic class) manager
                 base_manager_fullname = helpers.get_class_fullname(manager.__class__.__bases__[0])
-                manager_info = self.get_generated_manager_info(manager_fullname, base_manager_fullname)
-                if manager_info is None:
+                if manager_fullname not in self.get_generated_manager_mappings(base_manager_fullname):
                     # Manager doesn't appear to be generated. Track that we encountered an
                     # incomplete definition and skip
-                    encountered_incomplete_manager_def = True
-                    continue
-                _, manager_class_name = manager_info.fullname.rsplit(".", maxsplit=1)
+                    incomplete_manager_defs.add(manager_name)
+                continue
 
             if manager_name not in self.model_classdef.info.names:
                 manager_type = Instance(manager_info, [Instance(self.model_classdef.info, [])])
@@ -275,9 +273,38 @@ def run_with_model_cls(self, model_cls: Type[Model]) -> None:
 
                 self.add_new_node_to_model_class(manager_name, custom_manager_type)
 
-        if encountered_incomplete_manager_def and not self.api.final_iteration:
-            #  Unless we're on the final round, see if another round could figuring out all manager types
+        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"
+                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
+                    for expr in self.ctx.cls.defs.body
+                    if (
+                        isinstance(expr, AssignmentStmt)
+                        and isinstance(expr.lvalues[0], NameExpr)
+                        and expr.lvalues[0].name == manager_name
+                    )
+                ]
+                manager_fullname = f"{self.model_classdef.fullname}.{manager_name}"
+                self.api.fail(
+                    f'Could not resolve manager type for "{manager_fullname}"',
+                    manager_expr[0] if manager_expr else self.ctx.cls,
+                    code=MANAGER_MISSING,
+                )
 
 
 class AddDefaultManagerAttribute(ModelClassInitializer):

tests/typecheck/fields/test_related.yml

@@ -639,43 +639,6 @@
               class Article(LibraryEntity):
                   pass
 
-
--   case: test_related_managers_when_manager_is_dynamically_generated_and_cannot_be_imported
-    main: |
-        from myapp import models
-    installed_apps:
-        - myapp
-    files:
-        -   path: myapp/__init__.py
-        -   path: myapp/models.py
-            content: |
-                from django.db import models
-
-                class User(models.Model):
-                    name = models.TextField()
-
-                def DynamicManager() -> models.Manager:
-                    class InnerManager(models.Manager):
-                        def some_method(self, arg: str) -> None:
-                            return None
-
-                    return InnerManager()
-
-                class Booking(models.Model):
-                    renter = models.ForeignKey(User, on_delete=models.PROTECT)
-                    owner = models.ForeignKey(User, on_delete=models.PROTECT, related_name='bookingowner_set')
-
-                    objects = DynamicManager()
-
-                def process_booking(user: User):
-                    reveal_type(user.bookingowner_set)
-                    reveal_type(user.booking_set)
-    out: |
-        myapp/models:3: error: Couldn't resolve related manager for relation 'booking' (from myapp.models.Booking.myapp.Booking.renter).
-        myapp/models:3: error: Couldn't resolve related manager for relation 'bookingowner_set' (from myapp.models.Booking.myapp.Booking.owner).
-        myapp/models:20: note: Revealed type is "django.db.models.manager.RelatedManager[myapp.models.Booking]"
-        myapp/models:21: note: Revealed type is "django.db.models.manager.RelatedManager[myapp.models.Booking]"
-
 -   case: foreign_key_relationship_for_models_with_custom_manager
     main: |
         from myapp.models import Transaction
@@ -686,10 +649,12 @@
         -   path: myapp/models.py
             content: |
                 from django.db import models
+                from django.db.models.manager import BaseManager
                 class TransactionQuerySet(models.QuerySet):
                     pass
+                TransactionManager = BaseManager.from_queryset(TransactionQuerySet)
                 class Transaction(models.Model):
-                    objects = TransactionQuerySet.as_manager()
+                    objects = TransactionManager()
                     def test(self) -> None:
                         self.transactionlog_set
                 class TransactionLog(models.Model):

tests/typecheck/managers/test_managers.yml

@@ -442,3 +442,82 @@
                 class MyModel(models.Model):
                     site = models.ForeignKey(Site, on_delete=models.CASCADE)
                     on_site = CurrentSiteManager()
+
+-   case: test_emits_error_for_unresolved_managers
+    main: |
+        from myapp import models
+    installed_apps:
+        - myapp
+    files:
+        -   path: myapp/__init__.py
+        -   path: myapp/models.py
+            content: |
+                from django.db import models
+
+                def LocalManager() -> models.Manager:
+                    """
+                    Returns a manager instance of an inlined manager type that can't
+                    be resolved.
+                    """
+                    class InnerManager(models.Manager):
+                        ...
+
+                    return InnerManager()
+
+                class User(models.Model):
+                    name = models.TextField()
+
+                class Booking(models.Model):
+                    renter = models.ForeignKey(User, on_delete=models.PROTECT)
+                    owner = models.ForeignKey(User, on_delete=models.PROTECT, related_name='bookingowner_set')
+
+                    objects = LocalManager()
+
+                class TwoUnresolvable(models.Model):
+                    objects = LocalManager()
+                    second_objects = LocalManager()
+
+                class AbstractUnresolvable(models.Model):
+                    objects = LocalManager()
+
+                    class Meta:
+                        abstract = True
+
+                class InvisibleUnresolvable(AbstractUnresolvable):
+                    text = models.TextField()
+
+                def process_booking(user: User):
+                    reveal_type(User.objects)
+                    reveal_type(User._default_manager)
+
+                    reveal_type(Booking.objects)
+                    reveal_type(Booking._default_manager)
+
+                    reveal_type(TwoUnresolvable.objects)
+                    reveal_type(TwoUnresolvable.second_objects)
+                    reveal_type(TwoUnresolvable._default_manager)
+
+                    reveal_type(InvisibleUnresolvable.objects)
+                    reveal_type(InvisibleUnresolvable._default_manager)
+
+                    reveal_type(user.bookingowner_set)
+                    reveal_type(user.booking_set)
+    out: |
+        myapp/models:13: error: Couldn't resolve related manager for relation 'booking' (from myapp.models.Booking.myapp.Booking.renter).
+        myapp/models:13: error: Couldn't resolve related manager for relation 'bookingowner_set' (from myapp.models.Booking.myapp.Booking.owner).
+        myapp/models:20: error: Could not resolve manager type for "myapp.models.Booking.objects"
+        myapp/models:23: error: Could not resolve manager type for "myapp.models.TwoUnresolvable.objects"
+        myapp/models:24: error: Could not resolve manager type for "myapp.models.TwoUnresolvable.second_objects"
+        myapp/models:27: error: Could not resolve manager type for "myapp.models.AbstractUnresolvable.objects"
+        myapp/models:32: error: Could not resolve manager type for "myapp.models.InvisibleUnresolvable.objects"
+        myapp/models:36: note: Revealed type is "django.db.models.manager.Manager[myapp.models.User]"
+        myapp/models:37: note: Revealed type is "django.db.models.manager.Manager[myapp.models.User]"
+        myapp/models:39: note: Revealed type is "django.db.models.manager.Manager[myapp.models.Booking]"
+        myapp/models:40: note: Revealed type is "django.db.models.manager.BaseManager[myapp.models.Booking]"
+        myapp/models:42: note: Revealed type is "django.db.models.manager.Manager[myapp.models.TwoUnresolvable]"
+        myapp/models:43: note: Revealed type is "django.db.models.manager.Manager[myapp.models.TwoUnresolvable]"
+        myapp/models:44: note: Revealed type is "django.db.models.manager.BaseManager[myapp.models.TwoUnresolvable]"
+        myapp/models:46: note: Revealed type is "django.db.models.manager.Manager[myapp.models.InvisibleUnresolvable]"
+        myapp/models:47: note: Revealed type is "django.db.models.manager.BaseManager[myapp.models.InvisibleUnresolvable]"
+        myapp/models:49: note: Revealed type is "django.db.models.manager.RelatedManager[myapp.models.Booking]"
+        myapp/models:50: note: Revealed type is "django.db.models.manager.RelatedManager[myapp.models.Booking]"