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

Use django-stubs with type checkers other than mypy #579

Open
kylebebak opened this issue Mar 20, 2021 · 14 comments
Open

Use django-stubs with type checkers other than mypy #579

kylebebak opened this issue Mar 20, 2021 · 14 comments
Labels
enhancement New feature or request meta Meta-issues and discussion pyright Related to pyright type checker

Comments

@kylebebak
Copy link

kylebebak commented Mar 20, 2021

Hey all,

I know the README says it doesn't make sense to use django-stubs without mypy, because mypy uses a plugin for cases where type hints alone don't work. Sorry if this question is a waste of time, and feel free to close this issue.

That said, this library ships with some very thorough type stubs for just about everything in Django. Is it possible to get a subset of the benefits from these stubs using a different type checker, like pyright?

I tried using the stubs from the latest commit, without the mypy plugin, with pyright, but have found that pyright can't infer the types of model fields, which would be very useful. Nor can it infer types returned by Manager methods.

I'm sure pyright is sourcing the stubs just fine; I can manually edit the stubs for something like timezone.now(), and pyright picks it up immediately.

@sobolevn @mkurnikov

Is it expected that most things having to do with the ORM don't work without running the mypy plugin? Is there manual monkey-patching that can be done, without running the mypy plugin, to help type checkers infer the correct types from model fields and Manager methods?

If this were possible it would be really awesome, and would help more users of type hints get lots of value out of django-stubs =)

I'm using Django 3.1.6, and stubs from the latest commit.

Examples

Here are some images of pyright doing type inference in my text editor:

Screen Shot 2021-03-20 at 2 01 14 PM

Screen Shot 2021-03-20 at 2 01 26 PM

I instantiate a prod from a Product models, which has a name field that's a CharField.

pyright knows prod is a Product, but it thinks prod.name has a type of Unknown. It thinks Product.name has a type of CharField[Unknown, Unknown]

Screen Shot 2021-03-20 at 2 35 42 PM

@mkurnikov
Copy link
Member

Does pyright support descriptors? It's what drives model field support.

@kylebebak
Copy link
Author

kylebebak commented Mar 22, 2021

Hey @mkurnikov ,

pyright supports descriptors, and features from just about all the other PEPs related to type annotations!

There's a fork of this library maintained by @sbdchd, which makes some changes so the stubs work with pyright, and with mypy without the plugin. Getting the types to work for models requires some manual type annotations, but it's not a lot of work.

Here's an example:

from __future__ import annotations

# ...

class Thing(models.Model):

    objects: Manager[Thing]
    related_things: RelatedManager[RelatedThing]
    other_thing: models.ForeignKey[OtherThing]
    other_thing_id: UUID

I added types to all the models in a fairly large code base in less than a day, and the code base is now a lot easier to work with and navigate.

The maintainer of the fork told me there was a decent amount of work to get the annotations to work without the plugin. I wonder whether there's a way for this repo to ship types that work with and without the plugin, so users of other type checkers can use these annotations. They're very powerful.

@syastrov
Copy link
Contributor

syastrov commented Apr 1, 2021

I don't see any reason why the django-stubs couldn't also use the approach by the fork.

For example, of using overloads with Literal types to handle nullable fields:
sbdchd@64a5b3b#diff-876a5d116ef70c86039fbb8498623871324fa93fc1ae51c7c09ba2a0f7612e99R187

I doubt it would require a lot of changes to the plugin. I think the main reason this wasn't done earlier was because using overloads on __init__ with narrowing of the self type wasn't supported by any type-checkers at the time the stubs were written.

I am not sure about some of the other changes made by in the fork, but they are also likely possible to integrate into django-stubs.

It would be nice if the maintainers of the 2 projects could talk together about an approach to getting both mypy (with or without the plugin) + pyright to work on the same set of stubs, rather than having 2 separate versions.

@sobolevn
Copy link
Member

sobolevn commented Apr 1, 2021

Yes, making django-stubs available for other type-checkers and minimizing plugin part is 100% a goal of this project.

@bellini666
Copy link

Hey guys!

What is the progress of this issue? Like other have said here, the work that @sbdchd has done on https://github.com/sbdchd/django-types is amazing, and the way dealt with some overloads is pretty clever. I can't see why django-stubs couldn't do the same (probably merge his changes?), and only do mypy magic where it needs to be done.

Basically, based on my experience using his fork and trying to type everything in all projects that I have, here are the problems that can't be solved by the typing/stubs lib itself, and some thoughts regarding each:

  1. No automatic resolution to Model.objects:

