Move ModelBase.objects declaration to Model.objects, for mypy 1.5.0

[andersk]

I have made things!

mypy 1.5.0 was fixed to understand that metaclass attributes take precedence over attributes in the regular class. So we need to declare objects in the regular class to allow it to be overridden in subclasses.

Related issues

• Fixes Custom managers broken with mypy 1.5.0 #1648.

[sobolevn]

Looks like ModelBase only has _default_manager and _base_manager: https://github.com/django/django/blob/9946f0b0d9356b55e819f861b31615fa5b548f99/django/db/models/base.py#L430-L436

And objects is really not found 🤔

[intgr]

Looks like ModelBase only has _default_manager and _base_manager

The objects attribute is dynamically created in ModelBase._prepare method:

https://github.com/django/django/blob/9946f0b0d9356b55e819f861b31615fa5b548f99/django/db/models/base.py#L417-L419

But the change seems good to me. 👍 Thanks!

[intgr]

Do we know why this new error appears in abstract models?

Are there any use cases for accessing objects of an abstract model? I can't think of any.

[andersk]

I think the mypy plugin is why we only get this for abstract models, but I haven’t looked at the details.

An abstract model does not have an objects attribute at all at runtime, so extra errors for that are fine.

[flaeppe]

Are there any use cases for accessing objects of an abstract model? I can't think of any.

I added this test in #1393. Just want to point out that accessing objects on an abstract model wasn't part of any use case, but rather an attempt for completeness of coverage in the plugin code

[intgr]

I tried this out: actually one of my projects relies on this in several places, and new django-stubs now causes errors. Let me open an issue describing the use case and we can discuss it there.

[intgr]

Please have a look at issue #1653

[flaeppe]

Hm, the error also appears for e.g. mypy==1.4.1. Error is emitted by mypy here and they have an example in a comment close by looking like this:

# Check if original variable type has type variables. For example:
class C(Generic[T]):
    x: T
C.x  # Error, ambiguous access
C[int].x  # Also an error, since C[int] is same as C at runtime

I think C.x should be the case that we run in to here(?)

The plugin is populating managers based on the runtime, but since abstract models don't have any, it stays as objects: BaseManager[Self]. Otherwise objects is (normally) overwritten by the plugin, that's why we're only seeing it for abstract models.

django-stubs/mypy_django_plugin/transformers/models.py

Lines 316 to 339 in 85e1f3f

	def run_with_model_cls(self, model_cls: Type[Model]) -> None:
	manager_info: Optional[TypeInfo]
	incomplete_manager_defs = set()
	for manager_name, manager in model_cls._meta.managers_map.items():
	manager_node = self.model_classdef.info.names.get(manager_name, None)
	manager_fullname = helpers.get_class_fullname(manager.__class__)
	manager_info = self.lookup_manager(manager_fullname, manager)
	if manager_node and manager_node.type is not None:
	# Manager is already typed -> do nothing unless it's a dynamically generated manager
	self.reparametrize_dynamically_created_manager(manager_name, manager_info)
	continue
	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
	manager_type = Instance(manager_info, [Instance(self.model_classdef.info, [])])
	self.add_new_node_to_model_class(manager_name, manager_type)

So, I would suspect that if you try to annotate any manager to a model, this error would appear. e.g.

from django.db import models
from typing_extensions import Self

class MyModel(models.Model):
    other_manager: models.Manager[Self]

reveal_type(MyModel.other_manager)

Is there something up with typing_extensions.Self here, or do we need to change something?

[andersk]

You can make mypy accept your example using ClassVar (PEP 526).

class MyModel(models.Model):
    other_manager: ClassVar[models.Manager[Self]]

Another way that more accurately reflects what’s happening at runtime might be

ModelT = TypeVar("ModelT", bound=models.Model)

class ManagerDescriptor:
    def __get__(self, instance: None, owner: type[ModelT]) -> models.Manager[ModelT]: ...

class MyModel(models.Model):
    other_manager: ManagerDescriptor = models.Manager()

This PR has no effect on your other_manager, of course. I didn’t annotate django.db.models.Model.objects itself as a ClassVar or descriptor because that would seem to break more things than it fixes: every subclass of Model that overrides objects would then be required to annotate it as a ClassVar or descriptor too.

(Maybe it would be better to delete the objects annotation from the stub and rely entirely on the plugin for it?)

[flaeppe]

Another way that more accurately reflects what’s happening at runtime might be

ModelT = TypeVar("ModelT", bound=models.Model)

class ManagerDescriptor:
    def __get__(self, instance: None, owner: type[ModelT]) -> models.Manager[ModelT]: ...

class MyModel(models.Model):
    other_manager: ManagerDescriptor = models.Manager()

