Define __getattribute__() on slotted classes with cached properties

Method `__getattribute__()` is documented as the "way to to actually get
total control over attribute access" [1] so we change the implementation
of slotted classes with cached properties by defining a
`__getattribute__()` method instead of `__getattr__()` previously.

[1]: https://docs.python.org/3/reference/datamodel.html#customizing-attribute-access

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.

In tests, we replace most custom `__getattr__()` definitions by
equivalent `__getattribute__()` ones, except in regression tests where
`__getattr__()` is explicitly involved. Also, in
test_slots_with_multiple_cached_property_subclasses_works(), we replace
the `if hasattr(super(), "__getattr__"):` by a `try:`/`except
AttributeError:` as using `hasattr(..., "__getattribute__")` would be
meaningless since `__getattribute__()` is always defined.

Fix #1288

Author: dlax

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.

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.
 

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.

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):