Skip to content

Add special support for @django.cached_property needed in django-stubs #18959

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 1 commit into
base: master
Choose a base branch
from

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Apr 24, 2025

Hi!

We, in django-stubs, have a lot of usages of @cached_property decorator that is a part of django: https://docs.djangoproject.com/en/5.2/ref/utils/#django.utils.functional.cached_property

All usages of it we have to add to subtest/allowlist.txt, which is not great. In typing we reuse @functools.cached_property to have all the benefits of its inference: https://github.com/typeddjango/django-stubs/blob/ee8e8b11c37866969ff0406be20591a067dfa983/django-stubs/utils/functional.pyi#L3-L4

But, stubtest is not happy with this move: because in runtime objects have django.utils.functional.cached_property type and we see the following error:


error: django.http.response.HttpResponse.text is inconsistent, cannot reconcile @property on stub with runtime object
Stub: in file /home/runner/work/django-stubs/django-stubs/django-stubs/http/response.pyi:106
def (self: django.http.response.HttpResponse) -> builtins.str
Runtime:
<django.utils.functional.cached_property object at 0x7f7ce7704920>

So, we add all @django.utils.functional.cached_property usages to our allowlist.txt. There are LOTS of entries there: https://github.com/typeddjango/django-stubs/blob/ee8e8b11c37866969ff0406be20591a067dfa983/scripts/stubtest/allowlist.txt#L158-L425

Moreover, we have to always tell about this problem to new contributors on review :(

That's why I propose to special case this as we do with other property-likes.
I've tested locally and it works perfectly. I don't want to complicate the CI with django installation and special tests. So, I added # pragma: no cover to indicate that it is not tested.

Comment on lines +1282 to 1287
if _is_django_cached_property(runtime):
yield from _verify_final_method(stub.func, runtime.func, MISSING)
return
if inspect.isdatadescriptor(runtime):
# It's enough like a property...
return
Copy link
Member

@AlexWaygood AlexWaygood Apr 24, 2025

Choose a reason for hiding this comment

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

this method is called _verify_readonly_property, but inspect.isdatadescriptor() only returns True if an object has a __set__ method (and descriptors with __set__ methods are analogous to read/write properties rather than read-only properties). Could we avoid adding special-casing for django here if we just checked whether runtime is an object with a __get__ method rather than checking if the object is a data descriptor?

Something like this? (Though you probably want to use getattr_static or something since the getattr() call here might raise TypeError or something crazy)

Suggested change
if _is_django_cached_property(runtime):
yield from _verify_final_method(stub.func, runtime.func, MISSING)
return
if inspect.isdatadescriptor(runtime):
# It's enough like a property...
return
if callable(getattr(type(runtime), "__get__", None)):
# It's enough like a property...
return

Copy link
Member Author

Choose a reason for hiding this comment

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

This way I can't verify that stub.func and runtime.func are the same :(
But, this extra check can be added on its own in a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants