Merge pull request #281 from jakkdl/abstract_class_empty_method_no_ab…

…stract_decorator

Add B027: empty method in abstract base class with no abstract decorator

README.rst

@@ -154,7 +154,7 @@ positives due to similarly named user-defined functions.
 the loop, because `late-binding closures are a classic gotcha
 <https://docs.python-guide.org/writing/gotchas/#late-binding-closures>`__.
 
-**B024**: Abstract base class with no abstract method. Remember to use @abstractmethod, @abstractclassmethod, and/or @abstractproperty decorators.
+**B024**: Abstract base class with no abstract method. You might have forgotten to add @abstractmethod.
 
 **B025**: ``try-except`` block with duplicate exceptions found.
 This check identifies exception types that are specified in multiple ``except``
@@ -167,6 +167,8 @@ There was `cpython discussion of disallowing this syntax
 <https://github.com/python/cpython/issues/82741>`_, but legacy usage and parser
 limitations make it difficult.
 
+**B027**: Empty method in abstract base class, but has no abstract decorator. Consider adding @abstractmethod.
+
 Opinionated warnings
 ~~~~~~~~~~~~~~~~~~~~
 
@@ -294,17 +296,21 @@ MIT
 
 Change Log
 ----------
+Future
+~~~~~~~~~
+
+* Add B027: Empty method in abstract base class with no abstract decorator
 
 22.9.23
 ~~~~~~~~~~
 
