Skip to content

Feat/builtin models types #2590

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

FallenDeity
Copy link

@FallenDeity FallenDeity commented Mar 30, 2025

I have made things!

This PR aims to add type hints to builtin model fields, i.e for example models in contrib, admin, auth etc.

Base generic fields are modified to use the default= from PEP-696 to allow the following behaviour for models

_ST_IntegerField = TypeVar("_ST_IntegerField", default=float | int | str | Combinable)
_GT_IntegerField = TypeVar("_GT_IntegerField", default=int)

class IntegerField(Field[_ST_IntegerField, _GT_IntegerField]):
    _pyi_private_set_type: float | int | str | Combinable
    _pyi_private_get_type: int
    _pyi_lookup_exact_type: str | int
class Redirect(models.Model):
    id: models.AutoField
    pk: models.AutoField
    site: models.ForeignKey[Site | Combinable, Site]
    site_id: int
    old_path: models.CharField
    new_path: models.CharField

It eliminates the need to add generic arguments explicitly at each step whenever defining models, and ensures all the models being used internally have type hints.

This PR is not complete yet as I have a few questions to ask as I am not entirely clear about the whole process here are a few points I had doubts on, and wanted some feedback before proceeding

  1. Is there any particular reason models for contrib/gis/db/backends are typed Any and not included in allowlist_todo.txt, since they seem to have types here, should I type out the model fields as such
    edit: models typed in line with source
class OracleGeometryColumns(models.Model):
    table_name: models.CharField
    column_name: models.CharField
    srid: models.IntegerField
    objects: ClassVar[Manager[Self]]
  1. I noticed the presence of most of the inbuilt model fields and methods in allowlist_todo.txt so how would that be handled do I write tests under the assert_type and typecheck/contrib folder for these models and move them to allowlist.txt manually if they are type hinted and tested?
    edit: Resolved created a category

  2. Final question, there is one case where the current model field typing system might need some explicit type hinting from the user in case of where fields become optional or with null=True, because with current system I don't think there is a way to infer types from the field parameters passed, Here is an example

text: models.CharField # get or return type is str, as expected
# But when params like blank=True, null=True are passed no way to autoinfer that and change _GT, _ST
# i.e without explicit typehints or generic params it would still show `str`, if we were to do `reveal_type`
# Redundant verbose example
text_nullable = models.CharField[Optional[Union[str, int, Combinable]], Optional[str]](max_length=100, null=True)
# A more generic user might have to do this
text_nullable = models.CharField[str | None, str | None](max_length=100, null=True)

This is fixable if we modify the django-mypy-plugin code but not sure if thats the best option, here is how one might go about it

def set_descriptor_types_for_field(
    ctx: FunctionContext, *, is_set_nullable: bool = False, is_get_nullable: bool = False
) -> Instance:
    default_return_type = cast(Instance, ctx.default_return_type)

    # Check for null_expr and primary key stuff
    ...

+  # We get expected nullable types here
    set_type, get_type = get_field_descriptor_types(
        default_return_type.type,
        is_set_nullable=is_set_nullable or is_nullable,
        is_get_nullable=is_get_nullable or is_nullable,
    )

    # reconcile set and get types with the base field class
    base_field_type = next(base for base in default_return_type.type.mro if base.fullname == fullnames.FIELD_FULLNAME)
    mapped_instance = map_instance_to_supertype(default_return_type, base_field_type)
+  # But mapped types give use the generic types we have in our fields without None
    mapped_set_type, mapped_get_type = tuple(get_proper_type(arg) for arg in mapped_instance.args)

    # bail if either mapped_set_type or mapped_get_type have type Never
    if not (isinstance(mapped_set_type, UninhabitedType) or isinstance(mapped_get_type, UninhabitedType)):
        # always replace set_type and get_type with (non-Any) mapped types
        set_type = helpers.convert_any_to_type(mapped_set_type, set_type)
        get_type = get_proper_type(helpers.convert_any_to_type(mapped_get_type, get_type))

-       # the get_type must be optional if the field is nullable
+      # Instead of nullable expression is set to True we make out mapped type optional
        if (is_get_nullable or is_nullable) and not (
            isinstance(get_type, NoneType) or helpers.is_optional(get_type) or isinstance(get_type, AnyType)
        ):
