Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom managers broken with mypy 1.5.0 #1648

Closed
andersk opened this issue Aug 10, 2023 · 9 comments
Closed

Custom managers broken with mypy 1.5.0 #1648

andersk opened this issue Aug 10, 2023 · 9 comments
Labels
bug Something isn't working

Comments

@andersk
Copy link
Contributor

andersk commented Aug 10, 2023

Bug report

What's wrong

Mypy 1.5.0 was just released and breaks lots of django-stubs tests. Obviously that’s semi-expected since 1.5.0 is outside the django-stubs[compatible-mypy] bound of mypy==1.4.*. But upon investigating anyway, I think mypy is correct that there’s a django-stubs problem here. The failures began with

which fixes mypy to understand that metaclass attributes take precedence over attributes defined in regular classes, matching Python’s runtime behavior. Unfortunately, django-stubs was relying on the previous misunderstanding:

class ModelBase(type):
@property
def objects(cls: type[_Self]) -> BaseManager[_Self]: ... # type: ignore[misc]
@property
def _default_manager(cls: type[_Self]) -> BaseManager[_Self]: ... # type: ignore[misc]
@property
def _base_manager(cls: type[_Self]) -> BaseManager[_Self]: ... # type: ignore[misc]
class Model(metaclass=ModelBase):

class ContentType(models.Model):
id: int
app_label: models.CharField
model: models.CharField
objects: ContentTypeManager

mypy 1.4.1 thinks ContentType.objects has type ContentTypeManager, but mypy 1.5.0 thinks ContentType.objects has type BaseManager[ContentType] from the metaclass.

This affects real projects too, such as Zulip (after bumping its pinned version of mypy):

corporate/models.py:80: error: "BaseManager[ContentType]" has no attribute "get_for_model"  [attr-defined]
zerver/actions/invites.py:458: error: "BaseManager[ContentType]" has no attribute "get_for_model"  [attr-defined]
zerver/actions/invites.py:469: error: "BaseManager[ContentType]" has no attribute "get_for_model"  [attr-defined]
zerver/tests/test_users.py:769: error: "BaseManager[ContentType]" has no attribute "clear_cache"  [attr-defined]
zerver/tests/test_signup.py:929: error: "BaseManager[ContentType]" has no attribute "clear_cache"  [attr-defined]
corporate/views/webhook.py:70: error: "BaseManager[ContentType]" has no attribute "get_for_model"  [attr-defined]
corporate/views/webhook.py:85: error: "BaseManager[ContentType]" has no attribute "get_for_model"  [attr-defined]
corporate/views/webhook.py:100: error: "BaseManager[ContentType]" has no attribute "get_for_model"  [attr-defined]

How is that should be

We need to somehow convince mypy 1.5.0 that ContentType.objects has type ContentTypeManager.

System information

  • OS: Linux (NixOS 23.11 x86-64)
  • python version: 3.10.2
  • django version: 4.2.4
  • mypy version: 1.5.0
  • django-stubs version: 4.2.3
  • django-stubs-ext version: 4.2.2
@andersk andersk added the bug Something isn't working label Aug 10, 2023
andersk added a commit to andersk/django-stubs that referenced this issue Aug 10, 2023
mypy 1.5.0 was fixed to understand that metaclass attributes take
precedence over attributes in the regular class.  So we need to
declare `objects` in the regular class to allow it to be overridden in
subclasses.

Fixes typeddjango#1648.

Signed-off-by: Anders Kaseorg <[email protected]>
andersk added a commit to andersk/django-stubs that referenced this issue Aug 11, 2023
mypy 1.5.0 was fixed to understand that metaclass attributes take
precedence over attributes in the regular class.  So we need to
declare `objects` in the regular class to allow it to be overridden in
subclasses.

Fixes typeddjango#1648.

Signed-off-by: Anders Kaseorg <[email protected]>
andersk added a commit to andersk/django-stubs that referenced this issue Aug 11, 2023
mypy 1.5.0 was fixed to understand that metaclass attributes take
precedence over attributes in the regular class.  So we need to
declare `objects` in the regular class to allow it to be overridden in
subclasses.

