add B906: visit_ function with no calls to a visit function (#316)

Author: jakkdl

README.rst

@@ -205,6 +205,10 @@ to raise a ``ValueError`` if the arguments are exhausted at differing lengths. T
 was added in Python 3.10, so don't enable this flag for code that should work on <3.10.
 For more information: https://peps.python.org/pep-0618/
 
+**B906**: ``visit_`` function with no further call to a ``visit`` function. This is often an error, and will stop the visitor from recursing into the subnodes of a visited node. Consider adding a call ``self.generic_visit(node)`` at the end of the function.
+Will only trigger on function names where the part after ``visit_`` is a valid ``ast`` type with a non-empty ``_fields`` attribute.
+This is meant to be enabled by developers writing visitors using the ``ast`` module, such as flake8 plugin writers.
+
 **B950**: Line too long. This is a pragmatic equivalent of
 ``pycodestyle``'s ``E501``: it considers "max-line-length" but only triggers
 when the value has been exceeded by **more than 10%**. You will no
@@ -302,6 +306,11 @@ MIT
 Change Log
 ----------
 
+Future
+~~~~~~~~~
+
+* Add B906: ``visit_`` function with no further calls to a ``visit`` function. (#313)
+
 22.12.6
 ~~~~~~~~~
 

bugbear.py

@@ -413,6 +413,7 @@ def visit_FunctionDef(self, node):
         self.check_for_b018(node)
         self.check_for_b019(node)
         self.check_for_b021(node)
+        self.check_for_b906(node)
         self.generic_visit(node)
 
     def visit_ClassDef(self, node):
@@ -969,6 +970,27 @@ def check_for_b905(self, node):
         ):
             self.errors.append(B905(node.lineno, node.col_offset))
 
+    def check_for_b906(self, node: ast.FunctionDef):
+        if not node.name.startswith("visit_"):
+            return
+
+        # extract what's visited, only error if it's a valid ast subclass
+        # with a non-empty _fields attribute - which is what's iterated over in
+        # ast.NodeVisitor.generic_visit
+        class_name = node.name[len("visit_") :]
+        class_type = getattr(ast, class_name, None)
+        if class_type is None or not getattr(class_type, "_fields", None):
+            return
+
+        for n in itertools.chain.from_iterable(ast.walk(nn) for nn in node.body):
+            if isinstance(n, ast.Call) and (
+                (isinstance(n.func, ast.Attribute) and "visit" in n.func.attr)
+                or (isinstance(n.func, ast.Name) and "visit" in n.func.id)
+            ):
+                break
+        else:
+            self.errors.append(B906(node.lineno, node.col_offset))
+
 
 def compose_call_path(node):
     if isinstance(node, ast.Attribute):
@@ -990,7 +1012,7 @@ class NameFinder(ast.NodeVisitor):
 
     names = attr.ib(default=attr.Factory(dict))
 
-    def visit_Name(self, node):
+    def visit_Name(self, node):  # noqa: B906 # names don't contain other names
         self.names.setdefault(node.id, []).append(node)
 
     def visit(self, node):
@@ -1054,7 +1076,7 @@ def visit_Call(self, node):
         # Check for nested functions.
         self.generic_visit(node)
 
-    def visit_Lambda(self, node):
+    def visit_Lambda(self, node):  # noqa: B906
         # Don't recurse into lambda expressions
         # as they are evaluated at call time.
         pass
@@ -1371,6 +1393,14 @@ def visit_Lambda(self, node):
 
 B905 = Error(message="B905 `zip()` without an explicit `strict=` parameter.")
 
+B906 = Error(
+    message=(
+        "B906 `visit_` function with no further calls to a visit function, which might"
+        " prevent the `ast` visitor from properly visiting all nodes."
+        " Consider adding a call to `self.generic_visit(node)`."
+    )
+)
+
 B950 = Error(message="B950 line too long ({} > {} characters)")
 
-disabled_by_default = ["B901", "B902", "B903", "B904", "B905", "B950"]
+disabled_by_default = ["B901", "B902", "B903", "B904", "B905", "B906", "B950"]

tests/b906.py

@@ -0,0 +1,55 @@
+import ast
+
+# error if method name starts with `visit_`, the type is a valid `ast` type
+# which has subfields, and contains no call to a method name containing `visit`
+# anywhere in it's body
+
+# error
+def visit_For():
+    ...
+
+
+# has call to visit function
+def visit_For():
+    foo_visit_bar()
+
+
+# has call to visit method
+def visit_While():
+    foo.bar_visit_bar()
+
+
+# this visit call clearly won't run, but is treated as safe
+def visit_If():
+    def foo():
+        a_visit_function()
+
+
+# not a valid AST class, no error
+def visit_foo():
+    ...
+
+
+# Break has no subfields to visit, so no error
+def visit_Break():
+    ...
+
+
+# explicitly check `visit` and `generic_visit`
+# doesn't start with _visit, safe
+def visit():
+    ...
+
+
+# doesn't start with _visit, safe
+def generic_visit():
+    ...
+
+
+# check no crash on short name
+def a():
+    ...
+
+
+def visit_():
+    ...

tests/test_bugbear.py

@@ -43,6 +43,7 @@
     B903,
     B904,
     B905,
+    B906,
     B950,
     BugBearChecker,
     BugBearVisitor,
@@ -501,6 +502,15 @@ def test_b905(self):
         ]
         self.assertEqual(errors, self.errors(*expected))
 
+    def test_b906(self):
+        filename = Path(__file__).absolute().parent / "b906.py"
+        bbc = BugBearChecker(filename=str(filename))
+        errors = list(bbc.run())
+        expected = [
+            B906(8, 0),
+        ]
+        self.assertEqual(errors, self.errors(*expected))
+
     def test_b950(self):
         filename = Path(__file__).absolute().parent / "b950.py"
         bbc = BugBearChecker(filename=str(filename))