Skip to content

Commit

Permalink
Fix inheritance resolution of cached properties in slotted class
Browse files Browse the repository at this point in the history
This resolves the case where a sub-class of a slotted class defining
some cached properties has a custom __getattr__() method. In that case,
we need to build the custom __getattr__ implementation (see
in _make_cached_property_getattr()) using cached properties from all
classes in the MRO. In order to keep references of cached properties
defined the inheritance hierarchy, we store them in a new
__attrs_cached_properties__ attribute and finally build the
"cached_properties" value, passed to _make_cached_property_getattr(),
by combining current class' cached properties with that of all its
parents. Also, when building __attrs_cached_properties__, we now clear
current class' __dict__ (name 'cd'), thus saving an extra loop.

Fix #1288
  • Loading branch information
dlax committed Jun 3, 2024
1 parent 5618e6f commit c46ffe5
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 7 deletions.
1 change: 1 addition & 0 deletions changelog.d/1289.change.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix inheritance resolution of cached properties in slotted class when subclasses do not define any `@cached_property` themselves but do define a custom `__getattr__()` method.
26 changes: 19 additions & 7 deletions src/attr/_make.py
Original file line number Diff line number Diff line change
Expand Up @@ -917,11 +917,27 @@ def _create_slots_class(self):
names += ("__weakref__",)

if PY_3_8_PLUS:
# Store class cached properties for further use by subclasses
# (below) while clearing them out from __dict__ to avoid name
# clashing.
cd["__attrs_cached_properties__"] = {
name: cd.pop(name).func
for name in [
name
for name, cached_property in cd.items()
if isinstance(cached_property, functools.cached_property)
]
}
# Gather cached properties from parent classes.
cached_properties = {
name: cached_property.func
for name, cached_property in cd.items()
if isinstance(cached_property, functools.cached_property)
name: func
for base_cls in self._cls.__mro__[1:-1]
for name, func in base_cls.__dict__.get(
"__attrs_cached_properties__", {}
).items()
}
# Then from this class.
cached_properties.update(cd["__attrs_cached_properties__"])
else:
# `functools.cached_property` was introduced in 3.8.
# So can't be used before this.
Expand All @@ -934,10 +950,6 @@ def _create_slots_class(self):
# Add cached properties to names for slotting.
names += tuple(cached_properties.keys())

for name in cached_properties:
# Clear out function from class to avoid clashing.
del cd[name]

additional_closure_functions_to_update.extend(
cached_properties.values()
)
Expand Down
44 changes: 44 additions & 0 deletions tests/test_slots.py
Original file line number Diff line number Diff line change
Expand Up @@ -891,6 +891,50 @@ def f(self):
assert b.z == "z"


@pytest.mark.skipif(not PY_3_8_PLUS, reason="cached_property is 3.8+")
def test_slots_getattr_in_subclass_without_cached_property():
"""
Ensure that when a subclass of a slotted class with cached properties
defines a __getattr__ but has no cached property itself, parent's cached
properties are reachable.
Cover definition and usage of __attrs_cached_properties__ internal
attribute.
Regression test for issue https://github.com/python-attrs/attrs/issues/1288
"""

@attr.s(slots=True)
class A:
@functools.cached_property
def f(self):
return 0

@attr.s(slots=True)
class B(A):
def __getattr__(self, item):
return item

@attr.s(slots=True)
class C(B):
@functools.cached_property
def g(self):
return 1

b = B()
assert b.f == 0
assert b.z == "z"

c = C()
assert c.f == 0
assert c.g == 1
assert c.a == "a"

assert list(A.__attrs_cached_properties__) == ["f"]
assert not B.__attrs_cached_properties__
assert list(C.__attrs_cached_properties__) == ["g"]


@pytest.mark.skipif(not PY_3_8_PLUS, reason="cached_property is 3.8+")
def test_slots_getattr_in_subclass_gets_superclass_cached_property():
"""
Expand Down

0 comments on commit c46ffe5

Please sign in to comment.