Should we remove abstract-method?

[mbyrnepr2]

Current problem

abstract-method documentation.

The documentation states that this message is emitted when an abstract method (i.e. raise NotImplementedError) is not overridden in concrete class.

A concrete class is one where all the abstract methods have been implemented.

The problem with abstract-method is that we don't know for sure if a class is intended to be a concrete class or not. The only way we would know for sure is if it does contain all of the implemented abstract methods - but by that point the check is not useful.

The following scenario (modified from one of the examples in the documentation) is valid Python but it would emit abstract-method:

class Pet:
    def make_sound(self):
        raise NotImplementedError


class Cat(
    Pet
):  #  W0223: Method 'make_sound' is abstract in class 'Pet' but is not overridden in child class 'Cat' (abstract-method)
    def jump_up(self):
        print("jump!")


class Tiger(Cat):
    """This is the concrete class"""
    def make_sound(self):
        print("rrr!")


Tiger().make_sound()
Tiger().jump_up()

The other example from the documentation emits abstract-method; however the base class does not inherit from abc.ABC, so this example emits a warning even though the class, which is not implementing the abstract method, instantiates without error:

import abc


class WildAnimal:
    @abc.abstractmethod
    def make_sound(self):
        pass


class Panther(WildAnimal):  # [abstract-method]
    pass


Panther().make_sound()

Desired solution

Deprecate and remove abstract-method.

Additional context

#9979
#7950
#3098

Related message https://pylint.readthedocs.io/en/latest/user_guide/messages/error/abstract-class-instantiated.html

[clauspruefer]

Please check my comments in #5793 and #10192.

[mbyrnepr2]

Thanks for the engagement.

As I understand your comments, the answer to the question - Should we remove abstract-method - would be no if Pylint were to define abstract methods strictly as those that are decorated by abc.abstractmethod.

What I would say is, my description on this issue probably wasn't clear enough.

