Fix manager types scope

[sterliakov]

Fixed wrong scope of managers methods when manager is imported from another file.

On master branch the following is enough to reproduce the issue:

from django.db import models
from django.contrib.sites.models import Site
from django.contrib.sites.managers import CurrentSiteManager


class MyModel(models.Model):
    site = models.ForeignKey(Site, on_delete=models.CASCADE)

    on_site = CurrentSiteManager()

It results in weird mypy error:

myproject/myapp/models.py:6: error: Name "Optional" is not defined
myproject/myapp/models.py:6: note: Did you forget to import it from "typing"? (Suggestion: "from typing import Optional")

The changes are self-explanatory, but here's a short explanation:

• Before this PR all types were resolved as if manager is defined in current module. However, this assumption doesn't always hold.
• After passing original_module_name, two other tests began to fail. It happened because api.anal_type was expected, but ran only if module_name is None. So we want to call it even if module name was given, but only if initial solution (with lookup_fully_qualified) fails to return something meaningful.
• While researching this issue, I found out that with incremental mode some tests were wrongly passing. After cleaning cache this changed, so incremental mode is a bad idea for testing.
• Added regression test for this particular issue.

Related issues

This issue was discovered in this SO question.

[sobolevn]

We had this flag to support mypy in incremental mode as the first-class-citizen. It is important for:

• large projects that use --incremental to speed up their checks
• IDEs that use mypy plugins

I am not sure that removing it is a good idea.

[sterliakov]

Got it, okay. Reverted this and added explicit mention in CONTRIBUTING.md

[sobolevn]

Can you please post unexpected results here? What were they?

[sterliakov]

The unexpected result was quite simple: I started from this regression test and confirmed that it fails. Then I attempted first fix (added module_name) and rerun test suite - this reg.test passed as expected and two others failed. Then I reverted everything and... got fully passing test suite, including reg.test - which is obviously wrong. Removing mypy cache helped to make reg. test fail again, so incremental mode is harmful during development. Maybe we should have different configs for CI and local development?

[sobolevn]

Maybe we should have different configs for CI and local development?

Yes, this is a good option!

[sobolevn]

👍

[sobolevn]

Can you please drop a few lines about why do we need this file?

[sobolevn]

Awesome work!

[ljodal]

In multiple of our repos this commit introduces a phantom warning, saying <...>/models.py:106: error: Name "Sequence" is not defined. Any ideas why? We're using mypy==0.950 in both repos, but I've also tested with 0.961

[sterliakov]

Same here. Will investigate this tomorrow. I'm surprised it was not caught by test suite.
I should've checked this...

UPD: this Sequence comes from django.db.models.query, line 106, referring to annotation of select_for_update method.

[ljodal]

Yeah, I found that as well, but it doesn't help that much. I'm 99% sure the phantom warning comes from calling api.anal_type(), which didn't happen before this change, but I'm not familiar enough with mypy to understand what goes wrong. When debugging #958 I made some changes similar to this and ended up with similar symptoms. It's clear to me that the copy_method_to_another_class is doing something weird, but I don't understand what.

[flaeppe]

Yeah, I think @ljodal is right here. The problem is most likely running api.anal_type(<Sequence>) while processing a <...>/models.py that doesn't import a Sequence and gets hold of that Sequence value, from copying method django.db.models.query._QuerySet.select_for_update to <...>/models.py.

I can't track down where in mypy the error will be emitted from. But supposedly from somewhere in: https://github.com/python/mypy/blob/203bd64d8c14e8ddbed7911eb6f8f8aa61d728de/mypy/semanal.py#L639

Be aware of that the code emitting the error is from the other call site of copy_method_to_another_class:

django-stubs/mypy_django_plugin/transformers/managers.py

Lines 281 to 288 in 1a29ad4

	helpers.copy_method_to_another_class(
	class_def_context,
	self_type,
	new_method_name=name,
	method_node=func_node,
	return_type=return_type,
	original_module_name=class_mro_info.module_name,
	)

