From 3e31d8c83a1823bde9717fa5db46dad644db8a68 Mon Sep 17 00:00:00 2001 From: Danny Cooper Date: Fri, 17 Nov 2023 15:46:51 +0000 Subject: [PATCH 01/12] Add support for cached_properties to slotted attrs classes. --- src/attr/_make.py | 48 +++++++++++++- tests/test_slots.py | 158 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 204 insertions(+), 2 deletions(-) diff --git a/src/attr/_make.py b/src/attr/_make.py index 9dd0e2e37..d36ddddb7 100644 --- a/src/attr/_make.py +++ b/src/attr/_make.py @@ -3,12 +3,14 @@ import contextlib import copy import enum +import functools import inspect import linecache import sys import types import typing +from _thread import RLock from operator import itemgetter # We need to import _compat itself in addition to the _compat members to avoid @@ -597,6 +599,28 @@ def _transform_attrs( return _Attributes((AttrsClass(attrs), base_attrs, base_attr_map)) +def _make_cached_property_getattr(cached_properties, original_getattr=None): + lock = RLock() # This is global for the class, can that be avoided? + + def __getattr__(instance, item: str): + func = cached_properties.get(item) + if func is not None: + with lock: + try: + # In case another thread has set it. + return object.__getattribute__(instance, item) + except AttributeError: + result = func(instance) + object.__setattr__(instance, item, result) + return result + elif original_getattr is not None: + return original_getattr(instance, item) + else: + raise AttributeError(item) + + return __getattr__ + + def _frozen_setattrs(self, name, value): """ Attached to frozen classes as __setattr__. @@ -857,9 +881,31 @@ def _create_slots_class(self): ): names += ("__weakref__",) + cached_properties = { + name: cached_property.func + for name, cached_property in cd.items() + if isinstance(cached_property, functools.cached_property) + } + + if cached_properties: + # Add cached properties to names for slotting. + names += tuple(cached_properties.keys()) + + if "__annotations__" in cd: + for name, func in cached_properties.items(): + annotation = inspect.signature(func).return_annotation + if annotation is not inspect.Parameter.empty: + cd["__annotations__"][name] = annotation + + cd["__getattr__"] = _make_cached_property_getattr( + cached_properties, cd.get("__getattr__") + ) + del cd[name] + # 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 +919,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__ @@ -909,7 +956,6 @@ def _create_slots_class(self): else: if match: cell.cell_contents = cls - return cls def add_repr(self, ns): diff --git a/tests/test_slots.py b/tests/test_slots.py index 89d838d3a..a826f4987 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,6 +12,7 @@ import pytest import attr +import attrs from attr._compat import PYPY @@ -715,6 +716,161 @@ def f(self): assert B(17).f == 289 +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 + + +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) + + +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} + + +def test_slots_sub_class_avoids_duplicated_slots(): + """ + Duplicating the slots is a wast 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__ == () + + +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__ == () + + +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 + + +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 + + +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 + + @attr.s(slots=True) class A: x = attr.ib() From 6bbc0a6f4fb7ad423402a2791e4509d90134f064 Mon Sep 17 00:00:00 2001 From: Danny Cooper Date: Fri, 17 Nov 2023 15:46:54 +0000 Subject: [PATCH 02/12] Remove locking from implementation --- src/attr/_make.py | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/src/attr/_make.py b/src/attr/_make.py index d36ddddb7..67226492a 100644 --- a/src/attr/_make.py +++ b/src/attr/_make.py @@ -10,7 +10,6 @@ import types import typing -from _thread import RLock from operator import itemgetter # We need to import _compat itself in addition to the _compat members to avoid @@ -600,23 +599,15 @@ def _transform_attrs( def _make_cached_property_getattr(cached_properties, original_getattr=None): - lock = RLock() # This is global for the class, can that be avoided? - def __getattr__(instance, item: str): func = cached_properties.get(item) if func is not None: - with lock: - try: - # In case another thread has set it. - return object.__getattribute__(instance, item) - except AttributeError: - result = func(instance) - object.__setattr__(instance, item, result) - return result - elif original_getattr is not None: + result = func(instance) + object.__setattr__(instance, item, result) + return result + if original_getattr is not None: return original_getattr(instance, item) - else: - raise AttributeError(item) + raise AttributeError(item) return __getattr__ From 6737a28371722773aec39e22e76bd1f3cef35344 Mon Sep 17 00:00:00 2001 From: Danny Cooper Date: Fri, 17 Nov 2023 15:46:55 +0000 Subject: [PATCH 03/12] Add test for multiple cached properties and fix bug --- src/attr/_make.py | 5 ++++- tests/test_slots.py | 23 +++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/src/attr/_make.py b/src/attr/_make.py index 67226492a..9a730e0c7 100644 --- a/src/attr/_make.py +++ b/src/attr/_make.py @@ -882,6 +882,10 @@ 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] + if "__annotations__" in cd: for name, func in cached_properties.items(): annotation = inspect.signature(func).return_annotation @@ -891,7 +895,6 @@ def _create_slots_class(self): cd["__getattr__"] = _make_cached_property_getattr( cached_properties, cd.get("__getattr__") ) - del cd[name] # We only add the names of attributes that aren't inherited. # Setting __slots__ to inherited attributes wastes memory. diff --git a/tests/test_slots.py b/tests/test_slots.py index a826f4987..1328ac255 100644 --- a/tests/test_slots.py +++ b/tests/test_slots.py @@ -871,6 +871,29 @@ def f(self): assert obj_2.f == 2 +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() From f8d2739491f7bac4f343954ea288dd10533a72d6 Mon Sep 17 00:00:00 2001 From: Danny Cooper Date: Fri, 17 Nov 2023 15:46:56 +0000 Subject: [PATCH 04/12] Add changelog file --- changelog.d/1200.change.md | 1 + 1 file changed, 1 insertion(+) 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. From 6860fafa74a4301d070871f00a98483c022786cb Mon Sep 17 00:00:00 2001 From: Danny Cooper Date: Fri, 17 Nov 2023 15:46:56 +0000 Subject: [PATCH 05/12] Document slotted cached properties --- docs/how-does-it-work.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) 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. From 3e7ad4fe51aaa071a75e2a362c38f9712bed34b2 Mon Sep 17 00:00:00 2001 From: Danny Cooper Date: Fri, 17 Nov 2023 15:46:57 +0000 Subject: [PATCH 06/12] Add cached_property hypothesis check. --- tests/strategies.py | 22 ++++++++++++++++++++-- tests/test_3rd_party.py | 2 +- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/tests/strategies.py b/tests/strategies.py index 55598750b..b776f485a 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 @@ -111,13 +111,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 +163,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 +186,20 @@ def init(self, *args, **kwargs): cls_dict["__init__"] = init + bases = (object,) + if cached_property or (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. From 74205c56531b3ab883f2f6257d710dd5b1e65b98 Mon Sep 17 00:00:00 2001 From: Danny Cooper Date: Fri, 17 Nov 2023 15:46:58 +0000 Subject: [PATCH 07/12] Only run cached_property imports on python 3.8+ --- src/attr/_compat.py | 1 + src/attr/_make.py | 16 +++++++++++----- tests/strategies.py | 6 +++++- tests/test_slots.py | 11 ++++++++++- 4 files changed, 27 insertions(+), 7 deletions(-) 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 9a730e0c7..c19e0e869 100644 --- a/src/attr/_make.py +++ b/src/attr/_make.py @@ -17,6 +17,7 @@ from . import _compat, _config, setters from ._compat import ( PY310, + PY_3_8_PLUS, _AnnotationExtractor, get_generic_base, ) @@ -872,11 +873,16 @@ def _create_slots_class(self): ): names += ("__weakref__",) - cached_properties = { - name: cached_property.func - for name, cached_property in cd.items() - if isinstance(cached_property, functools.cached_property) - } + 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 = {} if cached_properties: # Add cached properties to names for slotting. diff --git a/tests/strategies.py b/tests/strategies.py index b776f485a..783058f83 100644 --- a/tests/strategies.py +++ b/tests/strategies.py @@ -13,6 +13,8 @@ import attr +from attr._compat import PY_3_8_PLUS + from .utils import make_class @@ -187,7 +189,9 @@ def init(self, *args, **kwargs): cls_dict["__init__"] = init bases = (object,) - if cached_property or (cached_property is None and cached_property_flag): + if cached_property or ( + PY_3_8_PLUS and cached_property is None and cached_property_flag + ): class BaseWithCachedProperty: @functools.cached_property diff --git a/tests/test_slots.py b/tests/test_slots.py index 1328ac255..c8e3cefb7 100644 --- a/tests/test_slots.py +++ b/tests/test_slots.py @@ -14,7 +14,7 @@ import attr import attrs -from attr._compat import PYPY +from attr._compat import PY_3_8_PLUS, PYPY # Pympler doesn't work on PyPy. @@ -716,6 +716,7 @@ 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. @@ -732,6 +733,7 @@ def f(self): 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. @@ -749,6 +751,7 @@ def f(self): 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_infers_type(): """ Infers type of cached property. @@ -765,6 +768,7 @@ def f(self) -> int: assert A.__annotations__ == {"x": int, "f": int} +@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 wast of memory. @@ -788,6 +792,7 @@ def f(self): 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. @@ -809,6 +814,7 @@ class B(A): 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. @@ -829,6 +835,7 @@ def f(self): 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. @@ -851,6 +858,7 @@ def f(self): 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. @@ -871,6 +879,7 @@ def f(self): 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. From 804d33c2e01b1b4bd189dcf05618a7dd9dcd05a3 Mon Sep 17 00:00:00 2001 From: Danny Cooper Date: Fri, 17 Nov 2023 15:46:58 +0000 Subject: [PATCH 08/12] Use cached _obj_setattr instead of `object.__setattr__` --- src/attr/_make.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/attr/_make.py b/src/attr/_make.py index c19e0e869..e700498ee 100644 --- a/src/attr/_make.py +++ b/src/attr/_make.py @@ -604,7 +604,7 @@ def __getattr__(instance, item: str): func = cached_properties.get(item) if func is not None: result = func(instance) - object.__setattr__(instance, item, result) + _obj_setattr(instance, item, result) return result if original_getattr is not None: return original_getattr(instance, item) From 80e8a6b8cda020818c77688f6a306275887c8aa3 Mon Sep 17 00:00:00 2001 From: Danny Cooper Date: Fri, 17 Nov 2023 15:46:59 +0000 Subject: [PATCH 09/12] Correctly resolve mro for __getattr__ in cached properties --- src/attr/_make.py | 56 +++++++++++--- tests/test_slots.py | 177 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 220 insertions(+), 13 deletions(-) diff --git a/src/attr/_make.py b/src/attr/_make.py index e700498ee..d96113bee 100644 --- a/src/attr/_make.py +++ b/src/attr/_make.py @@ -599,18 +599,46 @@ def _transform_attrs( return _Attributes((AttrsClass(attrs), base_attrs, base_attr_map)) -def _make_cached_property_getattr(cached_properties, original_getattr=None): - def __getattr__(instance, item: str): - func = cached_properties.get(item) - if func is not None: - result = func(instance) - _obj_setattr(instance, item, result) - return result - if original_getattr is not None: - return original_getattr(instance, item) - raise AttributeError(item) +def _make_cached_property_getattr( + cached_properties, + 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, cached_properties, _cached_setattr_get):", + " __class__ = _cls", + " def __getattr__(self, item):", + " func = cached_properties.get(item)", + " if func is not None:", + " result = func(self)", + " _setter = _cached_setattr_get(self)", + " _setter(item, result)", + " return result", + " if '__attrs_original_getattr__' in vars(__class__):", + " return __class__.__attrs_original_getattr__(self, item)", + " if hasattr(super(), '__getattr__'):", + " return super().__getattr__(item)", + " original_error = f\"'{self.__class__.__name__}' object has no attribute '{item}'\"", + " raise AttributeError(original_error)", + " return __getattr__", + "__getattr__ = wrapper(_cls, cached_properties, _cached_setattr_get)", + ] - return __getattr__ + unique_filename = _generate_unique_filename(cls, "getattr") + + glob = { + "cached_properties": cached_properties, + "_cached_setattr_get": _obj_setattr.__get__, + "_cls": cls, + } + + return _make_method( + "__getattr__", + "\n".join(lines), + unique_filename, + glob, + ) def _frozen_setattrs(self, name, value): @@ -898,8 +926,12 @@ def _create_slots_class(self): if annotation is not inspect.Parameter.empty: cd["__annotations__"][name] = annotation + original_getattr = cd.get("__getattr__") + if original_getattr is not None: + cd["__attrs_original_getattr__"] = original_getattr + cd["__getattr__"] = _make_cached_property_getattr( - cached_properties, cd.get("__getattr__") + cached_properties, 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 c8e3cefb7..26365ab0d 100644 --- a/tests/test_slots.py +++ b/tests/test_slots.py @@ -751,6 +751,23 @@ def f(self): 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(): """ @@ -768,10 +785,168 @@ def f(self) -> int: 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 wast of memory. + Duplicating the slots is a waste of memory. """ @attr.s(slots=True) From b0d87e3508769a631e66a6e9f503dab2f1b5c0b7 Mon Sep 17 00:00:00 2001 From: Danny Cooper Date: Fri, 17 Nov 2023 15:47:00 +0000 Subject: [PATCH 10/12] Use _get_annotations rather than branching on class dict entry --- src/attr/_make.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/attr/_make.py b/src/attr/_make.py index d96113bee..55ab31fab 100644 --- a/src/attr/_make.py +++ b/src/attr/_make.py @@ -920,11 +920,11 @@ def _create_slots_class(self): # Clear out function from class to avoid clashing. del cd[name] - if "__annotations__" in cd: - for name, func in cached_properties.items(): - annotation = inspect.signature(func).return_annotation - if annotation is not inspect.Parameter.empty: - cd["__annotations__"][name] = annotation + 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: From 07741dd5ada79f5c2c6aed096e77a8bd66c97275 Mon Sep 17 00:00:00 2001 From: Danny Cooper Date: Mon, 27 Nov 2023 13:33:17 +0000 Subject: [PATCH 11/12] Optimise __getattr__ code by front loading branching, and injecting locasl variables --- src/attr/_make.py | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/src/attr/_make.py b/src/attr/_make.py index 55ab31fab..327978f7d 100644 --- a/src/attr/_make.py +++ b/src/attr/_make.py @@ -601,29 +601,42 @@ def _transform_attrs( 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(_cls, cached_properties, _cached_setattr_get):", + "def wrapper():", " __class__ = _cls", - " def __getattr__(self, item):", + " 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 '__attrs_original_getattr__' in vars(__class__):", - " return __class__.__attrs_original_getattr__(self, item)", - " if hasattr(super(), '__getattr__'):", - " return super().__getattr__(item)", - " original_error = f\"'{self.__class__.__name__}' object has no attribute '{item}'\"", - " raise AttributeError(original_error)", - " return __getattr__", - "__getattr__ = wrapper(_cls, cached_properties, _cached_setattr_get)", ] + 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") @@ -631,6 +644,7 @@ def _make_cached_property_getattr( "cached_properties": cached_properties, "_cached_setattr_get": _obj_setattr.__get__, "_cls": cls, + "original_getattr": original_getattr, } return _make_method( @@ -931,7 +945,7 @@ def _create_slots_class(self): cd["__attrs_original_getattr__"] = original_getattr cd["__getattr__"] = _make_cached_property_getattr( - cached_properties, self._cls + cached_properties, original_getattr, self._cls ) # We only add the names of attributes that aren't inherited. From bac209ecd17a2ce918178b31d4c40d1378e57d2d Mon Sep 17 00:00:00 2001 From: Danny Cooper Date: Mon, 27 Nov 2023 13:38:08 +0000 Subject: [PATCH 12/12] Remove unnecessary `__attrs_original_getattr__` from class dictionary. --- src/attr/_make.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/attr/_make.py b/src/attr/_make.py index 327978f7d..32d30819d 100644 --- a/src/attr/_make.py +++ b/src/attr/_make.py @@ -5,6 +5,7 @@ import enum import functools import inspect +import itertools import linecache import sys import types @@ -926,6 +927,9 @@ def _create_slots_class(self): # 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()) @@ -942,7 +946,7 @@ def _create_slots_class(self): original_getattr = cd.get("__getattr__") if original_getattr is not None: - cd["__attrs_original_getattr__"] = original_getattr + additional_closure_functions_to_update.append(original_getattr) cd["__getattr__"] = _make_cached_property_getattr( cached_properties, original_getattr, self._cls @@ -979,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.