classification
Title: add update_abstractmethods function to update an ABC's abstract methods
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.10
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: avrahami.ben, bar.harel, eric.smith, gvanrossum, python-dev, rhettinger
Priority: normal Keywords: patch

Created on 2020-10-01 15:54 by avrahami.ben, last changed 2020-10-06 17:41 by gvanrossum. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 22485 merged python-dev, 2020-10-01 20:58
Messages (23)
msg377769 - (view) Author: Ben Avrahami (avrahami.ben) * Date: 2020-10-01 15:54
python-ideas discussion:
https://mail.python.org/archives/list/python-ideas@python.org/thread/6BNJ3YSEBPHEPGXSAZGBW3TJ64ZGZIHE/

In order to allow "decorator mixins" (most notably dataclass and total_ordering) to implement ABCs, new functionality is needed. I propose a new python function in `abc.py` called `update_abstractmethods`. The function will accept a class and, if the class is an instance of ABCMeta, will update the class's `__abstractmethods__` attribute to not include implemented attributes, and to include new abstractmethods (proposed implementation in thread).

Both dataclass and total_ordering will be modified to call this function on the subject class before returning it, and 3rd-party libraries which implement mixin decorators (like attrs) will be to do the same.

Also, the function can be used as a decorator individually, this is especially useful in cases where 3rd party decorators do not call the function.
msg377782 - (view) Author: Ben Avrahami (avrahami.ben) * Date: 2020-10-01 21:57
for the functionality to work, `total_ordering` needs to change to also override abstract methods. I believe this is an OK change since total_ordering implicitly dictates that the comparison methods are interchangable. Thus, implementing some comparison operators, but leaving others unchanged is undesired behaviour.
msg377790 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2020-10-01 23:58
The downside of this proposal is that it is tightly coupled with class creation process.  It requires all existing class decorators, metaclasses, and code generators to start caring about something that isn't part of their core functionality (very few tools are ABC aware).

I'm usually dubious about proposed solutions to problems that 1) rarely occur in practice and 2) require everyone else to change their code.

Am not sure why total_ordering was mentioned?  No current collections ABC requires the ordering methods and hand-rolled numeric classes typically don't use total_ordering because it is simpler, faster, and cleaner to implement the methods directly.
msg377802 - (view) Author: Ben Avrahami (avrahami.ben) * Date: 2020-10-02 08:04
Good points all, that I will try to address one by one:

Firstly, this function is by no means mandatory for "post-creation mixins". Such tools can still be used without calling it (and remain ABC unaware), with absolutely no change in functionality. Indeed, the function itself can be used as a decorator for just such cases, where a tool is ABC unaware (either from deciding that ABC aren't a use case for them, or from simply not considering that possibility), but the user wants to also subclass an ABC. The problem is that, right now, post-creation tools simply can't be ABC aware.

The thought that few tools are ABC aware is not, I think, a good reason to neglect tools that would like to be. Just as dispatch and type-routing tools might not be ABC aware to use `abc.get_cache_token()`, but it is still added for tools that want to be ABC aware and sidestep this confusing behavior.

As for not occurring in practice: a simple github search reveals many instances of comparison operators being abstract, and while likely few of them likely use dataclasses, that is a possibility that I believe should be addressed (of course, we could decide that this is desired behavior, and scrap this whole issue).

Finally, total_ordering: I originally added it to the issue because it was the other decorator mixin in the standard library, after I rand into this behavior using dataclasses. If total_ordering proves to be a barrier, we can remove it from the PR (and perhaps re-introduce it later if that is deemed necessary). I will only remark that I find total_ordering used in many hand-rolled classes where users would sacrifice some performance for cleaner code and an assertion that total order is enforced.

Sorry for the scroll and thanks for the scrutiny :-)
msg377820 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2020-10-02 16:27
You might be misunderstanding what @total_ordering does.  It can't be used by subclasses to override abstract methods.

>>> from abc import abstractmethod, ABC
>>> from functools import total_ordering

>>> class X(ABC):
	@abstractmethod
	def __lt__(self, other):
		raise RuntimeError('abstract called')

>>> @total_ordering
class Y(X):
    def __le__(self, other):
        return True

