Fix annotate leak across functions

[UnknownPlatypus]

I have made things!

This tries this approach #3324 (comment)

I think this is more correct.
cc @delfick , it fixes your bug (See testannotate_does_not_leak_across_functions) and also the one #3271 was trying to address (see custom_queryset_with_annotations_issue_1049)

A bunch of tests are failing, I expect it's mostly snapshots changing but I need to check more.
Opening to get initial feedback

Related issues

Fixes #3324

AI Policy

• I have read and agree to the AI Policy, removed any "Co-Authored-By" lines attributing coding agents, and manually reviewed the final result

[delfick]

@UnknownPlatypus yeah, this feels better than trying to make copies of a TypeInfo.

I will note that I've realised that I needed to remove the mypy_cache for this code to actually run, and now I've realised the solution I gave yesterday actually fails (AttributeError: type object 'TypeInfo' has no attribute '__slots__'). And I'm a little confused.

So with this PR specifically, compared to without these changes

before: 650
after: 601
Same: 101, Added: 500, Removed: 549

which is at least removing more than it adds :p

Seems the new errors is a lot of error: Variance of TypeVar "T_MyTypeVar" incompatible with variance in parent type [type-var] errors, which appear to be where we are creating a subclass of a queryset

so

class _MyConcreteQuerySet(MyAbstractQuerySet[MyConcreteModel]):
    pass

And also there seems to be a problem where it doesn't realise .values returns a Values QuerySet.

[UnknownPlatypus]

Should be solvable, I think it’s still the coreect direction. Not sure I'll have some time this week to finish this, feel free to poke around if you have time.

[delfick]

if you have time.

I'd certainly love to have more time! I might have time next week to have a deeper dive into this if you don't get there first :)

[delfick]

this week has been a lot for me, so I haven't had much time to spend on it. But I've had a quick look now and I think the problem is that we end up with types that think classes are generic when they are not.

For example

class MyQuerySet(models.QuerySet["MyModel"]):
    pass

Is not a generic class, but with this change we end up with mypy providing types like MyQuerySet[MyModel@AnnotateWith[...]], ...]

The queryset needs to be changed in the code to be like

class MyQuerySet[T_Model: MyModel = MyModel](models.QuerySet[T_Model]):
   pass

For that to be valid.

[federicobond]

Yes, this indeed looks like the correct type for any custom QuerySet moving forward:

class MyQuerySet[T_Model: MyModel = MyModel](models.QuerySet[T_Model]):
   pass

Any queryset instance must be able to reparametrize in the presence of .annotate calls.

[delfick]

yeah, we've found for quite a while doing that fixes problems where mypy doesn't see an annotate. But we have over 2000 django models and sometimes doing that also requires other refactoring to deal with new problems mypy sees so we haven't had the space to do that refactor pro-actively

[UnknownPlatypus]

I think the problem is that we end up with types that think classes are generic when they are not.

Actually, we have a plugin hook that injects the missing typevar (see reparametrize_any_*) to ensure any custom queryset is generic.

I dont think it ads the typevar default to the model however

[delfick]

to ensure any custom queryset is generic.

oh, right. Perhaps that's not actually something that's possible with mypy. Cause we should probs only do that on Instance objects and that ends up requiring the ability to make a copy of the type but with different bases.

I think the reality is django-stubs would require mypy to have a copy_modified on TypeInfo for this to be possible and the fix is instead to make reparametrize_queryset not pretend querysets are generic when they are not:

def reparametrize_queryset(instance: Instance, args: list[MypyType]) -> Instance:
    """Reparametrize a QuerySet instance with new type arguments.

    Note this only works when the queryset is already defined as generic
    """
    if instance.type.is_generic():
        return instance.copy_modified(args=args)

    return instance

also, I see now why deleting the mypy cache made a difference to what mypy could see (and I imagine is part of why TypeInfo doesn't already have a copy_modified)

Seems with that change I still have a number of models where I do have to go change the queryset to be generic now to avoid "cannot resolve keyword into field" errors, but that's fine. Seems also django-stubs doesn't realise .values returns a ValuesQuerySet, but I assume that's a different issue I should raise a github issue about.

[UnknownPlatypus]

oh, right. Perhaps that's not actually something that's possible with mypy. Cause we should probs only do that on Instance objects and that ends up requiring the ability to make a copy of the type but with different bases.

I don't think so, but I think we do it wrong somehow now.
The idea was to modify the type to mimic a manual I do have to go change the queryset to be generic, like a convenience to limit mass changes. We don't need a copy of the base type, we want to alter it to look like if it was written as generic in the first place so that we can then do regular Instance.copy_modified to fix the types around
[edit] I've just push something that seem to work and do that, let me know how it goes for you

the fix is instead to make reparametrize_queryset not pretend querysets are generic when they are not:

This would bring us to square one with the types being completely lost when using non-generic custom queryset. Maybe we have to live with that but that's a lot of churn. I got kinda stuck with #2776 because of that, turn out it revealed 300+ false positives in my work codebase because custom queryset started getting typechecked in most cases.

If the user can write this which works with annotates etc:

class MyQuerySet[T_Model: MyModel = MyModel](models.QuerySet[T_Model]):
   pass

We should be able to mimic that in the plugin by fixing the TypeInfo mypy processed when looking at

class MyQuerySet(models.QuerySet):
   pass

or

class MyQuerySet(models.QuerySet[MyModel]):
   pass

Seems also django-stubs doesn't realise .values returns a ValuesQuerySet, but I assume that's a different issue I should raise a github issue about.

I think we removed ValuesQuerySet in #2104

[delfick]

We don't need a copy of the base type, we want to alter it to look like if it was written as generic in the first place so that we can then do regular Instance.copy_modified to fix the types around

right, yeah, that would work. I imagine we'll wanna introduce a __version__ variable next to

django-stubs/mypy_django_plugin/main.py

Line 363 in 9048fdc

def plugin(version: str) -> type[NewSemanalDjangoPlugin]:

so that mypy knows to invalidate the cache (mypy looks for such a module level variable to determine the version of a plugin)

I've just push something that seem to work and do that, let me know how it goes for you

Nice! I'll try it out in a bit :)

turn out it revealed 300+ errors

I got several hundred errors too. Has kicked us into gear of actually fixing them :)

I think we removed ValuesQuerySet

correct, I'm being lazy with what I mean. It seems mypy is sad about dictionary access when it shouldn't be.

[delfick]

I imagine we'll wanna introduce a version variable next to

though, if mypy_django_plugin/main.py is changed betwen 6.0.3 and 6.0.4, then it'll already consider the plugin different and invalidate the cache

I'll try it out in a bit :)

I think what you've done here works (it's a little hard to tell cause I still do have 336 errors but seems it's mostly other errors (certainly not the errors that made me create that github issue in the first place!) and a lot of "is not indexable" errors (what I'm referring to above with a values queryset), which I imagine is indeed a separate issue that I'll end up making a new github issue for when I get to it)

Thanks!

[UnknownPlatypus]

Awesome, let me know if you still find issue related to this pr, otherwise I'll merge it during the day.

As for the cache I think it will be invalidated with the new version but Ill try to verify that

Looking forward for the other issues, real world exemple from large codebase are really valuable here

[UnknownPlatypus]

Can confirm it also fixes the same pollution error I had in my work codebase.
Awesome, merging now, thanks again for the report and help on getting to the bottom of this @delfick

[delfick]

Thanks!

[delfick]

@UnknownPlatypus , for papertrail, I finally posted something about my problem with querysets that should be seen as indexable, #602 (comment) (though my time at the moment is very constrained and so it's only a test case to reproduce the problem)