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.

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.

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.

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

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

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.

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

> 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.

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.

> 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

> 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.

Why would that require more code? That’s already how it worked. Maybe you misunderstand what I tried to say?

> 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?

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

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.

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.

> 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.

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)

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".

> 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.

> 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.

New changeset bef7d299eb911086ea5a7ccf7a9da337e38a8491 by Ben Avrahami in branch 'master':
bpo-41905: Add abc.update_abstractmethods() (GH-22485)
https://github.com/python/cpython/commit/bef7d299eb911086ea5a7ccf7a9da337e38a8491

Thanks all!