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

django-stubs '1.10.0' introduced a regression when used with Django 3.2, that has persisted in all subsequent releases and currently still exists in 'master' #1113

Open
javulticat opened this issue Aug 17, 2022 · 0 comments
Labels
bug Something isn't working

Comments

@javulticat
Copy link
Contributor

javulticat commented Aug 17, 2022

Bug report

What's wrong

It is indicated in the readme that all versions of django-stubs >= 1.9.0 support Django 3.2 (note: 1.12.0 is the latest django-stubs release at the time of writing). But, ever since #683 was merged and released as part of django-stubs 1.10.0, support for Django 3.2 breaks in at least one place. Specifically, I've noticed the stub for the .bulk_update() method on both the QuerySet and Manager no longer work for Django 3.2, because they were changed in 1.10.0 to return an int instead of None (as they did in version 1.9.0). However, this presents problems for Django 3.2 because, prior to Django 4.0, .bulk_update() did indeed return None. It wasn't until Django 4.0 that .bulk_update() started returning an int.

To quote from the Django 4.0 docs on .bulk_update():

This method efficiently updates the given fields on the provided model instances, generally with one query, and returns the number of objects updated: [emphasis mine]

>>> objs = [
...    Entry.objects.create(headline='Entry 1'),
...    Entry.objects.create(headline='Entry 2'),
... ]
>>> objs[0].headline = 'This is entry 1'
>>> objs[1].headline = 'This is entry 2'
>>> Entry.objects.bulk_update(objs, ['headline'])
2

Changed in Django 4.0:
The return value of the number of objects updated was added.

Now contrast to the Django 3.2 docs on .bulk_update():

This method efficiently updates the given fields on the provided model instances, generally with one query: [note the lack of any mention of any return value in the description, and the int "2" is not being returned by the method, as contrasted to the Django 4.0 docs]

>>> objs = [
...    Entry.objects.create(headline='Entry 1'),
...    Entry.objects.create(headline='Entry 2'),
... ]
>>> objs[0].headline = 'This is entry 1'
>>> objs[1].headline = 'This is entry 2'
>>> Entry.objects.bulk_update(objs, ['headline'])

Here is very simple code snippet that should demonstrate this behavior on Django 3.2 with django-stubs >= 1.10.0 installed:

from django.db import models

class MyQuerySet(models.QuerySet):
    def bulk_update(
        self,
        objs: Iterable[models.Model],
        fields: Iterable[str],
        batch_size: Optional[int] = None,
    ) -> None:
        super().bulk_update(objs, fields, batch_size)

This will raise the following error in mypy (again, using Django 3.2 and django-stubs >= 1.10.0):

error: Return type "None" of "bulk_update" incompatible with return type "int" in supertype "_QuerySet"

How should it be

mypy should not be raising an error here when a subclass of QuerySet/Manager is created that has an overriden .bulk_update() method annotated with None as the return type when Django 3.2 is being used, because the return type of .bulk_update() in Django 3.2 is indeed None and not an int.

I have a few ideas for ways this could be fixed, ordered by what I think the best option is (1) to the worst option (3):

  1. Changing the return value of .bulk_update() stub to be Optional[int], which should support both the Django 3.2 behavior (returns None) and the Django >= 4.0 behavior (returns int). See below code snippet for a tiny proof of concept to show how using Optional[int] as the return type in the base class allows subclasses to return either None or int, which results in no mypy errors. This change can/should be released as part of version 1.13.0 (assuming that version is still intended to support Django 3.2, which I would hope so since Django 3.2 is currently the only supported LTS version of Django and will be supported until at least 2024). I would also think this should include back-porting this fix to all versions of django-stubs >= 1.10.0 (ie. versions 1.10.2 (1.10.1 already exists), 1.11.1, 1.12.1) so that they support Django 3.2 as they purport to do in the readme.

    1. If django-stubs 1.13.0 is nearly ready to be released, and it is intended to still support Django 3.2, perhaps back-porting the change to previous releases could be considered non-essential. Ie. Just make the change and release it as part of 1.13.0 and encourage users to upgrade to that version if they run into this issue. However, we may want to back-port in any case to pin the Django dependency in setup.py for django-stubs versions >=1.10.0,<=1.12.0 to be >=4.0, in which case, we might as well include the fix for this issue in the back-port. (This overlaps with option 2 below.)

      1. Or, at the very least, update the readme to indicate that certain Django 3.2 annotations are broken for django-stubs versions 1.10.x, 1.11.0, 1.12.0 and hope that folks read it, but experience tells me that most folks won't. (This also overlaps with option 2 below.)
