Fix plugin

[sterliakov]

Bug fixes:

• This pull requests fixes a bug introduced in Fix manager types scope #991 that throws "Sequence is not defined". bind_or_analyze_type should not check node.type, returning it anyway (None too). It make sense, because if lookup succeeded, we have nothing to do to resolve the type and should defer instead.
• Fixed a crash when manager, defined from .from_queryset in class body, is used in lookup. It resulted in internal error before. The solution is to always replace this manager with Any in such case. Rejected alternative was using default manager (models.Manager) instead. It looks worse, because manager could override existing methods. IMO, we'd rather say "this was declared in unexpected place, so we give up and do not check calls to it" (at least because such behavior is documented in README). Surprisingly, this was not reported before.
• Fixed false-positive with staticmethod of queryset (this was not reported as issue)

Other changes:

• Changed handling of managers with missing type arguments. To avoid manual construction of new managers, now the following:

class MyManager(models.Manager): ...

is interpreted as

_T_co = typing.TypeVar('_T_co', covariant=True)
class MyManager(models.Manager[_T_co]): ...

Using explicit Any (class MyManager(models.Manager[typing.Any])) preserves Any and prevents reparametrization (including subclasses).

• Python code is reformatted to 88 chars line length. I don't really insist on this style, but it is really inconvenient to read 120 chars with 2-3 columns layout (and it is useful to see two files or code + test). 88 is black's default and looks better, I think. Stub files remain formatted to 120 chars.
• UserManager class is made covariant by type variable (so that model subclass (ChildUser) can safely override manager with UserManager[ChildUser])
• Separator in error message "Couldn't resolve manager method..." is changed to the hyphen, because the dot was extremely confusing, rendering as Couldn't resolve related manager for relation 'appointment' (from appointments.models.Appointment.appointments.Appointment.owner) (message from my project).
• Removed black from requirements.txt, because we don't use it outside of pre-commit, so it is not a real dependency.

[flaeppe]

Sounds like this includes quite a bit of changes discussed very recently in:

• error: Name "Sequence" is not defined #1022
• Resolve all queryset methods on managers as attributes #1028
• Alias argument to QuerySet.using() typed as _SpecialForm #958
• Add failing test for Name "Sequence" is not defined false positive #1017

To get a better view could, at least, the linting changes be broken out to a separate PR?

[sobolevn]

Python code is reformatted to 88 chars line length. I don't really insist on this style, but it is really inconvenient to read 120 chars with 2-3 columns layout (and it is useful to see two files or code + test). 88 is black's default and looks better, I think. Stub files remain formatted to 120 chars.

I agree, but, please, let's keep our history consistent and our diffs as minimal as possible.

I don't like this style, but I've learned that things above ^ are more important (I did it the hard way).
So, let's keep things as they are.

Removed black from requirements.txt, because we don't use it outside of pre-c

Sounds good, but let's do it in a separate PR.

[sterliakov]

Okay, I'll split this into separate PR's now, one problem per each.

Any ideas where does the difference with 3.8 and 3.10 originate? One failing test that wants more annotations, I don't quite get the reason. 3.10 (local) runs without failures.

[flaeppe]

I tried these changes out locally and got into trouble when chaining queryset/manager methods from Django's manager with custom queryset methods. What I tried on one of our projects was:

MyModel.objects.select_for_update().custom_queryset_method()

Resulted in these sort of errors:

error: "_QuerySet[MyModel, MyModel]" has no attribute "custom_queryset_method"  [attr-defined]

But I feel like this should already be tested in our suite here?

Worth noting is that the manager is created with this approach (not sure if this will be full a repro though):

from django.db import models
from django.db.models.manager import BaseManager

class MyQuerySet(models.QuerySet["MyModel"]):
    def custom_queryset_method(self) -> MyQuerySet:
        ...


MyManager = BaseManager.from_queryset(MyQuerySet)
class MyModel(models.Model):
    objects = MyManager()

[sterliakov]

@flaeppe This is not a repro, unfortunately. N: Revealed type is "project.app.models.MyQuerySet", works. Isn't it affected by incremental mode (reproduces without cache)? If not, could you try to extract the real reason?

[flaeppe]

@flaeppe This is not a repro, unfortunately. N: Revealed type is "project.app.models.MyQuerySet", works. Isn't it affected by incremental mode (reproduces without cache)? If not, could you try to extract the real reason?

