gh-106292: restore checking __dict__ in cached_property.__get__ #106380
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* main: pythongh-106368: Increase Argument Clinic test coverage (python#106389) pythongh-106320: Fix _PyImport_GetModuleAttr() declaration (python#106386) pythongh-106368: Harden Argument Clinic parser tests (python#106384) pythongh-106320: Remove private _PyImport C API functions (python#106383) pythongh-86085: Remove _PyCodec_Forget() declaration (python#106377) pythongh-106320: Remove more private _PyUnicode C API functions (python#106382) pythongh-104050: Annotate more Argument Clinic DSLParser state methods (python#106376) pythongh-106368: Clean up Argument Clinic tests (python#106373)
corona10
reviewed
Jul 4, 2023
Misc/NEWS.d/next/Library/2023-07-03-15-09-44.gh-issue-106292.3npldV.rst
Outdated
Show resolved
Hide resolved
Misc/NEWS.d/next/Library/2023-07-03-15-09-44.gh-issue-106292.3npldV.rst
Outdated
Show resolved
Hide resolved
Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>
|
Thanks @carljm for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12. |
miss-islington
pushed a commit
to miss-islington/cpython
that referenced
this pull request
Jul 5, 2023
…pythonGH-106380) * pythongh-106292: restore checking __dict__ in cached_property.__get__ (cherry picked from commit 838406b) Co-authored-by: Carl Meyer <carl@oddbird.net> Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>
|
GH-106469 is a backport of this pull request to the 3.12 branch. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When locking was removed from
functools.cached_propertyin #101890, the__get__method also stopped pre-checking the instance dictionary for a cached value.This removed pre-check is not necessary for the correct behavior of
cached_propertyin and of itself: since it's a non-data descriptor (does not define__set__or__delete__), the instance dictionary takes precedence over__get__, so__get__should never be reached if a cached value is set in the instance dictionary.But it seems that some users (including at least one popular library; see issue for details) are subclassing
cached_propertyand adding a__set__method, turning it into a data descriptor, which takes precedence over the instance dictionary. Prior to 3.12, this still worked OK (did not recompute the value on every access) because of the additional check for cached value in__get__. But with 3.12 as it currently stands, this code would silently stop caching the value due to the added__set__method.Although it's nice and clever for
cached_propertyto be maximally efficient by taking advantage of being a non-data descriptor, I think in this case backwards-compatibility and avoiding a footgun is more important. Clearly some people are subclassingcached_propertyand adding__set__methods, and it's not realistic to expect every Python user to understand the subtleties of data vs non-data descriptors and how this could breakcached_property. The cost here is also minimal: just one dictionary lookup, that should only occur once per instance (not on repeated access of the cached property).