From 1be48d0211bbc93cc16e59b116b858a01a4bac4d Mon Sep 17 00:00:00 2001 From: Denis Laxalde Date: Mon, 3 Jun 2024 13:51:47 +0200 Subject: [PATCH] Define __getattribute__() instead of __getattr__() on slotted classes with cached properties The __getattribute__() is documented as the "way to to actually get total control over attribute access", which is really what we want. Just changing that preserves the current behaviour, according to the test suite, but also makes sub-classing work better, e.g. when the subclass is not an attr-class and also defines a custom __getattr__() as evidenced in added test. Fix https://github.com/python-attrs/attrs/issues/1288 --- changelog.d/1291.change.md | 1 + src/attr/_make.py | 63 +++++++++++++++++++++----------------- tests/test_slots.py | 57 ++++++++++++++++++++++++++++++++++ 3 files changed, 93 insertions(+), 28 deletions(-) create mode 100644 changelog.d/1291.change.md diff --git a/changelog.d/1291.change.md b/changelog.d/1291.change.md new file mode 100644 index 000000000..a1a1e6857 --- /dev/null +++ b/changelog.d/1291.change.md @@ -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. diff --git a/src/attr/_make.py b/src/attr/_make.py index d3bfb440f..95b9e038c 100644 --- a/src/attr/_make.py +++ b/src/attr/_make.py @@ -598,55 +598,60 @@ def _transform_attrs( return _Attributes((AttrsClass(attrs), base_attrs, base_attr_map)) -def _make_cached_property_getattr(cached_properties, original_getattr, cls): +def _make_cached_property_getattribute( + cached_properties, original_getattribute, cls +): lines = [ # Wrapped to get `__class__` into closure cell for super() # (It will be replaced with the newly constructed class after construction). "def wrapper(_cls):", " __class__ = _cls", - " def __getattr__(self, item, cached_properties=cached_properties, original_getattr=original_getattr, _cached_setattr_get=_cached_setattr_get):", - " func = cached_properties.get(item)", - " if func is not None:", - " result = func(self)", - " _setter = _cached_setattr_get(self)", - " _setter(item, result)", - " return result", + " def __getattribute__(self, item, cached_properties=cached_properties, original_getattribute=original_getattribute, _cached_setattr_get=_cached_setattr_get):", + " try:", + " return object.__getattribute__(self, item)", + " except AttributeError:", + " func = cached_properties.get(item)", + " if func is not None:", + " result = func(self)", + " _setter = _cached_setattr_get(self)", + " _setter(item, result)", + " return result", ] - if original_getattr is not None: + if original_getattribute is not None: lines.append( - " return original_getattr(self, item)", + " return original_getattribute(self, item)", ) else: lines.extend( [ - " try:", - " return super().__getattribute__(item)", - " except AttributeError:", - " if not hasattr(super(), '__getattr__'):", - " raise", - " return super().__getattr__(item)", - " original_error = f\"'{self.__class__.__name__}' object has no attribute '{item}'\"", - " raise AttributeError(original_error)", + " try:", + " return super().__getattribute__(item)", + " except AttributeError:", + " if not hasattr(super(), '__getattribute__'):", + " raise", + " return super().__getattribute__(item)", + " original_error = f\"'{self.__class__.__name__}' object has no attribute '{item}'\"", + " raise AttributeError(original_error)", ] ) lines.extend( [ - " return __getattr__", - "__getattr__ = wrapper(_cls)", + " return __getattribute__", + "__getattribute__ = wrapper(_cls)", ] ) - unique_filename = _generate_unique_filename(cls, "getattr") + unique_filename = _generate_unique_filename(cls, "getattribute") glob = { "cached_properties": cached_properties, "_cached_setattr_get": _OBJ_SETATTR.__get__, - "original_getattr": original_getattr, + "original_getattribute": original_getattribute, } return _make_method( - "__getattr__", + "__getattribute__", "\n".join(lines), unique_filename, glob, @@ -948,12 +953,14 @@ def _create_slots_class(self): if annotation is not inspect.Parameter.empty: class_annotations[name] = annotation - original_getattr = cd.get("__getattr__") - if original_getattr is not None: - additional_closure_functions_to_update.append(original_getattr) + original_getattribute = cd.get("__getattribute__") + if original_getattribute is not None: + additional_closure_functions_to_update.append( + original_getattribute + ) - cd["__getattr__"] = _make_cached_property_getattr( - cached_properties, original_getattr, self._cls + cd["__getattribute__"] = _make_cached_property_getattribute( + cached_properties, original_getattribute, self._cls ) # We only add the names of attributes that aren't inherited. diff --git a/tests/test_slots.py b/tests/test_slots.py index 78215ea18..9bd2b3ef3 100644 --- a/tests/test_slots.py +++ b/tests/test_slots.py @@ -891,6 +891,63 @@ 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. + + Regression test for issue https://github.com/python-attrs/attrs/issues/1288 + """ + + # Reference behaviour, without attr. + class P: + __slots__ = () + + @functools.cached_property + def f(self) -> int: + return 0 + + class C(P): + def __getattr__(self, item: str) -> str: + return item + + assert not C.__slots__ + c = C() + assert c.x == "x" + assert c.__getattribute__("f") == 0 + assert c.f == 0 + + # Same with a base attr class. + @attr.s(slots=True) + class A: + @functools.cached_property + def f(self) -> int: + return 0 + + # But subclass is not an attr-class. + class B(A): + def __getattr__(self, item: str) -> str: + return item + + b = B() + assert b.z == "z" + assert b.__getattribute__("f") == 0 + assert b.f == 0 + + # And also if subclass is an attr-class. + @attr.s(slots=True) + class D(A): + def __getattr__(self, item: str) -> str: + return item + + d = D() + assert d.z == "z" + assert d.__getattribute__("f") == 0 + assert d.f == 0 + + @pytest.mark.skipif(not PY_3_8_PLUS, reason="cached_property is 3.8+") def test_slots_getattr_in_subclass_gets_superclass_cached_property(): """