Skip to content

Conversation

UnknownPlatypus
Copy link
Contributor

I have made things!

(Review per commit advised, the first one is a minor cleanup)

Implement the suggestion in #1375 (comment) providing automatic type-checking of Django models Meta class by artificially inserting TypedModelMeta in the mro.

The error message might be slightly confusing since the user did not add an explicit base class but I think is comprehensible enough and that the value outweigh the slightly weird message. For ex:

Incompatible types in assignment (expression has type "int", base class "TypedModelMeta" defined the type as "bool")

I've also tested this change on a rather large Django codebase (1M python LoC) adding errors that were all caught properly.

@UnknownPlatypus UnknownPlatypus force-pushed the auto-inherit-from-typedmodelmeta branch from b12ec1a to 8d06a49 Compare September 1, 2025 16:09
@UnknownPlatypus UnknownPlatypus force-pushed the auto-inherit-from-typedmodelmeta branch from 8d06a49 to 7610690 Compare September 1, 2025 17:34
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.

Please, see why test is failing

typed_model_meta_info = self.lookup_typeinfo(fullnames.TYPED_MODEL_META_FULLNAME)
if typed_model_meta_info and not meta_node.has_base(fullnames.TYPED_MODEL_META_FULLNAME):
# Insert TypedModelMeta just before `object` to leverage mypy's class-body semantic analysis.
meta_node.mro.insert(-1, typed_model_meta_info)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to add this base class to ClassDef as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think because we could endup with multiple TypedModelMeta in the mro for multiple inheritance cases.

@UnknownPlatypus
Copy link
Contributor Author

UnknownPlatypus commented Sep 2, 2025

Please, see why test is failing

I think I found the issue, this was because self._new_dependency("django_stubs_ext.db.models") was not called for dangling abstract django models because of how self.django_context.model_modules collect modules (It uses django logic to get concrete models, and then walk the mro to find abstract ones, hence missing dangling abstract models)

So depending on how mypy was resolving the module deps, we could sometime check a file before "django_stubs_ext.db.models" was checked, hence the flaky test. And yeah, abstract = 7 basically mean abstract = True.

I've updated the test to have concrete models at some point. Supporting dangling abstract model will add too much complexity, I don't think it's worth it

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.

Great stuff! Thanks again!

@sobolevn sobolevn merged commit cb14502 into typeddjango:master Sep 2, 2025
37 checks passed
@UnknownPlatypus UnknownPlatypus deleted the auto-inherit-from-typedmodelmeta branch September 2, 2025 15:10
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