Improved support for abstract base classes with descriptors #55819
Comments
|
I posted a suggestion at python-ideas that the declaration of abstract properties could be improved in such a way that they could be declared with either the long-form or decorator syntax using the built-in property and abc.abstractmethod: {{{ class C(metaclass=ABCMeta):
@MyProperty
@abstractmethod
def x(self):
pass
@x.setter
@abstractmethod
def x(self, val):
passclass D(C):
'D does not define a concrete setter and cannot be instantiated'
@C.x.setter
def x(self):
return 1
class E(D):
'E has a concrete getter and setter, and can be instantiated'
@D.x.setter
def x(self, val):
pass
}}}It is hopefully evident that a relatively minor extension can be made to the built-in property such that @abstractproperty would no longer be needed. I have prepared a patch, complete with documentation and unit tests, but unfortunately I have not been able to test it because I have not been able to build Python from a mercurial checkout on either Ubuntu 11.04 or OS X 10.6.6 (for reasons unrelated to the patch.) BDFL thought the idea sounded good for inclusion in Python-3.3, and requested I submit the patch here. |
dsdale24
mannequin
added
stdlib
|
The discussion on python-ideas: http://mail.python.org/pipermail/python-ideas/2011-March/009411.html |
|
I looked at the patch (I didn't test it yet), my comments are on Rietveld. |
|
Here is a new patch that addresses a couple problems found in review:
|
I tried to test your patch, but the build dies with this error:
Fatal Python error: Py_Initialize: can't initialize sys standard streams
Traceback (most recent call last):
File ".../cpython/Lib/io.py", line 60, in <module>
AbortedI don't know why is this, but I get this error consistently with your patch, and no error without the patch. On Sat, Mar 19, 2011 at 22:13, <dsdale24@gmail.com> wrote:
I'm guessing you're referring to "There should be one-- and preferably only one --obvious way to do it." That is a good point. But currently the one way to:
With your proposed change the one way to:
http://docs.python.org/dev/py3k/c-api/object.html#PyObject_GetAttrString |
durban
mannequin
added
the
interpreter-core
On Sun, Mar 20, 2011 at 5:18 AM, Daniel Urban <report@bugs.python.org> wrote:
>
> Daniel Urban <urban.dani+py@gmail.com> added the comment:
>
> I tried to test your patch, but the build dies with this error:
> Fatal Python error: Py_Initialize: can't initialize sys standard streams
> Traceback (most recent call last):
> File ".../cpython/Lib/io.py", line 60, in <module>
> Aborted
>
> I don't know why is this, but I get this error consistently with your patch, and no error without the patch.
>
> On Sat, Mar 19, 2011 at 22:13, <dsdale24@gmail.com> wrote:
>> Thank you for the feedback. The reason I suggested deprecating
>> abstractproperty is that I think it is essentially broken. Subclasses
>> have to redeclare the entire property, and if they forget to declare
>> the setter for what is supposed to be a read/write property, there is
>> no way to catch it. With the new approach, it is possible to ensure
>> that all the required features of the property have been implemented.
> ...
>> On 2011/03/19 21:36:09, durban wrote:
>> > I don't think abstractproperty should be deprecated. It is still
>> > perfectly good to define a read-only abstract property (with one
>> > decorator instead of two).
>>
>> Zen of python.
>
> I'm guessing you're referring to "There should be one-- and preferably only one --obvious way to do it." That is a good point. But currently the one way to:
> - create an abstract static method: @abstractstaticmethod
> - create an abstract class method: @abstractclassmethod
> - create an abstract property: @abstractproperty (as you pointed out, this has some problems)
>
> With your proposed change the one way to:
> - create an abstract static method: @abstractstaticmethod
> - create an abstract class method: @abstractclassmethod
> - create an abstract property: @abstractmethod + @property
> This is not a very good API.Unlike methods, properties are composite objects. It is therefore
Thank you for pointing that out. I've followed up with him at
I'm familiar with that page. Do you know of any documentation |
|
I think a better idea would be to override getter and friends on the abstractproperty class. |
|
On Sun, Mar 20, 2011 at 12:19 PM, Benjamin Peterson
I just suggested the same at python-ideas. I'll work on an alternate patch. |
On Sun, Mar 20, 2011 at 5:18 AM, Daniel Urban <report@bugs.python.org> wrote:
>
> Daniel Urban <urban.dani+py@gmail.com> added the comment:
>
> I tried to test your patch, but the build dies with this error:
> Fatal Python error: Py_Initialize: can't initialize sys standard streams
> Traceback (most recent call last):
> File ".../cpython/Lib/io.py", line 60, in <module>
> Aborted
>
> I don't know why is this, but I get this error consistently with your patch, and no error without the patch.Have you added any print statements to the patch? I'm working on a |
2011/3/20 Darren Dale <report@bugs.python.org>:
>
> Darren Dale <dsdale24@gmail.com> added the comment:
>
> On Sun, Mar 20, 2011 at 5:18 AM, Daniel Urban <report@bugs.python.org> wrote:
>>
>> Daniel Urban <urban.dani+py@gmail.com> added the comment:
>>
>> I tried to test your patch, but the build dies with this error:
>> Fatal Python error: Py_Initialize: can't initialize sys standard streams
>> Traceback (most recent call last):
>> File ".../cpython/Lib/io.py", line 60, in <module>
>> Aborted
>>
>> I don't know why is this, but I get this error consistently with your patch, and no error without the patch.
>
> Have you added any print statements to the patch? I'm working on a
> completely new patch, which only touches abc.py on an existing
> python3.2 install. When I add a print statement to the abstract
> property creation routine, and run test_abc.py, I get the same error.That's likely because the io library depends on abcs, so using print |
|
Thank you Daniel and Benjamin for the helpful feedback. I think the attached patch is a much better approach. It only touches abc.abstractproperty (instead of the builtin property), and uses a class method as a factory to return instances of either property or abstractproperty, depending on whether it holds any references to abstractmethods. The patch is backwards compatible with the existing (though in my opinion, still broken) syntax of passing concrete methods to the abstractproperty constructor. I say that syntax is broken because properties are composite objects, and it is the methods that inherently make a property abstract or concrete. If one passes concrete getters and setters to abstractproperty, how do we know when the property has become concrete? Thus, I changed the documentation to refer to the more robust approach of passing abstractproperty methods that have been decorated with abstractmethod. Unit tests are also provided. I still have not been able to build from my hg checkout, but I copied abc.py into my working 3.2 installation and ran the new test_abc.py without errors. I'll be happy to address any additional concerns. |
|
Here is a new version of the patch. I think it addresses all of the issues that have been raised to date. I had to comment out the -lintl line in Modules/Setup to build on OS X, this seems to be a similar issue to http://bugs.python.org/issue6154 . So I don't have a _locale module, and I also don't have _scproxy. I ran "make test", and get the same results with and without the patch: 315 passes, 22 failed, 15 skipped. All of the failures are due to missing _locale and _scproxy, with the exception of an error during the sax test that is unrelated to my changes. |
|
(Darren, what version of OS X and what arguments did you use for ./configure ? In general, for testing purposes, a vanilla ./configure with no args should work fine for building a Python that works right from your source build directory. If you want to build something to be installed, avoid using --enable-shared on OS X, see, for instance, bpo-11445) |
|
(Ned, I'm running 10.6.6 with a 64-bit kernel. I've tried running ./configure without any arguments, and also with --prefix=/opt/local, since I install essentially everything with MacPorts.) |
|
(Darren, I'm not sure why you are running into problems with that setup. I test with what sounds to be a very similar one including a MacPorts gettext port providing libintl although I do install ports as +universal (i386, x86_64) by default. And I don't know why you would have problems with _scproxy. If you would like to pursue, please open a separate issue with the results of your configure and make.) |
|
I have an idea. How about instead of reusing abstractmethod for abstract getters and setters, you add abstractproperty.abstractgetter/setter/deleter? |
|
Benjamin: have you thought this idea through? |
|
2011/3/29 Darren Dale <report@bugs.python.org>:
Perhaps inadequately? |
|
I see some problems with this approach, but maybe I haven't fully appreciated it. Let me summarize the goals and constraints as I see them:
The current approach actually satisfies all of the goals and constraints. It fits well with the existing semantics, there are no surprises and no changes in behavior for any existing code. It is even compatible with anyone who may have used @AbstractMethod to decorate methods destined to be passed to @abstractproperty using the long-form property declaration (which would have worked even though it was not documented!) The benefit of abstractproperty.abstract{...} is that one decorator is required instead of two, right? Are there others? It is true that one could define abstract{getter,setter,deleter} decorators that would take care of setting the __isabstractmethod__ attribute on the function received, so that the @AbstractMethod decorator would not be needed *once the property has been created*. But if @AbstractMethod is discouraged in favor of abstractproperty.abstractgetter and friends, abstractproperty would have to tag each method passed to its constructor as abstract (in order to support the long-form syntax and also the initial declaration with the decorator syntax) which would actually be a change in behavior with potential consequences. For example, maybe a third party defined a concrete getter in an abstract base class, and python-3.3 can't instantiate the subclasses because that getter was automatically tagged as abstract by the new abstractproperty constructor. So @AbstractMethod would still be needed for methods passed to the constructor, meaning sometimes @AbstractMethod would be needed, and sometimes it would not. |
|
2011/3/29 Darren Dale <report@bugs.python.org>:
Mostly it doesn't create a weird asymmetry between a @abstractproperty
That's not true. The method could be tagged in @abstractgetter decorator. |
|
On Tue, Mar 29, 2011 at 9:31 PM, Benjamin Peterson
Did you read the documentation I provided in the patch? There is no class AbstractFoo(metaclass=ABCMeta):
def get_bar(self): ...
def set_bar(self, val): ...
bar = abstractproperty(get_bar, set_bar)The documentation indicates that a subclass will not be instantiable class Foo(AbstractFoo):
bar = property(AbstractFoo.get_bar, AbstractFoo.set_bar)and it is instantiable. On the other hand, for AbstractFoo to assert This patch simply extends abstractproperty so it can respect the Therefore, there is no asymmetry between when @AbstractMethod is
I think you misunderstood my point. I agreed with you that it could be (By the way, in review of bpo-11610.patch, GVR said he thought I had Darren |
On Tue, Mar 29, 2011 at 10:24 PM, Darren Dale <report@bugs.python.org> wrote:
>
> Darren Dale <dsdale24@gmail.com> added the comment:
>
> On Tue, Mar 29, 2011 at 9:31 PM, Benjamin Peterson
> <report@bugs.python.org> wrote:
>> 2011/3/29 Darren Dale <report@bugs.python.org>:
>>> The benefit of abstractproperty.abstract{...} is that one decorator is required instead of two, right? Are there others?
>>
>> Mostly it doesn't create a weird asymmetry between a @abstractproperty
>> decorated function not needing @abstractmethod but
>> @someabstractprop.setter needing it.
>
> Did you read the documentation I provided in the patch? There is no
> asymmetry, the documentation and examples provided by previous python
> releases are demonstrably inadequate. For example:
>
> class AbstractFoo(metaclass=ABCMeta):
> def get_bar(self): ...
> def set_bar(self, val): ...
> bar = abstractproperty(get_bar, set_bar)
>
> The documentation indicates that a subclass will not be instantiable
> until all of its abstract methods and properties are overridden. What
> is abstract about the bar property? Was it the getter, setter, or
> both, or neither? The answer is neither. A subclass can simply do:
>
> class Foo(AbstractFoo):
> bar = property(AbstractFoo.get_bar, AbstractFoo.set_bar)
>
> and it is instantiable. On the other hand, for AbstractFoo to assert
> that subclasses must provide concrete implementations of the get_bar
> and set_bar methods, it must decorate get_bar and set_bar with
> @abstractproperty.Sorry, that should have read @AbstractMethod. |
|
So, are there objections to this patch, or can it be merged? |
|
Is there anything preventing this patch from being merged? |
|
2011/5/14 Darren Dale <report@bugs.python.org>:
I have to make time to think about the API a bit more. |
|
On Sat, May 14, 2011 at 12:20 PM, Benjamin Peterson
Ok. Maybe you will come up with another alternative that hadn't Darrren |
|
It doesn't work with staticmethod: >>> import abc
>>>
>>> class C(metaclass=abc.ABCMeta):
... @staticmethod
... @abc.abstractmethod
... def foo(x):
... raise NotImplementedError()
...
>>> class D(C):
... @staticmethod
... def foo(x):
... return x + 1
...
>>> D()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: Can't instantiate abstract class D with abstract methods foo.__func__
>>> |
On Sat, Jun 11, 2011 at 3:11 AM, Daniel Urban <report@bugs.python.org> wrote:
>
> Daniel Urban <urban.dani+py@gmail.com> added the comment:
>
> It doesn't work with staticmethod:
>
>>>> import abc
>>>>
>>>> class C(metaclass=abc.ABCMeta):
> ... @staticmethod
> ... @abc.abstractmethod
> ... def foo(x):
> ... raise NotImplementedError()
> ...
>>>> class D(C):
> ... @staticmethod
> ... def foo(x):
> ... return x + 1
> ...
>>>> D()
> Traceback (most recent call last):
> File "<stdin>", line 1, in <module>
> TypeError: Can't instantiate abstract class D with abstract methods foo.__func__You still need to use @abc.abstractstaticmethod. |
[...]
>> Traceback (most recent call last):
>> File "<stdin>", line 1, in <module>
>> TypeError: Can't instantiate abstract class D with abstract methods foo.__func__
>
> You still need to use @abc.abstractstaticmethod.Thinking about this some more, I think the error you found should be You helped bring up another point: all functions are descriptors, |
|
inspect.getattr_static has the necessary logic to search for descriptors without invoking them. However, it may be better to revert to the idea of pushing this functionality back onto the individual descriptors and have the problematic descriptors like property and staticmethod simply implement __isabstractmethod__ as a property. property: staticmethod/classmethod: @property
def __isabstractmethod__(self):
return self.__func__.__isabstractmethod__With this approach, the "one true way" to handle abstract descriptors would be to do: #instance method
@abstractmethod
def f(self):
...
@property
@abstractmethod
def f(self):
...
@classmethod
@abstractmethod
def f(self):
...
@staticmethod
@abstractmethod
def f(self):
...This wouldn't allow for the prettier error messages, but it's much cleaner than having ABCMeta trawling through class attribute dir() lists. |
|
On Sat, Jun 11, 2011 at 8:55 AM, Nick Coghlan <report@bugs.python.org> wrote:
Unfortunately, we can't import inspect, even inside ABCMeta.__new__.
That's a good idea.
Those classes could conceivably do: @property
def __abstractmethods__(self):
return ("...", ...)It would not be required, but if ABCMeta found it, it could use it. Darren |
|
[...]
I think there is another reason to do it this way. Suppose I have a class C:
@property
@abstractmethod
def foo(self): return 1
class D:
@MyProperty
def foo(self): return 2if ABCMeta determines C.foo.fget is abstract, it will not recognize The downside to making __isabstractmethod__ a property is that if C Darren |
|
Perhaps rather than changing ABCMeta, provide a base descriptor class that has __isabstractmethod__ implemented to calculate the abstractness. Then property could use that, as could any of the other relevant descriptors we have around. The __isabstractmethod__ attribute of the descriptor would itself need to be a data-descriptor (which property() is). That would ensure that __isabstractmethod__ is not set directly on the descriptor instance. I agree with Nick that it may be better to have the descriptor classes take care of managing __isabstractmethod__ themselves, instead of having ABCMeta compute it. Special-casing ABCMeta to handle custom __isabstractmethod__ calculation for any specific type seems like something we should avoid. Per your last message, if a specific descriptor has an abstract setter then the descriptor should be considered abstract. If the implementation of that attribute is not a descriptor should it raise a TypeError? If it is a descriptor but it does not have a setter, should it likewise fail? I'm not so sure. We already don't enforce types on abstract attributes. You don't have to implement an abstractproperty with a property, an abstractstaticmethod with a staticmethod, nor an abstractclassmethod with a classmethod. For that matter you don't have to implement an abstractmethod with a function. It just has to be an object bound to the same name. Should descriptors get special treatment over any other type that implements __isabstractmethod__? That brings up a concern of mine. I've found the name abstractmethod (specifically the "method" part) to be misleading. Any attribute can be "abstract" and ABCMeta only cares about __isabstractmethod__. Maybe having "method" in the name is meant to imply the expected use case? I would rather they were called __isabstractattribute__, and __abstractattributes__, which would be less confusing when using ABCs, particularly at first. Having "attribute" in the name is nice, since it makes it clear we're talking about attributes, rather than other abstraction contexts. Having a specific abstractmethod decorator is still good since we only decorate functions in an ABC. I'm +1 for deprecating abstractproperty and really all the decorators except abstractmethod (if the use cases were addressed). If abstractmethod were smart about on which object it sets __isabstractmethod__, the other decorators would be unnecessary; and the order in which you decorate would not matter as much. The other decorators could become simple wrappers around abstractmethod if we felt the need to keep them around. It's nice that the decorators say with what you are expecting to replace them, but you can get that by using the corresponding normal decorators. |
|
Remember the goal here is *not* to completely eliminate the need to test that objects implement an ABC correctly. It's to make it easier to declare the expected interface in a way that helps readers of the ABC definition to figure out what is going on, and to reduce the proliferation of abstract descriptors. Since we made a deliberate choice not to do signature checks when ABCs were added to the language, there's nothing an ABC can do to stop someone (for example) overriding an abstract method or descriptor "foo" with "foo = 1". That's almost certainly wrong, but it's wrong at a level that ABCs don't care about. If someone incorrectly overrides a read/write property with a read-only property and there's nothing in their test suite that picks that up, then that's a flaw in the test suite, not a flaw in the ABC itself. Regarding the naming, @AbstractMethod and __isabstractmethod__ are definitely about methods, or method-based descriptors (such as property). There *could* be a case to be made to allow for abstract data attributes, but really, using an abstract property should cover that use case well enough. |
|
Didn't mean to sidetrack. :) I really appreciate the effort Darren has put into this. |
I’ve find it useful to use an abstractproperty to specify an attribute that concrete subclasses have to define. Was that wrong? From a technical viewpoint, I replaced a method with a data attribute, but from a doc/human viewpoint, replacing a property with a regular attribute did not seem wrong to me. So, if there are guidelines about “almost certainly wrong” uses of the ABC machinery, they should IMO be documented. |
|
In that paragraph, I was only talking about cases where "foo = 1" *isn't* a valid override (which, I hope you'll agree, it typically won't be). Your described approach of declaring an abstract property and then overriding it with an ordinary class attribute is part of the answer I gave Eric in pointing out why a separate concept of an abstract attribute isn't really necessary. |
|
On Sat, Jun 11, 2011 at 7:32 PM, Eric Snow <report@bugs.python.org> wrote:
Consider a framework like Enthought's Traits or Riverbank Computing's class AbstractDescriptor(metaclass=abc.ABCMeta):
@property
@abc.abstractmethod
def __abstractmethods__(self):
# it would be nice if descriptors new their own names here,
# __abstractmethods__ could return: ('bar.fget', 'bar.fset')
return frozenset(m for m in ('fget', 'fset', 'fdel')
if getattr(getattr(self, m, None),
'__isabstractmethod__', False))
@property
def __isabstractmethod__(self):
return True if self.__abstractmethods__
AbstractDescriptor.register(property)Of course, not all descriptors would be required to derive from Having said all that, I think the above suggestion including The inspect module or something like it may still be needed in ABCMeta (aside: unlike ABCMeta.__new__, ABCMeta.register makes no attempt to My work is going to keep me pretty busy over the next three weeks, and Darren |
|
Non-conformant explicit registration is permitted on purpose to allow developers to only supply partial implementations when it is known that that is all a given application requires. Extremely impure, but quite practical :) Note that the core logic of inspect.getattr_static isn't all that complicated. If necessary, it could be moved inside the module with ABCMeta and then wrapped or reference by the inspect module. |
|
Here is attempt #4. This patch extends the property, classmethod and staticmethod builtins with an __isabstractmethod__ descriptor. Docs and tests are updated as well. "make test" runs without failures. This is my first real attempt with the C-API, and I think I am finally over the learning curve, but comments would be greatly appreciated. |
|
I've posted some comments on Rietveld. |
|
I've requested additional feedback based on comments at Rietveld. |
|
Here is a new version of the patch, addressing points raised in the review of the previous version. |
|
Any additional comments? |
|
It would be nice if this patch could be merged in time for python-3.3... |
|
I noticed this in an older message:
3.3 is still months away; there’s time for your patch. If nobody replies here, ping python-dev again. |
|
I just double-checked that the unit tests do not raise any warnings with this patch. Can it be merged? |
|
I'll bump this one last time. |
|
Here is a new patch addressing comments raised in review. It supersedes previous patch submissions. |
|
Patch addressing latest comments in review. Notable change: defines _PyObject_IsAbstract in object.c/object.h, rather than repeating the code in multiple files and functions. |
|
Added some review comments in Reitveld - core functionality changes look good, but there are some other aspects that need addressing before the patch will be good to go (primarily relating to only doing a minimal documented deprecation of the legacy APIs without actually harming their documentation, testing or functionality). |
|
New patch addressing comments in review. |
|
Is this patch ready to go? I haven't heard any feedback on the most recent version. |
|
New changeset e979b26a9172 by Benjamin Peterson in branch 'default': |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: