Don't remove objects attribute from Model in plugin

[flaeppe]

Partially reverts #1672

#2276 started looking through the mro for manager attributes, but that also conflicts with mypy_django_plugin.transformers.models.MetaclassAdjustments, which removes objects from django.db.models.Model. Because if a lookup through the mro is done before MetaclassAdjustments has run, the plugin fools itself that the objects attribute exists.

I figured that reverting the removal of objects had most value, since most people use the default behaviour and it seems that not having an objects is quite rare.

It's currently only our plugin users that gets to follow along Django implementing the default manager attribute as an abstract attribute. But instead of trying to provide our home built one I think we should wait for any built in support of abstract attributes and then see if we can get any last pieces to work via the plugin.

Related issues

• Refs: Declare manager class attributes on models as ClassVars #1672
• Refs: lookup manager type via mro #2276
• Closes: Type[SomeConcreteModel] has no attribute "objects" -- but it does at runtime #1744
• Refs: Add objects attribute to Token djangorestframework-stubs#644 (comment)
• Refs: [Idea] Add typing.Abstract to allow hinting abstract attributes python/cpython#106248
• Refs: type[Model] no longer allows access to .objects in django-stubs 4.2.4 #1684
• Closes [5.0.3 regression] error: "type[RealmFilter]" has no attribute "objects" [attr-defined] #2304

[flaeppe]

Well, with these changes we end up with having to declare ClassVar for objects when declared explicitly.

No surprise, I suppose, but probably a bunch of changes required for many.

[flaeppe]

It actually is surprising, we shouldn't get complaints on overriding class with instance variable in all the failing tests. I think that we're missing something in the plugin, gonna dig around and see

[sobolevn]

@flaeppe can you please add a regression test from #2304 to this PR?

[flaeppe]

Yeah sure

[flaeppe]

@sobolevn I've tried out the test, locally, but it doesn't break on master. It's probably some difference between how our tests are run and how it's run in https://github.com/andersk/no-attribute-objects. Since that whole issue is depending on how mypy builds or decides to process some internal structure I would guess.

I can still add it in if you like but it's unfortunately not reproducing anything

[flaeppe]

Test can be found in 3366841

[flaeppe]

This was a strange discovery I had to battle with for some time to find any sort of workaround.

I realised that the error "overriding a class variable with an instance variable" from mypy uses the body/expressions instances e.g. AssignmentStmt.lvalues[0]. But we don't overwrite the expressions when inserting a new Var in any symbol table. So the checker uses the lingering reference to a Var instance that is not the same as in the symbol table and that one doesn't set/have is_classvar the way we want.

[sobolevn]

I am a bit confused about new ClassVar annotations in tests.

[sobolevn]

Why do we need this change?

[flaeppe]

Looked around a bit and It comes from the plugin no longer forcing is_classvar = True on an all manager Vars

I don't think we should need to set ClassVar explicitly either, but mypy otherwise gives us an error on attribute access

main:2: error: Access to generic instance variables via class is ambiguous  [misc]

Not a big thing that the plugin forces is_classvar = True, but optimally I don't think it should be necessary. Not sure what to make of the error though, I don't see why it's a generic instance variable..

[flaeppe]

Right, I think I've found the problem. Found that one exception that avoids the error accessing generic instance variable via class is "Self type wrapped in ClassVar"

Ref: https://github.com/python/mypy/blob/570b90a7a368f04c64f60af339d0ac1808c49c15/mypy/checkmember.py#L1065-L1070

But our plugin blindly adds Self as a generic argument to a manager, regardless of is_classvar=True or not.

django-stubs/mypy_django_plugin/transformers/querysets.py

Lines 42 to 54 in 972a4e5

	def determine_proper_manager_type(ctx: FunctionContext) -> MypyType:
	default_return_type = ctx.default_return_type
	assert isinstance(default_return_type, Instance)
	outer_model_info = helpers.get_typechecker_api(ctx).scope.active_class()
	if (
	outer_model_info is None
	or not outer_model_info.has_base(fullnames.MODEL_CLASS_FULLNAME)
	or outer_model_info.self_type is None
	):
	return default_return_type
	return helpers.reparametrize_instance(default_return_type, [outer_model_info.self_type])

Which means that the error will appear for e.g. Manager[Self]

[flaeppe]

I suppose there's 2 ways out to keep working with Self:

1. Force is_classvar = True
2. Don't add a type argument Self when is_classvar = False

[flaeppe]

I went with alternative 1. and we're forcing ClassVar, as that's what we did previously. That should leave those parts unchanged I'm thinking.

[flaeppe]

Well, now I just found that I went down this exact rabbit hole before: #1649 (comment)

Had completely forgotten about that..

[sobolevn]

Let's also wait for @intgr's review. This part is very complex.
Thanks a lot for your work!

[delfick]

Without passing on an opinion about this PR, and not at all any kind of blocker, I just wanna take a moment and quietly point out the way I've been resolving this problem at my workplace (6 million line django app with over 2000 models, still on mypy 1.4, django 4.1) is I wrote this https://github.com/delfick/extended-mypy-django-plugin so I can say Concrete[AbstractModel] that gets translated into a union of all the concrete children (and DefaultQuerySet[Model] to find the default queryset for a model, or union of concrete default querysets). So essentially a type that represents everything on all the concrete children without taking away the things the abstract model does not have. I was able to use it to get all the errors down so we can do an upgrade. I intend on making a github issue here at some point pointing it out and asking for opinions. It ends up solving more problems than just "objects" getting attr-defined errors, but introduces a bunch of changes I imagine would take discussion if any of this gets put upstream here.

[flaeppe]

It sounds interesting, feel free to open any discussions/issues as you feel relevant

[sobolevn]

I like the idea of Concrete type. But, I think that from users' point of view, most SomeAbstractModel annotations are concrete. Not the other way around. So, we can add AbstractModel[SomeAbstractModel] to specify that we are indeed working with abstract model that might not have .objects, etc.

@delfick please, feel free to discuss / backport any features / ideas. I would be happy to help!

[delfick]

@sobolevn it's more about saying "I want the concrete models that extend this abstract model". Especially useful when statically you know about more models than are available at runtime. I'll expand when I get to making a github issue. I wanted to get us onto newer versions of mypy/django/django-stubs before I did that because of how long this process is taking. Hopefully not too much longer now (also figuring out how to make the mypy plugin took an astounding amount of churn to make it so it works without causing problems)

though, rereading your comment, I get what you mean. I could be wrong but my instinct would be mypy wouldn't make that possible without enhancements to what hooks are available.