From b96027296277c210bbd412c78842f6f731366d0d Mon Sep 17 00:00:00 2001 From: John Litborn <11260241+jakkdl@users.noreply.github.com> Date: Wed, 11 Dec 2024 21:38:26 +0100 Subject: [PATCH] add except* support to B012&B025 (#500) * add except* support to B012&B025, add tests for any except-handling rules * now prints except* in error messages --- README.rst | 5 ++ bugbear.py | 63 ++++++++++++----- tests/b012_py311.py | 29 ++++++++ tests/b013_py311.py | 40 +++++++++++ tests/b014_py311.py | 76 ++++++++++++++++++++ tests/b025_py311.py | 38 ++++++++++ tests/b029_py311.py | 14 ++++ tests/b030_py311.py | 40 +++++++++++ tests/b036_py311.py | 59 ++++++++++++++++ tests/b904_py311.py | 55 +++++++++++++++ tests/test_bugbear.py | 158 +++++++++++++++++++++++++++++++++++------- 11 files changed, 535 insertions(+), 42 deletions(-) create mode 100644 tests/b012_py311.py create mode 100644 tests/b013_py311.py create mode 100644 tests/b014_py311.py create mode 100644 tests/b025_py311.py create mode 100644 tests/b029_py311.py create mode 100644 tests/b030_py311.py create mode 100644 tests/b036_py311.py create mode 100644 tests/b904_py311.py diff --git a/README.rst b/README.rst index 38ea0f5..0b6ad7a 100644 --- a/README.rst +++ b/README.rst @@ -364,6 +364,11 @@ Change Log ---------- +FUTURE +~~~~~~ + +* B012 and B025 now also handle try/except* + 24.10.31 ~~~~~~~~ diff --git a/bugbear.py b/bugbear.py index 53abddc..6d8dcbc 100644 --- a/bugbear.py +++ b/bugbear.py @@ -243,7 +243,7 @@ def _flatten_excepthandler(node: ast.expr | None) -> Iterator[ast.expr | None]: yield expr -def _check_redundant_excepthandlers(names: Sequence[str], node): +def _check_redundant_excepthandlers(names: Sequence[str], node, in_trystar): # 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 @@ -275,7 +275,7 @@ def _check_redundant_excepthandlers(names: Sequence[str], node): return B014( node.lineno, node.col_offset, - vars=(", ".join(names), as_, desc), + vars=(", ".join(names), as_, desc, in_trystar), ) return None @@ -388,6 +388,9 @@ class BugBearVisitor(ast.NodeVisitor): _b023_seen: set[error] = attr.ib(factory=set, init=False) _b005_imports: set[str] = attr.ib(factory=set, init=False) + # set to "*" when inside a try/except*, for correctly printing errors + in_trystar: str = attr.ib(default="") + if False: # Useful for tracing what the hell is going on. @@ -457,7 +460,7 @@ def visit_ExceptHandler(self, node: ast.ExceptHandler) -> None: else: self.b040_caught_exception = B040CaughtException(node.name, False) - names = self.check_for_b013_b029_b030(node) + names = self.check_for_b013_b014_b029_b030(node) if ( "BaseException" in names @@ -605,6 +608,12 @@ def visit_Try(self, node) -> None: self.check_for_b025(node) self.generic_visit(node) + def visit_TryStar(self, node) -> None: + outer_trystar = self.in_trystar + self.in_trystar = "*" + self.visit_Try(node) + self.in_trystar = outer_trystar + def visit_Compare(self, node) -> None: self.check_for_b015(node) self.generic_visit(node) @@ -763,7 +772,9 @@ def _loop(node, bad_node_types) -> None: bad_node_types = (ast.Return,) elif isinstance(node, bad_node_types): - self.errors.append(B012(node.lineno, node.col_offset)) + self.errors.append( + B012(node.lineno, node.col_offset, vars=(self.in_trystar,)) + ) for child in ast.iter_child_nodes(node): _loop(child, bad_node_types) @@ -771,7 +782,7 @@ def _loop(node, bad_node_types) -> None: for child in node.finalbody: _loop(child, (ast.Return, ast.Continue, ast.Break)) - def check_for_b013_b029_b030(self, node: ast.ExceptHandler) -> list[str]: + def check_for_b013_b014_b029_b030(self, node: ast.ExceptHandler) -> list[str]: handlers: Iterable[ast.expr | None] = _flatten_excepthandler(node.type) names: list[str] = [] bad_handlers: list[object] = [] @@ -791,16 +802,27 @@ def check_for_b013_b029_b030(self, node: ast.ExceptHandler) -> list[str]: if bad_handlers: self.errors.append(B030(node.lineno, node.col_offset)) if len(names) == 0 and not bad_handlers and not ignored_handlers: - self.errors.append(B029(node.lineno, node.col_offset)) + self.errors.append( + B029(node.lineno, node.col_offset, vars=(self.in_trystar,)) + ) elif ( len(names) == 1 and not bad_handlers and not ignored_handlers and isinstance(node.type, ast.Tuple) ): - self.errors.append(B013(node.lineno, node.col_offset, vars=names)) + self.errors.append( + B013( + node.lineno, + node.col_offset, + vars=( + *names, + self.in_trystar, + ), + ) + ) else: - maybe_error = _check_redundant_excepthandlers(names, node) + maybe_error = _check_redundant_excepthandlers(names, node, self.in_trystar) if maybe_error is not None: self.errors.append(maybe_error) return names @@ -1216,7 +1238,9 @@ def check_for_b904(self, node) -> None: and not (isinstance(node.exc, ast.Name) and node.exc.id.islower()) and any(isinstance(n, ast.ExceptHandler) for n in self.node_stack) ): - self.errors.append(B904(node.lineno, node.col_offset)) + self.errors.append( + B904(node.lineno, node.col_offset, vars=(self.in_trystar,)) + ) def walk_function_body(self, node): def _loop(parent, node): @@ -1480,7 +1504,9 @@ def check_for_b025(self, node) -> None: # sort to have a deterministic output duplicates = sorted({x for x in seen if seen.count(x) > 1}) for duplicate in duplicates: - self.errors.append(B025(node.lineno, node.col_offset, vars=(duplicate,))) + self.errors.append( + B025(node.lineno, node.col_offset, vars=(duplicate, self.in_trystar)) + ) @staticmethod def _is_infinite_iterator(node: ast.expr) -> bool: @@ -2073,6 +2099,7 @@ def visit_Lambda(self, node) -> None: error = namedtuple("error", "lineno col message type vars") Error = partial(partial, error, type=BugBearChecker, vars=()) +# note: bare except* is a syntax error, so B001 does not need to handle it B001 = Error( message=( "B001 Do not use bare `except:`, it also catches unexpected " @@ -2194,20 +2221,20 @@ def visit_Lambda(self, node) -> None: B012 = Error( message=( "B012 return/continue/break inside finally blocks cause exceptions " - "to be silenced. Exceptions should be silenced in except blocks. Control " + "to be silenced. Exceptions should be silenced in except{0} blocks. Control " "statements can be moved outside the finally block." ) ) B013 = Error( message=( "B013 A length-one tuple literal is redundant. " - "Write `except {0}:` instead of `except ({0},):`." + "Write `except{1} {0}:` instead of `except{1} ({0},):`." ) ) B014 = Error( message=( - "B014 Redundant exception types in `except ({0}){1}:`. " - "Write `except {2}{1}:`, which catches exactly the same exceptions." + "B014 Redundant exception types in `except{3} ({0}){1}:`. " + "Write `except{3} {2}{1}:`, which catches exactly the same exceptions." ) ) B014_REDUNDANT_EXCEPTIONS = { @@ -2296,8 +2323,8 @@ def visit_Lambda(self, node) -> None: ) B025 = Error( message=( - "B025 Exception `{0}` has been caught multiple times. Only the first except" - " will be considered and all other except catches can be safely removed." + "B025 Exception `{0}` has been caught multiple times. Only the first except{0}" + " will be considered and all other except{0} catches can be safely removed." ) ) B026 = Error( @@ -2325,7 +2352,7 @@ def visit_Lambda(self, node) -> None: ) B029 = Error( message=( - "B029 Using `except ():` with an empty tuple does not handle/catch " + "B029 Using `except{0} ():` with an empty tuple does not handle/catch " "anything. Add exceptions to handle." ) ) @@ -2414,7 +2441,7 @@ def visit_Lambda(self, node) -> None: B904 = Error( message=( - "B904 Within an `except` clause, raise exceptions with `raise ... from err` or" + "B904 Within an `except{0}` clause, raise exceptions with `raise ... from err` or" " `raise ... from None` to distinguish them from errors in exception handling. " " See https://docs.python.org/3/tutorial/errors.html#exception-chaining for" " details." diff --git a/tests/b012_py311.py b/tests/b012_py311.py new file mode 100644 index 0000000..9e788fa --- /dev/null +++ b/tests/b012_py311.py @@ -0,0 +1,29 @@ +def a(): + try: + pass + except* Exception: + pass + finally: + return # warning + + +def b(): + try: + pass + except* Exception: + pass + finally: + if 1 + 0 == 2 - 1: + return # warning + + +def c(): + try: + pass + except* Exception: + pass + finally: + try: + return # warning + except* Exception: + pass diff --git a/tests/b013_py311.py b/tests/b013_py311.py new file mode 100644 index 0000000..27af470 --- /dev/null +++ b/tests/b013_py311.py @@ -0,0 +1,40 @@ +""" +Should emit: +B013 - on lines 10 and 28 +""" + +import re + +try: + pass +except* (ValueError,): + # pointless use of tuple + pass + +# fmt: off +# Turn off black to keep brackets around +# single except*ion for testing purposes. +try: + pass +except* (ValueError): + # not using a tuple means it's OK (if odd) + pass +# fmt: on + +try: + pass +except* ValueError: + # no warning here, all good + pass + +try: + pass +except* (re.error,): + # pointless use of tuple with dotted attribute + pass + +try: + pass +except* (a.b.c.d, b.c.d): + # attribute of attribute, etc. + pass diff --git a/tests/b014_py311.py b/tests/b014_py311.py new file mode 100644 index 0000000..2979a51 --- /dev/null +++ b/tests/b014_py311.py @@ -0,0 +1,76 @@ +""" +This is a copy of b014 but with except* instead. Should emit: +B014 - on lines 11, 17, 28, 42, 49, 56, and 74. +""" + +import binascii +import re + +try: + pass +except* (Exception, TypeError): + # TypeError is a subclass of Exception, so it doesn't add anything + pass + +try: + pass +except* (OSError, OSError) as err: + # Duplicate exception types are useless + pass + + +class MyError(Exception): + pass + + +try: + pass +except* (MyError, MyError): + # Detect duplicate non-builtin errors + pass + + +try: + pass +except* (MyError, Exception) as e: + # Don't assume that we're all subclasses of Exception + pass + + +try: + pass +except* (MyError, BaseException) as e: + # But we *can* assume that everything is a subclass of BaseException + raise e + + +try: + pass +except* (re.error, re.error): + # Duplicate exception types as attributes + pass + + +try: + pass +except* (IOError, EnvironmentError, OSError): + # Detect if a primary exception and any its aliases are present. + # + # Since Python 3.3, IOError, EnvironmentError, WindowsError, mmap.error, + # socket.error and select.error are aliases of OSError. See PEP 3151 for + # more info. + pass + + +try: + pass +except* (MyException, NotImplemented): + # NotImplemented is not an exception, let's not crash on it. + pass + + +try: + pass +except* (ValueError, binascii.Error): + # binascii.Error is a subclass of ValueError. + pass diff --git a/tests/b025_py311.py b/tests/b025_py311.py new file mode 100644 index 0000000..d1d9914 --- /dev/null +++ b/tests/b025_py311.py @@ -0,0 +1,38 @@ +""" +Should emit: +B025 - on lines 15, 22, 31 +""" + +import pickle + +try: + a = 1 +except* ValueError: + a = 2 +finally: + a = 3 + +try: + a = 1 +except* ValueError: + a = 2 +except* ValueError: + a = 2 + +try: + a = 1 +except* pickle.PickleError: + a = 2 +except* ValueError: + a = 2 +except* pickle.PickleError: + a = 2 + +try: + a = 1 +except* (ValueError, TypeError): + a = 2 +except* ValueError: + a = 2 +except* (OSError, TypeError): + a = 2 diff --git a/tests/b029_py311.py b/tests/b029_py311.py new file mode 100644 index 0000000..b121436 --- /dev/null +++ b/tests/b029_py311.py @@ -0,0 +1,14 @@ +""" +Should emit: +B029 - on lines 8 and 13 +""" + +try: + pass +except* (): + pass + +try: + pass +except* () as e: + pass diff --git a/tests/b030_py311.py b/tests/b030_py311.py new file mode 100644 index 0000000..beca578 --- /dev/null +++ b/tests/b030_py311.py @@ -0,0 +1,40 @@ +try: + pass +except* (ValueError, (RuntimeError, (KeyError, TypeError))): # error + pass + +try: + pass +except* (ValueError, *(RuntimeError, *(KeyError, TypeError))): # ok + pass + +try: + pass +except* 1: # error + pass + +try: + pass +except* (1, ValueError): # error + pass + +try: + pass +except* (ValueError, *(RuntimeError, TypeError)): # ok + pass + + +def what_to_catch(): + return (ValueError, TypeError) + + +try: + pass +except* what_to_catch(): # ok + pass + + +try: + pass +except* a.b[1].c: # ok + pass diff --git a/tests/b036_py311.py b/tests/b036_py311.py new file mode 100644 index 0000000..f9ca110 --- /dev/null +++ b/tests/b036_py311.py @@ -0,0 +1,59 @@ + +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 in the main one + +try: + pass +except* BaseException: + raise a.b from None # bad (regression test for #449) diff --git a/tests/b904_py311.py b/tests/b904_py311.py new file mode 100644 index 0000000..43cb4e9 --- /dev/null +++ b/tests/b904_py311.py @@ -0,0 +1,55 @@ +""" +Should emit: +B904 - on lines 10, 11 and 16 +""" + +try: + raise ValueError +except* ValueError: + if "abc": + raise TypeError + raise UserWarning +except* AssertionError: + raise # Bare `raise` should not be an error +except* Exception as err: + assert err + raise Exception("No cause here...") +except* BaseException as base_err: + # Might use this instead of bare raise with the `.with_traceback()` method + raise base_err +finally: + raise Exception("Nothing to chain from, so no warning here") + +try: + raise ValueError +except* ValueError: + # should not emit, since we are not raising something + def proxy(): + raise NameError + + +try: + from preferred_library import Thing +except* ImportError: + try: + from fallback_library import Thing + except* ImportError: + + class Thing: + def __getattr__(self, name): + # same as the case above, should not emit. + raise AttributeError + + +try: + from preferred_library import Thing +except* ImportError: + try: + from fallback_library import Thing + except* ImportError: + + def context_switch(): + try: + raise ValueError + except* ValueError: + raise Exception diff --git a/tests/test_bugbear.py b/tests/test_bugbear.py index 50cb23a..44426bf 100644 --- a/tests/test_bugbear.py +++ b/tests/test_bugbear.py @@ -208,16 +208,28 @@ def test_b012(self): bbc = BugBearChecker(filename=str(filename)) errors = list(bbc.run()) all_errors = [ - B012(5, 8), - B012(13, 12), - B012(21, 12), - B012(31, 12), - B012(44, 20), - B012(66, 12), - B012(78, 12), - B012(94, 12), - B012(101, 8), - B012(107, 8), + B012(5, 8, vars=("",)), + B012(13, 12, vars=("",)), + B012(21, 12, vars=("",)), + B012(31, 12, vars=("",)), + B012(44, 20, vars=("",)), + B012(66, 12, vars=("",)), + B012(78, 12, vars=("",)), + B012(94, 12, vars=("",)), + B012(101, 8, vars=("",)), + B012(107, 8, vars=("",)), + ] + self.assertEqual(errors, self.errors(*all_errors)) + + @unittest.skipIf(sys.version_info < (3, 11), "requires 3.11+") + def test_b012_py311(self): + filename = Path(__file__).absolute().parent / "b012_py311.py" + bbc = BugBearChecker(filename=str(filename)) + errors = list(bbc.run()) + all_errors = [ + B012(7, 8, vars=("*",)), + B012(17, 12, vars=("*",)), + B012(27, 12, vars=("*",)), ] self.assertEqual(errors, self.errors(*all_errors)) @@ -226,7 +238,19 @@ def test_b013(self): bbc = BugBearChecker(filename=str(filename)) errors = list(bbc.run()) expected = self.errors( - B013(10, 0, vars=("ValueError",)), B013(32, 0, vars=("re.error",)) + B013(10, 0, vars=("ValueError", "")), + B013(32, 0, vars=("re.error", "")), + ) + self.assertEqual(errors, expected) + + @unittest.skipIf(sys.version_info < (3, 11), "requires 3.11+") + def test_b013_py311(self): + filename = Path(__file__).absolute().parent / "b013_py311.py" + bbc = BugBearChecker(filename=str(filename)) + errors = list(bbc.run()) + expected = self.errors( + B013(10, 0, vars=("ValueError", "*")), + B013(32, 0, vars=("re.error", "*")), ) self.assertEqual(errors, expected) @@ -235,17 +259,37 @@ def test_b014(self): bbc = BugBearChecker(filename=str(filename)) errors = list(bbc.run()) expected = self.errors( - B014(11, 0, vars=("Exception, TypeError", "", "Exception")), - B014(17, 0, vars=("OSError, OSError", " as err", "OSError")), - B014(28, 0, vars=("MyError, MyError", "", "MyError")), - B014(42, 0, vars=("MyError, BaseException", " as e", "BaseException")), - B014(49, 0, vars=("re.error, re.error", "", "re.error")), + B014(11, 0, vars=("Exception, TypeError", "", "Exception", "")), + B014(17, 0, vars=("OSError, OSError", " as err", "OSError", "")), + B014(28, 0, vars=("MyError, MyError", "", "MyError", "")), + B014(42, 0, vars=("MyError, BaseException", " as e", "BaseException", "")), + B014(49, 0, vars=("re.error, re.error", "", "re.error", "")), + B014( + 56, + 0, + vars=("IOError, EnvironmentError, OSError", "", "OSError", ""), + ), + B014(74, 0, vars=("ValueError, binascii.Error", "", "ValueError", "")), + ) + self.assertEqual(errors, expected) + + @unittest.skipIf(sys.version_info < (3, 11), "requires 3.11+") + def test_b014_py311(self): + filename = Path(__file__).absolute().parent / "b014_py311.py" + bbc = BugBearChecker(filename=str(filename)) + errors = list(bbc.run()) + expected = self.errors( + B014(11, 0, vars=("Exception, TypeError", "", "Exception", "*")), + B014(17, 0, vars=("OSError, OSError", " as err", "OSError", "*")), + B014(28, 0, vars=("MyError, MyError", "", "MyError", "*")), + B014(42, 0, vars=("MyError, BaseException", " as e", "BaseException", "*")), + B014(49, 0, vars=("re.error, re.error", "", "re.error", "*")), B014( 56, 0, - vars=("IOError, EnvironmentError, OSError", "", "OSError"), + vars=("IOError, EnvironmentError, OSError", "", "OSError", "*"), ), - B014(74, 0, vars=("ValueError, binascii.Error", "", "ValueError")), + B014(74, 0, vars=("ValueError, binascii.Error", "", "ValueError", "*")), ) self.assertEqual(errors, expected) @@ -482,6 +526,21 @@ def test_b025(self): ), ) + @unittest.skipIf(sys.version_info < (3, 11), "requires 3.11+") + def test_b025_py311(self): + filename = Path(__file__).absolute().parent / "b025_py311.py" + bbc = BugBearChecker(filename=str(filename)) + errors = list(bbc.run()) + self.assertEqual( + errors, + self.errors( + B025(15, 0, vars=("ValueError",)), + B025(22, 0, vars=("pickle.PickleError",)), + B025(31, 0, vars=("TypeError",)), + B025(31, 0, vars=("ValueError",)), + ), + ) + def test_b026(self): filename = Path(__file__).absolute().parent / "b026.py" bbc = BugBearChecker(filename=str(filename)) @@ -525,8 +584,19 @@ def test_b029(self): bbc = BugBearChecker(filename=str(filename)) errors = list(bbc.run()) expected = self.errors( - B029(8, 0), - B029(13, 0), + B029(8, 0, vars=("",)), + B029(13, 0, vars=("",)), + ) + self.assertEqual(errors, expected) + + @unittest.skipIf(sys.version_info < (3, 11), "requires 3.11+") + def test_b029_py311(self): + filename = Path(__file__).absolute().parent / "b029_py311.py" + bbc = BugBearChecker(filename=str(filename)) + errors = list(bbc.run()) + expected = self.errors( + B029(8, 0, vars=("*",)), + B029(13, 0, vars=("*",)), ) self.assertEqual(errors, expected) @@ -541,6 +611,18 @@ def test_b030(self): ) self.assertEqual(errors, expected) + @unittest.skipIf(sys.version_info < (3, 11), "requires 3.11+") + def test_b030_py311(self): + filename = Path(__file__).absolute().parent / "b030_py311.py" + bbc = BugBearChecker(filename=str(filename)) + errors = list(bbc.run()) + expected = self.errors( + B030(3, 0), + B030(13, 0), + B030(18, 0), + ) + self.assertEqual(errors, expected) + def test_b031(self): filename = Path(__file__).absolute().parent / "b031.py" bbc = BugBearChecker(filename=str(filename)) @@ -629,6 +711,21 @@ def test_b036(self) -> None: ) self.assertEqual(errors, expected) + @unittest.skipIf(sys.version_info < (3, 11), "requires 3.11+") + def test_b036_py311(self) -> None: + filename = Path(__file__).absolute().parent / "b036_py311.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), + B036(58, 0), + ) + self.assertEqual(errors, expected) + def test_b037(self) -> None: filename = Path(__file__).absolute().parent / "b037.py" bbc = BugBearChecker(filename=str(filename)) @@ -903,10 +1000,23 @@ def test_b904(self): bbc = BugBearChecker(filename=str(filename)) errors = list(bbc.run()) expected = [ - B904(10, 8), - B904(11, 4), - B904(16, 4), - B904(55, 16), + B904(10, 8, vars=("",)), + B904(11, 4, vars=("",)), + B904(16, 4, vars=("",)), + B904(55, 16, vars=("",)), + ] + self.assertEqual(errors, self.errors(*expected)) + + @unittest.skipIf(sys.version_info < (3, 11), "requires 3.11+") + def test_b904_py311(self): + filename = Path(__file__).absolute().parent / "b904_py311.py" + bbc = BugBearChecker(filename=str(filename)) + errors = list(bbc.run()) + expected = [ + B904(10, 8, vars=("*",)), + B904(11, 4, vars=("*",)), + B904(16, 4, vars=("*",)), + B904(55, 16, vars=("*",)), ] self.assertEqual(errors, self.errors(*expected))