-* add B026: find argument unpacking after keyword argument (#287)
+* Add B026: find argument unpacking after keyword argument (#287)
 * Move to setup.cfg like flake8 (#288)
 
 22.9.11
 ~~~~~~~~~~
 
-* add B025: find duplicate except clauses (#284)
+* Add B025: find duplicate except clauses (#284)
 
 22.8.23
 ~~~~~~~~~~

bugbear.py

@@ -418,7 +418,7 @@ def visit_ClassDef(self, node):
         self.check_for_b903(node)
         self.check_for_b018(node)
         self.check_for_b021(node)
-        self.check_for_b024(node)
+        self.check_for_b024_and_b027(node)
         self.generic_visit(node)
 
     def visit_Try(self, node):
@@ -612,17 +612,19 @@ def check_for_b023(self, loop_node):
             if reassigned_in_loop.issuperset(err.vars):
                 self.errors.append(err)
 
-    def check_for_b024(self, node: ast.ClassDef):
+    def check_for_b024_and_b027(self, node: ast.ClassDef):  # noqa: C901
         """Check for inheritance from abstract classes in abc and lack of
         any methods decorated with abstract*"""
 
-        def is_abc_class(value):
+        def is_abc_class(value, name="ABC"):
+            # class foo(metaclass = [abc.]ABCMeta)
             if isinstance(value, ast.keyword):
-                return value.arg == "metaclass" and is_abc_class(value.value)
-            abc_names = ("ABC", "ABCMeta")
-            return (isinstance(value, ast.Name) and value.id in abc_names) or (
+                return value.arg == "metaclass" and is_abc_class(value.value, "ABCMeta")
+            # class foo(ABC)
+            # class foo(abc.ABC)
+            return (isinstance(value, ast.Name) and value.id == name) or (
                 isinstance(value, ast.Attribute)
-                and value.attr in abc_names
+                and value.attr == name
                 and isinstance(value.value, ast.Name)
                 and value.value.id == "abc"
             )
@@ -632,16 +634,56 @@ def is_abstract_decorator(expr):
                 isinstance(expr, ast.Attribute) and expr.attr[:8] == "abstract"
             )
 
+        def empty_body(body) -> bool:
+            def is_str_or_ellipsis(node):
+                # ast.Ellipsis and ast.Str used in python<3.8
+                return isinstance(node, (ast.Ellipsis, ast.Str)) or (
+                    isinstance(node, ast.Constant)
+                    and (node.value is Ellipsis or isinstance(node.value, str))
+                )
+
+            # Function body consist solely of `pass`, `...`, and/or (doc)string literals
+            return all(
+                isinstance(stmt, ast.Pass)
+                or (isinstance(stmt, ast.Expr) and is_str_or_ellipsis(stmt.value))
+                for stmt in body
+            )
+
+        # don't check multiple inheritance
+        # https://github.com/PyCQA/flake8-bugbear/issues/277
+        if len(node.bases) + len(node.keywords) > 1:
+            return
+
+        # only check abstract classes
         if not any(map(is_abc_class, (*node.bases, *node.keywords))):
             return
 
+        has_abstract_method = False
+
         for stmt in node.body:
-            if isinstance(stmt, (ast.FunctionDef, ast.AsyncFunctionDef)) and any(
+            # https://github.com/PyCQA/flake8-bugbear/issues/293
+            # Ignore abc's that declares a class attribute that must be set
+            if isinstance(stmt, (ast.AnnAssign, ast.Assign)):
+                has_abstract_method = True
+                continue
+
+            # only check function defs
+            if not isinstance(stmt, (ast.FunctionDef, ast.AsyncFunctionDef)):
+                continue
+
+            has_abstract_decorator = any(
                 map(is_abstract_decorator, stmt.decorator_list)
-            ):
-                return
+            )
+
+            has_abstract_method |= has_abstract_decorator
+
+            if not has_abstract_decorator and empty_body(stmt.body):
+                self.errors.append(
+                    B027(stmt.lineno, stmt.col_offset, vars=(stmt.name,))
+                )
 
-        self.errors.append(B024(node.lineno, node.col_offset, vars=(node.name,)))
+        if not has_abstract_method:
+            self.errors.append(B024(node.lineno, node.col_offset, vars=(node.name,)))
 
     def check_for_b026(self, call: ast.Call):
         if not call.keywords:
@@ -1211,8 +1253,8 @@ def visit_Lambda(self, node):
 B024 = Error(
     message=(
         "B024 {} is an abstract base class, but it has no abstract methods. Remember to"
-        " use @abstractmethod, @abstractclassmethod and/or @abstractproperty"
-        " decorators."
+        " use the @abstractmethod decorator, potentially in conjunction with"
+        " @classmethod, @property and/or @staticmethod."
     )
 )
 B025 = Error(
@@ -1229,6 +1271,12 @@ def visit_Lambda(self, node):
         "surprise and mislead readers."
     )
 )
+B027 = Error(
+    message=(
+        "B027 {} is an empty method in an abstract base class, but has no abstract"
+        " decorator. Consider adding @abstractmethod."
+    )
+)
 
 # Warnings disabled by default.
 B901 = Error(

tests/b024.py

@@ -16,76 +16,114 @@
 
 class Base_1(ABC):  # error
     def method(self):
-        ...
+        foo()
 
 
 class Base_2(ABC):
     @abstractmethod
     def method(self):
-        ...
+        foo()
 
 
 class Base_3(ABC):
     @abc.abstractmethod
     def method(self):
-        ...
+        foo()
 
 
 class Base_4(ABC):
     @notabc.abstractmethod
     def method(self):
-        ...
+        foo()
 
 
 class Base_5(ABC):
     @abstract
     def method(self):
-        ...
+        foo()
 
 
 class Base_6(ABC):
     @abstractaoeuaoeuaoeu
     def method(self):
-        ...
+        foo()
 
 
 class Base_7(ABC):  # error
     @notabstract
     def method(self):
-        ...
+        foo()
 
 
 class MetaBase_1(metaclass=ABCMeta):  # error
     def method(self):
-        ...
+        foo()
 
 
 class MetaBase_2(metaclass=ABCMeta):
     @abstractmethod
     def method(self):
-        ...
+        foo()
 
 
 class abc_Base_1(abc.ABC):  # error
     def method(self):
-        ...
+        foo()
 
 
 class abc_Base_2(metaclass=abc.ABCMeta):  # error
     def method(self):
-        ...
+        foo()
 
 
 class notabc_Base_1(notabc.ABC):  # safe
     def method(self):
-        ...
+        foo()
 
 
-class multi_super_1(notabc.ABC, abc.ABCMeta):  # error
+class multi_super_1(notabc.ABC, abc.ABCMeta):  # safe
     def method(self):
-        ...
+        foo()
 
 
-class multi_super_2(notabc.ABC, metaclass=abc.ABCMeta):  # error
+class multi_super_2(notabc.ABC, metaclass=abc.ABCMeta):  # safe
     def method(self):
-        ...
+        foo()
+
+
+class non_keyword_abcmeta_1(ABCMeta):  # safe
+    def method(self):
+        foo()
+
+
+class non_keyword_abcmeta_2(abc.ABCMeta):  # safe
+    def method(self):
+        foo()
+
+
+# very invalid code, but that's up to mypy et al to check
+class keyword_abc_1(metaclass=ABC):  # safe
+    def method(self):
+        foo()
+
+
+class keyword_abc_2(metaclass=abc.ABC):  # safe
+    def method(self):
+        foo()
+
+
+class abc_set_class_variable_1(ABC):  # safe
+    foo: int
+
+
+class abc_set_class_variable_2(ABC):  # safe
+    foo = 2
+
+
+class abc_set_class_variable_3(ABC):  # safe
+    foo: int = 2
+
+
+# this doesn't actually declare a class variable, it's just an expression
+class abc_set_class_variable_4(ABC):  # error
+    foo

tests/b027.py

@@ -0,0 +1,59 @@
+import abc
+from abc import ABC
+from abc import abstractmethod
+from abc import abstractmethod as notabstract
+
+"""
+Should emit:
+B025 - on lines 13, 16, 19, 23, 31
+"""
+
+
+class AbstractClass(ABC):
+    def empty_1(self):  # error
+        ...
+
+    def empty_2(self):  # error
+        pass
+
+    def empty_3(self):  # error
+        """docstring"""
+        ...
+
+    def empty_4(self):  # error
+        """multiple ellipsis/pass"""
+        ...
+        pass
+        ...
+        pass
+
+    @notabstract
+    def empty_5(self):  # error
+        ...
+
+    @abstractmethod
+    def abstract_1(self):
+        ...
+
+    @abstractmethod
+    def abstract_2(self):
+        pass
+
+    @abc.abstractmethod
+    def abstract_3(self):
+        ...
+
+    def body_1(self):
+        print("foo")
+        ...
+
+    def body_2(self):
+        self.body_1()
+
+
+class NonAbstractClass:
+    def empty_1(self):  # safe
+        ...
+
+    def empty_2(self):  # safe
+        pass

tests/test_bugbear.py

@@ -37,6 +37,7 @@
     B024,
     B025,
     B026,
+    B027,
     B901,
     B902,
     B903,
@@ -363,8 +364,7 @@ def test_b024(self):
             B024(58, 0, vars=("MetaBase_1",)),
             B024(69, 0, vars=("abc_Base_1",)),
             B024(74, 0, vars=("abc_Base_2",)),
-            B024(84, 0, vars=("multi_super_1",)),
-            B024(89, 0, vars=("multi_super_2",)),
+            B024(128, 0, vars=("abc_set_class_variable_4",)),
         )
         self.assertEqual(errors, expected)
 
@@ -399,6 +399,19 @@ def test_b026(self):
             ),
         )
 
+    def test_b027(self):
+        filename = Path(__file__).absolute().parent / "b027.py"
+        bbc = BugBearChecker(filename=str(filename))
+        errors = list(bbc.run())
+        expected = self.errors(
+            B027(13, 4, vars=("empty_1",)),
+            B027(16, 4, vars=("empty_2",)),
+            B027(19, 4, vars=("empty_3",)),
+            B027(23, 4, vars=("empty_4",)),
+            B027(31 if sys.version_info >= (3, 8) else 30, 4, vars=("empty_5",)),
+        )
+        self.assertEqual(errors, expected)
+
     def test_b901(self):
         filename = Path(__file__).absolute().parent / "b901.py"
         bbc = BugBearChecker(filename=str(filename))