Skip to content

Use Model instead of Self in parms and add AltersData inheritance#3228

Open
emmanuel-ferdman wants to merge 3 commits intotypeddjango:masterfrom
emmanuel-ferdman:fix-self-contravariant
Open

Use Model instead of Self in parms and add AltersData inheritance#3228
emmanuel-ferdman wants to merge 3 commits intotypeddjango:masterfrom
emmanuel-ferdman:fix-self-contravariant

Conversation

@emmanuel-ferdman
Copy link
Contributor

PR Summary

refresh_from_db, arefresh_from_db, _do_update, _get_pk_val, and unique_error_message used Self in their parameter types, which is unsound in contravariant positions - overriding these methods in a subclass with the same Self pattern triggers reportIncompatibleMethodOverride in pyright/ty/pyrefly because QuerySet[Model] is not assignable to QuerySet[MyModel] (LSP violation). This PR changes these to use Model instead, which is the widest valid type and what the runtime actually accepts. Also this PR adds the missing AltersData to Model and QuerySet base classes to match the runtime MRO.

Fixes #2605.

…ritance

Signed-off-by: Emmanuel Ferdman <emmanuelferdman@gmail.com>
Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's please add a assert_type test for this, it should be possible to override Model.refresh_from_db method with the correct annotation.

…ritance

Signed-off-by: Emmanuel Ferdman <emmanuelferdman@gmail.com>
@emmanuel-ferdman
Copy link
Contributor Author

@sobolevn had it ready in my local tests :)

self,
using: str | None = None,
fields: Iterable[str] | None = None,
from_queryset: QuerySet[models.Model] | None = None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MyModel won't work here, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, QuerySet[MyModel] errors on both mypy and pyright since it narrows the parameter type. Should we add another test case for it?

Copy link
Member

@sobolevn sobolevn Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please. But, it seems kinda right to have it typed as QuerySet[MyModel]?

Because passing from_queryset=QuerySet[SomeOther] with no error does seem like a false negative to me.

Do you have any ideas on how we can make this right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could try self: _Self with QuerySet[_Self] - this ties the queryset to the actual model type. So user.refresh_from_db(from_queryset=article_qs) would correctly error. It's stricter than runtime since QuerySet[Model] gets rejected for typed subclass instances, but I think passing a wrong model's queryset is a bug waiting to happen anyway :)

…ritance

Signed-off-by: Emmanuel Ferdman <emmanuelferdman@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Self used in contravariant positions

2 participants