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.

Sign up for GitHub

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

Remove some unnecessary defer rounds #2252

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Remove some unnecessary defer rounds #2252

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
07503e6
Remove some unnecessary defer rounds
flaeppe Jul 5, 2024
04cdf3b
fixup! Remove some unnecessary defer rounds
flaeppe Jul 13, 2024
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
57 changes: 33 additions & 24 deletions mypy_django_plugin/transformers/managers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,17 +315,20 @@ def create_new_manager_class_from_from_queryset_method(ctx: DynamicClassDefConte
# This is just a deferral run where our work is already finished
return

new_manager_info = create_manager_info_from_from_queryset_call(semanal_api, ctx.call, ctx.name)
if new_manager_info is None:
try:
new_manager_info = create_manager_info_from_from_queryset_call(semanal_api, ctx.call, ctx.name)
except helpers.IncompleteDefnException:
if not ctx.api.final_iteration:
# XXX: hack for python/mypy#17402
ph = PlaceholderNode(ctx.api.qualified_name(ctx.name), ctx.call, ctx.call.line, becomes_typeinfo=True)
ctx.api.add_symbol_table_node(ctx.name, SymbolTableNode(GDEF, ph))
ctx.api.defer()
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)
if new_manager_info is not None:
# 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)


def register_dynamically_created_manager(fullname: str, manager_name: str, manager_base: TypeInfo) -> None:
Expand All @@ -345,27 +348,37 @@ def create_manager_info_from_from_queryset_call(
"""

if (
# Check that this is a from_queryset call on a manager subclass
# Check that this is a from_queryset call
not isinstance(call_expr.callee, MemberExpr)
or not isinstance(call_expr.callee.expr, RefExpr)
or not isinstance(call_expr.callee.expr.node, TypeInfo)
or not call_expr.callee.expr.node.has_base(fullnames.BASE_MANAGER_CLASS_FULLNAME)
or not call_expr.callee.name == "from_queryset"
# Check that the call has one or two arguments and that the first is a
# QuerySet subclass
# Check that the call has one or two arguments
or not 1 <= len(call_expr.args) <= 2
or not isinstance(call_expr.args[0], RefExpr)
or not isinstance(call_expr.args[0].node, TypeInfo)
or not call_expr.args[0].node.has_base(fullnames.QUERYSET_CLASS_FULLNAME)
):
return None

base_manager_info, queryset_info = call_expr.callee.expr.node, call_expr.args[0].node
if queryset_info.fullname is None:
if (
# Handle potentially forwarded types
base_manager_info is None
or queryset_info is None
# In some cases, due to the way the semantic analyzer works, only
# passed_queryset.name is available. But it should be analyzed again,
# so this isn't a problem.
return None # type: ignore[unreachable]
or queryset_info.fullname is None
):
raise helpers.IncompleteDefnException
elif (
not isinstance(base_manager_info, TypeInfo)
or not isinstance(queryset_info, TypeInfo)
# Check that the from_queryset call is on a manager subclass and that a first
# argument is a QuerySet subclass. Otherwise we've encountered a function call
# that is not relevant for us
or not base_manager_info.has_base(fullnames.BASE_MANAGER_CLASS_FULLNAME)
or not queryset_info.has_base(fullnames.QUERYSET_CLASS_FULLNAME)
):
return None

if len(call_expr.args) == 2 and isinstance(call_expr.args[1], StrExpr):
manager_name = call_expr.args[1].value
Expand All @@ -384,17 +397,13 @@ def create_manager_info_from_from_queryset_call(
new_manager_info = manager_sym.node
else:
# Create a new `TypeInfo` instance for the manager type
try:
new_manager_info = create_manager_class(
api=api,
base_manager_info=base_manager_info,
name=manager_name,
line=call_expr.line,
with_unique_name=name is not None and name != manager_name,
)
except helpers.IncompleteDefnException:
return None

new_manager_info = create_manager_class(
api=api,
base_manager_info=base_manager_info,
name=manager_name,
line=call_expr.line,
with_unique_name=name is not None and name != manager_name,
)
populate_manager_from_queryset(new_manager_info, queryset_info)
register_dynamically_created_manager(
fullname=new_manager_info.fullname,
Expand Down
15 changes: 8 additions & 7 deletions mypy_django_plugin/transformers/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,14 +338,15 @@ def run_with_model_cls(self, model_cls: Type[Model]) -> None:

if manager_info is None:
# We couldn't find a manager type, see if we should create one
manager_info = self.create_manager_from_from_queryset(manager_name)

if manager_info is None:
incomplete_manager_defs.add(manager_name)
continue
try:
manager_info = self.create_manager_from_from_queryset(manager_name)
except helpers.IncompleteDefnException:
incomplete_manager_defs.add(manager_name)
continue

manager_type = Instance(manager_info, [Instance(self.model_classdef.info, [])])
self.add_new_node_to_model_class(manager_name, manager_type, is_classvar=True)
if manager_info is not None:
manager_type = Instance(manager_info, [Instance(self.model_classdef.info, [])])
self.add_new_node_to_model_class(manager_name, manager_type, is_classvar=True)

if incomplete_manager_defs:
if not self.api.final_iteration:
Expand Down
4 changes: 2 additions & 2 deletions tests/typecheck/fields/test_related.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -748,9 +748,9 @@
class TransactionLog(models.Model):
transaction = models.ForeignKey(Transaction, on_delete=models.CASCADE)
out: |
myapp/models:11: error: Could not resolve manager type for "myapp.models.Transaction.objects" [django-manager-missing]
myapp/models:13: note: Revealed type is "django.db.models.fields.related_descriptors.RelatedManager[myapp.models.TransactionLog]"
myapp/models:15: note: Revealed type is "myapp.models.UnknownManager[myapp.models.Transaction]"
myapp/models:15: note: Revealed type is "django.db.models.manager.BaseManager[Any]"
myapp/models:16: error: "BaseManager[Any]" has no attribute "custom" [attr-defined]
myapp/models:16: note: Revealed type is "Any"


Expand Down
7 changes: 7 additions & 0 deletions tests/typecheck/managers/querysets/test_from_queryset.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -906,3 +906,10 @@
main:12: error: Argument 1 to "from_queryset" of "BaseManager" has incompatible type "<typing special form>"; expected "Type[QuerySet[Model, Model]]" [arg-type]
main:17: note: Revealed type is "Type[django.db.models.manager.Manager[django.db.models.base.Model]]"
main:17: error: Argument 1 to "from_queryset" of "BaseManager" has incompatible type "Type[NonQSGeneric[Any]]"; expected "Type[QuerySet[Model, Model]]" [arg-type]

- case: test_from_queryset_handles_passed_a_non_queryset
main: |
from typing import Any
from django.db import models
class Invalid: ...
manager = models.Manager[Any].from_queryset(Invalid) # E: Argument 1 to "from_queryset" of "BaseManager" has incompatible type "Type[Invalid]"; expected "Type[QuerySet[Any, Any]]" [arg-type]
29 changes: 13 additions & 16 deletions tests/typecheck/managers/test_managers.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -519,7 +519,7 @@
reveal_type(user.bookingowner_set)
reveal_type(user.booking_set)

# Check QuerySet methods on UnknownManager
# Check QuerySet methods on unknown manager
reveal_type(Booking.objects.all)
reveal_type(Booking.objects.custom)
reveal_type(Booking.objects.all().filter)
Expand All @@ -539,30 +539,27 @@
out: |
myapp/models:13: error: Couldn't resolve related manager 'booking_set' for relation 'myapp.models.Booking.renter'. [django-manager-missing]
myapp/models:13: error: Couldn't resolve related manager 'bookingowner_set' for relation 'myapp.models.Booking.owner'. [django-manager-missing]
myapp/models:20: error: Could not resolve manager type for "myapp.models.Booking.objects" [django-manager-missing]
myapp/models:23: error: Could not resolve manager type for "myapp.models.TwoUnresolvable.objects" [django-manager-missing]
myapp/models:24: error: Could not resolve manager type for "myapp.models.TwoUnresolvable.second_objects" [django-manager-missing]
myapp/models:27: error: Could not resolve manager type for "myapp.models.AbstractUnresolvable.objects" [django-manager-missing]
myapp/models:32: error: Could not resolve manager type for "myapp.models.InvisibleUnresolvable.objects" [django-manager-missing]
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 "myapp.models.UnknownManager[myapp.models.Booking]"
myapp/models:39: note: Revealed type is "django.db.models.manager.Manager[Any]"
myapp/models:40: note: Revealed type is "django.db.models.manager.Manager[myapp.models.Booking]"
myapp/models:42: note: Revealed type is "myapp.models.UnknownManager[myapp.models.TwoUnresolvable]"
myapp/models:43: note: Revealed type is "myapp.models.UnknownManager[myapp.models.TwoUnresolvable]"
myapp/models:42: note: Revealed type is "django.db.models.manager.Manager[Any]"
myapp/models:43: note: Revealed type is "django.db.models.manager.Manager[Any]"
myapp/models:44: note: Revealed type is "django.db.models.manager.Manager[myapp.models.TwoUnresolvable]"
myapp/models:46: note: Revealed type is "myapp.models.UnknownManager[myapp.models.InvisibleUnresolvable]"
myapp/models:46: note: Revealed type is "django.db.models.manager.Manager[Any]"
myapp/models:47: note: Revealed type is "django.db.models.manager.Manager[myapp.models.InvisibleUnresolvable]"
myapp/models:49: note: Revealed type is "django.db.models.fields.related_descriptors.RelatedManager[myapp.models.Booking]"
myapp/models:50: note: Revealed type is "django.db.models.fields.related_descriptors.RelatedManager[myapp.models.Booking]"
myapp/models:53: note: Revealed type is "def () -> myapp.models.UnknownQuerySet[myapp.models.Booking, myapp.models.Booking]"
myapp/models:53: note: Revealed type is "def () -> django.db.models.query.QuerySet[Any, Any]"
myapp/models:54: error: "Manager[Any]" has no attribute "custom" [attr-defined]
myapp/models:54: note: Revealed type is "Any"
myapp/models:55: note: Revealed type is "def (*args: Any, **kwargs: Any) -> myapp.models.UnknownQuerySet[myapp.models.Booking, myapp.models.Booking]"
myapp/models:55: note: Revealed type is "def (*args: Any, **kwargs: Any) -> django.db.models.query.QuerySet[Any, Any]"
myapp/models:56: error: "QuerySet[Any, Any]" has no attribute "custom" [attr-defined]
myapp/models:56: note: Revealed type is "Any"
myapp/models:57: note: Revealed type is "Union[myapp.models.Booking, None]"
myapp/models:58: note: Revealed type is "myapp.models.Booking"
myapp/models:59: note: Revealed type is "builtins.list[myapp.models.Booking]"
myapp/models:60: note: Revealed type is "builtins.list[myapp.models.Booking]"
myapp/models:57: note: Revealed type is "Union[Any, None]"
myapp/models:58: note: Revealed type is "Any"
myapp/models:59: note: Revealed type is "builtins.list[Any]"
myapp/models:60: note: Revealed type is "builtins.list[Any]"
Comment on lines +544 to +562
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to argue that the changes here improves correctness. Since the manager in the test comes from a function that specifies a manager as return type:

def LocalManager() -> models.Manager:
    """
    Returns a manager instance of an inlined manager type that can't
    be resolved.
    """
    class InnerManager(models.Manager): ...
    return InnerManager()

This (now) assigns a type models.Manager[Any], the return type of the function. I would say that the previous behaviour did too much, since it tried to express that assigned an invalid manager. But to know that it had to draw conclusions from the function body and thus "override" the return type.

The plugin should now only consider calls of the format: objects = <Manager>.from_queryset(<QuerySet>)() or objects = <QuerySet>.as_manager() in this case

myapp/models:64: note: Revealed type is "def () -> django.db.models.query.QuerySet[myapp.models.Booking, myapp.models.Booking]"
myapp/models:65: error: "RelatedManager[Booking]" has no attribute "custom" [attr-defined]
myapp/models:65: note: Revealed type is "Any"
Expand Down
Loading