>>> sorted(vars(Y))    # note that __lt__ is missing and Y() won't instantiate
['__abstractmethods__', '__doc__', '__ge__', '__gt__', '__le__', '__module__', '_abc_impl']
msg377822 - (view) Author: Ben Avrahami (avrahami.ben) * Date: 2020-10-02 16:38
This is a behavior that the PR changes. total_ordering should be able to override/implement abstract methods (in my opinion). If this ends up a strickling point then we can exclude total_ordering from this issue.

Regardless, I think that this behavior is extremely unintuitive.
msg377839 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2020-10-02 23:20
I'm going to ignore this issue until you two have reached agreement. I recommend using some example code.
msg377850 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2020-10-03 03:48
> I'm going to ignore this issue until you two have reached agreement.
> I recommend using some example code.

That won't be necessary.  I now understand what the OP is trying to do and am going to recommend against it.  

Guido, this really a decision for you to make.  Formerly ABC logic existed independent of any other class logic.  Nothing else in the standard library or in third-party tooling was required to take it into account.  A developer could leave or take it if they pleased.

The OP is proposing much tighter coupling and interdependence.  Effectively, he wants two new general rules to apply pervasively to existing tooling that previously had no reason to interact with ABCs at all.

1) When checking to see if a method exists, perhaps by using hasattr(), it should exclude abstract methods as determined by a follow-up getattr() call.

2) If a tool dynamically adds a method, it has duty to call update_abstractmethods() if even a slim possibility exists that ABCs are being used.

Presumably, none of this is unique to the total_ordering() decorator and it would apply to anything that dynamically inspects or updates classes whether by direct call, by class decorator, or by metaclass.

My recommendation is to not go down this path.  In the particular case of total_ordering(), the value add is slightly above zero.  In the decade long history of ABCs and the total_ordering() decorator, the need for this has arisen zero times.

P.S.  In the standard library, only the numbers module has ABCs with abstract rich comparison methods.  Typically, these would be used by registering the ABC rather than by inheriting from it.  That may be one reason why this hasn't come up.
msg377851 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2020-10-03 04:55
I still like to have a helper that recalculates the abstractness status of a class after adding some new methods (or deleting some).

