Introduce B030; fix crash on weird except handlers (#346)

* Introduce B029; fix crash on weird except handlers

* black

* fix merge

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

Author: JelleZijlstra

README.rst

@@ -179,6 +179,8 @@ It is therefore recommended to use a stacklevel of 2 or greater to provide more
 
 **B029**: Using ``except: ()`` with an empty tuple does not handle/catch anything. Add exceptions to handle.
 
+**B030**: Except handlers should only be exception classes or tuples of exception classes.
+
 Opinionated warnings
 ~~~~~~~~~~~~~~~~~~~~
 

bugbear.py

@@ -189,13 +189,59 @@ def _is_identifier(arg):
     return re.match(r"^[A-Za-z_][A-Za-z0-9_]*$", arg.s) is not None
 
 
+def _flatten_excepthandler(node):
+    if isinstance(node, ast.Tuple):
+        for elt in node.elts:
+            yield from _flatten_excepthandler(elt)
+    else:
+        yield node
+
+
+def _check_redundant_excepthandlers(names, node):
+    # See if any of the given exception names could be removed, e.g. from:
+    #    (MyError, MyError)  # duplicate names
+    #    (MyError, BaseException)  # everything derives from the Base
+    #    (Exception, TypeError)  # builtins where one subclasses another
+    #    (IOError, OSError)  # IOError is an alias of OSError since Python3.3
+    # but note that other cases are impractical to handle from the AST.
+    # We expect this is mostly useful for users who do not have the
+    # builtin exception hierarchy memorised, and include a 'shadowed'
+    # subtype without realising that it's redundant.
+    good = sorted(set(names), key=names.index)
+    if "BaseException" in good:
+        good = ["BaseException"]
+    # Remove redundant exceptions that the automatic system either handles
+    # poorly (usually aliases) or can't be checked (e.g. it's not an
+    # built-in exception).
+    for primary, equivalents in B014.redundant_exceptions.items():
+        if primary in good:
+            good = [g for g in good if g not in equivalents]
+
+    for name, other in itertools.permutations(tuple(good), 2):
+        if _typesafe_issubclass(
+            getattr(builtins, name, type), getattr(builtins, other, ())
+        ):
+            if name in good:
+                good.remove(name)
+    if good != names:
+        desc = good[0] if len(good) == 1 else "({})".format(", ".join(good))
+        as_ = " as " + node.name if node.name is not None else ""
+        return B014(
+            node.lineno,
+            node.col_offset,
+            vars=(", ".join(names), as_, desc),
+        )
+    return None
+
+
 def _to_name_str(node):
     # Turn Name and Attribute nodes to strings, e.g "ValueError" or
     # "pkg.mod.error", handling any depth of attribute accesses.
     if isinstance(node, ast.Name):
         return node.id
     if isinstance(node, ast.Call):
         return _to_name_str(node.func)
+    assert isinstance(node, ast.Attribute), f"Unexpected node type: {type(node)}"
     try:
         return _to_name_str(node.value) + "." + node.attr
     except AttributeError:
@@ -277,48 +323,27 @@ def visit(self, node):
     def visit_ExceptHandler(self, node):
         if node.type is None:
             self.errors.append(B001(node.lineno, node.col_offset))
-        elif isinstance(node.type, ast.Tuple):
-            names = [_to_name_str(e) for e in node.type.elts]
-            as_ = " as " + node.name if node.name is not None else ""
-            if len(names) == 0:
-                self.errors.append(B029(node.lineno, node.col_offset))
-            elif len(names) == 1:
-                self.errors.append(B013(node.lineno, node.col_offset, vars=names))
+            self.generic_visit(node)
+            return
+        handlers = _flatten_excepthandler(node.type)
+        good_handlers = []
+        bad_handlers = []
+        for handler in handlers:
+            if isinstance(handler, (ast.Name, ast.Attribute)):
+                good_handlers.append(handler)
             else:
-                # See if any of the given exception names could be removed, e.g. from:
-                #    (MyError, MyError)  # duplicate names
-                #    (MyError, BaseException)  # everything derives from the Base
-                #    (Exception, TypeError)  # builtins where one subclasses another
-                #    (IOError, OSError)  # IOError is an alias of OSError since Python3.3
-                # but note that other cases are impractical to handle from the AST.
-                # We expect this is mostly useful for users who do not have the
-                # builtin exception hierarchy memorised, and include a 'shadowed'
-                # subtype without realising that it's redundant.
-                good = sorted(set(names), key=names.index)
-                if "BaseException" in good:
-                    good = ["BaseException"]
-                # Remove redundant exceptions that the automatic system either handles
-                # poorly (usually aliases) or can't be checked (e.g. it's not an
-                # built-in exception).
-                for primary, equivalents in B014.redundant_exceptions.items():
-                    if primary in good:
-                        good = [g for g in good if g not in equivalents]
-
-                for name, other in itertools.permutations(tuple(good), 2):
-                    if _typesafe_issubclass(
-                        getattr(builtins, name, type), getattr(builtins, other, ())
-                    ):
-                        if name in good:
-                            good.remove(name)
-                if good != names:
-                    desc = good[0] if len(good) == 1 else "({})".format(", ".join(good))
-                    self.errors.append(
-                        B014(
-                            node.lineno,
-                            node.col_offset,
-                            vars=(", ".join(names), as_, desc),
-                        )
-                    )
+                bad_handlers.append(handler)
+        if bad_handlers:
+            self.errors.append(B030(node.lineno, node.col_offset))
+        names = [_to_name_str(e) for e in good_handlers]
+        if len(names) == 0 and not bad_handlers:
+            self.errors.append(B029(node.lineno, node.col_offset))
+        elif len(names) == 1 and not bad_handlers and isinstance(node.type, ast.Tuple):
+            self.errors.append(B013(node.lineno, node.col_offset, vars=names))
+        else:
+            maybe_error = _check_redundant_excepthandlers(names, node)
+            if maybe_error is not None:
+                self.errors.append(maybe_error)
         self.generic_visit(node)
 
     def visit_UAdd(self, node):
@@ -1533,6 +1558,7 @@ def visit_Lambda(self, node):
         "anything. Add exceptions to handle."
     )
 )
+B030 = Error(message="B030 Except handlers should only be names of exception classes")
 
 # Warnings disabled by default.
 B901 = Error(

tests/b030.py

@@ -0,0 +1,14 @@
+try:
+    pass
+except (ValueError, (RuntimeError, (KeyError, TypeError))):  # ok
+    pass
+
+try:
+    pass
+except 1:  # error
+    pass
+
+try:
+    pass
+except (1, ValueError):  # error
+    pass

tests/test_bugbear.py

@@ -41,6 +41,7 @@
     B027,
     B028,
     B029,
+    B030,
     B901,
     B902,
     B903,
@@ -448,6 +449,16 @@ def test_b029(self):
         )
         self.assertEqual(errors, expected)
 
+    def test_b030(self):
+        filename = Path(__file__).absolute().parent / "b030.py"
+        bbc = BugBearChecker(filename=str(filename))
+        errors = list(bbc.run())
+        expected = self.errors(
+            B030(8, 0),
+            B030(13, 0),
+        )
+        self.assertEqual(errors, expected)
+
     @unittest.skipIf(sys.version_info < (3, 8), "not implemented for <3.8")
     def test_b907(self):
         filename = Path(__file__).absolute().parent / "b907.py"