attrs icon indicating copy to clipboard operation
attrs copied to clipboard

Abstract property can't be found by mixins when property is a `attr.ib`

Open OneRaynyDay opened this issue 1 year ago • 13 comments

Same context here

The following runs fine:

from abc import ABC, abstractmethod
import attr

class A(ABC):
    @abstractmethod
    def prop(self):
        pass

    def foo(self):
        print(self.prop())

class B:
    def prop(self):
        return 5

class C(B, A):
    pass

C()

Here, C has a function prop inherited from B which is found first in the MRO order so A wouldn't complain about its existence. I tried this with a dataclass as well:

from abc import ABC, abstractmethod
from dataclasses import dataclass, field

class A(ABC):
    @property
    @abstractmethod
    def prop(self):
        pass

    def foo(self):
        print(self.prop())

@dataclass
class B:
    prop: int = field(default=5)

class C(B, A):
    pass

C()

However, this doesn't work with attr:

from abc import ABC, abstractmethod
import attr

@attr.s
class A(ABC):
    @property
    @abstractmethod
    def prop(self):
        pass

    def foo(self):
        print(self.prop())

@attr.s
class B:
    prop: int = attr.ib(default=5)

@attr.s
class C(B, A):
    pass

C()

Running it:

❯ python test.py                                                                                  
Traceback (most recent call last):
  File "test_mro_abstract_property.py", line 22, in <module>
    C()
TypeError: Can't instantiate abstract class C with abstract methods prop

Python version: 3.8.3, attrs version 22.1.0

OneRaynyDay avatar Aug 10 '22 21:08 OneRaynyDay

As I've answered on SO, making B slotted fixes it and I'm curious why that is (especially given dataclasses seem to work).

hynek avatar Aug 11 '22 05:08 hynek

I got briefly excited by seeing https://docs.python.org/3.10/library/abc.html#abc.update_abstractmethods but that doesn't fix it either. I'll open an PR adding it anyways if I'll find a test for it.

hynek avatar Aug 11 '22 05:08 hynek

So the reason why dataclasses work seems to be that they store default values as class variables!?

(Pdb++) pp B.__dict__
mappingproxy({'__annotations__': {'prop': <class 'int'>},
              '__dataclass_fields__': {'prop': Field(name='prop',type=<class 'int'>,default=5,default_factory=<dataclasses._MISSING_TYPE object at 0x10eb6afe0>,init=True,repr=True,hash=None,compare=True,metadata=mappingproxy({}),kw_only=False,_field_type=_FIELD)},
              '__dataclass_params__': _DataclassParams(init=True,repr=True,eq=True,order=False,unsafe_hash=False,frozen=False),
              '__dict__': <attribute '__dict__' of 'B' objects>,
              '__doc__': 'B(prop: int = 5)',
              '__eq__': <function B.__eq__ at 0x10f0e52d0>,
              '__hash__': None,
              '__init__': <function B.__init__ at 0x10f0e5000>,
              '__match_args__': ('prop',),
              '__module__': '__main__',
              '__repr__': <function B.__repr__ at 0x10f0e5090>,
              '__weakref__': <attribute '__weakref__' of 'B' objects>,
              'prop': 5}) # <---- !!!

Not sure what to do about all of this.

hynek avatar Aug 11 '22 06:08 hynek

JFTR dataclasses behave inconsistenly here. If you make prop a list it stops working:

@dataclass
class B:
    prop: list = field(default_factory=list)

I think I prefer attrs consistent behavior here. It probably works with slots because abc checks __slots__ for the existence of the attribute?

hynek avatar Aug 11 '22 06:08 hynek

For posterity here's a test, but I don't think this is a bug anymore:

def test_abstract_property(slots):
    @attrs.define(slots=slots)
    class A(abc.ABC):
        @property
        @abc.abstractmethod
        def prop(self):
            pass

        def foo(self):
            print(self.prop())

    @attrs.define(slots=slots)
    class B:
        prop: int = attrs.field(default=5)

    @attrs.define(slots=slots)
    class C(B, A):
        pass

    C()

hynek avatar Aug 11 '22 07:08 hynek

So the reason why dataclasses work seems to be that they store default values as class variables!?

Even if it's a slot class? 👀

Tinche avatar Aug 11 '22 09:08 Tinche

Ah thank you for the quick response! Unfortunately I'm in a bit of a sticky situation in that it's really risky to make B slotted. B is a base class used by a lot (like hundreds) of classes, and it may introduce subtle unexpected behavioral changes.

I'll respond in this thread when I've at least given it a try though!

Also, I'm aware of attr.define and the new interface - I just can't do that yet because our pylint astroid brain doesn't understand that syntax yet (we're a little behind on that version)

OneRaynyDay avatar Aug 11 '22 17:08 OneRaynyDay

So you really need it to be a n attribute with a default value btw? Couldn't you make it a ClassVar?

hynek avatar Aug 11 '22 18:08 hynek

Yeah, actually it's not defaulted in the actual code - I just gave it a default in the example. Sorry for the confusion - does that change anything about the solution?

OneRaynyDay avatar Aug 11 '22 19:08 OneRaynyDay

Only that it probably doesn't work with dataclasses at all either. :)

hynek avatar Aug 11 '22 19:08 hynek

oh... that's even worse news :(

OneRaynyDay avatar Aug 11 '22 19:08 OneRaynyDay

Not elegant, but you can do this for now:

from abc import ABC, abstractmethod, ABCMeta
import attr

@attr.s
class A(ABC):
    @property
    @abstractmethod
    def prop(self):
        pass

    def foo(self):
        print(self.prop())