Fixes typeddjango#1648.

Signed-off-by: Anders Kaseorg <[email protected]>
@adambirds
Copy link
Contributor

@andersk I'm getting this error which i think is caused by the same thing:

error: "BaseManager[User]" has no attribute "normalize_email"  [attr-defined]

so happy to help test any fix when its available as would like to switch to mypy 1.5.0 when possible.

@andersk
Copy link
Contributor Author

andersk commented Aug 11, 2023

#1649 seems to fix most of the failures.

@adambirds
Copy link
Contributor

@andersk just installed from that PR and can confirm it fixed my error. Had no other errors switching to 1.5.0. Thanks

@orsinium
Copy link

confirmed to, with the fix from #1649 there are no other errors switching to mypy 1.5.0.

andersk added a commit to andersk/django-stubs that referenced this issue Aug 11, 2023
mypy 1.5.0 was fixed to understand that metaclass attributes take
precedence over attributes in the regular class.  So we need to
declare `objects` in the regular class to allow it to be overridden in
subclasses.

Fixes typeddjango#1648.

Signed-off-by: Anders Kaseorg <[email protected]>
@johnthagen
Copy link

johnthagen commented Aug 14, 2023

Related symptom from

from django.contrib.auth.models import User

user = User.objects.create_user(username="user")

Generates a type checking error on Mypy 1.5.0:

main.py:3: error: "BaseManager[User]" has no attribute "create_user"  [attr-defined]

marteinn added a commit to Frojd/Wagtail-Pipit that referenced this issue Aug 15, 2023
intgr pushed a commit to intgr/django-stubs that referenced this issue Aug 17, 2023
mypy 1.5.0 was fixed to understand that metaclass attributes take
precedence over attributes in the regular class.  So we need to
declare `objects` in the regular class to allow it to be overridden in
subclasses.

Fixes typeddjango#1648.

Signed-off-by: Anders Kaseorg <[email protected]>
@tylerlaprade
Copy link

tylerlaprade commented Aug 23, 2023

I just started seeing this error from Pyright (only in CI). I assume it's due to their latest release fixing the same bug that mypy fixed a couple weeks ago. Is there something I can do with my django-stubs usage to avoid this error? For now I'm just pinning my Pyright version in the GitHub Action, but then I miss out on other bugfixes.

@sobolevn
Copy link
Member

@tylerlaprade use django-stubs[compatible-mypy]

@tylerlaprade
Copy link

@sobolevn, thanks for the response! However, my error is coming from Pyright, not from mypy, so it still persists even after I made that change.

@orsinium
Copy link

I don't think django-stubs is tested to work with pyright. I mean, it works with pyright in vscode for the purpose of autocomplete, but there are no tests to ensure that pyright works as a linter. Especially considering that django-stubs provides a mypy plugin to infer quite a lot of useful stuff but pyright does not support plugins and never going to.

I'd suggest using a more mature and reliable mypy for type checking on CI. If it's, for some reason, not an option for your project, take a look at django-types, a community fork specifically designed to be compatible with pyright.

andersk added a commit to andersk/django-stubs that referenced this issue Sep 2, 2023
mypy 1.5.0 was fixed to understand that metaclass attributes take
precedence over attributes in the regular class.  So we need to
declare `objects` in the regular class to allow it to be overridden in
subclasses.

Fixes typeddjango#1648.

Signed-off-by: Anders Kaseorg <[email protected]>
andersk added a commit to andersk/django-stubs that referenced this issue Sep 2, 2023
mypy 1.5.0 was fixed to understand that metaclass attributes take
precedence over attributes in the regular class.  So we need to
declare `objects` in the regular class to allow it to be overridden in
subclasses.

Fixes typeddjango#1648.

Signed-off-by: Anders Kaseorg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging a pull request may close this issue.

6 participants