I don't think this is correctly reflected? The runtime type of objects is actually models.Manager, so it's rather that BaseManager.__get__ that should be implemented? You'd never get hold of the descriptor class itself. Anyways, adding tyst method doesn't change anything either.. Edit: never mind, it's the descriptor doing the work

This is a side note but I think that should be an @overload method with anything other than class access having -> NoReturn:

(Maybe it would be better to delete the objects annotation from the stub and rely entirely on the plugin for it?)

Yes, I've been thinking about this too and I think it's something we should do. I did it just recently with DoesNotExist and MultipleObjectsReturned in #1663. Though we're leaving other type checkers behind by removing objects: from the .pyi. Would've been nice if that attribute would stay, even though it's a bit incorrect, mainly for their quality of life situation.

[flaeppe]

Although, perhaps it's fair to say that it should be declared as a ClassVar? The plugin can auto generate properly etc. But doing it explicitly on a subclass one would have to declare it a ClassVar?

[flaeppe]

I took the liberty to extend the work here and try out changing managers to ClassVars and adjust objects to one too. While the plugin will only add objects to models where it exists runtime.

It can be found in #1672

[intgr]

Changing this to @classproperty seems to fix the create_animal_generic part of issue #1653:

Suggested change

	objects: BaseManager[Self]
	@classproperty
	def objects(cls: type[_Self]) -> BaseManager[_Self]: ...

With import

from django.utils.functional import classproperty

[intgr]

DOH! That causes new errors to appear...

django-stubs/django-stubs/contrib/contenttypes/models.pyi:18: error: Incompatible types in assignment (expression has type "ContentTypeManager", base class "Model" defined the type as "Callable[[Type[_Self]], BaseManager[_Self]]")

[andersk]

classproperty can’t help us with typing this: its constructor annotates the cls parameter as Any, so there’s nothing to connect _Self to the actual model type.

django-stubs/django-stubs/utils/functional.pyi

Lines 99 to 101 in 325006c

	class classproperty(Generic[_Get]):
	fget: Callable[[Any], _Get] | None
	def __init__(self, method: Callable[[Any], _Get] | None = ...) -> None: ...

[intgr]

It does help. The Any doesn't matter here.

Before classproperty even comes into play, mypy has already resolved the relationship between the parameter and return value. The method signature def objects(cls: type[_Self]) -> BaseManager[_Self]: ... is evaluated first, before decorators.

But as I point out above, this breaks overriding objects attribute in a custom model.

[andersk]

Ignoring the decorator is clearly a mypy bug. And it still doesn’t help us, because ignoring the decorator makes mypy incorrectly assume the first parameter is an instance of Model, not any kind of type.

from django.utils.functional import classproperty

class Model:
    @classproperty
    def foo(cls: "Model") -> "Model":  # incorrectly allowed
        return cls

    @classproperty
    def bar(cls: "type[Model]") -> "type[Model]":  # incorrect error: The erased type of self "Type[__main__.Model]" is not a supertype of its class "__main__.Model"
        return cls

print(Model.foo is Model)  # True at runtime
reveal_type(Model)  # Revealed type is "def () -> __main__.Model"
reveal_type(Model.foo)  # Revealed type is "__main__.Model"

See also

• Make classproperty descriptor work python/mypy#2563

[intgr]

@andersk This PR solves some of the issues with mypy 1.5, but there are other test failures that remain involving the objects manager.

Did you look at the remaining errors? https://github.com/typeddjango/django-stubs/actions/runs/5888209826

[andersk]

Yeah I looked for a while but didn’t make further progress.

I did note that some of the tests seem clearly bogus—for example, in the managers_that_defined_on_other_models_do_not_influence case, myapp.models.PublishedBookManager[myapp.models.Book] cannot possibly be a valid type to reveal because the PublishedBookManager class has no generic parameters. But for some reason that test isn’t failing, so I didn’t investigate further.

django-stubs/tests/typecheck/managers/test_managers.yml

Line 172 in 3c77add

reveal_type(Book.published_objects) # N: Revealed type is "myapp.models.PublishedBookManager[myapp.models.Book]"

My proposal for now would be to release this without bumping compatible-mypy, so that at least some users are able to upgrade their projects to mypy 1.5 and give us more data about what real problems may remain.

[flaeppe]

mypy 1.5.0 was fixed to understand that metaclass attributes take precedence over attributes in the regular class. So we need to declare objects in the regular class to allow it to be overridden in subclasses.

I think I hunted down the PR in mypy that is affecting us, just wanted to paste it here for traceability: python/mypy#14988.

[andersk]

Yeah that’s the one I linked from the issue #1648.

[flaeppe]

Yeah that’s the one I linked from the issue #1648.

Oh, I completely missed that.

[flaeppe]

This was merged via #1672