@attr.s(slots=True)
class B_:
    prop: int = attr.ib(default=5)

@attr.s
class B(B_):
    pass

@attr.s
class C(B, A):
    pass

C()

B().x = 42  # works!
C().y = 23  # too!

hynek avatar Aug 12 '22 06:08 hynek

Sorry for jumping in here, I ended up at this ticket because I wondered if I can implement abstract properties using attr. Short answer: Yes, when using slots.

Here is a doctest example that explains what's going on, including a sketch for a way to implement this:

Abstract properties
===================

You can mark properties as abstract in Python like this:

    >>> import abc

    >>> class Abstract(metaclass=abc.ABCMeta):
    ...     @property
    ...     @abc.abstractmethod
    ...     def value(self):
    ...        pass


As the getter of `value` is backed by an abstract method, the class is
abstract:

    >>> Abstract()
    Traceback (most recent call last):
      ...
    TypeError: Can't instantiate abstract class Abstract without an implementation for abstract method 'value'


Naive implementation of abstract properties
-------------------------------------------

This looks simple, but Python actually makes it hard to implement abstract
properties. Namely this actually requires to write properties, it's not
possible to implement abstract properties by fields:

    >>> class ConcreteNaive(Abstract):
    ...     def __init__(self, value):
    ...         self.value = value  # fulfill property value as attribute

    >>> ConcreteNaive(42)
    Traceback (most recent call last):
      ...
    TypeError: Can't instantiate abstract class ConcreteNaive without an implementation for abstract method 'value'

The reason for this is that the `property` is backed by a descriptor at the
class level:

    >>> vars(Abstract)["value"]                         # doctest: +ELLIPSIS
    <property object at 0x...>

As nothing replaced the entry in the derived class, the property is still
there and marked abstract:

    >>> vars(ConcreteNaive)["value"]
    Traceback (most recent call last):
      ..
    KeyError: 'value'
    >>> ConcreteNaive.value                             # doctest: +ELLIPSIS
    <property object at 0x...>
    >>> ConcreteNaive.value.__isabstractmethod__
    True


Hack: Hide the abstract property
--------------------------------

If suffices to hide the property in derived classes by overriding it with
something else:

    >>> class ConcreteHideProperty(Abstract):
    ...     value = None  # hide abstract property
    ...
    ...     def __init__(self, value):
    ...         self.value = value  # fulfill property value as attribute

    >>> ConcreteHideProperty(42).value
    42

This can have interesting side effects though:

    >>> t = ConcreteHideProperty(42)
    >>> del t.value
    >>> repr(t.value)
    'None'


Clean: Actually implement a property
------------------------------------

To implement the property without any tricks, one has to override the
descriptor of the base class. E.g.:

    >>> class ConcreteClean(Abstract):
    ...     def __init__(self, value):
    ...         self._value = value  # backing store for property
    ...
    ...     @property
    ...     def value(self):
    ...         return self._value

This behaves as expected:

    >>> t = ConcreteClean(42)
    >>> t.value
    42
    >>> del t.value
    Traceback (most recent call last):
      ...
    AttributeError: property 'value' of 'ConcreteClean' object has no deleter


Expert approach: Use slots
--------------------------

Slots are implemented as special descriptors, as the normal access (look
into the instance dict) is not available without a dict. The side effect
is that the property is implemented:

    >>> class ConcreteSlots(Abstract):
    ...     __slots__ = "value",
    ...
    ...     def __init__(self, value):
    ...         self.value = value

This behaves just a bit differently, as the slot mechanism implements a
deleter:

    >>> t = ConcreteSlots(42)
    >>> t.value
    42
    >>> del t.value
    >>> t.value
    Traceback (most recent call last):
      ...
    AttributeError: 'ConcreteSlots' object has no attribute 'value'

We can check that the class dict now contains a descriptor for that slot:

    >>> vars(ConcreteSlots)["value"]
    <member 'value' of 'ConcreteSlots' objects>
    >>> ConcreteSlots.value
    <member 'value' of 'ConcreteSlots' objects>


What should attrs do?
---------------------

The question remains: How to improve the situation when using attrs?

    >>> from attrs import define
    >>> @define(slots=False)
    ... class ConcreteAttrs(Abstract):
    ...     value : int = 7337

    >>> t = ConcreteAttrs(value=42)
    Traceback (most recent call last):
      ...
    TypeError: Can't instantiate abstract class ConcreteAttrs without an implementation for abstract method 'value'

One approach would be to create properties for all attrs.fields after
extracting them from the class dict. Something like:

    >>> def field_property(name):
    ...     def getter(self):
    ...         d = vars(self)
    ...         try:
    ...             return d[name]
    ...         except KeyError:
    ...             pass
    ...         raise AttributeError(name)
    ...            
    ...     def setter(self, value):
    ...         vars(self)[name] = value
    ...
    ...     def deleter(self):
    ...         d = vars(self)
    ...         try:
    ...             del d[name]
    ...             return
    ...         except KeyError:
    ...             pass
    ...         raise AttributeError(name)
    ...
    ...     return property(getter, setter, deleter)

    >>> class FixedConcreteAttrs(ConcreteAttrs):
    ...     value = field_property("value")

    >>> t = FixedConcreteAttrs(42)
    >>> t.value
    42

This is only a sketch, of course. Especially, this would require patching
in the `on_setattr` hooks into the setter and applying this only for
descriptors in the base class that are marked with `__isabstractmethod__`.

tlandschoff-scale avatar Apr 12 '23 22:04 tlandschoff-scale