The main takeaway of point 1. is that the Tiger class is the concrete one which implements the abstract method, and therefore, abstract-method should not be emitted on the Cat class (it shouldn't be emitted at all in this example).
The problem being that Pylint doesn't know enough here and I think the false-positive to value ratio doesn't feel justified; I noticed it in our primer results here.

Note the example doesn't use abc.abstractmethod but the same principle applies because the Pylint behaviour should be the same in either situation. However, point 2. in the description shows that if you do inherit from abc.ABC, then the message isn't emitted at all, which is a bug in itself, i.e:
This emits abstract-method:

from abc import abstractmethod


class Base:
    @abstractmethod
    def meow(self):
        print("MEOW")


class Cat(Base):
    def scratch(self):
        """Scratch my back"""

While this does not:

from abc import abstractmethod, ABC


class Base(ABC):
    @abstractmethod
    def meow(self):
        print("MEOW")


class Cat(Base):
    def scratch(self):
        """Scratch my back"""

Even if we fix the bug in point 2., I feel that abstract-method is too problematic because of it's scope to any subclass.
Also we do have abstract-class-instantiated which catches this issue at call-time.

[clauspruefer]

Even if we fix the bug in point 2., I feel that abstract-method is too problematic because of it's scope to any subclass. Also we do have abstract-class-instantiated which catches this issue at call-time.

That makes perfect sense and pylint more precise. But please also check my post in #5793, this could be relevant for abstract-class-instantiated (analyzing) too.

In my opinion a deriving abstract class definition (child class) must consist of the exact same definition (including parameter / count) as the base class definition. If not, then it can no longer call itself abstract.

For #5793 it would make sense to only check "arguments-differ" for abstract methods in child classes (or also remove).

Wrong:

import abc

class Base(metaclass=abc.ABCMeta):
    @abc.abstractmethod
    def _testmethod(self, arg1):
        pass

class Child1(Base):
    @abc.abstractmethod
    def _testmethod(self, arg1, arg2):
        print(arg1*arg2)

class Child2(Base):
    @abc.abstractmethod
    def _testmethod(self, arg1, arg2):
        print(arg1*arg2*2)

Correct (two real abstract definitions in the base must be overloaded in both child classes)

import abc

class Base(metaclass=abc.ABCMeta):
    @abc.abstractmethod
    def _testmethod(self, arg1):
        pass
    @abc.abstractmethod
    def _testmethod(self, arg1, arg2):
        pass

class Child1(Base):
    @abc.abstractmethod
    def _testmethod(self, arg1):
        print(arg1*1)
    def _testmethod(self, arg1, arg2):
        print(arg1*arg2)

class Child2(Base):
    @abc.abstractmethod
    def _testmethod(self, arg1):
        print(arg1*2)
    def _testmethod(self, arg1, arg2):
        print(arg1*arg2*2)

[mbyrnepr2]

Thanks for the clarification.
I think the goal of arguments-differ is to point out if the LSP is violated by having a different signature in the overriding method in the subclass.
So, I think that applies to method overriding in the subclass regardless if the overridden method is abstract or not.

I think there is an issue with your example, because this isn't overloading. In other words, _testmethod(self, arg1) in Child1 and Child2 are replaced by the subsequent method with the same name - _testmethod(self, arg1, arg2).
Calling Child2(). _testmethod("hey") gives:
TypeError: Child2._testmethod() missing 1 required positional argument: 'arg2'

[clauspruefer]

Thanks for the clarification. I think the goal of arguments-differ is to point out if the LSP is violated by having a different signature in the overriding method in the subclass. So, I think that applies to method overriding in the subclass regardless if the overridden method is abstract or not.

I think there is an issue with your example, because this isn't overloading. In other words, _testmethod(self, arg1) in Child1 and Child2 are replaced by the subsequent method with the same name - _testmethod(self, arg1, arg2). Calling Child2(). _testmethod("hey") gives: TypeError: Child2._testmethod() missing 1 required positional argument: 'arg2'

Understand. My assumption of Python class method overloading in combination with positional parameters was wrong, which Pylint corrected now 👍.

[clauspruefer]

But returning to the current problem.

Consider: An abstract class method is only abstract if it is decorated with the abc.ABCMeta @abc.abstractmethoddecorator.
It is impossible to differentiate otherwise.

[mbyrnepr2]

But returning to the current problem.

Consider: An abstract class method is only abstract if it is decorated with the abc.ABCMeta @abc.abstractmethoddecorator.
It is impossible to differentiate otherwise.

I guess we should keep discussion about that on the #10192 issue.

[muhrin]

Sorry to bring up an old thread, but I feel that this hasn't been settled, at least not the particular issue I'm having with this warning.

Going back to basics, if we stick with the case where everything is explicit i.e. we use abc.ABCMeta (or abc.ABC), and at least one method is marked as abc.abstractmethod this clearly makes a class abstract. If a child class does not overwrite all abstractmethods it too is abstract, e.g. using this modified example from @mbyrnepr2 :

import abc

class Base(abc.ABC):
    @abc.abstractmethod
    def meow(self):
        print("MEOW") # Here we just provide a default implementation

class Cat(Base):
    def scratch(self):
        """Scratch my back"""

Cat is abstract too by virtue of it not having overwritten meow. This means that pylint should not warn about Cat not overriding meow as this is totally fine if the user wishes Cat to remain abstract. This is the way abstract class work in other languages (Java, C++, etc) and makes perfect sense. However, @clauspruefer , it seems like from some of your comments that you don't think Cat is/should be abstract here (without the explicit addition of ABCMeta) - though please do correct me if I'm wrongly putting words into your mouth.

In case you still think Cat should be abstract in this scenario, consider:

1. subclassing, in the simple case (e.g. no call of super(), single inheritanc, ..), is essentially analogous to copying-and-pasting the parent class with a new name and adding/changing methods and members.
2. Cat still has abc.ABCMeta as its metaclass, and so adding it again is a no-op.
3. inspect.isabstract(Cat) is True (in recognition of the logic I've outlined above).
4. Cat() raises a TypeError because it is abstract

So my question is this: can pylint not just use inspect.isabstract() to decide whether to warn about abstract methods not being overridden?

[DanielNoord]

The short answer is No unfortunately. We don't have access to to the objects at runtime but only the AST. Therefore, calling inspect is impossible.

[muhrin]

Thanks Daniel, I thought that might be the case. Do you have any idea how hard it is to implement isabstract-like logic using the AST:

def isabstract(object):
    """Return true if the object is an abstract base class (ABC)."""
    if not isinstance(object, type):
        return False
    if object.__flags__ & TPFLAGS_IS_ABSTRACT:
        return True
    if not issubclass(type(object), abc.ABCMeta):
        return False
    if hasattr(object, '__abstractmethods__'):
        # It looks like ABCMeta.__new__ has finished running;
        # TPFLAGS_IS_ABSTRACT should have been accurate.
        return False
    # It looks like ABCMeta.__new__ has not finished running yet; we're
    # probably in __init_subclass__. We'll look for abstractmethods manually.
    for name, value in object.__dict__.items():
        if getattr(value, "__isabstractmethod__", False):
            return True
    for base in object.__bases__:
        for name in getattr(base, "__abstractmethods__", ()):
            value = getattr(object, name, None)
            if getattr(value, "__isabstractmethod__", False):
                return True
    return False

My example above already returns on the TPFLAGS_IS_ABSTRACT test (which I guess is one of the simpler conditions to implement using the AST). It may be that this could be enough to catch most of the false-positives pylint is currently generating.

[DanielNoord]

We might be able to, but there is also the issue of this being a breaking change and (until now) nobody feeling strong enough about any provided solution to argue that we should potentially break all current CIs for this change.

[clauspruefer]

@DanielNoord i know you are talking about implementing (changing) the abstract method inspection code into pylint.

On my to-do list is still the point to inform Python about the faulty documentation. If successful this could inspire the pylint developer community a bit.

[muhrin]

Thanks @DanielNoord , I agree that it's key that someone is bothered enough to do something about it. I'm sadly overloaded so (even if it were to be accepted by pylint), it's not something I can tackle at the moment.

I would say though, that while it is true that this is a breaking change, from what I can see it would only be 'breaking' in the sense that it would no longer warn in situations that it used to warn in. It wouldn't warn in any new situations. Furthermore, the current implementation gives false-positives and so a change now would essentially restore the 'correct' behaviour. Given that pylint is used to write 'correct' (or at least less error prone code), this would seem to be a good thing.

[DanielNoord]

Even not warning will mean that people might need to remove ignore comments from their code. This all contributes to having to do manual work when you bump the version of pylint.

[muhrin]

Good point, I hadn't thought of that!