Add failing test for Name "Sequence" is not defined false positive

[christianbundy]

Problem: There seems to be a bug in this code, but the current tests don't fail.

Solution: Add a new failing test.

Fixes: #709 (comment) #1022

[christianbundy]

Reproduced in CI! https://github.com/christianbundy/django-stubs/runs/7057281103?check_suite_focus=true

=================================== FAILURES ===================================
_____________________ from_queryset_custom_auth_user_model _____________________
/home/runner/work/django-stubs/django-stubs/tests/typecheck/managers/querysets/test_from_queryset.yml:390: 
E   pytest_mypy_plugins.utils.TypecheckAssertionError: Output is not expected: 
E   Actual:
E     users/models:106: error: Name "Sequence" is not defined (diff)
E   Expected:
E     (empty)
=========================== short test summary info ============================
FAILED tests/typecheck/managers/querysets/test_from_queryset.yml::from_queryset_custom_auth_user_model
================== 1 failed, 301 passed in 1040.45s (0:17:20) ==================

(I'm using my fork to run CI since I think I have to have each workflow approved in this repo.) The problem was the --mypy-ini-file=mypy.ini switcheroo that was added in the original PR.

[christianbundy]

I think this is coming from bind_or_analyze_type, but I'm not completely sure that I understand what's happening.

t # Sequence?[str?]
t.name # 'Sequence'
module_name # 'django.db.models.query'
node # <mypy.nodes.SymbolTableNode object at 0x10ffd1720>
node.type # None
analyzed = api.anal_type(t)
analyzed # Any

[christianbundy]

Crystal clear repro.

https://github.com/christianbundy/django-stubs/actions/runs/2562518850

[christianbundy]

This seems to be happening when helpers.copy_method_to_another_class is copying select_for_update -- maybe the type annotations aren't being copied correctly?

Looks like the problematic call is:

bound_arg_type = bind_or_analyze_type(arg_type, semanal_api, original_module_name)

[flaeppe]

Does the failing test (from_queryset_custom_auth_user_model) pass if #991 is reverted?

(I think the only change that is needed to revert is to remove the original_module_name argument to copy_method_to_another_class and when anal_type is invoked)

[flaeppe]

It's interesting this error hasn't been caught by the from_queryset_queryset_imported_from_other_module test? Can we know what causes that one to pass, but not the new one here?

django-stubs/tests/typecheck/managers/querysets/test_from_queryset.yml

Lines 25 to 72 in 1a29ad4

	- case: from_queryset_queryset_imported_from_other_module
	main: |
	from myapp.models import MyModel
	reveal_type(MyModel.objects) # N: Revealed type is "myapp.models.NewManager[myapp.models.MyModel]"
	reveal_type(MyModel.objects) # N: Revealed type is "myapp.models.NewManager[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]"
	reveal_type(MyModel.objects.queryset_method_3()) # N: Revealed type is "builtins.str"
	reveal_type(MyModel.objects.queryset_method_4([])) # N: Revealed type is "None"
	reveal_type(MyModel.objects.filter(id=1).queryset_method()) # N: Revealed type is "myapp.querysets.ModelQuerySet"
	reveal_type(MyModel.objects.filter(id=1)) # N: Revealed type is "myapp.querysets.ModelQuerySet[myapp.models.MyModel]"
	installed_apps:
	- myapp
	files:
	- path: myapp/__init__.py
	- path: myapp/querysets.py
	content: |
	from typing import TYPE_CHECKING, Iterable, Sequence
	from django.db import models
	if TYPE_CHECKING:
	from .models import MyModel
	class Custom:
	...
	class ModelQuerySet(models.QuerySet["MyModel"]):
	def queryset_method(self) -> "ModelQuerySet":
	return self.filter()
	def queryset_method_2(self) -> Iterable[Custom]:
	return []
	def queryset_method_3(self) -> str:
	return 'hello'
	def queryset_method_4(self, arg: Sequence) -> None:
	return None
	- 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)
	class MyModel(models.Model):
	objects = NewManager()

[sobolevn]

Maybe adding typing and typing_extensions as deps will solve this problem?

django-stubs/mypy_django_plugin/main.py

Line 123 in 1a29ad4

def get_additional_deps(self, file: MypyFile) -> List[Tuple[int, str, int]]:

[ljodal]

It's interesting this error hasn't been caught by the from_queryset_queryset_imported_from_other_module test? Can we know what causes that one to pass, but not the new one here?

django-stubs/tests/typecheck/managers/querysets/test_from_queryset.yml