from typing import Optional

class Foo:
    def foo(self) -> Optional[int]:
        return 123

class Bar(Foo):
    def foo(self) -> None:
        return None

class Baz(Foo):
    def foo(self) -> int:
        return 456
  1. Updating the readme to indicate that django-stubs >= 1.10.0 only supports Django >= 4.0 (which leaves the only version of django-stubs supporting Django 3.2 to be 1.9.0, which isn't ideal, since many bugs that were present in 1.9.0 have been fixed since then), or, at least, as mentioned above, add a disclaimer to the readme to indicate that some annotations for Django 3.2 do not work with these versions. of django-stubs.

    1. This could be improved somewhat by also back-porting new versions (ie. releasing versions 1.10.2 (1.10.1 already exists), 1.11.1, 1.12.1) to change the requirement boundaries set in setup.py to indicate that Django >= 4.0 is need for these versions. Without doing that, dependency management tools (pipfile, poetry, etc.) will still happily allow versions 1.10.0, 1.11.0, and 1.12.0 to be installed on Django 3.2 projects, causing mypy to fail when this issue is encountered.
  2. Do nothing and hope users of Django 3.2 figure out that they need to either downgrade to django-stubs 1.9.0, or be forced to use # type: ignore whenever they run into this issue using a newer version. In my experience, the more # type: ignores a repository has, the more likely it is to accidentally deploy broken code that mypy could have caught. So, IMHO, I think this should be the last resort.

Since both options 1 and 2 are of the same, pretty low lift, I would recommend going with option 1 - though I am, of course, open to any argument for one of the other options - or any other options that I haven't thought of.

PR for merging option 1 into master is here: #1114

Also happy to back-port the changes to versions 1.10.1, 1.11.0, and 1.12.0 if this proposal is accepted.

System information

Re-created the issue on 2 systems, but it's pretty clear that this is just a direct incompatibility for the behavior of .bulk_update(), as Django 3.2 returns None, but the annotations found in django-stubs >= 1.10.0 indicate the method needs to return an int, which is only the behavior in Django >= 4.0.

System 1

  • OS: MacOS
  • python version: 3.9.13
  • django version: 3.2.15
  • mypy version: 0.910 (with 1.9.0), 0.942 (with 1.10.0 and 1.10.1), and 0.950 (with 1.11.0 and 1.12.0)
  • django-stubs version: 1.10.0, 1.10.1, 1.11.0, and 1.12.0 were all attempted (along with 1.9.0, which works fine since this breaking change for users of Django 3.2 wasn't introduced until 1.10.0)
  • django-stubs-ext version: 0.5.0

System 2

  • OS: Debian 10
  • python version: 3.9.5
  • django version: 3.2.15
  • mypy version: 0.910 (with 1.9.0) and 0.931 (with 1.10.0, 1.10.1, 1.11.0, and 1.12.0)
  • django-stubs version: 1.10.0, 1.10.1, 1.11.0, and 1.12.0 were all attempted (as above, 1.9.0 was tested and works fine)
  • django-stubs-ext version: 0.3.0 (with 1.9.0 and 1.10.0) and 0.4.0 (with all versions >= 1.9.0, <= 1.12.0)
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

No branches or pull requests

1 participant