[Django] #30278: Using del on uncalled cached_property throws exception.
Django
-------------------------------------+-------------------------------------
Reporter: Matthew | Owner: nobody
Schinckel |
Type: Bug | Status: new
Component: Utilities | Version: master
Severity: Normal | Keywords: cached_property
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
It's explicitly documented that you may use `del` (or `delattr`) to force
a cached_property to recalculate the next time it's called.
However, if you try to `del` a cached_property that has never been
evaluated, then an AttributeError is raised.
There doesn't seem to be a way to detect if a cached_property has been
evaluated or not, as `hasattr(foo, 'bar')` results in the evaluation of
the cached_property.
It's entirely possible that this is something that is beyond the
capability of the way cached_property is implemented, but it still feels
wrong to me.
(Likewise, if you attempt to `del foo.bar` twice without evaluation in
between, you will raise an AttributeError).
--
Ticket URL: <https://code.djangoproject.com/ticket/30278>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Matthew Schinckel | Owner: nobody
Type: Bug | Status: new
Component: Utilities | Version: master
Keywords: cached_property | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Curtis Maloney):
The problem here is (a) fundamental to how Python works, and (b)
complicated by `cached_property` "cheating".
So, `cached_property` works by saving the value it calculates into the
instances `__dict__`.
This means next time that attribute is accessed, _Python_ will check
`__dict__` first, and not even try the property.
Calling `del` will remove the entry from the instances `__dict__`.
However, if it's not there, it will fall back to the class's property.
This causes the attribute error, because the property class does not have
a `__delete__` method.
If it did have a `__delete__` method, Python's lookup would differ, and it
would _not_ check `__dict__` before calling the property. (the official
documentation mentions this about `__get__`, but a simple test shows it
applies to `__delete__` equally)
https://docs.python.org/3/reference/datamodel.html#invoking-descriptors
So, we can't have _both_ - if we support del on uninvoked
`cached_property` we lose the utility of `cached_property` (or, at least,
the performance)
--
Ticket URL: <https://code.djangoproject.com/ticket/30278#comment:1>
Django
Reporter: Matthew Schinckel | Owner: nobody
Component: Utilities | Version: master
Keywords: cached_property | Triage Stage: Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Matthew Schinckel):
Gotcha.
(In practice, it's possible to catch an exception raised by code that is
calling `del foo.bar` when it's possible `foo.bar` has not been referenced
yet, so I guess that will have to do).
--
Ticket URL: <https://code.djangoproject.com/ticket/30278#comment:2>
Django
Reporter: Matthew Schinckel | Owner: nobody
Component: Utilities | Version: master
Severity: Normal | Resolution: invalid
Keywords: cached_property | Triage Stage: Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Matthew Schinckel):
* status: new => closed
* resolution: => invalid
--
Ticket URL: <https://code.djangoproject.com/ticket/30278#comment:3>
Django
Reporter: Matthew Schinckel | Owner: nobody
Type: Bug | Status: closed
Severity: Normal | Resolution: invalid
Keywords: cached_property | Triage Stage: Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
Great discussion. Thank you both!
Just before this disappears over the horizon, is it worth adjusting the
`del` example in the docs here do we think?
--
Ticket URL: <https://code.djangoproject.com/ticket/30278#comment:4>
Django
Reporter: Matthew Schinckel | Owner: nobody
Type: Bug | Status: closed
Severity: Normal | Resolution: invalid
Keywords: cached_property | Triage Stage: Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Matthew Schinckel):
Replying to [comment:4 Carlton Gibson]:
> Just before this disappears over the horizon, is it worth adjusting the
`del` example in the docs here do we think?
I did wonder that: maybe it is worth having a note in the docs.
--
Ticket URL: <https://code.djangoproject.com/ticket/30278#comment:5>
Django
Reporter: Matthew Schinckel | Owner: nobody
Type: Bug | Status: closed
Severity: Normal | Resolution: invalid
Keywords: cached_property | Triage Stage: Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"c3c2ec54f59428cdf0a35abce594fd2ada35c209" c3c2ec54]:
{{{
#!CommitTicketReference repository=""
revision="c3c2ec54f59428cdf0a35abce594fd2ada35c209"
Refs #30278 -- Doc'd behavior of del on an unaccessed cached_property.
Thanks to Curtis Maloney for the description of the problem.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30278#comment:6>
Django
Reporter: Matthew Schinckel | Owner: nobody
Type: Bug | Status: closed
Severity: Normal | Resolution: invalid
Keywords: cached_property | Triage Stage: Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"f14170406c8a1f97eacbc38830a7af62a17a31dd" f141704]:
{{{
#!CommitTicketReference repository=""
revision="f14170406c8a1f97eacbc38830a7af62a17a31dd"
[2.2.x] Refs #30278 -- Doc'd behavior of del on an unaccessed
cached_property.
Thanks to Curtis Maloney for the description of the problem.
Backport of c3c2ec54f59428cdf0a35abce594fd2ada35c209 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30278#comment:7>
Django
Reporter: Matthew Schinckel | Owner: nobody
Type: Bug | Status: closed
Severity: Normal | Resolution: invalid
Keywords: cached_property | Triage Stage: Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"b9455b010e41d1c6e68faa11115212d50de3c231" b9455b01]:
{{{
#!CommitTicketReference repository=""
revision="b9455b010e41d1c6e68faa11115212d50de3c231"
Refs #30278 -- Fixed link in cached_property docs.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30278#comment:8>
Django
Reporter: Matthew Schinckel | Owner: nobody
Type: Bug | Status: closed
Severity: Normal | Resolution: invalid
Keywords: cached_property | Triage Stage: Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"fc708f32f50b2c5ef35ffc0fccae362ded5f93f1" fc708f3]:
{{{
#!CommitTicketReference repository=""
revision="fc708f32f50b2c5ef35ffc0fccae362ded5f93f1"
[2.2.x] Refs #30278 -- Fixed link in cached_property docs.
Backport of b9455b010e41d1c6e68faa11115212d50de3c231 from master.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30278#comment:9>