Lines 25 to 72 in 1a29ad4

	- case: from_queryset_queryset_imported_from_other_module
	main: |
	from myapp.models import MyModel
	reveal_type(MyModel.objects) # N: Revealed type is "myapp.models.NewManager[myapp.models.MyModel]"
	reveal_type(MyModel.objects) # N: Revealed type is "myapp.models.NewManager[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]"
	reveal_type(MyModel.objects.queryset_method_3()) # N: Revealed type is "builtins.str"
	reveal_type(MyModel.objects.queryset_method_4([])) # N: Revealed type is "None"
	reveal_type(MyModel.objects.filter(id=1).queryset_method()) # N: Revealed type is "myapp.querysets.ModelQuerySet"
	reveal_type(MyModel.objects.filter(id=1)) # N: Revealed type is "myapp.querysets.ModelQuerySet[myapp.models.MyModel]"
	installed_apps:
	- myapp
	files:
	- path: myapp/__init__.py
	- path: myapp/querysets.py
	content: |
	from typing import TYPE_CHECKING, Iterable, Sequence
	from django.db import models
	if TYPE_CHECKING:
	from .models import MyModel
	class Custom:
	...
	class ModelQuerySet(models.QuerySet["MyModel"]):
	def queryset_method(self) -> "ModelQuerySet":
	return self.filter()
	def queryset_method_2(self) -> Iterable[Custom]:
	return []
	def queryset_method_3(self) -> str:
	return 'hello'
	def queryset_method_4(self, arg: Sequence) -> None:
	return None
	- 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)
	class MyModel(models.Model):
	objects = NewManager()

I think the difference is that in the new test the model is used as AUTH_USER_MODEL, that seemed to trigger the problem when I was trying to reproduce it

[christianbundy]

Does the failing test (from_queryset_custom_auth_user_model) pass if #991 is reverted?

I've tried reverting the changes like so:

