python-attrs / attrs

Python Classes Without Boilerplate
https://www.attrs.org/
MIT License
5.23k stars 364 forks source link

cached_property in slotted classes doesn't work if overridden in child class #1333

Open Hnasar opened 1 month ago

Hnasar commented 1 month ago

With attrs 24.2.0 and 23.2.0, if you use cached_property and try to override it in a child, and access the parent value, you get an AttributeError. We ran into this in our code when converting some classes to use slots.

I see there was support added in 24.2.0 for

Allow super() calls in slotted cached properties

But attrs still doesn't support _"super() calls of overridden cachedproperty's in slotted classes":

from functools import cached_property
import attrs

@attrs.define(slots=True)
class Parent:
    @cached_property
    def name(self) -> str:
        return "Alice"

@attrs.define(slots=True)
class Child(Parent):
    @cached_property
    def name(self) -> str:
        return f"Bob (son of {super().name})"

p = Parent()
print(p.name)  # prints Alice
c = Child()
print(c.name)  # error:
Traceback (most recent call last):
  File "foo.py", line 23, in <module>
    print(c.name)
          ^^^^^^
  File "<attrs generated getattr __main__.Child>", line 6, in __getattr__
  File "foo.py", line 18, in name
    return f"Bob (son of {super().name})"
                          ^^^^^^^^^^^^
AttributeError: 'Child' object has no attribute 'name'

Workarounds:

Elaboration:

I see the special handling is described in the docs and I see in the code that it generates a special __getattr__ that sets the generated value in the slot after being called. Perhaps when constructing a class, attrs should also walk the MRO for cached_propertys and handle any overridden cached_property methods — perhaps create some sort of namespaced/name-manged properties for the parent.

hynek commented 4 weeks ago

I think this is related to #1288? As we've established there, it seems to be a rather complicated issue and all solutions either led the concept of caching ad absurdum (read: performance impact made it questionable to be worth it) or just added another edge case. :|