Yeah, it's not cached and not incremental. Happens each run. But currently it seems that it's only .select_for_update that results a signature looking like: (haven't had time to check all manager methods yet though)

reveal_type(MyModel.objects.select_for_update)  # Revealed type is "def (nowait: builtins.bool =, skip_locked: builtins.bool =, of: typing.Sequence[builtins.str] =, no_key: builtins.bool =) -> django.db.models.query._QuerySet[MyModel, MyModel]"

[sterliakov]

One question: how?

note: Revealed type is "def (nowait: builtins.bool =, skip_locked: builtins.bool =, of: typing.Sequence[builtins.str] =, no_key: builtins.bool =) -> project.app.models.MyQuerySet[project.app.models.MyModel]"

[sterliakov]

Could you run pytest -k regression_manager_scope_foreign for the failing CI test?) Maybe you're lucky with your setup and will break that test too...

[flaeppe]

One question: how?

note: Revealed type is "def (nowait: builtins.bool =, skip_locked: builtins.bool =, of: typing.Sequence[builtins.str] =, no_key: builtins.bool =) -> project.app.models.MyQuerySet[project.app.models.MyModel]"

I didn't know just yet. But now I found a repro. Problematic part, similar to #1017 (comment), is AUTH_USER_MODEL:

-   case: from_queryset_select_for_update
    disable_cache: true
    main: |
        from myapp.models import MyModel
        reveal_type(MyModel.objects.select_for_update)  # N: Revealed type is "def (nowait: builtins.bool =, skip_locked: builtins.bool =, of: typing.Sequence[builtins.str] =, no_key: builtins.bool =) -> myapp.managers.ModelQuerySet[myapp.models.MyModel]"
    custom_settings: |
        AUTH_USER_MODEL = "users.User"
        INSTALLED_APPS = ("django.contrib.auth", "django.contrib.contenttypes", "users", "myapp")
    files:
        -   path: myapp/__init__.py
        -   path: myapp/models.py
            content: |
                from django.conf import settings
                from django.db import models
                from django.db.models.manager import BaseManager
                from .managers import ModelQuerySet

                NewManager = BaseManager.from_queryset(ModelQuerySet)
                class MyModel(models.Model):
                    created_by = models.ForeignKey(to=settings.AUTH_USER_MODEL, on_delete=models.PROTECT)
                    objects = NewManager()
        -   path: myapp/managers.py
            content: |
                from __future__ import annotations
                from typing import TYPE_CHECKING
                from django.db import models

                if TYPE_CHECKING:
                    from .models import MyModel

                class ModelQuerySet(models.QuerySet["MyModel"]):
                    def queryset_method(self) -> ModelQuerySet:
                        return self.all()
        -   path: users/__init__.py
        -   path: users/models.py
            content: |
                from django.contrib.auth.models import AbstractBaseUser
                from django.db import models
                class User(AbstractBaseUser):
                    email = models.EmailField(unique=True)
                    USERNAME_FIELD = "email"

This is a similar setup to what failed in the project I ran it on.

[sterliakov]

Well, I figured it out! bind_or_analyze_type was not implemented properly initially, we're doing wrong things. When analyzing sequence, we get SymbolTableNode from lookup. It is absolutely fine that it has node.type is None: we're not always interested in node.type. We need node.node, which is a Var (in "regular cases" like builtins.bool) or TypeInfo (or something else probably, need to check these). The latter should be used in absolutely another way: we need to check t.args as well and construct new instance. So something like this behaves as expected:

