Implement B036 check for except BaseException without re-raising (#438

)

* Implement B036 check for `except BaseException` without re-raising

* handle case where exception is re-raised by name. Fix unittests

* recursively search for re-raise

README.rst

@@ -195,6 +195,8 @@ second usage. Save the result to a list if the result is needed multiple times.
 
 **B035**: Found dict comprehension with a static key - either a constant value or variable not from the comprehension expression. This will result in a dict with a single key that was repeatedly overwritten.
 
+**B036**: Found ``except BaseException:`` without re-raising (no ``raise`` in the top-level of the ``except`` block). This catches all kinds of things (Exception, SystemExit, KeyboardInterrupt...) and may prevent a program from exiting as expected.
+
 Opinionated warnings
 ~~~~~~~~~~~~~~~~~~~~
 

bugbear.py

@@ -324,6 +324,31 @@ def _typesafe_issubclass(cls, class_or_tuple):
         return False
 
 
+class ExceptBaseExceptionVisitor(ast.NodeVisitor):
+    def __init__(self, except_node: ast.ExceptHandler) -> None:
+        super().__init__()
+        self.root = except_node
+        self._re_raised = False
+
+    def re_raised(self) -> bool:
+        self.visit(self.root)
+        return self._re_raised
+
+    def visit_Raise(self, node: ast.Raise):
+        """If we find a corresponding `raise` or `raise e` where e was from
+        `except BaseException as e:` then we mark re_raised as True and can
+        stop scanning."""
+        if node.exc is None or node.exc.id == self.root.name:
+            self._re_raised = True
+            return
+        return super().generic_visit(node)
+
+    def visit_ExceptHandler(self, node: ast.ExceptHandler):
+        if node is not self.root:
+            return  # entered a nested except - stop searching
+        return super().generic_visit(node)
+
+
 @attr.s
 class BugBearVisitor(ast.NodeVisitor):
     filename = attr.ib()
@@ -407,6 +432,12 @@ def visit_ExceptHandler(self, node):
             maybe_error = _check_redundant_excepthandlers(names, node)
             if maybe_error is not None:
                 self.errors.append(maybe_error)
+        if (
+            "BaseException" in names
+            and not ExceptBaseExceptionVisitor(node).re_raised()
+        ):
+            self.errors.append(B036(node.lineno, node.col_offset))
+
         self.generic_visit(node)
 
     def visit_UAdd(self, node):
@@ -668,7 +699,7 @@ def check_for_b017(self, node):
                             and isinstance(item_context.func.value, ast.Name)
                             and item_context.func.value.id == "pytest"
                             and "match"
-                            not in [kwd.arg for kwd in item_context.keywords]
+                            not in (kwd.arg for kwd in item_context.keywords)
                         )
                     )
                 )
@@ -677,7 +708,7 @@ def check_for_b017(self, node):
                     and item_context.func.id == "raises"
                     and isinstance(item_context.func.ctx, ast.Load)
                     and "pytest.raises" in self._b005_imports
-                    and "match" not in [kwd.arg for kwd in item_context.keywords]
+                    and "match" not in (kwd.arg for kwd in item_context.keywords)
                 )
             )
             and len(item_context.args) == 1
@@ -1671,8 +1702,8 @@ def visit_Lambda(self, node):
     message=(
         "B001 Do not use bare `except:`, it also catches unexpected "
         "events like memory errors, interrupts, system exit, and so on.  "
-        "Prefer `except Exception:`.  If you're sure what you're doing, "
-        "be explicit and write `except BaseException:`."
+        "Prefer excepting specific exceptions  If you're sure what you're "
+        "doing, be explicit and write `except BaseException:`."
     )
 )
 
@@ -1953,6 +1984,9 @@ def visit_Lambda(self, node):
 )
 B035 = Error(message="B035 Static key in dict comprehension {!r}.")
 
+B036 = Error(
+    message="B036 Don't except `BaseException` unless you plan to re-raise it."
+)
 
 # Warnings disabled by default.
 B901 = Error(

tests/b001.py

@@ -23,13 +23,13 @@
 
 try:
     pass
-except BaseException as be:
+except ValueError as be:
     # no warning here, all good
     pass
 
 try:
     pass
-except BaseException:
+except IndexError:
     # no warning here, all good
     pass
 

tests/b014.py

@@ -41,7 +41,7 @@ class MyError(Exception):
     pass
 except (MyError, BaseException) as e:
     # But we *can* assume that everything is a subclass of BaseException
-    pass
+    raise e
 
 
 try:

tests/b036.py

@@ -0,0 +1,54 @@
+
+try:
+    pass
+except BaseException:  # bad
+    print("aaa")
+    pass
+
+
+try:
+    pass
+except BaseException as ex:  # bad
+    print(ex)
+    pass
+
+
+try:
+    pass
+except ValueError:
+    raise
+except BaseException:  # bad
+    pass
+
+
+try:
+    pass
+except BaseException:  # ok - reraised
+    print("aaa")
+    raise
+
+
+try:
+    pass
+except BaseException as ex:  # bad - raised something else
+    print("aaa")
+    raise KeyError from ex
+
+try:
+    pass
+except BaseException as e:
+    raise e  # ok - raising same thing
+
+try:
+    pass
+except BaseException:
+    if 0:
+        raise  # ok - raised somewhere within branch
+
+try:
+    pass
+except BaseException:
+    try:  # nested try
+        pass
+    except ValueError:
+        raise  # bad - raising within a nested try/except, but not within the main one

tests/test_bugbear.py

@@ -44,6 +44,7 @@
     B033,
     B034,
     B035,
+    B036,
     B901,
     B902,
     B903,
@@ -597,6 +598,19 @@ def test_b035(self):
         )
         self.assertEqual(errors, expected)
 
+    def test_b036(self) -> None:
+        filename = Path(__file__).absolute().parent / "b036.py"
+        bbc = BugBearChecker(filename=str(filename))
+        errors = list(bbc.run())
+        expected = self.errors(
+            B036(4, 0),
+            B036(11, 0),
+            B036(20, 0),
+            B036(33, 0),
+            B036(50, 0),
+        )
+        self.assertEqual(errors, expected)
+
     def test_b908(self):
         filename = Path(__file__).absolute().parent / "b908.py"
         bbc = BugBearChecker(filename=str(filename))