+            get_type = helpers.make_optional_type(get_type)
+           set_type = helpers.make_optional_type(set_type)
-            ctx.api.fail(
-               f"{default_return_type.type.name} is nullable but its generic get type parameter is not optional",
-                ctx.context,
-           )

    return helpers.reparametrize_instance(default_return_type, [set_type, get_type])

This was the only way I could think of going about it, and user needs to have the plugin installed.

Related issues

TODO

  • Add more tests
  • Work on the contrib/gis/backends models (most probably)
  • Plugin Update to handle null

@sobolevn sobolevn requested review from adamchainz and sobolevn March 30, 2025 08:57
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.

Thanks, this is not a full review.

@FallenDeity
Copy link
Author

FallenDeity commented Mar 30, 2025

Resolved most of the comments so far

Additionally wanted to ask do I go ahead and also add types for the contrib/db/gis/backend models, wrt to here?

Edit: Added types and tests

@FallenDeity
Copy link
Author

FallenDeity commented Apr 11, 2025

@sobolevn I think we should be all done, should we also update the django_mypy_plugin to handle null=True?

there is one case where the current model field typing system might need some explicit type hinting from the user in case of where fields become optional or with null=True, because with current system I don't think there is a way to infer types from the field parameters passed, Here is an example

text: models.CharField # get or return type is str, as expected
# But when params like null=True is passed no way to autoinfer that and change _GT, _ST
# i.e without explicit typehints or generic params it would still show `str`, if we were to do `reveal_type`
# Typehinting for null=True would require manual typehints like the following:
# Redundant verbose example
text_nullable = models.CharField[Optional[Union[str, int, Combinable]], Optional[str]](max_length=100, null=True)
# A more generic user might have to do this
text_nullable = models.CharField[str | None, str | None](max_length=100, null=True)

@sobolevn
Copy link
Member

null=True should already work 🤔

@sobolevn sobolevn requested review from intgr and flaeppe April 11, 2025 18:39
@FallenDeity
Copy link
Author

FallenDeity commented Apr 11, 2025

null=True should already work 🤔

Thats because of the extension too, here is how the extension handles nulls, first it checks if the expression is nullable then based on that it fetches set_type and get_type making them nullable

set_type, get_type = get_field_descriptor_types(
        default_return_type.type,
        is_set_nullable=is_set_nullable or is_nullable,
        is_get_nullable=is_get_nullable or is_nullable,
    )

after that it fetches mapped types which is the types if generic params are provided and replace the default types gotten above with the mapped types

# bail if either mapped_set_type or mapped_get_type have type Never
    if not (isinstance(mapped_set_type, UninhabitedType) or isinstance(mapped_get_type, UninhabitedType)):
        # always replace set_type and get_type with (non-Any) mapped types
        set_type = helpers.convert_any_to_type(mapped_set_type, set_type)
        get_type = get_proper_type(helpers.convert_any_to_type(mapped_get_type, get_type))

the reason it works currently is because mapped types are Never and just the default get_type and set_type are returned

from django.db import models
from datetime import date as Date
from typing_extensions import reveal_type

class MyUser(models.Model):
    date = models.DateField(null=True)
    my_date = models.DateField[Date, Date](null=True)

u = MyUser()
reveal_type(u.date) # unknown for pyright/pylance, date | None for mypy
reveal_type(u.my_date) # type date for both
mypy .\test.py
null_expr=<mypy.nodes.NameExpr object at 0x00000115725D7CE0>, is_nullable=True
set_type=Union[builtins.str, datetime.date, django.db.models.expressions.Combinable, None], get_type=Union[datetime.date, None]
mapped_set_type=Never, mapped_get_type=Never
null_expr=<mypy.nodes.NameExpr object at 0x00000115725D7F60>, is_nullable=True
set_type=Union[builtins.str, datetime.date, django.db.models.expressions.Combinable, None], get_type=Union[datetime.date, None]
mapped_set_type=datetime.date, mapped_get_type=datetime.date
set_type=datetime.date, get_type=datetime.date
test.py:7: error: DateField is nullable but its generic get type parameter is not optional  [misc]
test.py:11: note: Revealed type is "Union[datetime.date, None]"
test.py:12: note: Revealed type is "datetime.date"

As you can see if types are mapped the default expression types are overwritten which is whats happening since all our fields are now inherently mapped with generic type params

So to fix this I think we can just make the mapped types optional too if the field is nullable

@FallenDeity FallenDeity requested a review from sobolevn April 11, 2025 19:59
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.

2 participants