def bind_or_analyze_type(
    t: MypyType, api: SemanticAnalyzer, module_name: Optional[str] = None
) -> Optional[MypyType]:
    """Analyze a type. If an unbound type, try to look it up in the given module name.

    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:
            if isinstance(node.node, TypeInfo):
                return Instance(
                    node.node,
                    [bind_or_analyze_type(a, api, module_name) for a in t.args],
                )
            elif isinstance(node.node, Var):
                return node.node.type

    # If lookup failed or type was bound, analyze type. May be `None` too.
    return api.anal_type(t)

(and this is not related to AUTH_USER_MODEL, fortunately)

[sobolevn]

Looks ready to me. Do others agree? :)

[flaeppe]

I think some comments marked as resolved hasn't been changed yet?

e.g. #1030 (comment)

[sterliakov]

Not ready, sorry: thanks to this comment a critical bug was found.

class MyModel(models.Model):
    objects = BaseManager()
reveal_type(MyModel.objects)

This produces need type annotation for "objects" for some reason, but revealed type is fine. I spent a day trying to find out, but probably need some more time.

[ljodal]

Looks ready to me. Do others agree? :)

Other than what's said above I'm not aware of any issues caused by this, but as I've noted previously it would be great if this was split up into multiple PRs (one for each fix). That makes it much easier to review and also if there are any issues it's much easier to identify what caused them

[sterliakov]

That problem was related to metadata, I think it may be nice to get rid of manager metadata and rely on subclassing solely (instead of _get_current_manager_bases check if resolved type info inherits from BaseManager).

[flaeppe]

Is this hook called on every iteration? If so, then we're deferring on every iteration, right? A first reaction is that it feels unnecessary

[sterliakov]

Should be better now

[sobolevn]

There are merge conflicts now 😞
Can you please resolve them?

[sterliakov]

If we allow .from_queryset inside class body, the following dies forever:

-   case: test_queryset_in_model_class_body_subclass
    main: |
        from myapp.models import MyModel
        reveal_type(MyModel.objects)  # N: Revealed type is "myapp.models.BaseManagerFromBaseQuerySet"
        reveal_type(MyModel.objects.get())  # N: Revealed type is "myapp.models.MyModel"
    installed_apps:
        - myapp
    files:
        -   path: myapp/__init__.py
        -   path: myapp/models.py
            content: |
                from django.db import models

                class BaseManager(models.Manager["BaseModel"]):
                    pass

                class BaseQuerySet(models.QuerySet["BaseModel"]):
                    def custom(self) -> "BaseQuerySet":
                        pass

                class BaseModel(models.Model):
                    objects = BaseManager.from_queryset(BaseQuerySet)()

                class MyModel(BaseModel):
                    pass

Because we create model-parametrized manager for BaseModel in class and cannot subclass it later, so in MyModel manager remains attached to BaseModel and MyModel.objects.get() is BaseModel.

This conflict looks quite difficult. I'll try to resolve this, but now I don't see any solution - maybe this PR will be abandoned. Should I submit separate PR with just fix for "Sequence is not defined" to fix this popular issue and release?

[ljodal]

If we allow .from_queryset inside class body, the following dies forever:

-   case: test_queryset_in_model_class_body_subclass
    main: |
        from myapp.models import MyModel
        reveal_type(MyModel.objects)  # N: Revealed type is "myapp.models.BaseManagerFromBaseQuerySet"
        reveal_type(MyModel.objects.get())  # N: Revealed type is "myapp.models.MyModel"
    installed_apps:
        - myapp
    files:
        -   path: myapp/__init__.py
        -   path: myapp/models.py
            content: |
                from django.db import models

                class BaseManager(models.Manager["BaseModel"]):
                    pass

                class BaseQuerySet(models.QuerySet["BaseModel"]):
                    def custom(self) -> "BaseQuerySet":
                        pass

                class BaseModel(models.Model):
                    objects = BaseManager.from_queryset(BaseQuerySet)()

                class MyModel(BaseModel):
                    pass

Because we create model-parametrized manager for BaseModel in class and cannot subclass it later, so in MyModel manager remains attached to BaseModel and MyModel.objects.get() is BaseModel.

Hmm, I didn't really consider that case, but technically it shouldn't really a problem. The class we create has to be generic (just like it is at runtime) and the type of BaseModel.objects should be BaseManagerFromBaseQuerySet[BaseModel]. The plugin can then correct that in the subclass to set the type to BaseManagerFromBaseQuerySet[MyModel]. There's a bunch of other issues with generic querysets/managers though, so I think it's be better to fix that in a separate PR?

I think @flaeppe has an implementation of the latter (or something very closely related here): flaeppe#29

This conflict looks quite difficult. I'll try to resolve this, but now I don't see any solution - maybe this PR will be abandoned. Should I submit separate PR with just fix for "Sequence is not defined" to fix this popular issue and release?

That error is already fixed (or should be, I've not seen it since) in 84eff75, but there's at least a few other things from this PR that would be good to get merged. I don't have a full overview of exactly what's in here at the moment, but the reparametrization of manager classes is a really interesting approach and we can use it to get rid of the copying of methods (instead of trying to fix it)

On a general level my experience is that it's much easier to both review and keep alive smaller PRs, so if you're able to I'd recommend trying to split this up into separate PRs.

[sterliakov]

Unfortunately I'm 100% unable to split this into multiple chunks, because there was black change (reverted later) and trying to play with history will surely result in something broken/inconsistent.

There's a bunch of other issues with generic querysets/managers though, so I think it's be better to fix that in a separate PR?

Could you elaborate on that? After merging this is the only problem arising (I don't talk about inconsistent QuerySet parametrization now, it is a subject of separate PR and needs later discussion anyway).

I'll try to recover this information from generic types now, maybe it is easier than seemed initially for me.

[flaeppe]

Without having looked deeper. I'm fairly certain that AddManagers should be able to resolve the missing parametrization when subclassing. Or have I missed something?

It currently traverses parents mapping managers at least

[ljodal]

There's a bunch of other issues with generic querysets/managers though, so I think it's be better to fix that in a separate PR?

Could you elaborate on that? After merging this is the only problem arising (I don't talk about inconsistent QuerySet parametrization now, it is a subject of separate PR and needs later discussion anyway).

I'll try to recover this information from generic types now, maybe it is easier than seemed initially for me.

There's at least #960, #1049, and #1046. Not entirely sure where all of them stem from, but at least #960 is because of the way we resolve types of methods copied to managers. We also incorrectly parametrize managers/querysets that are already parametrized. For example in this case:

class MyQuerySet(QuerySet["MyModel"]):
    my_custom_method(self) -> "MyQuerySet": pass

When this is combined with a manager the return type is changed to MyQuerySet[MyModel] which isn't correct (though not causing any problems either it seems).

Basically the resolve_manager_method hook has to be expanded to be much smarter, but it's not really related to what you've been fixing here so I think it'd be better to take it in a separate PR. Much easier to to find (and fix) regressions with git bisect when the commits are small :)

[sterliakov]

I'm dumb, sorry. This failure is not related to declaration inside body. This is specific to my original implementation:

-   case: inheritance_with_parents_manager_parametrized_with_parent
    main: |
        from myapp.models import ParentOfMyModel, MyModel
        reveal_type(ParentOfMyModel.objects.first()) # N: Revealed type is "Union[myapp.models.ParentOfMyModel, None]"
        reveal_type(MyModel.objects.first()) # N: Revealed type is "Union[myapp.models.MyModel, None]"
    installed_apps:
        - myapp
    files:
        -   path: myapp/__init__.py
        -   path: myapp/models.py
            content: |
                from django.db import models

                class MyManager(models.Manager['ParentOfMyModel']):
                    pass

                class ParentOfMyModel(models.Model):
                    objects = MyManager()

                class MyModel(ParentOfMyModel):
                    pass

This is failing as well.

E   pytest_mypy_plugins.utils.TypecheckAssertionError: Invalid output: 
E   Actual:
E     ...
E     main:3: note: Revealed type is "Union[myapp.models.ParentOfMyModel, None]" (diff)
E   Expected:
E     ...
E     main:3: note: Revealed type is "Union[myapp.models.MyModel, None]" (diff)
E   Alignment of first line difference:
E     E: ... "Union[myapp.models.MyModel, None]"
E     A: ... "Union[myapp.models.ParentOfMyModel, None]"

My implementation never reparametrizes managers that already have model parameters. Idk how this was missing in the test suite. So I'm going to fix this, thanks for the new test! It should be not this hard, I hope.

[sterliakov]

After some investigation I think that the behavior I reported in previous comment is a correct one (and consistent with regular typing in python). Consider the following scenario:

from django.db.models import Manager, Model
from typing import TypeVar

_M = TypeVar('_M', bound=Parent)

class MyManagerFixed(Manager[Parent]):
    def get(self, **kwargs: Any) -> Parent:
        return Parent.objects.first()  # I wanna ALWAYS return a Parent, because this is intended only for use on Parent
class MyManagerDynamic(Manager[_M]):
    pass
    
class Parent(Model):
    fixed_objects = MyManagerFixed()
    dynamic_objects = MyManagerDynamic()
     
class Child(Parent):
    fixed_objects = MyManagerFixed()
    dynamic_objects = MyManagerDynamic()

Now this is 100% valid on Parent, but very suspicious on Child: mypy thinks that fixed_objects returns Parent instances despite being used on Child. This is valid (since Child <: Parent) and does not violate get definition on MyManagerFixed. If we allow reparametrization with child models, then it is equivalent to class MyManagerFixed[_M]. This violates at least "one and preferably only one", plus does not allow to express the concept of "manager for exactly this model", and IMO the current way is more obvious. Also this will create an invalid manager class: it'll be Manager[Child] with get: (**kwargs: Any) -> Parent, and mypy should complain about this. There's nothing wrong with explicitly declaring manager as parametrized with bound TypeVar.

This contradicts with the fact that on runtime managers are really model-parametrized, though. IMO the most cheap solution here is to require using TypeVar's as Manager parameter unless other behavior is required. Plus, if we decide to reparametrize, we need to verify that manager is used really on model subtype only, and this is really tricky (mind TypeVar's, aliases, NewType's, recursive things which are supported now, variadic generics, ...). And this will require copying all methods to new plugin-generated Manager subclass parametrized with TypeVar or Child model, because we should not change real objects created by user (I mean we cannot just take MyManagerFixed and redefine everything in it to make our plugin happy, but should create another instance instead).

So I ask for maintainer's opinion here: I can either resolve conflicts and prepare this to be merged again (if my arguments convinced you) or try to implement reparametrization here. There are some conflicts now, but the idea of this PR does not really interfere with these changes.