diff --git a/changelog.d/1291.change.md b/changelog.d/1291.change.md new file mode 100644 index 000000000..bfd3be461 --- /dev/null +++ b/changelog.d/1291.change.md @@ -0,0 +1,2 @@ +Rework handling of `functools.cached_property` in slotted classes by defining a `__getattribute__()` method instead of `__getattr__()` previously. +While this is closer to the guidance for [customizing attribute access](https://docs.python.org/3/reference/datamodel.html#customizing-attribute-access), this fixes inheritance resolution of cached properties in slotted classes when subclasses are not attr-decorated but define a custom `__getattr__()` method. diff --git a/docs/how-does-it-work.md b/docs/how-does-it-work.md index 70ecd4551..7ba4212aa 100644 --- a/docs/how-does-it-work.md +++ b/docs/how-does-it-work.md @@ -111,7 +111,7 @@ Therefore, *attrs* converts `cached_property`-decorated methods when constructin Getting this working is achieved by: * Adding names to `__slots__` for the wrapped methods. -* Adding a `__getattr__` method to set values on the wrapped methods. +* Adding a `__getattribute__` method to set values on the wrapped methods. For most users, this should mean that it works transparently. diff --git a/src/attr/_make.py b/src/attr/_make.py index 097534fbb..061d78466 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, @@ -942,12 +947,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..711073e8b 100644 --- a/tests/test_slots.py +++ b/tests/test_slots.py @@ -786,7 +786,7 @@ def f(self) -> int: @pytest.mark.skipif(not PY_3_8_PLUS, reason="cached_property is 3.8+") -def test_slots_cached_property_with_empty_getattr_raises_attribute_error_of_requested(): +def test_slots_cached_property_with_empty_getattribute_raises_attribute_error_of_requested(): """ Ensures error information is not lost. """ @@ -809,8 +809,8 @@ def f(self): @pytest.mark.skipif(not PY_3_8_PLUS, reason="cached_property is 3.8+") def test_slots_cached_property_raising_attributeerror(): """ - Ensures AttributeError raised by a property is preserved by __getattr__() - implementation. + Ensures AttributeError raised by a property is preserved by + __getattribute__() implementation. Regression test for issue https://github.com/python-attrs/attrs/issues/1230 """ @@ -846,9 +846,9 @@ def q(self): @pytest.mark.skipif(not PY_3_8_PLUS, reason="cached_property is 3.8+") -def test_slots_cached_property_with_getattr_calls_getattr_for_missing_attributes(): +def test_slots_cached_property_with_getattribute_calls_getattr_for_missing_attributes(): """ - Ensure __getattr__ implementation is maintained for non cached_properties. + Ensure __getattribute__ implementation is maintained for non cached_properties. """ @attr.s(slots=True) @@ -859,7 +859,7 @@ class A: def f(self): return self.x - def __getattr__(self, item): + def __getattribute__(self, item): return item a = A(1) @@ -868,16 +868,16 @@ def __getattr__(self, item): @pytest.mark.skipif(not PY_3_8_PLUS, reason="cached_property is 3.8+") -def test_slots_getattr_in_superclass__is_called_for_missing_attributes_when_cached_property_present(): +def test_slots_getattribute_in_superclass__is_called_for_missing_attributes_when_cached_property_present(): """ - Ensure __getattr__ implementation is maintained in subclass. + Ensure __getattribute__ implementation is maintained in subclass. """ @attr.s(slots=True) class A: x = attr.ib() - def __getattr__(self, item): + def __getattribute__(self, item): return item @attr.s(slots=True) @@ -892,9 +892,66 @@ def f(self): @pytest.mark.skipif(not PY_3_8_PLUS, reason="cached_property is 3.8+") -def test_slots_getattr_in_subclass_gets_superclass_cached_property(): +def test_slots_getattr_in_subclass_without_cached_property(): """ - Ensure super() in __getattr__ is not broken through cached_property re-write. + 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_getattribute_in_subclass_gets_superclass_cached_property(): + """ + Ensure super() in __getattribute__ is not broken through cached_property re-write. """ @attr.s(slots=True) @@ -905,7 +962,7 @@ class A: def f(self): return self.x - def __getattr__(self, item): + def __getattribute__(self, item): return item @attr.s(slots=True) @@ -914,8 +971,8 @@ class B(A): def g(self): return self.x - def __getattr__(self, item): - return super().__getattr__(item) + def __getattribute__(self, item): + return super().__getattribute__(item) b = B(1) assert b.f == 1 @@ -966,10 +1023,11 @@ class B: def g(self): return self.x * 2 - def __getattr__(self, item): - if hasattr(super(), "__getattr__"): - return super().__getattr__(item) - return item + def __getattribute__(self, item): + try: + return super().__getattribute__(item) + except AttributeError: + return item @attr.s(slots=True) class AB(A, B):