diff --git a/bench/test_benchmarks.py b/bench/test_benchmarks.py index 80129a449..fc462bf40 100644 --- a/bench/test_benchmarks.py +++ b/bench/test_benchmarks.py @@ -214,3 +214,19 @@ def test_repeated_access(self): for _ in range(ROUNDS): _ = c.cached + + def test_create_cached_property_class(self): + """ + Benchmark creating a class with a cached property + """ + for _ in range(ROUNDS): + + @attrs.define + class LocalC: + x: int + y: str + z: dict[str, int] + + @functools.cached_property + def cached(self): + return 42 diff --git a/docs/how-does-it-work.md b/docs/how-does-it-work.md index 96dc5c303..313c800ea 100644 --- a/docs/how-does-it-work.md +++ b/docs/how-does-it-work.md @@ -110,8 +110,9 @@ Therefore, *attrs* converts `cached_property`-decorated methods when constructin Getting this working is achieved by: +* Removing the wrapped methods from the original class and storing them. * Adding names to `__slots__` for the wrapped methods. -* Adding a `__getattr__` method to set values on the wrapped methods. +* Creating new `_SlottedCachedProperty` wrappers that wrap the slot descriptors together with the original functions. For most users, this should mean that it works transparently. diff --git a/src/attr/_make.py b/src/attr/_make.py index d24d9ba98..048eeee0a 100644 --- a/src/attr/_make.py +++ b/src/attr/_make.py @@ -496,56 +496,47 @@ 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(_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", - ] - if original_getattr is not None: - lines.append( - " return original_getattr(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)", - ] - ) +class _SlottedCachedProperty: + """ + This is a class that is used to wrap both a slot and a cached property. + *It is not to be used directly.* + Users should just use `functools.cached_property`. - lines.extend( - [ - " return __getattr__", - "__getattr__ = wrapper(_cls)", - ] - ) + attrs' slotting behaviour will remove cached_property instances, + add the names to `__slots__` and after constructing the class, + replace those slot descriptors with instances of this class + """ - unique_filename = _generate_unique_filename(cls, "getattr") + def __init__(self, slot, func): + self.slot = slot + self.func = func + self.__doc__ = self.func.__doc__ + self.__module__ = self.func.__module__ - glob = { - "cached_properties": cached_properties, - "_cached_setattr_get": _OBJ_SETATTR.__get__, - "original_getattr": original_getattr, - } + self._slotget = slot.__get__ + self._slotset = slot.__set__ + self._slotdelete = slot.__delete__ - return _linecache_and_compile( - "\n".join(lines), unique_filename, glob, locals={"_cls": cls} - )["__getattr__"] + def __get__(self, instance, owner=None): + if instance is None: + return self + + try: + return self._slotget(instance, owner) + except AttributeError: + pass + + result = self.func(instance) + + self._slotset(instance, result) + + return result + + def __set__(self, obj, value): + self._slotset(obj, value) + + def __delete__(self, obj): + self._slotdelete(obj) def _frozen_setattrs(self, name, value): @@ -918,18 +909,11 @@ def _create_slots_class(self): names += (name,) # Clear out function from class to avoid clashing. del cd[name] - additional_closure_functions_to_update.append(func) 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 - ) + additional_closure_functions_to_update.append(func) # We only add the names of attributes that aren't inherited. # Setting __slots__ to inherited attributes wastes memory. @@ -956,6 +940,14 @@ def _create_slots_class(self): # Create new class based on old class and our methods. cls = type(self._cls)(self._cls.__name__, self._cls.__bases__, cd) + # Now add back the wrapped cached properties + for name, func in cached_properties.items(): + slot = getattr(cls, name) + if isinstance(slot, _SlottedCachedProperty): + slot = slot.slot + slotted_property = _SlottedCachedProperty(slot, func) + setattr(cls, name, slotted_property) + # The following is a fix for # . # If a method mentions `__class__` or uses the no-arg super(), the diff --git a/tests/test_slots.py b/tests/test_slots.py index a74c32b03..f971db382 100644 --- a/tests/test_slots.py +++ b/tests/test_slots.py @@ -885,6 +885,178 @@ def __getattr__(self, item): assert a.z == "z" +def test_slots_cached_property_retains_doc(): + """ + Cached property's docstring is retained + + See: https://github.com/python-attrs/attrs/issues/1325 + """ + + @attr.s(slots=True) + class A: + x = attr.ib() + + @functools.cached_property + def f(self): + """ + This is a docstring. + """ + return self.x + + assert "This is a docstring." in A.f.__doc__ + + +def test_slots_cached_property_super_works(): + """ + Calling super() with a cached property should correctly call from the parent + + See: https://github.com/python-attrs/attrs/issues/1333 + """ + + @attr.s(slots=True) + class Parent: + @functools.cached_property + def name(self) -> str: + return "Alice" + + @attr.s(slots=True) + class Child(Parent): + @functools.cached_property + def name(self) -> str: + return f"Bob (son of {super().name})" + + p = Parent() + c = Child() + + assert p.name == "Alice" + assert c.name == "Bob (son of Alice)" + + +def test_slots_cached_property_skips_child_getattr(): + """ + __getattr__ on child should not interfere with cached_properties + + See: https://github.com/python-attrs/attrs/issues/1288 + """ + + @attrs.define + class Bob: + @functools.cached_property + def howdy(self): + return 3 + + class Sup(Bob): + def __getattr__(self, name): + raise AttributeError(name) + + b = Sup() + + assert b.howdy == 3 + + +def test_slots_cached_property_direct(): + """ + Test getting the wrapped cached property directly + """ + from attr._make import _SlottedCachedProperty + + @attr.s(slots=True) + class Parent: + @functools.cached_property + def name(self) -> str: + return "Alice" + + assert isinstance(Parent.name, _SlottedCachedProperty) + + +def test_slots_cached_property_set_delete(): + """ + Set and Delete should work on the descriptor as on a regular + cached property + """ + + @attr.s(slots=True) + class Parent: + @functools.cached_property + def name(self) -> str: + return "Alice" + + p = Parent() + p.name = "Bob" + assert p.name == "Bob" + del p.name + assert p.name == "Alice" + + +@pytest.mark.parametrize( + "slotted", + [ + pytest.param( + True, + marks=pytest.mark.xfail( + reason="field names are not checked for cached properties" + ), + ), + False, + ], +) +def test_cached_property_overriding_field(slotted): + """ + Discrepancy in cached property overriding behaviour + + attrs only considers fields that are not fields to become + cached properties. + + c.name is not converted to a cached property + """ + + @attrs.define(slots=slotted) + class Parent: + name: str = "Alice" + + @attrs.define(slots=slotted) + class Child(Parent): + @functools.cached_property + def name(self): + return "Bob" + + # This isn't to imply that this is good + # just that it's consistent + p = Parent() + c = Child() + + assert p.name == "Alice" + assert c.name == "Alice" + del c.name + assert c.name == "Bob" # Errors under slots + + +@pytest.mark.parametrize("slotted", [True, False]) +def test_field_overriding_cached_property(slotted): + """ + Check that overriding a cached property with a field + works the same slotted or unslotted + """ + + @attrs.define(slots=slotted) + class Parent: + @functools.cached_property + def name(self): + return "Alice" + + @attrs.define(slots=slotted) + class Child(Parent): + name: str = "Bob" + + p = Parent() + c = Child() + + assert p.name == "Alice" + assert c.name == "Bob" + del c.name + assert c.name == "Alice" + + def test_slots_getattr_in_superclass__is_called_for_missing_attributes_when_cached_property_present(): """ Ensure __getattr__ implementation is maintained in subclass.