---

Half side note:

IMO that copy_method_to_another_class is quite brutal. Spontaneously I'm thinking that it shouldn't be that complex to "follow along" the dynamic call to builtins.type with 3 arguments(here). IIRC the copy_method_to_another_class is almost reimplementing what the MRO should/can help out with? But I haven't been able to figure out if or how the copying can be replaced using the MRO instead.

It's just that, runtime, that's kind of what Django does. "Only" thing being that the class is dynamically created..

[ljodal]

Half side note:

IMO that copy_method_to_another_class is quite brutal. Spontaneously I'm thinking that it shouldn't be that complex to "follow along" the dynamic call to builtins.type with 3 arguments(here). IIRC the copy_method_to_another_class is almost reimplementing what the MRO should/can help out with? But I haven't been able to figure out if or how the copying can be replaced using the MRO instead.

The problem with doing this through MRO, other than it not matching the actual runtime types, is that Django is skipping some methods when copying/redefining queryset methods on managers. So if we simply say that the generated manager inherits from the queryset we would expose methods that doesn't actually exist on the manager at runtime. Methods can be explicitly marked as skipped or be skipped because they're already implemented by the manager: https://github.com/django/django/blob/b2eff16806057095c7dd3daa9402ad615e51627f/django/db/models/manager.py#L95-L102

I'm wondering if we're doing something wrong further upstream. I'm really curious about why this "binding" stage is required and if we should rather be deferring to the next mypy iteration at some point maybe 🤔

[flaeppe]

... is that Django is skipping some methods when copying/redefining queryset methods on managers. So if we simply say that the generated manager inherits from the queryset we would expose methods that doesn't actually exist on the manager at runtime. Methods can be explicitly marked as skipped or be skipped because they're already implemented by the manager ...

I'm pretty sure we're not honoring skipping methods currently:

django-stubs/mypy_django_plugin/transformers/managers.py

Lines 215 to 288 in 926661a

	# We collect and mark up all methods before django.db.models.query.QuerySet as class members
	for class_mro_info in derived_queryset_info.mro:
	if class_mro_info.fullname == fullnames.QUERYSET_CLASS_FULLNAME:
	break
	for name, sym in class_mro_info.names.items():
	if not isinstance(sym.node, (FuncDef, Decorator)):
	continue
	# Insert the queryset method name as a class member. Note that the type of
	# the method is set as Any. Figuring out the type is the job of the
	# 'resolve_manager_method' attribute hook, which comes later.
	#
	# class BaseManagerFromMyQuerySet(BaseManager):
	# queryset_method: Any = ...
	#
	helpers.add_new_sym_for_info(
	new_manager_info,
	name=name,
	sym_type=AnyType(TypeOfAny.special_form),
	)
	# we need to copy all methods in MRO before django.db.models.query.QuerySet
	# Gather names of all BaseManager methods
	manager_method_names = []
	for manager_mro_info in new_manager_info.mro:
	if manager_mro_info.fullname == fullnames.BASE_MANAGER_CLASS_FULLNAME:
	for name, sym in manager_mro_info.names.items():
	manager_method_names.append(name)
	# Copy/alter all methods in common between BaseManager/QuerySet over to the new manager if their return type is
	# the QuerySet's self-type. Alter the return type to be the custom queryset, parameterized by the manager's model
	# type variable.
	for class_mro_info in derived_queryset_info.mro:
	if class_mro_info.fullname != fullnames.QUERYSET_CLASS_FULLNAME:
	continue
	for name, sym in class_mro_info.names.items():
	if name not in manager_method_names:
	continue
	if isinstance(sym.node, FuncDef):
	func_node = sym.node
	elif isinstance(sym.node, Decorator):
	func_node = sym.node.func
	else:
	continue
	method_type = func_node.type
	if not isinstance(method_type, CallableType):
	if not semanal_api.final_iteration:
	semanal_api.defer()
	return None
	original_return_type = method_type.ret_type
	# Skip any method that doesn't return _QS
	original_return_type = get_proper_type(original_return_type)
	if isinstance(original_return_type, UnboundType):
	if original_return_type.name != "_QS":
	continue
	elif isinstance(original_return_type, TypeVarType):
	if original_return_type.name != "_QS":
	continue
	else:
	continue
	# Return the custom queryset parameterized by the manager's type vars
	return_type = Instance(derived_queryset_info, self_type.args)
	helpers.copy_method_to_another_class(
	class_def_context,
	self_type,
	new_method_name=name,
	method_node=func_node,
	return_type=return_type,
	original_module_name=class_mro_info.module_name,
	)

