Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion mypy_django_plugin/errorcodes.py
Original file line number Diff line number Diff line change
@@ -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")
45 changes: 36 additions & 9 deletions mypy_django_plugin/transformers/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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__)
Expand All @@ -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
Comment on lines +246 to +250
Copy link
Member Author

@flaeppe flaeppe Jun 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changeset triggered some presumably premature assumption that dynamically generated managers can be looked up and have a type/typeinfo during semantic analysis (where they are prepared). That shouldn't be possible until the type checker has run. Which, I think, only happens after semantic analysis.

That's why I dropped the continuation if manager_info was found, I don't think it's possible for it to happen.


if manager_name not in self.model_classdef.info.names:
manager_type = Instance(manager_info, [Instance(self.model_classdef.info, [])])
Expand All @@ -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):
Expand Down
41 changes: 3 additions & 38 deletions tests/typecheck/fields/test_related.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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):
Expand Down
79 changes: 79 additions & 0 deletions tests/typecheck/managers/test_managers.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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]"