Before this would require manual typing or mypy magic, but this can be solved by creating this inside ModelBase (Model's metaclass):

_M = TypeVar("_M", bound=Any)
class ModelBase(type):
    @property
    def objects(cls: Type[_M]) -> BaseManager[_M]: ...

So, something like this can be incorporated without having to rely on mypy magic.

  1. No default id field in the model:

This one requires mypy magic. Without it, anyone using something else needs to type the id manually (if it is used)

  1. ForeignKey ids:

The same as the previous one, it requires mypy magic. Without it, anyone using something else needs to type the id manually: E.g.

class User(models.Model):
    team_id: Optional[int]
    team = models.ForeignKey(
        Team,
        null=True,
        on_delete=models.SET_NULL,
    )
    role_id: int
    role = models.ForeignKey["Role"](
        "Role",
        null=False,
        on_delete=models.SET_NULL,
        related_name="users",
    )
  1. Related fields:

Same as the previous. This one can be workarounded by:

class Team(models.Model):
    if TYPE_CHECKING:
        user_set = RelatedManager["User"]()

class Role(models.Model):
    if TYPE_CHECKING:
        users = RelatedManager["User"]()
  1. Validate values in Model.__init__ and Model.objecrs.create:

This one doesn't have really a way to be fixed, but, maybe this can be used to help we get around it: https://github.com/microsoft/pyright/blob/main/specs/dataclass_transforms.md

  1. Validate lookups in Manager.filter.

This one really needs mypy magic. Can't think of any kind of workaround for now.

============

So, other than the 6 cases above (or anything more that I forgot to mention), everything else can be solved by the current typing specifications without the need of the mypy plugin.

Also, number 1 can probably be fixed too like I mentioned and maybe 5 depending on how dataclass_transforms works. Everything else is something that the mypy plugin will still be needed.

@kylebebak
Copy link
Author

@sobolevn @mkurnikov @syastrov

Any reason we can't do what @bellini666 is suggesting and merge most of the changes that have been made in https://github.com/sbdchd/django-types into this repo?

Making these type hints usable with type checkers other than mypy, e.g. pyright, would make this excellent library useful to a lot more people.

@sobolevn
Copy link
Member

I would love to merge it 🙂

If anyone can send a PR, this would be amazing.

@syastrov
Copy link
Contributor

syastrov commented Apr 16, 2022

@bellini666 Thanks for putting together this list. It's interesting to see the areas in Django which are not currently covered by Python's static type system listed in one place.

  1. ForeignKey ids:

The same as the previous one, it requires mypy magic. Without it, anyone using something else needs to type the id manually: E.g.

class User(models.Model):
    team_id: Optional[int]
    team = models.ForeignKey(
        Team,
        null=True,
        on_delete=models.SET_NULL,
    )
    role_id: int
    role = models.ForeignKey["Role"](
        "Role",
        null=False,
        on_delete=models.SET_NULL,
        related_name="users",
    )

I wonder if Django could be taught that you can either pass int or a Role instance to user.role here (and the same logic could arguably apply to Role.__init__, which could compose well with the dataclass transforms PEP if that were to support Django's use case).
Also, when accessing user.role.id, instead of fetching the entire Role object when it wasn't previously fetched (e.g. by select_related), a "lazy" Role object could be constructed which only has the id field populated -- to avoid an extra query to the database.
This would not help for existing code, but for new code, it would help avoid this manual work in getting static typing to work. The old way could be deprecated if people agree that this is a better way forward.

That said, I'm not sure how amenable the Django project is to these kinds of changes.

An alternative would be to propose some sort of PEP that allows a descriptor defined on a class to add arbitrary extra attributes to a class, whose name is based on its name and whose type is somehow related to its own type. That second part could get really nasty though: you'd have to get it to extract the type of the primary key of the referenced model. This entirely seems too specific to Django, so I don't see it getting much acceptance.

  1. Related fields:

Same as the previous. This one can be workarounded by:

class Team(models.Model):
    if TYPE_CHECKING:
        user_set = RelatedManager["User"]()

class Role(models.Model):
    if TYPE_CHECKING:
        users = RelatedManager["User"]()

It's a bit nasty that it requires an if TYPE_CHECKING, but arguably, it is nice to be explicit here. I often find myself forgetting that Django autogenerates the related managers when related_name is not specified, and people end up exposing the "ugly" default name (e.g. team.user_set) rather than a custom one (e.g. team.members).

Maybe Django could support something like this in the future (not sure if it's possible)?

class Team(models.Model):
  members = RelatedManager(User.team)  # This registers the related_name for the User.team foreign key

class User(models.Model):
  # related_name is not necessary here.
  # If specified, it should give a runtime error, since there is already a related_name registered above.
  team = ForeignKey(Team)
  1. Validate values in Model.__init__ and Model.objecrs.create:

This one doesn't have really a way to be fixed, but, maybe this can be used to help we get around it: https://github.com/microsoft/pyright/blob/main/specs/dataclass_transforms.md

I am also keeping an eye on that PEP. Unfortunately, it seems like it would only apply to Model.__init__.

  1. Validate lookups in Manager.filter.

This one really needs mypy magic. Can't think of any kind of workaround for now.

The mypy magic is fun for supporting the current state of things, but maybe Django (or a third-party package) could offer an API similar to SQLAlchemy where a reference to the field's definition is explicitly mentioned -- something like Model.objects.filter(Model.int_field > 3) (e.g. field descriptors such as Model.int_field, would have dunder methods such as __gt__ implemented on them as classmethods such that when called would produce the corresponding Q object which could be passed to filter and friend). It could also be spelled as Model.objects.filter(Model.int_field.gt(3))), which is less magical and could correspond more exactly to the names of the "lookup expressions".

I am doubtful that Python's static type system would ever come to support the current Django-way of lookups expressions (beyond having to write a million overloads), so this is why I think proposing an alternative spelling would be nice.
I am not sure if Django itself would be receptive to this, but it could at least be implemented in a third-party package through monkey-patching for those who are interested in static typing of conditions.

  1. Manager.from_queryset

To be typed properly without boilerplate and without mypy magic, it would probably require intersection types to be implemented in Python's type system (?).


There are probably other minor cases, but I think you've identified the important ones.

I see several possible ways of meeting these problems without a plugin which could be taken on a case-by-case basis:

  1. Propose PEPs which support the currently unsupported features to Python's static type system where they may be applicable to projects beyond just Django.
  2. Propose changes to Django that allow supporting better static typing and get old ways deprecated.
  3. Punt on typing it, since it doesn't provide enough value
  4. To avoid using a plugin at all, try and make a tool which watches for changes to the codebase and parses/analyzes the code and adds the additional type annotations/code to the code that you would otherwise have to add manually. An improvement on this could be to add the annotations to a separate stub file that could be "overlayed" on top of the existing code -- currently, Python typing does not support "partial" stub files afaik, so you'd have to include stubs for the existing code, with the additions merged in. Challenges: What language to write it in? How complicated will the parser/analyzer be -- it may have to depend on pyright or mypy in order to do it statically in a reliable way -- or you'd have to run Django (as the django-stubs mypy plugin does) to extract the necessary metadata?

I think it could be really interesting to try out (4), since it essentially would allow supporting a large subset of the magic, even in other type-checkers than mypy.

@craiglabenz
Copy link

I have a simple question - how are you all importing RelatedManager such that the following code works?

if TYPE_CHECKING:
    user_set = RelatedManager["User"]()

@sbdchd
Copy link

sbdchd commented Sep 18, 2022

@craiglabenz you can either monkey patch RelatedManager so that you can pass the ["User"] argument at runtime, or import RelatedManager behind a if TYPE_CHECKING statement like:

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    # This doesn't really exists on django so it always need to be imported this way
    from django.db.models.manager import RelatedManager
    from user.models import User


class Team(models.Model):
    if TYPE_CHECKING:
        user_set = RelatedManager["User"]()

or with monkey patching to avoid the if TYPE_CHECKING:

# in settings.py
from django.db.models import ForeignKey
from django.db.models.manager import BaseManager
from django.db.models.query import QuerySet

# NOTE: there are probably other items you'll need to monkey patch depending on
# your version.
for cls in [QuerySet, BaseManager, ForeignKey]:
    cls.__class_getitem__ = classmethod(lambda cls, *args, **kwargs: cls)  # type: ignore [attr-defined]

@intgr
Copy link
Collaborator

intgr commented Oct 31, 2023

Just to update this, django-stubs version 4.2.6 no longer has a dependency on mypy and we're trying to facilitate small pull requests to improve experience with other type checkers.

@pirate
Copy link
Contributor

pirate commented Aug 20, 2024

Does anyone have an example of a working pyproject.toml that sets up [tool.pyright] to work with django-stubs?

(I'm trying to get pylance/pyright working with django-stubs in VSCode)

@lucaspar
Copy link

@pirate I still had to ignore reportAttributeAccessIssue, but:

[tool.pyright]
    # https://github.com/microsoft/pyright/blob/main/docs/configuration.md
    exclude = [
        # ...
    ]
    
    # Pylint rule list:
    # https://pylint.readthedocs.io/en/stable/user_guide/checkers/features.html
    lint.ignore = [
        # "E501", # line too long
        "R0903", # too few public methods
    ]
    
    pythonPlatform = "Linux"

    # "none", "warning", "information", or "error"
    reportMissingTypeArgument = "information"
    reportPrivateUsage        = "information"
    typeCheckingMode          = "standard"    # "off", "basic", "standard", "strict"

    # Reports:
    #   https://github.com/microsoft/pyright/blob/main/docs/configuration.md#type-check-diagnostics-settings
    # place ignored rules here
    reportAttributeAccessIssue = false # too many false positives with Django models
    

@ldeluigi
Copy link

Apparently, the large amount of errors that appear in the CI pyright step is blocking its adoption in Pylance (part of the VS Code Python extension, very popular).

Source: microsoft/pylance-release#6029 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request meta Meta-issues and discussion pyright Related to pyright type checker
Development

No branches or pull requests