Can we improve support for abstract base classes with desciptors
I would like to try to address some shortfalls with the way python deals with abstract base classes containing descriptors. I originally was just concerned with improving support for defining abstract properties with the decorator syntax and converting between abstract and concrete properties, but recently realized that the problem extends to descriptors in general. ABCs ---- First, a bit of background may be in order. An abstract base class is defined by specifying its metaclass as ABCMeta (or a subclass thereof):: class MyABC(metaclass=ABCMeta): @abstractmethod def foo(self): pass When trying to instantiate MyABC or any of its subclasses, ABCMeta inspects the current class namespace for items tagged with __isabstractmethod__=True:: class ABCMeta(type): #[...] def __new__(mcls, name, bases, namespace): cls = super().__new__(mcls, name, bases, namespace) # Compute set of abstract method names abstracts = {name for name, value in namespace.items() if getattr(value, "__isabstractmethod__", False)} ABCMeta then checks if any of the base classes define any items tagged with __isabstractmethod__ and whether they remain abstract in the current class namespace:: for base in bases: for name in getattr(base, "__abstractmethods__", set()): value = getattr(cls, name, None) if getattr(value, "__isabstractmethod__", False): abstracts.add(name) cls.__abstractmethods__ = frozenset(abstracts) In Objects/typeobject.c, __abstractmethods__ is actually a descriptor, and setting it gives the type a chance to set an internal flag specifying if it has any abstract methods defined. When object_new is called in typeobject.c, the flag is checked and an error is raised if any abstract methods were identified. Issues with ABCs and descriptors -------------------------------- In order for this scheme to work, ABCMeta needs to identify all of the abstract methods, but there are some limitations when we consider descriptors. For example, Python's property is a composite object, whose behavior is defined by the getter, setter, and deleter methods with which it is composed. Since there is already an @abstractmethod decorator, I would have suspected that defining abstract properties would be intuitive:: class MyABC(metaclass=ABCMeta): @abstractmethod def _get_foo(self): pass @abstractmethod def _set_foo(self, val): pass foo = property(_get_foo, _set_foo) @property @abstractmethod def bar(self): pass @bar.setter @abstractmethod def bar(self, val): pass Ideally, one would want the flexibility of defining a concrete getter and an abstract setter, for example. However, ABCMeta does not inspect the descriptors of a class to see if they contain any abstract methods. It only inspects the descriptor itself for a True __isabstractmethod__ attribute. This places the burdon on every descriptor implementation to provide its own support for ABC compatibility. For example, support for abstract properties was attempted by adding abstractproperty to the abc module. abstractproperty subclasses the property builtin (as opposed to the relationship between every other abstract and concrete class in the python language). Here is the definition of abstractproperty, in its entirety (modulo docstrings):: class abstractproperty(property): __isabstractmethod__ = True A number of problems manifest with this approach, and I think they all can be traced to the fact that the abstractedness of a descriptor is currently not dependent upon the abstractedness of the methods with which it is composed. The documentation for abstractproperty doesn't suggest using @abstractmethod:: class C(metaclass=ABCMeta): def getx(self): ... def setx(self, value): ... x = abstractproperty(getx, setx) which leads to Issue #1: What is abstract about C.x? How does a subclass of C know whether it needs to override the getter or setter? Issue #2: The decorator syntax cannot be used to convert an abstract property into a concrete one. (This relates to Issue #1: how would a descriptor even know when such a conversion would be appropriate?) Running the following code:: from abc import ABCMeta, abstractmethod, abstractproperty class AbstractFoo(metaclass=ABCMeta): @abstractproperty def bar(self): return 1 @bar.setter def bar(self, val): pass class ConcreteFoo(AbstractFoo): @AbstractFoo.bar.getter def bar(self): return 1 @bar.setter def bar(self, val): pass foo = ConcreteFoo() yields:: TypeError: Can't instantiate abstract class ConcreteFoo with abstract methods bar Issue #3: The following class *is* instantiable, even though AbstractFoo declared that a setter for bar is required:: class ConcreteFoo(AbstractFoo): @property def bar(self): pass Previous attempt to improve abc.abstractproperty ------------------------------------------------ It seems to me that the strategy used by abc.abstractproperty is fundamentally ill-advised. I explored the possibility of extending abstractproperty, redefining its getter, setter, and deleter methods such that they would work in conjunction with the @abstractmethod decorator and yield an instance of the builtin property once all abstract methods were replaced with concrete ones (http://bugs.python.org/issue11610). Issues #1 and #2 were addressed, but there were still problems with that approach. It did not address Issue #3, and it also introduced a new issue, #4:: class AbstractFoo(metaclass=ABCMeta): @abstractproperty # bar would be an abstractproperty, even though the getter is concrete def bar(self): return 1 @bar.setter # bar.setter inspected the getter and the new setter, did not identify # any abstract methods, and thus returned an instance of the built-in # property def bar(self, val): pass @bar.deleter # bar is a concrete property, its deleter decorator does not know it # is supposed to check for abstract methods, so it will return an # instance of the built-in property: @abstractmethod def bar(self): pass By the time the deleter was specified, bar was a concrete property, which does not know it should return an instance of abstractproperty (in part because the inheritance diagram for property/abstractproperty is inverted). Thus, AbstractFoo was instantiable, even though it shouldn't be. Finally, issue #5: the current approach taken by ABCMeta and abstractproperty places the burdon on descriptors to identify themselves to ABCMeta as abstract. Considering the issues encountered with abstractproperty, this may be an onerous requirement. There has been a fair amount of discussion at http://bugs.python.org/issue11610 , which can be summarized as a) concerns about maintaining backward compatibility, and b) objections to requiring @abstractmethod to specify that a method being passed to abstractproperty is abstract. Extending ABCMeta: A Promising Way Forward ------------------------------------------ I think the key is to focus on Issue #3. ABCMeta needs to be improved to recognize descriptor objects, both in the current namespace as well as the base classes, and to identify any abstract methods associated with the descriptors. I suggest the following approach in ABCMeta:: def __new__(mcls, name, bases, namespace): cls = super().__new__(mcls, name, bases, namespace) # Compute set of abstract method names def isdescriptor(val): return hasattr(val, '__get__') or hasattr(val, '__set__') \ or hasattr(val, '__delete__') def getabstracts(ns): return [name for name, value in ns.items() if getattr(value, "__isabstractmethod__", False)] abstracts = getabstracts(namespace) for item, val in namespace.items(): # further inspect descriptors for abstract methods: if isdescriptor(val): ## unfortunately, can't import inspect: #from inspect import getmembers #d = dict(getmembers(val)) ## so instead, use the following: d = dict((k, getattr(val, k, None)) for k in dir(val)) for name in getabstracts(d): # add the abstract descriptor methods to the list: abstracts.append('%s.%s'%(item, name)) for base in bases: for name in getattr(base, "__abstractmethods__", set()): if '.' in name: # base class identified a descriptor abstract method: k, v = name.split('.') desc = getattr(cls, k, None) val = getattr(desc, v, None) else: val = getattr(cls, name, None) if val is None or getattr(val, "__isabstractmethod__", False): abstracts.append(name) cls.__abstractmethods__ = frozenset(abstracts) I rolled everything into __new__ just to keep it as simple as possible for the sake of discussion. Python already provides the rest of the framework needed for descriptors to work properly with ABCs. This implementation actually works; I've tested it with an existing python-3.2 install:: from abc import ABCMeta, abstractmethod class AbstractFoo(metaclass=ABCMeta): @property @abstractmethod def bar(self): return 1 @bar.setter @abstractmethod def bar(self, val): pass >>> abstractfoo = AbstractFoo() Traceback (most recent call last): File "temp.py", line 17, in <module> abstractfoo = AbstractFoo() TypeError: Can't instantiate abstract class AbstractFoo with abstract methods bar.fget, bar.fset as expected. Note the more informative error message indicating what about the bar property is abstract. Also:: class ConcreteFoo(AbstractFoo): @AbstractFoo.bar.getter def bar(self): return 1 >>> foo = ConcreteFoo() Traceback (most recent call last): File "temp.py", line 24, in <module> foo = ConcreteFoo() TypeError: Can't instantiate abstract class ConcreteFoo with abstract methods bar.fset So issue #1 is addressed, since we are explicitly specifying which descriptor methods are abstract. Issue #2 has been addressed, since the following class is instantiable:: class ConcreteFoo(AbstractFoo): @AbstractFoo.bar.getter def bar(self): return 1 @bar.setter def bar(self, val): pass Issue #3 is also addressed. In the following example, even though I redefine the bar property as a readonly property, ABCMeta recognizes that a setter is needed:: class ConcreteFoo(AbstractFoo): @property def bar(self): return 1 >>> foo = ConcreteFoo() Traceback (most recent call last): File "temp.py", line 24, in <module> foo = ConcreteFoo() TypeError: Can't instantiate abstract class ConcreteFoo with abstract methods bar.fset Issue #4 (introduced in a previous attempt to solve the problem using abstractproperty) is also addressed:: class AbstractFoo(metaclass=ABCMeta): @property def bar(self): return 1 @bar.setter def bar(self, val): pass @bar.deleter @abstractmethod def bar(self): pass >>> abstractfoo = AbstractFoo() Traceback (most recent call last): File "temp.py", line 15, in <module> abstractfoo = AbstractFoo() TypeError: Can't instantiate abstract class AbstractFoo with abstract methods bar.fdel Finally, this approach addresses Issue #5 by holding ABCMeta responsible for identifying the abstractedness of descriptor methods, rather than placing that burdon on the desciptor objects to identify themselves as abstract. If ABCMeta were extended as above to identify abstract methods associated with descriptors, third parties would simply decorate methods used to compose the descriptors with @abstractmethod. This change to ABCMeta would not effect the behavior of abstractproperty, so backward compatibility would be maintained in that respect. But I think abstractproperty should be deprecated, or at the very least removed from the documentation. The documentation for @abstractmethod in >=python-3.3 should be extended to provide examples with properties/descriptors. The syntax would be backward compatible with older python versions, but with <python-3.3 ABCMeta would simply not recognize descriptors' abstract methods. This leads to one source of potential forward compatibility:: class AbstractFoo(metaclass=ABCMeta): @property @abstractmethod def bar(self): return 1 class ConcreteFoo(AbstractFoo): pass Both above classes would be instantiable with <python-3.3, but not with
=python3.3. In my opinion, this is a feature: python-3.3 has identified a bug in ConcreteFoo. The developer would not have tagged that method as abstract unless they had intended for consumers of AbstractFoo to provide a concrete implementation.
Darren
On Thu, Jun 9, 2011 at 1:01 AM, Darren Dale <dsdale24@gmail.com> wrote: [snip excellent analysis of the problem] I have some suggestions regarding a few details of your current code, but your basic proposal looks sound to me. I would tweak __new__ along the following lines though: def __new__(mcls, name, bases, namespace): cls = super().__new__(mcls, name, bases, namespace) # Compute set of abstract method names # CHANGE 1: refactor descriptor and abstract method scan to happen in a single pass def is_descriptor(value): return (hasattr(value, '__get__') or hasattr(value, '__set__') or hasattr(value, '__delete__')) def is_abstract(value): return getattr(value, "__isabstractmethod__", False) def get_abstract_names_for_item(item): name, value = item if is_abstract(value): return [name] elif is_descriptor(value): # CHANGE 2: Deliberately ignore descriptors on the descriptor objects # CHANGE 3: Use new-style string formatting return ['{}.{}'.format(name, attr) for attr in dir(value) if is_abstract(getattr(value, attr))] return [] def get_abstract_names(ns): names = [] for item in ns.items(): names.extend(get_abstract_names_for_item(item)) return names abstract_names = get_abstract_names(namespace.items()) for base in bases: for name in getattr(base, "__abstractmethods__", ()): # CHANGE 4: Using rpartition better tolerates weird naming in the metaclass # (weird naming in descriptors will still blow up in the earlier search for abstract names) descr_name, is_descr, attr = name.rpartition('.') if is_descr: # base class identified a descriptor abstract method: descr = getattr(cls, descr_name, None) val = getattr(descr, attr, None) else: val = getattr(cls, name, None) if val is None or is_abstract(val): abstract_names.append(name) cls.__abstractmethods__ = frozenset(abstract_names) Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
On Wed, Jun 8, 2011 at 11:55 AM, Nick Coghlan <ncoghlan@gmail.com> wrote:
On Thu, Jun 9, 2011 at 1:01 AM, Darren Dale <dsdale24@gmail.com> wrote: [snip excellent analysis of the problem]
I have some suggestions regarding a few details of your current code, but your basic proposal looks sound to me.
I would tweak __new__ along the following lines though: [snip]
Thank you, I agree. Concerning the following block:
def get_abstract_names(ns): names = [] for item in ns.items(): names.extend(get_abstract_names_for_item(item)) return names
abstract_names = get_abstract_names(namespace.items())
That should be "get_abstract_names(namespace)", since ns.items() gets called again in the for loop. I think the get_abstract_names function isn't needed though, since it is only ever called that one time. Any reason not replace the above block with:: abstract_names = [] for item in namespace.items(): abstract_names.extend(get_abstract_names_for_item(item))
for base in bases: for name in getattr(base, "__abstractmethods__", ()): # CHANGE 4: Using rpartition better tolerates weird naming in the metaclass # (weird naming in descriptors will still blow up in the earlier search for abstract names)
Could you provide an example of weird naming? Darren
On Thu, Jun 9, 2011 at 8:51 AM, Darren Dale <dsdale24@gmail.com> wrote:
That should be "get_abstract_names(namespace)", since ns.items() gets called again in the for loop. I think the get_abstract_names function isn't needed though, since it is only ever called that one time. Any reason not replace the above block with::
abstract_names = [] for item in namespace.items(): abstract_names.extend(get_abstract_names_for_item(item))
Nope, inlining that part makes sense.
for base in bases: for name in getattr(base, "__abstractmethods__", ()): # CHANGE 4: Using rpartition better tolerates weird naming in the metaclass # (weird naming in descriptors will still blow up in the earlier search for abstract names)
Could you provide an example of weird naming?
class C(object): ... pass ... setattr(C, 'weird.name', staticmethod(int)) c = C() c.weird.name Traceback (most recent call last): File "<stdin>", line 1, in <module> AttributeError: 'C' object has no attribute 'weird' c.weird.name Traceback (most recent call last): File "<stdin>", line 1, in <module> AttributeError: 'C' object has no attribute 'weird' getattr(c, 'weird.name')() 0
This is definitely something that could legitimately be dismissed as "well, don't do that then" (particularly since similarly weird names on the descriptors will still break). However, I also prefer the way partition based code reads over split-based code, so I still like the modified version. Full tolerance for weird naming would require storing 2-tuples in __abstractmethods__ which would cause a whole new set of problems and isn't worth the hassle. Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
On Wed, Jun 8, 2011 at 10:01 PM, Nick Coghlan <ncoghlan@gmail.com> wrote:
On Thu, Jun 9, 2011 at 8:51 AM, Darren Dale <dsdale24@gmail.com> wrote:
for base in bases: for name in getattr(base, "__abstractmethods__", ()): # CHANGE 4: Using rpartition better tolerates weird naming in the metaclass # (weird naming in descriptors will still blow up in the earlier search for abstract names)
Could you provide an example of weird naming?
class C(object): ... pass ... setattr(C, 'weird.name', staticmethod(int)) [...]
This is definitely something that could legitimately be dismissed as "well, don't do that then" (particularly since similarly weird names on the descriptors will still break). However, I also prefer the way partition based code reads over split-based code, so I still like the modified version.
Yes, I like your modified version as well. I just wanted to understand your concern, since it had never occurred to me to try something like "setattr(C, 'pathological.name', ...)".
Full tolerance for weird naming would require storing 2-tuples in __abstractmethods__ which would cause a whole new set of problems and isn't worth the hassle.
I'm glad you feel that way. I'll work on a patch that includes docs and unit tests and post it at http://bugs.python.org/issue11610. What do you think about deprecating abstractproperty, or removing it from the documentation? Darren
On Thu, Jun 9, 2011 at 9:45 PM, Darren Dale <dsdale24@gmail.com> wrote:
What do you think about deprecating abstractproperty, or removing it from the documentation?
Unless anyone specifically howls at the idea, +1 to both (since abstractproperty doesn't actually achieve the intended purpose and will become redundant with the proper fix in 3.3). Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
participants (2)
-
Darren Dale
-
Nick Coghlan