From 2df5d4ccb0e62a6f673a3e6932d388b6af5b7de6 Mon Sep 17 00:00:00 2001 From: Petter Friberg Date: Fri, 26 Jul 2024 10:35:14 +0200 Subject: [PATCH 1/2] Don't remove `objects` attribute from `Model` in plugin Partially reverts #1672 --- django-stubs/db/models/base.pyi | 3 +- mypy_django_plugin/transformers/models.py | 33 ++++++++-- tests/typecheck/fields/test_related.yml | 34 ++++------ .../managers/querysets/test_from_queryset.yml | 65 ++++++++----------- tests/typecheck/models/test_abstract.yml | 2 +- tests/typecheck/models/test_meta_options.yml | 2 +- 6 files changed, 70 insertions(+), 69 deletions(-) diff --git a/django-stubs/db/models/base.pyi b/django-stubs/db/models/base.pyi index 38ac288ab..c529897b2 100644 --- a/django-stubs/db/models/base.pyi +++ b/django-stubs/db/models/base.pyi @@ -36,8 +36,7 @@ class Model(metaclass=ModelBase): # and re-add them to correct concrete subclasses of 'Model' DoesNotExist: Final[type[ObjectDoesNotExist]] MultipleObjectsReturned: Final[type[BaseMultipleObjectsReturned]] - # This 'objects' attribute will be deleted, via the plugin, in favor of managing it - # to only exist on subclasses it exists on during runtime. + objects: ClassVar[Manager[Self]] _meta: ClassVar[Options[Self]] diff --git a/mypy_django_plugin/transformers/models.py b/mypy_django_plugin/transformers/models.py index 92c3c8fc8..be76e81fb 100644 --- a/mypy_django_plugin/transformers/models.py +++ b/mypy_django_plugin/transformers/models.py @@ -319,11 +319,18 @@ def reparametrize_dynamically_created_manager(self, manager_name: str, manager_i assert manager_info is not None # Reparameterize dynamically created manager with model type manager_type = helpers.fill_manager(manager_info, Instance(self.model_classdef.info, [])) + manager_node = self.model_classdef.info.get(manager_name) + if manager_node and isinstance(manager_node.node, Var): + manager_node.node.type = manager_type self.add_new_node_to_model_class(manager_name, manager_type, is_classvar=True) def run_with_model_cls(self, model_cls: Type[Model]) -> None: manager_info: Optional[TypeInfo] + def cast_var_to_classvar(symbol: Optional[SymbolTableNode]) -> None: + if symbol and isinstance(symbol.node, Var): + symbol.node.is_classvar = True + incomplete_manager_defs = set() for manager_name, manager in model_cls._meta.managers_map.items(): manager_node = self.model_classdef.info.get(manager_name) @@ -345,7 +352,24 @@ def run_with_model_cls(self, model_cls: Type[Model]) -> None: assert self.model_classdef.info.self_type is not None manager_type = helpers.fill_manager(manager_info, self.model_classdef.info.self_type) - self.add_new_node_to_model_class(manager_name, manager_type, is_classvar=True) + # It seems that the type checker fetches a Var from expressions, but looks + # through the symbol table for the type(at some later stage?). Currently we + # don't overwrite the reference mypy holds from an expression to a Var + # instance when adding a new node, we only overwrite the reference to the + # Var in the symbol table. That means there's a lingering Var instance + # attached to expressions and if we don't flip that to a ClassVar, the + # checker will emit an error for overriding a class variable with an + # instance variable. As mypy seems to check that via the expression and not + # the symbol table. Optimally we want to just set a type on the existing Var + # like: + # manager_node.node.type = manager_type + # but for some reason that doesn't work. It only works replacing the + # existing Var with a new one in the symbol table. + cast_var_to_classvar(manager_node) + if manager_fullname == manager_info.fullname and manager_node and isinstance(manager_node.node, Var): + manager_node.node.type = manager_type + else: + self.add_new_node_to_model_class(manager_name, manager_type, is_classvar=True) if incomplete_manager_defs: if not self.api.final_iteration: @@ -360,6 +384,7 @@ def run_with_model_cls(self, model_cls: Type[Model]) -> None: # setting _some_ type fallback_manager_info = self.get_or_create_manager_with_any_fallback() if fallback_manager_info is not None: + cast_var_to_classvar(self.model_classdef.info.get(manager_name)) assert self.model_classdef.info.self_type is not None manager_type = helpers.fill_manager(fallback_manager_info, self.model_classdef.info.self_type) self.add_new_node_to_model_class(manager_name, manager_type, is_classvar=True) @@ -958,12 +983,6 @@ def adjust_model_class(cls, ctx: ClassDefContext) -> None: ): del ctx.cls.info.names["MultipleObjectsReturned"] - objects = ctx.cls.info.names.get("objects") - if objects is not None and isinstance(objects.node, Var) and not objects.plugin_generated: - del ctx.cls.info.names["objects"] - - return - def get_exception_bases(self, name: str) -> List[Instance]: bases = [] for model_base in self.model_classdef.info.direct_base_classes(): diff --git a/tests/typecheck/fields/test_related.yml b/tests/typecheck/fields/test_related.yml index 5ffb2eef9..9565a3b8f 100644 --- a/tests/typecheck/fields/test_related.yml +++ b/tests/typecheck/fields/test_related.yml @@ -727,10 +727,9 @@ - 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) + TransactionManager = models.Manager.from_queryset(TransactionQuerySet) class Transaction(models.Model): pk = 0 objects = TransactionManager() @@ -742,7 +741,7 @@ Transaction().test() -- case: foreign_key_relationship_for_models_with_custom_manager_unsolvable +- case: foreign_key_relationship_for_models_with_custom_manager_solvable_via_as_manager_type main: | from myapp.models import Transaction installed_apps: @@ -752,30 +751,27 @@ - path: myapp/models.py content: | from django.db import models - from django.db.models.manager import BaseManager class TransactionQuerySet(models.QuerySet): def custom(self) -> None: pass - def TransactionManager() -> BaseManager: - return BaseManager.from_queryset(TransactionQuerySet)() + def TransactionManager() -> models.Manager: + return models.Manager.from_queryset(TransactionQuerySet)() class Transaction(models.Model): objects = TransactionManager() def test(self) -> None: - reveal_type(self.transactionlog_set) - # We use a fallback Any type: - reveal_type(Transaction.objects) - reveal_type(Transaction.objects.custom()) + reveal_type(self.transactionlog_set) # N: Revealed type is "django.db.models.fields.related_descriptors.RelatedManager[myapp.models.TransactionLog]" + # We get a lucky shot here as long as the plugin predeclares a + # manager for `.as_manager` for every base class of QuerySet. + # It's just lucky that the runtime's manager name is the same + # name as for the predeclared manager. Resolving this wouldn't + # be possible without inspection of the runtime + reveal_type(Transaction.objects) # N: Revealed type is "myapp.models.ManagerFromTransactionQuerySet[myapp.models.Transaction]" + reveal_type(Transaction.objects.custom()) # N: Revealed type is "None" 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:16: note: Revealed type is "Any" - - case: resolve_primary_keys_for_foreign_keys_with_abstract_self_model main: | @@ -913,12 +909,11 @@ - path: myapp/models/purchase.py content: | from django.db import models - from django.db.models.manager import BaseManager from .querysets import PurchaseQuerySet from .store import Store from .user import User - PurchaseManager = BaseManager.from_queryset(PurchaseQuerySet) + PurchaseManager = models.Manager.from_queryset(PurchaseQuerySet) class Purchase(models.Model): objects = PurchaseManager() store = models.ForeignKey(to=Store, on_delete=models.CASCADE, related_name='purchases') @@ -936,7 +931,6 @@ - path: myapp/models.py content: | from django.db import models - from django.db.models.manager import BaseManager class User(models.Model): purchases: int @@ -945,7 +939,7 @@ def queryset_method(self) -> "PurchaseQuerySet": return self.all() - PurchaseManager = BaseManager.from_queryset(PurchaseQuerySet) + PurchaseManager = models.Manager.from_queryset(PurchaseQuerySet) class Purchase(models.Model): objects = PurchaseManager() user = models.ForeignKey(to=User, on_delete=models.CASCADE, related_name='purchases') diff --git a/tests/typecheck/managers/querysets/test_from_queryset.yml b/tests/typecheck/managers/querysets/test_from_queryset.yml index 60f6c765f..e631e2c14 100644 --- a/tests/typecheck/managers/querysets/test_from_queryset.yml +++ b/tests/typecheck/managers/querysets/test_from_queryset.yml @@ -14,13 +14,12 @@ - path: myapp/models.py content: | from django.db import models - from django.db.models.manager import BaseManager from typing import List, Dict, TypeVar from typing_extensions import Self M = TypeVar("M", covariant=True, bound=models.Model) - class CustomManager(BaseManager[M]): + class CustomManager(models.Manager[M]): def test_custom_manager(self) -> Self: ... class BaseQuerySet(models.QuerySet[M]): @@ -40,7 +39,7 @@ def method(self) -> "QuerySetWithoutSelf": return self - ManagerWithoutSelf = BaseManager.from_queryset(QuerySetWithoutSelf) + ManagerWithoutSelf = models.Manager.from_queryset(QuerySetWithoutSelf) class MyModelWithoutSelf(models.Model): objects = ManagerWithoutSelf() @@ -58,11 +57,10 @@ content: | from typing import ClassVar from typing_extensions import Self - from django.db.models import Model - from django.db.models.manager import BaseManager + from django.db.models import Model, Manager from django.db.models.query import QuerySet - MyManager = BaseManager.from_queryset(QuerySet, "MyManager") + MyManager = Manager.from_queryset(QuerySet, "MyManager") class Base(Model): objects: ClassVar[MyManager[Self]] = MyManager() @@ -95,13 +93,13 @@ return 'hello' NewManager = BaseManager.from_queryset(ModelQuerySet) class MyModel(models.Model): - objects = NewManager() + objects = NewManager() # E: Incompatible types in assignment (expression has type "BaseManagerFromModelQuerySet[Self]", base class "Model" defined the type as "Manager[MyModel]") [assignment] - case: from_queryset_queryset_imported_from_other_module main: | from myapp.models import MyModel - reveal_type(MyModel.objects) # N: Revealed type is "myapp.models.BaseManagerFromModelQuerySet[myapp.models.MyModel]" - reveal_type(MyModel.objects) # N: Revealed type is "myapp.models.BaseManagerFromModelQuerySet[myapp.models.MyModel]" + reveal_type(MyModel.objects) # N: Revealed type is "myapp.models.ManagerFromModelQuerySet[myapp.models.MyModel]" + reveal_type(MyModel.objects) # N: Revealed type is "myapp.models.ManagerFromModelQuerySet[myapp.models.MyModel]" reveal_type(MyModel.objects.get()) # N: Revealed type is "myapp.models.MyModel" reveal_type(MyModel.objects.queryset_method()) # N: Revealed type is "myapp.querysets.ModelQuerySet" reveal_type(MyModel.objects.queryset_method_2()) # N: Revealed type is "typing.Iterable[myapp.querysets.Custom]" @@ -139,10 +137,9 @@ - path: myapp/models.py content: | from django.db import models - from django.db.models.manager import BaseManager from .querysets import ModelQuerySet - NewManager = BaseManager.from_queryset(ModelQuerySet) + NewManager = models.Manager.from_queryset(ModelQuerySet) class MyModel(models.Model): objects = NewManager() @@ -159,7 +156,6 @@ from typing import TypeVar, TYPE_CHECKING from django.db import models - from django.db.models.manager import BaseManager if TYPE_CHECKING: from .models import MyModel @@ -174,10 +170,9 @@ - path: myapp/models.py content: | from django.db import models - from django.db.models.manager import BaseManager from .querysets import MyModelQuerySet - class TypedManager(BaseManager["MyModel"]): ... + class TypedManager(models.Manager["MyModel"]): ... NewManager = TypedManager.from_queryset(MyModelQuerySet) class MyModel(models.Model): @@ -195,7 +190,6 @@ from typing import TypeVar, TYPE_CHECKING from django.db import models - from django.db.models.manager import BaseManager if TYPE_CHECKING: from .models import MyModel @@ -211,17 +205,16 @@ - path: myapp/models.py content: | from django.db import models - from django.db.models.manager import BaseManager from .querysets import MyModelQuerySet - NewManager = BaseManager.from_queryset(MyModelQuerySet) + NewManager = models.Manager.from_queryset(MyModelQuerySet) class MyModel(models.Model): objects = NewManager() - case: from_queryset_generated_manager_imported_from_other_module main: | from myapp.models import MyModel - reveal_type(MyModel.objects) # N: Revealed type is "myapp.querysets.BaseManagerFromModelQuerySet[myapp.models.MyModel]" + reveal_type(MyModel.objects) # N: Revealed type is "myapp.querysets.ManagerFromModelQuerySet[myapp.models.MyModel]" reveal_type(MyModel.objects.get()) # N: Revealed type is "myapp.models.MyModel" reveal_type(MyModel.objects.queryset_method()) # N: Revealed type is "myapp.querysets.ModelQuerySet" reveal_type(MyModel.objects.queryset_method_2()) # N: Revealed type is "typing.Iterable[myapp.querysets.Custom]" @@ -237,7 +230,6 @@ content: | from typing import TYPE_CHECKING, Iterable, Sequence from django.db import models - from django.db.models.manager import BaseManager if TYPE_CHECKING: from .models import MyModel @@ -257,7 +249,7 @@ def queryset_method_4(self, arg: Sequence) -> None: return None - NewManager = BaseManager.from_queryset(ModelQuerySet) + NewManager = models.Manager.from_queryset(ModelQuerySet) - path: myapp/models.py content: | @@ -402,7 +394,7 @@ - case: from_queryset_with_class_inheritance main: | from myapp.models import MyModel - reveal_type(MyModel().objects) # N: Revealed type is "myapp.models.BaseManagerFromModelQuerySet[myapp.models.MyModel]" + reveal_type(MyModel().objects) # N: Revealed type is "myapp.models.ManagerFromModelQuerySet[myapp.models.MyModel]" reveal_type(MyModel().objects.get()) # N: Revealed type is "myapp.models.MyModel" reveal_type(MyModel().objects.queryset_method()) # N: Revealed type is "builtins.str" installed_apps: @@ -412,14 +404,13 @@ - path: myapp/models.py content: | from django.db import models - from django.db.models.manager import BaseManager class BaseQuerySet(models.QuerySet): def queryset_method(self) -> str: return 'hello' class ModelQuerySet(BaseQuerySet): pass - NewManager = BaseManager.from_queryset(ModelQuerySet) + NewManager = models.Manager.from_queryset(ModelQuerySet) class MyModel(models.Model): objects = NewManager() @@ -668,12 +659,12 @@ - path: myapp/models.py content: | from django.db import models - from django.db.models.manager import BaseManager + from django.db.models.manager import Manager class MyQuerySet(models.QuerySet["MyModel"]): ... - MyManager = BaseManager.from_queryset(MyQuerySet) + MyManager = Manager.from_queryset(MyQuerySet) class MyModel(models.Model): objects = MyManager() @@ -744,9 +735,9 @@ - case: handles_name_collision_with_generated_type main: | - from myapp.models import MyModel, BaseManagerFromModelQuerySet - reveal_type(BaseManagerFromModelQuerySet()) # N: Revealed type is "myapp.models.BaseManagerFromModelQuerySet[Never]" - reveal_type(MyModel.objects) # N: Revealed type is "myapp.models.BaseManagerFromModelQuerySet[myapp.models.MyModel]" + from myapp.models import MyModel, ManagerFromModelQuerySet + reveal_type(ManagerFromModelQuerySet()) # N: Revealed type is "myapp.models.ManagerFromModelQuerySet[Never]" + reveal_type(MyModel.objects) # N: Revealed type is "myapp.models.ManagerFromModelQuerySet[myapp.models.MyModel]" installed_apps: - myapp files: @@ -754,21 +745,20 @@ - path: myapp/models.py content: | from django.db import models - from django.db.models.manager import BaseManager class ModelQuerySet(models.QuerySet["MyModel"]): ... - BaseManagerFromModelQuerySet = BaseManager.from_queryset(ModelQuerySet) + ManagerFromModelQuerySet = models.Manager.from_queryset(ModelQuerySet) class MyModel(models.Model): - objects = BaseManagerFromModelQuerySet() + objects = ManagerFromModelQuerySet() - case: resolves_name_collision_with_other_module_level_object main: | - from myapp.models import MyModel, Generated, BaseManagerFromModelQuerySet - reveal_type(BaseManagerFromModelQuerySet) # N: Revealed type is "builtins.int" - reveal_type(Generated()) # N: Revealed type is "myapp.models.BaseManagerFromModelQuerySet1[Never]" - reveal_type(MyModel.objects) # N: Revealed type is "myapp.models.BaseManagerFromModelQuerySet1[myapp.models.MyModel]" + from myapp.models import MyModel, Generated, ManagerFromModelQuerySet + reveal_type(ManagerFromModelQuerySet) # N: Revealed type is "builtins.int" + reveal_type(Generated()) # N: Revealed type is "myapp.models.ManagerFromModelQuerySet2[Never]" + reveal_type(MyModel.objects) # N: Revealed type is "myapp.models.ManagerFromModelQuerySet2[myapp.models.MyModel]" installed_apps: - myapp files: @@ -776,13 +766,12 @@ - path: myapp/models.py content: | from django.db import models - from django.db.models.manager import BaseManager class ModelQuerySet(models.QuerySet["MyModel"]): ... - BaseManagerFromModelQuerySet = 1 - Generated = BaseManager.from_queryset(ModelQuerySet) + ManagerFromModelQuerySet = 1 + Generated = models.Manager.from_queryset(ModelQuerySet) class MyModel(models.Model): objects = Generated() diff --git a/tests/typecheck/models/test_abstract.yml b/tests/typecheck/models/test_abstract.yml index 446161a27..a7566d016 100644 --- a/tests/typecheck/models/test_abstract.yml +++ b/tests/typecheck/models/test_abstract.yml @@ -51,7 +51,7 @@ Recursive(parent=Recursive(parent=None)) Concrete(parent=Concrete(parent=None)) out: | - main:4: error: "Type[Recursive]" has no attribute "objects" [attr-defined] + main:4: error: Unexpected attribute "parent" for model "Recursive" [misc] main:5: error: Cannot instantiate abstract class "Recursive" with abstract attributes "DoesNotExist" and "MultipleObjectsReturned" [abstract] main:5: error: Unexpected attribute "parent" for model "Recursive" [misc] installed_apps: diff --git a/tests/typecheck/models/test_meta_options.yml b/tests/typecheck/models/test_meta_options.yml index f6e2b604f..36c0aa234 100644 --- a/tests/typecheck/models/test_meta_options.yml +++ b/tests/typecheck/models/test_meta_options.yml @@ -81,7 +81,7 @@ # Errors: AbstractModel() # E: Cannot instantiate abstract class "AbstractModel" with abstract attributes "DoesNotExist" and "MultipleObjectsReturned" [abstract] - AbstractModel.objects.create() # E: "Type[AbstractModel]" has no attribute "objects" [attr-defined] + AbstractModel.objects.create() installed_apps: - myapp files: From 030a217fe522108439e5f493274a0fbb3f70af9d Mon Sep 17 00:00:00 2001 From: Petter Friberg Date: Thu, 1 Aug 2024 20:14:19 +0200 Subject: [PATCH 2/2] Add regression test from #2304 --- tests/typecheck/managers/test_managers.yml | 62 ++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/tests/typecheck/managers/test_managers.yml b/tests/typecheck/managers/test_managers.yml index 1a9b0019f..32275a92b 100644 --- a/tests/typecheck/managers/test_managers.yml +++ b/tests/typecheck/managers/test_managers.yml @@ -761,3 +761,65 @@ generic_manager = models.Manager() generic_manager_from_generic_queryset = GenericManagerFromGenericQuerySet() generic_manager_from_populated_queryset = GenericManagerFromPopulatedQuerySet() + +# Regression test for #2304 +- case: test_objects_managers_is_kept_with_specific_import_graph + main: | + from zerver.models import RealmFilter + reveal_type(RealmFilter.objects) # N: Revealed type is "django.db.models.manager.Manager[zerver.models.linkifiers.RealmFilter]" + installed_apps: + - django.contrib.auth + - django.contrib.contenttypes + - confirmation + - zerver + files: + - path: confirmation/__init__.py + - path: confirmation/models.py + content: | + from django.db import models + + from zerver.models import Realm + + class Confirmation(models.Model): + realm = models.ForeignKey(Realm, on_delete=models.CASCADE) + - path: zerver/__init__.py + - path: zerver/models/__init__.py + content: | + from zerver.models.linkifiers import RealmFilter as RealmFilter + from zerver.models.realms import Realm as Realm + from zerver.models.streams import Stream as Stream + from zerver.models.users import UserProfile as UserProfile + + RealmFilter.objects + - path: zerver/models/linkifiers.py + content: | + from django.db import models + + + class RealmFilter(models.Model): + pass + - path: zerver/models/realms.py + content: | + from django.db import models + + + class Realm(models.Model): + pass + - path: zerver/models/streams.py + content: | + from django.db import models + + from zerver.models.realms import Realm + from zerver.models.users import UserProfile + + + class Stream(models.Model): + realm = models.ForeignKey(Realm, on_delete=models.RESTRICT) + creator = models.ForeignKey(UserProfile, on_delete=models.RESTRICT) + - path: zerver/models/users.py + content: | + from django.contrib.auth.models import AbstractBaseUser, PermissionsMixin + + + class UserProfile(AbstractBaseUser, PermissionsMixin): + pass