I would prefer the isinstance(cls, ABCMeta) check to be inside that helper, so that you could call it unconditionally after adding/deleting methods: abc.update_abstractmethods(cls).  (It doesn't really need a comment saying "Update abstract methods" either. :-)

In fact, its signature makes the helper feasible as a class decorator itself, so that users who are using a mixin class decorator that doesn't update abstractness can add it conveniently themselvs. E.g. suppose @total_ordering didn't make this call itself, then the user could write

@abc.update_abstractmethods
@functools.total_ordering
class MyClass(SomeAbstractBaseClass):
    def __lt__(self, other):
        whatever

But I think it would be nice if @total_ordering and @dataclass did include this call.

However, I don't see why @total_ordering needs to be changed to also override abstract methods *defined in the decorated class*.  If I write

@functools.total_ordering
class MyAbstractClass(abc.ABC):
    @abc.abstractmethod
    def __lt__(self, other):
        return NotImplemented

it's totally clear to me what @total_ordering should do -- it should define __le__, __gt__ and __ge__ in terms of __lt__, and leave __lt__ alone, for some subclass to implement.

With these two amendments, the changes to dataclasses.py and functools.py are limited to two lines (adding the call to abc.update_abstractmethod(cls)), plus an import change.
msg377854 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2020-10-03 05:50
> it's totally clear to me what @total_ordering should do 
> -- it should define __le__, __gt__ and __ge__ in terms 
> of __lt__, and leave __lt__ alone, for some subclass to 
> implement.

+1
msg377861 - (view) Author: Ben Avrahami (avrahami.ben) * Date: 2020-10-03 08:17
> I would prefer the isinstance(cls, ABCMeta) check to be inside that helper

I had a little debate about this in my mind, I'll change it.

> it's totally clear to me what @total_ordering should do -- it should define __le__, __gt__ and __ge__ in terms of __lt__, and leave __lt__ alone, for some subclass to implement.

Implementing this logic would require more than two lines. I will add it to the PR.
msg377886 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2020-10-03 15:19
Why would that require more code? That’s already how it worked. Maybe you misunderstand what I tried to say?
msg377887 - (view) Author: Ben Avrahami (avrahami.ben) * Date: 2020-10-03 15:29
> Maybe you misunderstand what I tried to say?

Possibly, right now, total_ordering avoids overriding any method that is declared, either in the class or its superclasses (except for object), regardless of whether or not it is abstract. For it to be ABC-aware, it would need the ability to override abstract methods defined in superclasses, so we need to check whether an exisitng method is abstract.

Additionally we want to not override abstract methods defined in the subject class, so we also need to check whether the method is defined in the class __dict__ (that's the extra logic I was referring to).

An argument could be made that total_ordering should override any method not defined in the subject class, abstract or no (implementing this logic would also speed up total_ordering, but that's beside the point).

Is this what you meant?
msg377888 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2020-10-03 15:43
Ah, okay. I wasn't familiar with @total_ordering so I assumed differently.  Sorry for the continued misunderstanding.

Perhaps we should just leave it alone completely then, since ISTM that any change to its implementation may cause subtle bugs in currently working code.

The problem you see only occurs when there are abstract ordering methods present. The cure is simple, just implement all four comparison functions (and do away with @total_ordering). This is not a great burden. Instead we should probably add a line or two to the docs explaining how it interacts with ABCs defining abstract ordering methods.

@dataclass uses a different approach ("is it defined in the current class?"). That is perfectly ready for a call to abc.update_abstractmethods(). (I checked, and dataclasses already depends on abc, indirectly via functools, so the extra import is free.)
msg377907 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2020-10-04 00:16
Guido, can the method update be made automatic?  When instantiation fails, recheck to see the missing abstract methods had been defined?

    >>> from abc import *
    >>> class P(ABC):
            @abstractmethod
            def m(self):
                    pass
            
    >>> class C(P):
            pass

    >>> C.m = lambda self: None
    >>> C()
    Traceback (most recent call last):
      File "<pyshell#11>", line 1, in <module>
        C()
    TypeError: Can't instantiate abstract class C with abstract method m

The latter message doesn't make sense that it should occur at all.

Roughly, the existing instantiation logic is:

     if cls.__abstractmethods__:
         raise TypeError(f"TypeError: Can't instantiate abstract class
                         {cls.__name} with abstract method {methname}")

Could that be changed to something like this:

     if cls.__abstractmethods__:
         for methname is cls.__abstractmethods__.copy():
             if getattr(cls, methname).__isabstractmethod__:
                 raise TypeError(f"TypeError: Can't instantiate abstract class
                                  {cls.__name} with abstract method {methname}")
             cls.__abstractmethods__.remove(methname)

I haven't thought this through.  Was just thinking that it would nice to have automatic updates rather than transferring the responsibility outside of the core ABC logic.
msg377908 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2020-10-04 00:43
Hm, you're effectively proposing to compute (or update) __abstractmethods__ lazily. There are a number of subtleties with that, e.g. __abstractmethods__ is currently a frozenset, and a base class's __abstractmethods__ is used when computing the __abstractmethods__ of new subclasses.

Moreover, the instantiation check is in object_new(), in typeobject.c.

The good news is that when you assign an empty iterable to cls.__abstractmethod__ the bit that object_new() checks is cleared, so other approaches are still open.

I think adding the API currently proposed is a reasonable compromise.
msg377914 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2020-10-04 01:16
> you're effectively proposing to compute (or update)
> __abstractmethods__ lazily.

Would it be possible to do the update whenever the class dictionary is modified, in PyType_Modified() perhaps (using logic similar to __mro__ updates__)?  

I not pushing for this; am just asking whether it is possible to make sure that __abstractmethods__ is always in sync with reality.
msg377915 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2020-10-04 01:39
Yes, we could override ABCMeta.__setattr__ to do that. Though it seems this
class is considered performance sensitive (else why have Modules/_abc.c) we
would probably have to write it in C.--
--Guido (mobile)
msg377923 - (view) Author: Ben Avrahami (avrahami.ben) * Date: 2020-10-04 07:46
Implementing this behavior automatically would be more complicated since you would also have to take subclasses into account. Current implementation enforces that the class is unused (as much as can be reasonably done) when its abstraction status is re-calculated, such checks cannot be done in an automatic implementation.

The question becomes whether or not we think its reasonable that an abstract class would change its abstraction status throughout its lifetime. I'd argue "not implicitly".
msg377956 - (view) Author: Bar Harel (bar.harel) * Date: 2020-10-04 16:15
> When instantiation fails, recheck to see the missing abstract methods had been defined?

I've written about it in the python-ideas discussion as well, and completely agree.

There is no other approach that would guarantee correctness across all APIs, both internal and 3rd party. Adding a function to recalculate will require everyone to use it, and worse, know that it even exists.

The overhead should be quite low as this case is probably rare and the recalculation can happen only on failure.

I'm not sure however, what are the implications of changing Py_TPFLAGS_IS_ABSTRACT during type creation.
msg377969 - (view) Author: Ben Avrahami (avrahami.ben) * Date: 2020-10-04 18:22
> Adding a function to recalculate will require everyone to use it

I'd argue that this is akin to `functools.update_wrapper`. It too is a function that must be called in virtually every function decorator (the only function decorators that don't/needn't call it are the built-in method decorators and, appropriately enough, @abstractmethod), and if not called, certain functionality won't work.

> and worse, know that it even exists

I think that knowing about such a function is a fair requirement. Mixin tools already has nuances and edge cases, such that it should only be done (and especially distributed) by advanced users.

Furthermore, not all class mixin tools have/need to be ABC-aware. Just as `total_ordering` is ABC-agnostic (indeed, it is also subclass-agnostic), any other mixin tool can decide that implementing abstract classes is simply not a use case for them. The upshot of the `update_abstractmethods` implementation is that, as long as their implementation isn't afraid to override methods that are already defined (like attrs does), the function can be slotted above ABC-agnostic wrappers and it suddenly becomes ABC-aware.
msg378130 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2020-10-06 17:41
New changeset bef7d299eb911086ea5a7ccf7a9da337e38a8491 by Ben Avrahami in branch 'master':
bpo-41905: Add abc.update_abstractmethods() (GH-22485)
https://github.com/python/cpython/commit/bef7d299eb911086ea5a7ccf7a9da337e38a8491
msg378131 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2020-10-06 17:41
Thanks all!
History
Date User Action Args
2020-10-06 17:41:55gvanrossumsetstatus: open -> closed
resolution: fixed
messages: + msg378131

stage: patch review -> resolved
2020-10-06 17:41:18gvanrossumsetmessages: + msg378130
2020-10-04 18:22:03avrahami.bensetmessages: + msg377969
2020-10-04 16:15:22bar.harelsetnosy: + bar.harel
messages: + msg377956
2020-10-04 07:46:35avrahami.bensetmessages: + msg377923
2020-10-04 01:39:19gvanrossumsetmessages: + msg377915
2020-10-04 01:16:20rhettingersetmessages: + msg377914
2020-10-04 00:43:54gvanrossumsetmessages: + msg377908
2020-10-04 00:16:09rhettingersetmessages: + msg377907
2020-10-03 15:43:43gvanrossumsetmessages: + msg377888
2020-10-03 15:29:55avrahami.bensetmessages: + msg377887
2020-10-03 15:19:19gvanrossumsetmessages: + msg377886
2020-10-03 08:17:58avrahami.bensetmessages: + msg377861
2020-10-03 05:50:29rhettingersetmessages: + msg377854
2020-10-03 04:55:04gvanrossumsetmessages: + msg377851
2020-10-03 03:49:00rhettingersetmessages: + msg377850
2020-10-02 23:20:27gvanrossumsetmessages: + msg377839
2020-10-02 16:38:18avrahami.bensetmessages: + msg377822
2020-10-02 16:27:58rhettingersetmessages: + msg377820
2020-10-02 08:04:15avrahami.bensetmessages: + msg377802
2020-10-01 23:58:37rhettingersetmessages: + msg377790
2020-10-01 23:22:37rhettingersetnosy: + gvanrossum
2020-10-01 23:22:24rhettingersetnosy: + rhettinger
2020-10-01 21:57:39avrahami.bensetmessages: + msg377782
2020-10-01 20:58:01python-devsetkeywords: + patch
nosy: + python-dev

pull_requests: + pull_request21502
stage: patch review
2020-10-01 16:03:05eric.smithsetnosy: + eric.smith
2020-10-01 15:54:52avrahami.bencreate