add update_abstractmethods function to update an ABC's abstract methods #86071
Comments
|
python-ideas discussion: 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 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
mannequin
added
3.10
|
for the functionality to work, |
|
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 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.
>>> 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. |
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.
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):
whateverBut 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 NotImplementedit'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. |
+1 |
I had a little debate about this in my mind, I'll change it.
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? |
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 mThe 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. |
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 |
|
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". |
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. |
I'd argue that this is akin to
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 |
|
Thanks all! |
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: