GeezerVison

I think you need this test case to cover the first loop:

import abc
class FooABC(metaclass=abc.ABCMeta):
    @abc.abstractmethod
    def bar(self):
        pass

del FooABC.bar
assert ('bar' in FooABC.__abstractmethods__)
assert ('bar' not in FooABC.__dict__)

@iritkatriel What does that test case accomplish other than showing that FooABC is now in an inconsistent state?

@gvanrossum In this comment I left out the update_abstractmethods, see Ben's version which he committed as test_update_del. Without this test the first loop in update_abstractmethods was not exercised.

Since you didn't check in this function that it's an ABC, it's not guaranteed that cls.__abstractmethods__ works:

>>> class FooABC: pass
... 
>>> FooABC.__abstractmethods__
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: __abstractmethods__

Maybe add a test for when this function is called for a non-ABC.

The nested getattr() isn't very clear.

Also, since this is in the inner loop for a dynamic decorator, it will slow the class creation process. I didn't used to think that was important but experience with named tuples show that people care a great deal about the amount of computation that occurs during an import.

would using a walrus operator be okay here?

roots = {...
         if (root:=getattr(cls, op, None)) is not getattr(object, op, None)
         and not getattr(root, '__isabstractmethod__', False)}

That seems fine to me.

Add tests for deeper inheritance - I think iterating over __bases__ may not be enough:

>>> class A: pass
... 
>>> class B(A): pass
... 
>>> class C(B): pass
... 
>>> C.__bases__
(<class '__main__.B'>,)
>>> B.__bases__
(<class '__main__.A'>,)

It actually is- we are assured that the parent's __abstractmethods__ are correct since, after a class is subclassed, its abstraction status cannot change:

class A(ABC):
  @abstractmethod
  def foo(self): pass

class B(A):
  pass

# after this point, A.__abstractmethods__ will never change, so we can rely on it always being correct when checking B's Abstraction status

I added a test case regardless

I see, interesting. How about this case? I think you might need to break after the first hit for the method:

class A(metaclass=abc.ABCMeta):
    @abc.abstractmethod
    def foo(self):
        pass

class B(metaclass=abc.ABCMeta):
    @abc.abstractmethod
    def foo(self):
        pass

class C(A, B):
    pass

A.foo = lambda self: None


assert(C.foo is not abstract)

This is expected (albeit confusing) behavior. Since abstraction status cannot change after subclassing, A is still considered abstract, even though its method is implemented. However, since namespace resolution occurs through the __dict__ attribute, The method is defined as far as C is concerned. Indeed, if you were to call update_abstractmethods(C) after this code, C would become concrete and usable, since its foo method implemented.

In essence, abstract methods shouldn't be changed after the class is used, so this use case is not handled by this implementation.

Well, I disagree with the check for empty __subclasses__(), and I think in Irit's last example the right thing to do is to call abc.update_abstractmethods(A) and then abc.update_abstractmethods(C).

Note that it's not technically true that the abstractness cannot change once subclassed -- it may be undocumented but one could just write A.__abstractmethods__ = ().

FWIW I do agree that the code should assume that the abstractness of the bases is correct. If some base lies about it, well, Garbage In, Garbage Out.

I don't think abc.update_abstractmethods(A) would have any impact here, A never changed.

Sorry, I was commenting on your earlier example which has A.foo = lambda ... -- in that example it certainly would have an effect. :-)

My disagreement was with this quote from @bentheiii:

In essence, abstract methods shouldn't be changed after the class is used, so this use case is not handled by this implementation.

I agree with @iritkatriel that adding that test would be useful (I didn't verify -- maybe it's already added?).

Sorry, I was commenting on your earlier example which has A.foo = lambda ... -- in that example it certainly would have an effect. :-)

Ah, in that case it raises an exception that A has been subclasses so you can't update it anymore. I think there's a test for that already but if not then there should be.

I see now - you're suggesting to remove that exception. In that case this test is probably needed.

I would like the summary to fit on one line.

I'm not sure I like this. This function is potentially expensive. It could even be detrimental -- I could imagine some very dynamic code that's messing around dynamically with a base class and then calls update_abstractmethods() for all subclasses. So I propose to remove this check (and the documentation about it).

That might be okay, However, removing this check means that we have to call update_abstractmethods on all of cls's subclasses, meaning that cls.__subclasses__() will still be called, so performance will not improve.

Only if you're not sure that there are no subclasses. In @dataclass we can assume there are no subclasses, and that's so in most class decorators (which are only applicable to freshly created classes).

I recommend that update_abstractmethods() should not try to fix up subclasses -- it's just the case that an ambitious dynamic class hacker could take two documented features (this and __subclasses__()) and create something that automatically updates affected subclasses.

I think that could cause a problem if user chain two decorators: one that updates subclasses and one that doesn't

def decoratorA(cls):
  """implements methods and creates a subclass, calling
  update_abstractmethods on cls and its subclasses
  """
  ...

def decoratorB(cls):
  """implements methods only, not updating subclasses
  """
  ...

@decoratorB
@decoratorA
class A(SomeABC):
  ...

# A's subclass is now out of sync with decoratorB's changes

Thinking about it, the previous implementation was indeed faulty since, even if a decorator that needed to subclass cls was responsible enough to delay subclassing until after calling update_abstractmethods, that would meant that follow-up decorators simply couldn't call it.

The only sensible solution is to call update_abstractmethods on all of the class's subclasses with each call (@dataclass can't be certain that the subject class wasn't subclassed in a previous mixin decorator).

If you do that, you get what you deserve.

Please keep it simple.

It's implemented in the latest commit, but I must say I'm not sure what you mean. Who "gets what they deserve"?

• Whoever implemented decoratorA (which may well be @dataclass) was right to assume there are no subclasses, since it did not create them, and wanted to avoid unnecessary checks.
• Whoever implemented decoratorB did their due diligence by making sure it returned the class and its subclasses in a synced state.
• Whoever wrote class A may be totally unaware that decoratorB creates subclasses or that decoratorA doesn't take them into account. Even if they did, they would need to implement a fix themselves, which would require at the least to iterate through A.__subclasses__(). Even that won't keep them 100% safe, however, since these subclasses can have subclasses of their own. Very far fetched, perhaps, but that's exactly what very dynamic code might produce.

Truth be told, if we do assume that decorators and tools are allowed to subclass (and perhaps other permanent operations like instancing) before calling update_abstractmethods, I'm leaning closer and closer to an automatic solution as proposed by Bar and Reymond.

Well, I disagree with the check for empty __subclasses__(), and I think in Irit's last example the right thing to do is to call abc.update_abstractmethods(A) and then abc.update_abstractmethods(C).

Note that it's not technically true that the abstractness cannot change once subclassed -- it may be undocumented but one could just write A.__abstractmethods__ = ().

FWIW I do agree that the code should assume that the abstractness of the bases is correct. If some base lies about it, well, Garbage In, Garbage Out.

@iritkatriel What does that test case accomplish other than showing that FooABC is now in an inconsistent state?