From 597c3a8adb5abeda66e0f6e9aed7fcaec26ee1a0 Mon Sep 17 00:00:00 2001 From: diabolo-dan Date: Fri, 8 Dec 2023 18:24:25 +0000 Subject: [PATCH] Add support for cached_properties to slotted attrs classes. (#1200) * Add support for cached_properties to slotted attrs classes. * Remove locking from implementation * Add test for multiple cached properties and fix bug * Add changelog file * Document slotted cached properties * Add cached_property hypothesis check. * Only run cached_property imports on python 3.8+ * Use cached _obj_setattr instead of `object.__setattr__` * Correctly resolve mro for __getattr__ in cached properties * Use _get_annotations rather than branching on class dict entry * Optimise __getattr__ code by front loading branching, and injecting locasl variables * Remove unnecessary `__attrs_original_getattr__` from class dictionary. --------- Co-authored-by: Hynek Schlawack --- changelog.d/1200.change.md | 1 + docs/how-does-it-work.md | 20 ++ src/attr/_compat.py | 1 + src/attr/_make.py | 102 ++++++++++- tests/strategies.py | 26 ++- tests/test_3rd_party.py | 2 +- tests/test_slots.py | 367 ++++++++++++++++++++++++++++++++++++- 7 files changed, 512 insertions(+), 7 deletions(-) create mode 100644 changelog.d/1200.change.md diff --git a/changelog.d/1200.change.md b/changelog.d/1200.change.md new file mode 100644 index 000000000..402252bba --- /dev/null +++ b/changelog.d/1200.change.md @@ -0,0 +1 @@ +Slotted classes now transform `functools.cached_property` decorated methods to support equivalent semantics. diff --git a/docs/how-does-it-work.md b/docs/how-does-it-work.md index 72972e753..7acc81213 100644 --- a/docs/how-does-it-work.md +++ b/docs/how-does-it-work.md @@ -96,3 +96,23 @@ Pick what's more important to you. You should avoid instantiating lots of frozen slotted classes (i.e. `@frozen`) in performance-critical code. Frozen dict classes have barely a performance impact, unfrozen slotted classes are even *faster* than unfrozen dict classes (i.e. regular classes). + + +(how-slotted-cached_property)= + +## Cached Properties on Slotted Classes. + +By default, the standard library `functools.cached_property` decorator does not work on slotted classes, +because it requires a `__dict__` to store the cached value. +This could be surprising when uses *attrs*, as makes using slotted classes so easy, +so attrs will convert `functools.cached_property` decorated methods, when constructing slotted classes. + +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. + +For most users this should mean that it works transparently. + +Note that the implementation does not guarantee that the wrapped method is called +only once in multi-threaded usage. This matches the implementation of `cached_property` +in python v3.12. diff --git a/src/attr/_compat.py b/src/attr/_compat.py index 26da29cf9..46b05ca45 100644 --- a/src/attr/_compat.py +++ b/src/attr/_compat.py @@ -10,6 +10,7 @@ PYPY = platform.python_implementation() == "PyPy" +PY_3_8_PLUS = sys.version_info[:2] >= (3, 8) PY_3_9_PLUS = sys.version_info[:2] >= (3, 9) PY310 = sys.version_info[:2] >= (3, 10) PY_3_12_PLUS = sys.version_info[:2] >= (3, 12) diff --git a/src/attr/_make.py b/src/attr/_make.py index 8a4e6581f..10b4eca77 100644 --- a/src/attr/_make.py +++ b/src/attr/_make.py @@ -3,7 +3,9 @@ import contextlib import copy import enum +import functools import inspect +import itertools import linecache import sys import types @@ -16,6 +18,7 @@ from . import _compat, _config, setters from ._compat import ( PY310, + PY_3_8_PLUS, _AnnotationExtractor, get_generic_base, ) @@ -597,6 +600,62 @@ def _transform_attrs( return _Attributes((AttrsClass(attrs), base_attrs, base_attr_map)) +def _make_cached_property_getattr( + cached_properties, + original_getattr, + cls, +): + lines = [ + # Wrapped to get `__class__` into closure cell for super() + # (It will be replaced with the newly constructed class after construction). + "def wrapper():", + " __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", + ] + if original_getattr is not None: + lines.append( + " return original_getattr(self, item)", + ) + else: + lines.extend( + [ + " if hasattr(super(), '__getattr__'):", + " return super().__getattr__(item)", + " original_error = f\"'{self.__class__.__name__}' object has no attribute '{item}'\"", + " raise AttributeError(original_error)", + ] + ) + + lines.extend( + [ + " return __getattr__", + "__getattr__ = wrapper()", + ] + ) + + unique_filename = _generate_unique_filename(cls, "getattr") + + glob = { + "cached_properties": cached_properties, + "_cached_setattr_get": _obj_setattr.__get__, + "_cls": cls, + "original_getattr": original_getattr, + } + + return _make_method( + "__getattr__", + "\n".join(lines), + unique_filename, + glob, + ) + + def _frozen_setattrs(self, name, value): """ Attached to frozen classes as __setattr__. @@ -857,9 +916,46 @@ def _create_slots_class(self): ): names += ("__weakref__",) + if PY_3_8_PLUS: + cached_properties = { + name: cached_property.func + for name, cached_property in cd.items() + if isinstance(cached_property, functools.cached_property) + } + else: + # `functools.cached_property` was introduced in 3.8. + # So can't be used before this. + cached_properties = {} + + # Collect methods with a `__class__` reference that are shadowed in the new class. + # To know to update them. + additional_closure_functions_to_update = [] + if cached_properties: + # 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] + + class_annotations = _get_annotations(self._cls) + for name, func in cached_properties.items(): + annotation = inspect.signature(func).return_annotation + 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) + + cd["__getattr__"] = _make_cached_property_getattr( + cached_properties, original_getattr, self._cls + ) + # We only add the names of attributes that aren't inherited. # Setting __slots__ to inherited attributes wastes memory. slot_names = [name for name in names if name not in base_names] + # There are slots for attributes from current class # that are defined in parent classes. # As their descriptors may be overridden by a child class, @@ -873,6 +969,7 @@ def _create_slots_class(self): cd.update(reused_slots) if self._cache_hash: slot_names.append(_hash_cache_field) + cd["__slots__"] = tuple(slot_names) cd["__qualname__"] = self._cls.__qualname__ @@ -886,7 +983,9 @@ def _create_slots_class(self): # compiler will bake a reference to the class in the method itself # as `method.__closure__`. Since we replace the class with a # clone, we rewrite these references so it keeps working. - for item in cls.__dict__.values(): + for item in itertools.chain( + cls.__dict__.values(), additional_closure_functions_to_update + ): if isinstance(item, (classmethod, staticmethod)): # Class- and staticmethods hide their functions inside. # These might need to be rewritten as well. @@ -909,7 +1008,6 @@ def _create_slots_class(self): else: if match: cell.cell_contents = cls - return cls def add_repr(self, ns): diff --git a/tests/strategies.py b/tests/strategies.py index 55598750b..783058f83 100644 --- a/tests/strategies.py +++ b/tests/strategies.py @@ -3,7 +3,7 @@ """ Testing strategies for Hypothesis-based tests. """ - +import functools import keyword import string @@ -13,6 +13,8 @@ import attr +from attr._compat import PY_3_8_PLUS + from .utils import make_class @@ -111,13 +113,19 @@ def simple_attrs_with_metadata(draw): simple_attrs = simple_attrs_without_metadata | simple_attrs_with_metadata() + # Python functions support up to 255 arguments. list_of_attrs = st.lists(simple_attrs, max_size=3) @st.composite def simple_classes( - draw, slots=None, frozen=None, weakref_slot=None, private_attrs=None + draw, + slots=None, + frozen=None, + weakref_slot=None, + private_attrs=None, + cached_property=None, ): """ A strategy that generates classes with default non-attr attributes. @@ -157,6 +165,7 @@ class HypClass: pre_init_flag = draw(st.booleans()) post_init_flag = draw(st.booleans()) init_flag = draw(st.booleans()) + cached_property_flag = draw(st.booleans()) if pre_init_flag: @@ -179,9 +188,22 @@ def init(self, *args, **kwargs): cls_dict["__init__"] = init + bases = (object,) + if cached_property or ( + PY_3_8_PLUS and cached_property is None and cached_property_flag + ): + + class BaseWithCachedProperty: + @functools.cached_property + def _cached_property(self) -> int: + return 1 + + bases = (BaseWithCachedProperty,) + return make_class( "HypClass", cls_dict, + bases=bases, slots=slots_flag if slots is None else slots, frozen=frozen_flag if frozen is None else frozen, weakref_slot=weakref_flag if weakref_slot is None else weakref_slot, diff --git a/tests/test_3rd_party.py b/tests/test_3rd_party.py index 0707b2cd2..b2ce06c29 100644 --- a/tests/test_3rd_party.py +++ b/tests/test_3rd_party.py @@ -19,7 +19,7 @@ class TestCloudpickleCompat: Tests for compatibility with ``cloudpickle``. """ - @given(simple_classes()) + @given(simple_classes(cached_property=False)) def test_repr(self, cls): """ attrs instances can be pickled and un-pickled with cloudpickle. diff --git a/tests/test_slots.py b/tests/test_slots.py index 89d838d3a..26365ab0d 100644 --- a/tests/test_slots.py +++ b/tests/test_slots.py @@ -3,7 +3,7 @@ """ Unit tests for slots-related functionality. """ - +import functools import pickle import weakref @@ -12,8 +12,9 @@ import pytest import attr +import attrs -from attr._compat import PYPY +from attr._compat import PY_3_8_PLUS, PYPY # Pympler doesn't work on PyPy. @@ -715,6 +716,368 @@ def f(self): assert B(17).f == 289 +@pytest.mark.skipif(not PY_3_8_PLUS, reason="cached_property is 3.8+") +def test_slots_cached_property_allows_call(): + """ + cached_property in slotted class allows call. + """ + + @attr.s(slots=True) + class A: + x = attr.ib() + + @functools.cached_property + def f(self): + return self.x + + assert A(11).f == 11 + + +@pytest.mark.skipif(not PY_3_8_PLUS, reason="cached_property is 3.8+") +def test_slots_cached_property_class_does_not_have__dict__(): + """ + slotted class with cached property has no __dict__ attribute. + """ + + @attr.s(slots=True) + class A: + x = attr.ib() + + @functools.cached_property + def f(self): + return self.x + + assert set(A.__slots__) == {"x", "f", "__weakref__"} + assert "__dict__" not in dir(A) + + +@pytest.mark.skipif(not PY_3_8_PLUS, reason="cached_property is 3.8+") +def test_slots_cached_property_works_on_frozen_isntances(): + """ + Infers type of cached property. + """ + + @attrs.frozen(slots=True) + class A: + x: int + + @functools.cached_property + def f(self) -> int: + return self.x + + assert A(x=1).f == 1 + + +@pytest.mark.skipif(not PY_3_8_PLUS, reason="cached_property is 3.8+") +def test_slots_cached_property_infers_type(): + """ + Infers type of cached property. + """ + + @attrs.frozen(slots=True) + class A: + x: int + + @functools.cached_property + def f(self) -> int: + return self.x + + assert A.__annotations__ == {"x": int, "f": 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(): + """ + Ensures error information is not lost. + """ + + @attr.s(slots=True) + class A: + x = attr.ib() + + @functools.cached_property + def f(self): + return self.x + + a = A(1) + with pytest.raises( + AttributeError, match="'A' object has no attribute 'z'" + ): + a.z + + +@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(): + """ + Ensure __getattr__ implementation is maintained for non cached_properties. + """ + + @attr.s(slots=True) + class A: + x = attr.ib() + + @functools.cached_property + def f(self): + return self.x + + def __getattr__(self, item): + return item + + a = A(1) + assert a.f == 1 + assert a.z == "z" + + +@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(): + """ + Ensure __getattr__ implementation is maintained in subclass. + """ + + @attr.s(slots=True) + class A: + x = attr.ib() + + def __getattr__(self, item): + return item + + @attr.s(slots=True) + class B(A): + @functools.cached_property + def f(self): + return self.x + + b = B(1) + assert b.f == 1 + assert b.z == "z" + + +@pytest.mark.skipif(not PY_3_8_PLUS, reason="cached_property is 3.8+") +def test_slots_getattr_in_subclass_gets_superclass_cached_property(): + """ + Ensure super() in __getattr__ is not broken through cached_property re-write. + """ + + @attr.s(slots=True) + class A: + x = attr.ib() + + @functools.cached_property + def f(self): + return self.x + + def __getattr__(self, item): + return item + + @attr.s(slots=True) + class B(A): + @functools.cached_property + def g(self): + return self.x + + def __getattr__(self, item): + return super().__getattr__(item) + + b = B(1) + assert b.f == 1 + assert b.z == "z" + + +@pytest.mark.skipif(not PY_3_8_PLUS, reason="cached_property is 3.8+") +def test_slots_sub_class_with_independent_cached_properties_both_work(): + """ + Subclassing shouldn't break cached properties. + """ + + @attr.s(slots=True) + class A: + x = attr.ib() + + @functools.cached_property + def f(self): + return self.x + + @attr.s(slots=True) + class B(A): + @functools.cached_property + def g(self): + return self.x * 2 + + assert B(1).f == 1 + assert B(1).g == 2 + + +@pytest.mark.skipif(not PY_3_8_PLUS, reason="cached_property is 3.8+") +def test_slots_with_multiple_cached_property_subclasses_works(): + """ + Multiple sub-classes shouldn't break cached properties. + """ + + @attr.s(slots=True) + class A: + x = attr.ib(kw_only=True) + + @functools.cached_property + def f(self): + return self.x + + @attr.s(slots=False) + class B: + @functools.cached_property + def g(self): + return self.x * 2 + + def __getattr__(self, item): + if hasattr(super(), "__getattr__"): + return super().__getattr__(item) + return item + + @attr.s(slots=True) + class AB(A, B): + pass + + ab = AB(x=1) + + assert ab.f == 1 + assert ab.g == 2 + assert ab.h == "h" + + +@pytest.mark.skipif(not PY_3_8_PLUS, reason="cached_property is 3.8+") +def test_slots_sub_class_avoids_duplicated_slots(): + """ + Duplicating the slots is a waste of memory. + """ + + @attr.s(slots=True) + class A: + x = attr.ib() + + @functools.cached_property + def f(self): + return self.x + + @attr.s(slots=True) + class B(A): + @functools.cached_property + def f(self): + return self.x * 2 + + assert B(1).f == 2 + assert B.__slots__ == () + + +@pytest.mark.skipif(not PY_3_8_PLUS, reason="cached_property is 3.8+") +def test_slots_sub_class_with_actual_slot(): + """ + A sub-class can have an explicit attrs field that replaces a cached property. + """ + + @attr.s(slots=True) + class A: # slots : (x, f) + x = attr.ib() + + @functools.cached_property + def f(self): + return self.x + + @attr.s(slots=True) + class B(A): + f: int = attr.ib() + + assert B(1, 2).f == 2 + assert B.__slots__ == () + + +@pytest.mark.skipif(not PY_3_8_PLUS, reason="cached_property is 3.8+") +def test_slots_cached_property_is_not_called_at_construction(): + """ + A cached property function should only be called at property access point. + """ + call_count = 0 + + @attr.s(slots=True) + class A: + x = attr.ib() + + @functools.cached_property + def f(self): + nonlocal call_count + call_count += 1 + return self.x + + A(1) + assert call_count == 0 + + +@pytest.mark.skipif(not PY_3_8_PLUS, reason="cached_property is 3.8+") +def test_slots_cached_property_repeat_call_only_once(): + """ + A cached property function should be called only once, on repeated attribute access. + """ + call_count = 0 + + @attr.s(slots=True) + class A: + x = attr.ib() + + @functools.cached_property + def f(self): + nonlocal call_count + call_count += 1 + return self.x + + obj = A(1) + obj.f + obj.f + assert call_count == 1 + + +@pytest.mark.skipif(not PY_3_8_PLUS, reason="cached_property is 3.8+") +def test_slots_cached_property_called_independent_across_instances(): + """ + A cached property value should be specific to the given instance. + """ + + @attr.s(slots=True) + class A: + x = attr.ib() + + @functools.cached_property + def f(self): + return self.x + + obj_1 = A(1) + obj_2 = A(2) + + assert obj_1.f == 1 + assert obj_2.f == 2 + + +@pytest.mark.skipif(not PY_3_8_PLUS, reason="cached_property is 3.8+") +def test_slots_cached_properties_work_independently(): + """ + Multiple cached properties should work independently. + """ + + @attr.s(slots=True) + class A: + x = attr.ib() + + @functools.cached_property + def f_1(self): + return self.x + + @functools.cached_property + def f_2(self): + return self.x * 2 + + obj = A(1) + + assert obj.f_1 == 1 + assert obj.f_2 == 2 + + @attr.s(slots=True) class A: x = attr.ib()