Skip to content

Fix type annotations for 3rd party models #2440

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.

Sign up for GitHub

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
Niicck wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix type annotations for 3rd party models #2440

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3e810f3
create handles_type_annotations_on_3rd_party_models test case
Niicck Nov 9, 2024
c200c4c
pass handles_type_annotations_on_3rd_party_models
Niicck Nov 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions mypy_django_plugin/lib/helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -526,6 +526,10 @@ def is_model_type(info: TypeInfo) -> bool:
return info.metaclass_type is not None and info.metaclass_type.type.has_base(fullnames.MODEL_METACLASS_FULLNAME)


def is_registered_model_type(info: TypeInfo, django_context: "DjangoContext") -> bool:
return info.fullname in {get_class_fullname(cls) for cls in django_context.all_registered_model_classes}


def get_model_from_expression(
expr: Expression,
*,
Expand Down
5 changes: 4 additions & 1 deletion mypy_django_plugin/transformers/fields.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,10 @@ def transform_into_proper_return_type(ctx: FunctionContext, django_context: Djan
assert isinstance(default_return_type, Instance)

outer_model_info = helpers.get_typechecker_api(ctx).scope.active_class()
if outer_model_info is None or not helpers.is_model_type(outer_model_info):
if outer_model_info is None or (
not helpers.is_model_type(outer_model_info)
and not helpers.is_registered_model_type(outer_model_info, django_context)
):
return ctx.default_return_type

assert isinstance(outer_model_info, TypeInfo)
Expand Down
3 changes: 3 additions & 0 deletions requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ Django==5.1.3; python_version >= '3.10'
-e ./ext
-e .[redis,compatible-mypy,oracle]

# 3rd Party Library for Testing
django-extensions==3.2.3

# Overrides:
mypy==1.13.0
pyright==1.1.388
15 changes: 15 additions & 0 deletions tests/typecheck/models/test_3rd_party_models.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
- case: handles_type_annotations_on_3rd_party_models
installed_apps:
- django_extensions
- myapp
main: |
from myapp.models import A
Copy link
Member

@sobolevn sobolevn Nov 10, 2024

Choose a reason for hiding this comment

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

please, add a reveal_type to A().name

files:
- path: myapp/__init__.py
- path: myapp/models.py
content: |
from django.db import models
from django_extensions.db.models import TimeStampedModel # type: ignore[import-untyped]
Copy link
Member

@flaeppe flaeppe Nov 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I actually disagree with the changes here. The main issue in #2341 is that django-extensions is a package that doesn't export any type hints.

The ignore comment I'm highlighting here is saying precisely that. What is experienced in #2341 is how mypy behaves when there are no type hints.

What is attempted to be repaired here is only the first line of issues, there could be an arbitrary amount of other problems to solve after this one, coming from it being an untyped package.

There are multiple other approaches to resolve this, that integrates better with the type checker. e.g.

  • Add and export type hints in django-extensions
  • Create a stubs package(.pyi) just like django-stubs for django-extrnsions
  • Add stubs (.pyi-files) for django-extensions to your project, locally
  • Perhaps django-stubs would consider including stubs in a similar fashion to typeshed, that includes/maintains a bunch of 3rd party stubs?

Regardless, I think merging the changes here is a step in the wrong direction as it's outside of django-stubs domain

Copy link

@rsiemens rsiemens Dec 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure I understand the "how mypy behaves when there are no type hints." bit.

Shouldn't --follow-imports=normal (the default) still allow for the top level class relationship to be constructed regardless of if the third part is typed or not?

I have models that inherit SealableModel, but when I inspect my model during transform_into_proper_return_type it seems like all the context is lost.

In [1]: outer_model_info.fullname
Out[1]: 'projects.models.Project'

In [2]: helpers.is_model_type(outer_model_info)
Out[2]: False

In [3]: outer_model_info.metaclass_type

In [4]: outer_model_info.is_metaclass()
Out[4]: True

edit: I realize I have ignore_missing_imports = true in my mypy.ini file. The fact that things worked before was somewhat happenstance thanks to the info.fullname in {get_class_fullname(cls) for cls in django_context.all_registered_model_classes} (for better or worse as you pointed out)


class A(TimeStampedModel):
name = models.CharField()
Loading