diff --git a/README.rst b/README.rst index fbf6329..730eb65 100644 --- a/README.rst +++ b/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 ~~~~~~~~~~~~~~~~~~~~ diff --git a/bugbear.py b/bugbear.py index 761e75a..1bfc304 100644 --- a/bugbear.py +++ b/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( diff --git a/tests/b001.py b/tests/b001.py index fe6654b..dbd4407 100644 --- a/tests/b001.py +++ b/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 diff --git a/tests/b014.py b/tests/b014.py index 3847345..3cb4570 100644 --- a/tests/b014.py +++ b/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: diff --git a/tests/b036.py b/tests/b036.py new file mode 100644 index 0000000..67f404d --- /dev/null +++ b/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 diff --git a/tests/test_bugbear.py b/tests/test_bugbear.py index b0043c8..94343ca 100644 --- a/tests/test_bugbear.py +++ b/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))