add update_abstractmethods function to update an ABC's abstract methods

[bentheiii]

BPO	41905
Nosy	@gvanrossum, @rhettinger, @ericvsmith, @bharel, @bentheiii
PRs	bpo-41905: added abc.update_abstractmethods #22485

^{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:

assignee = None
closed_at = <Date 2020-10-06.17:41:55.177>
created_at = <Date 2020-10-01.15:54:52.150>
labels = ['type-feature', 'library', '3.10']
title = "add update_abstractmethods function to update an ABC's abstract methods"
updated_at = <Date 2020-10-06.17:41:55.176>
user = 'https://github.com/bentheiii'

bugs.python.org fields:

activity = <Date 2020-10-06.17:41:55.176>
actor = 'gvanrossum'
assignee = 'none'
closed = True
closed_date = <Date 2020-10-06.17:41:55.177>
closer = 'gvanrossum'
components = ['Library (Lib)']
creation = <Date 2020-10-01.15:54:52.150>
creator = 'avrahami.ben'
dependencies = []
files = []
hgrepos = []
issue_num = 41905
keywords = ['patch']
message_count = 23.0
messages = ['377769', '377782', '377790', '377802', '377820', '377822', '377839', '377850', '377851', '377854', '377861', '377886', '377887', '377888', '377907', '377908', '377914', '377915', '377923', '377956', '377969', '378130', '378131']
nosy_count = 6.0
nosy_names = ['gvanrossum', 'rhettinger', 'eric.smith', 'python-dev', 'bar.harel', 'avrahami.ben']
pr_nums = ['22485']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue41905'
versions = ['Python 3.10']

[bentheiii]

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.

[bentheiii]

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.

[rhettinger]

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.

[bentheiii]

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 :-)

[rhettinger]

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']

[bentheiii]

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.

[gvanrossum]

I'm going to ignore this issue until you two have reached agreement. I recommend using some example code.

[rhettinger]

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.

[gvanrossum]

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.