diff --git a/mypy_django_plugin/lib/helpers.py b/mypy_django_plugin/lib/helpers.py
index 1ce73fd..147eb99 100644
--- a/mypy_django_plugin/lib/helpers.py
+++ b/mypy_django_plugin/lib/helpers.py
@@ -369,8 +369,9 @@ def bind_or_analyze_type(t: MypyType, api: SemanticAnalyzer, module_name: Option
     That should hopefully give a bound type."""
     if isinstance(t, UnboundType) and module_name is not None:
         node = api.lookup_fully_qualified_or_none(module_name + "." + t.name)
-        if node is not None and node.type is not None:
-            return node.type
+        if node is None:
+            return None
+        return node.type
 
     return api.anal_type(t)
 
diff --git a/mypy_django_plugin/transformers/models.py b/mypy_django_plugin/transformers/models.py
index 17a2272..45d96a4 100644
--- a/mypy_django_plugin/transformers/models.py
+++ b/mypy_django_plugin/transformers/models.py
@@ -217,7 +217,6 @@ class AddManagers(ModelClassInitializer):
                     self_type=custom_manager_type,
                     new_method_name=name,
                     method_node=sym.node,
-                    original_module_name=base_manager_info.module_name,
                 )
                 continue
 

This fixes the failing test, but causes a failure for regression_manager_scope_foreign:

E   pytest_mypy_plugins.utils.TypecheckAssertionError: Invalid output: 
E   Actual:
E     myapp/models:6: error: Name "Optional" is not defined (diff)
E     myapp/models:6: note: Did you forget to import it from "typing"? (Suggestion: "from typing import Optional") (diff)
E   Expected:
E     (empty)
FAILED tests/typecheck/managers/test_managers.yml::regression_manager_scope_foreign - 

Maybe adding typing and typing_extensions as deps will solve this problem?

I might've misunderstood, but I tried this and it didn't seem to fix the failing test.

diff --git a/mypy_django_plugin/main.py b/mypy_django_plugin/main.py
index 6d3550d..d854509 100644
--- a/mypy_django_plugin/main.py
+++ b/mypy_django_plugin/main.py
@@ -126,8 +126,8 @@ class NewSemanalDjangoPlugin(Plugin):
             return [self._new_dependency(self.django_context.django_settings_module)]
 
         # for values / values_list
-        if file.fullname == "django.db.models":
-            return [self._new_dependency("mypy_extensions"), self._new_dependency("typing")]
+        if file.fullname.startswith("django.db.models"):
+            return [self._new_dependency("mypy_extensions"), self._new_dependency("typing"), self._new_dependency('typing_extensions')]
 
         # for `get_user_model()`
         if self.django_context.settings:

[flaeppe]

I did a brief investigation and realised that cache matters. Adding disable_cache: true to the test case (from_queryset_custom_auth_user_model) will make the test fail on each run. When cache was enabled, it only failed on non-cached runs.

This makes me believe that the suggestion @sobolevn has in #1017 (comment) is on the right track to hunt down the cause, that the issue appears depending on which order modules are analyzed.

It seems that if django.db.models.query has already been analyzed, bind_or_analyze_type returns a type (via lookup_fully_qualified_or_none). While if it hasn't been analyzed, anal_type is called, emitting the error and returning Any.

---

Also, it seems that when copying from our 1st call site of copy_method_to_another_class we never want a fallback on anal_type:

django-stubs/mypy_django_plugin/transformers/managers.py

Lines 281 to 288 in 926661a

	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,
	)

While from our 2nd call site of copy_method_to_another_class we want to fallback on anal_type:

django-stubs/mypy_django_plugin/transformers/models.py

Lines 215 to 221 in 926661a

	helpers.copy_method_to_another_class(
	new_cls_def_context,
	self_type=custom_manager_type,
	new_method_name=name,
	method_node=sym.node,
	original_module_name=base_manager_info.module_name,
	)

At least given our test cases and coverage.

[flaeppe]

It also seems that, django.contrib.auth is always added as an additional dependency before django.db.models.query:

django-stubs/mypy_django_plugin/main.py

Lines 165 to 170 in 926661a

	return list(deps) + [
	# for QuerySet.annotate
	self._new_dependency("django_stubs_ext"),
	# For BaseManager.from_queryset
	self._new_dependency("django.db.models.query"),
	]

I'm gonna try and move django.db.models.query earlier and see what happens

[flaeppe]

Nevermind, I misread get_additional_deps function quite a lot. But I'm still thinking this could be the reason why the error only appears when involving AUTH_USER_MODEL..

[ljodal]

That this depends on cache/resolution order matches with my impression as well. I had to use --no-incremental in my test repo to get it to consistently reproduce. Otherwise it would sometimes not complain at all, indicating perhaps that the file where the queryset is defined was cached and thus the error never appeared when checking the models file

[ljodal]

I've been digging into this issue this morning and I've kinda landed on the conclusion that the underlaying issue here really is a missing generic on BaseManager: the type of the queryset.

All of the methods we copy to the new manager from the queryset are already defined on the manager (in the stubs), so the only reason we need to copy them is to fix the return type. That shouldn't be needed, as those methods should be correctly typed on the manager based on a generic queryset. I haven't figured out if it's actually possible to type this correctly yet. I'd expect we need something like class BaseManager(Generic[M, QS[M]): ..., but that doesn't appear to be accepted by mypy.

As an alternative I think we might be able to correct the return type of these methods using the get_method_signature_hook callback instead of copying the methods, but I haven't had time to investigate that approach yet.

Does this seem reasonable to you @flaeppe and @sobolevn?

Edit:

Actually this example works in mypy, it was pyright that was complaining:

from typing import Generic, TypeVar


class Model:
    pass


M = TypeVar("M", bound=Model)


class QuerySet(Generic[M]):
    def get(self) -> M:
        pass


QS = TypeVar("QS", bound=QuerySet)


class Manager(Generic[M, QS[M]]):
    def get_queryset(self) -> QS:
        pass

    def get(self) -> M:
        pass


m = Manager[Model, QuerySet[Model]]()

reveal_type(m.get_queryset().get())  # note: Revealed type is "something.Model"
reveal_type(m.get()) # note: Revealed type is "something.Model"

[sobolevn]

Yes, anything is better than directly coping methods 👍

[ljodal]

I gave the generics approach a quick try and immediately hit limitations, so that doesn't really appear to be supported by mypy. Guess it might be worth an issue in mypy, but this seems like a limitation of the typing system in Python, so I'm not sure if that's the right place for it.

So I think the get_method_signature_hook (or maybe get_method_hook) approach might be the one to use for a quicker fix. Let me know if someone has time to start looking into that. I have some other things I need to finish up at work, so I can't spend more time on this right now, but we still need this issue fixed before we can upgrade so it's on my priority list.

[flaeppe]

I think there's at least one, typing (or even django-stubs), issue with using get_method_hook. Which is that it won't be called when using for example:

reveal_type(MyModel.objects.select_for_update)

But it will be called when the method is invoked, e.g.

MyModel.objects.select_for_update()

[flaeppe]

I think there's at least one, typing (or even django-stubs), issue with using get_method_hook. Which is that it won't be called when using for example:

reveal_type(MyModel.objects.select_for_update)

But it will be called when the method is invoked, e.g.

MyModel.objects.select_for_update()

The same seems to apply for get_method_signature_hook, unfortunately..

[ljodal]

Hmm, yeah 😞 I guess an option is to go the same route as for custom queryset methods. Ie add the methods attributes with type Any and then resolve them using get_attribute_hook. I'd also say it's acceptable, although not ideal, that reveal_type(MyModel.objects.filter) doesn't work, as it has very limited value anyway when we can't spell out the accepted arguments and the only real value there is the return type

[ljodal]

Pretty sure I managed to fix this in #1028. I copied over the test setup from here, included my initial reproducer and changed the logic for resolving all queryset methods to be based on the attribute approach already used for custom queryset methods.