But I see your point with the difficulty of resolving that anyways, with the MRO.

---

Worth noting is also that the only copied methods through copy_method_to_another_class are manager methods:

django-stubs/mypy_django_plugin/transformers/managers.py

Lines 249 to 251 in 926661a

	for name, sym in class_mro_info.names.items():
	if name not in manager_method_names:
	continue

Methods coming from the queryset are resolved differently:

django-stubs/mypy_django_plugin/transformers/managers.py

Lines 215 to 233 in 926661a

	# We collect and mark up all methods before django.db.models.query.QuerySet as class members
	for class_mro_info in derived_queryset_info.mro:
	if class_mro_info.fullname == fullnames.QUERYSET_CLASS_FULLNAME:
	break
	for name, sym in class_mro_info.names.items():
	if not isinstance(sym.node, (FuncDef, Decorator)):
	continue
	# Insert the queryset method name as a class member. Note that the type of
	# the method is set as Any. Figuring out the type is the job of the
	# 'resolve_manager_method' attribute hook, which comes later.
	#
	# class BaseManagerFromMyQuerySet(BaseManager):
	# queryset_method: Any = ...
	#
	helpers.add_new_sym_for_info(
	new_manager_info,
	name=name,
	sym_type=AnyType(TypeOfAny.special_form),
	)

The queryset approach was implemented to resolve exactly these kind of "phantom errors", but for custom queryset methods, a while back through #820 (initial issue in #709)

[ljodal]

Oh, that's a very good point. I was planning on testing out the same plug-in callback logic for all queryset methods to see if that resolved #958 (which is a similar "something is wrong with the types after copying" issue), but never got around to that. That would probably be worth a try to see if it maybe resolves both issues

[ljodal]

Worth noting is also that the only copied methods through copy_method_to_another_class are manager methods:

django-stubs/mypy_django_plugin/transformers/managers.py

Lines 249 to 251 in 926661a

	for name, sym in class_mro_info.names.items():
	if name not in manager_method_names:
	continue

Methods coming from the queryset are resolved differently:

django-stubs/mypy_django_plugin/transformers/managers.py

Lines 215 to 233 in 926661a

	# We collect and mark up all methods before django.db.models.query.QuerySet as class members
	for class_mro_info in derived_queryset_info.mro:
	if class_mro_info.fullname == fullnames.QUERYSET_CLASS_FULLNAME:
	break
	for name, sym in class_mro_info.names.items():
	if not isinstance(sym.node, (FuncDef, Decorator)):
	continue
	# Insert the queryset method name as a class member. Note that the type of
	# the method is set as Any. Figuring out the type is the job of the
	# 'resolve_manager_method' attribute hook, which comes later.
	#
	# class BaseManagerFromMyQuerySet(BaseManager):
	# queryset_method: Any = ...
	#
	helpers.add_new_sym_for_info(
	new_manager_info,
	name=name,
	sym_type=AnyType(TypeOfAny.special_form),
	)

The queryset approach was implemented to resolve exactly these kind of "phantom errors", but for custom queryset methods, a while back through #820 (initial issue in #709)

I think you're slightly off here, methods from custom querysets are not copied, methods from the default QuerySet class are copied