From 3157b89d4168bc8264be3d2ca0a3c556c8d063fe Mon Sep 17 00:00:00 2001 From: John Litborn <11260241+jakkdl@users.noreply.github.com> Date: Sat, 29 Jun 2024 01:42:07 +0200 Subject: [PATCH] add b040: exception with note added not reraised or used (#477) * add b040: exception with note added not reraised or used * handle two more cases, with temp ugly code * add test case, clean up stray debug prints * break out b013,b029,b303 handler, clean up test file * simplify and clean up implementation. Replace `attr.ib(default=attr.Factory(...))` with `attr.ib(factory=...)` --- README.rst | 3 + bugbear.py | 166 +++++++++++++++++++++++++++---------- tests/b040.py | 184 ++++++++++++++++++++++++++++++++++++++++++ tests/test_bugbear.py | 15 ++++ 4 files changed, 325 insertions(+), 43 deletions(-) create mode 100644 tests/b040.py diff --git a/README.rst b/README.rst index a210968..cacab45 100644 --- a/README.rst +++ b/README.rst @@ -203,6 +203,8 @@ second usage. Save the result to a list if the result is needed multiple times. **B039**: ``ContextVar`` with mutable literal or function call as default. This is only evaluated once, and all subsequent calls to `.get()` would return the same instance of the default. This uses the same logic as B006 and B008, including ignoring values in ``extend-immutable-calls``. +**B040**: Caught exception with call to ``add_note`` not used. Did you forget to ``raise`` it? + Opinionated warnings ~~~~~~~~~~~~~~~~~~~~ @@ -357,6 +359,7 @@ FUTURE ~~~~~~ * Add B039, ``ContextVar`` with mutable literal or function call as default. +* Add B040: Exception with added note not reraised. (#474) 24.4.26 ~~~~~~~ diff --git a/bugbear.py b/bugbear.py index d3c3e7b..61aeaca 100644 --- a/bugbear.py +++ b/bugbear.py @@ -12,7 +12,7 @@ from contextlib import suppress from functools import lru_cache, partial from keyword import iskeyword -from typing import Dict, List, Set, Union +from typing import Dict, Iterable, Iterator, List, Set, Union import attr import pycodestyle @@ -55,7 +55,7 @@ class BugBearChecker: filename = attr.ib(default="(none)") lines = attr.ib(default=None) max_line_length = attr.ib(default=79) - visitor = attr.ib(init=False, default=attr.Factory(lambda: BugBearVisitor)) + visitor = attr.ib(init=False, factory=lambda: BugBearVisitor) options = attr.ib(default=None) def run(self): @@ -227,7 +227,7 @@ def _is_identifier(arg): return re.match(r"^[A-Za-z_][A-Za-z0-9_]*$", arg.value) is not None -def _flatten_excepthandler(node): +def _flatten_excepthandler(node: ast.expr | None) -> Iterator[ast.expr | None]: if not isinstance(node, ast.Tuple): yield node return @@ -356,16 +356,23 @@ def visit_ExceptHandler(self, node: ast.ExceptHandler): return super().generic_visit(node) +@attr.define +class B040CaughtException: + name: str + has_note: bool + + @attr.s class BugBearVisitor(ast.NodeVisitor): filename = attr.ib() lines = attr.ib() - b008_b039_extend_immutable_calls = attr.ib(default=attr.Factory(set)) - b902_classmethod_decorators = attr.ib(default=attr.Factory(set)) - node_window = attr.ib(default=attr.Factory(list)) - errors = attr.ib(default=attr.Factory(list)) - futures = attr.ib(default=attr.Factory(set)) - contexts = attr.ib(default=attr.Factory(list)) + b008_b039_extend_immutable_calls = attr.ib(factory=set) + b902_classmethod_decorators = attr.ib(factory=set) + node_window = attr.ib(factory=list) + errors = attr.ib(factory=list) + futures = attr.ib(factory=set) + contexts = attr.ib(factory=list) + b040_caught_exception: B040CaughtException | None = attr.ib(default=None) NODE_WINDOW_SIZE = 4 _b023_seen = attr.ib(factory=set, init=False) @@ -428,41 +435,20 @@ def visit(self, node): self.check_for_b018(node) - def visit_ExceptHandler(self, node): + def visit_ExceptHandler(self, node: ast.ExceptHandler) -> None: if node.type is None: self.errors.append(B001(node.lineno, node.col_offset)) self.generic_visit(node) return - handlers = _flatten_excepthandler(node.type) - names = [] - bad_handlers = [] - ignored_handlers = [] - for handler in handlers: - if isinstance(handler, (ast.Name, ast.Attribute)): - name = _to_name_str(handler) - if name is None: - ignored_handlers.append(handler) - else: - names.append(name) - elif isinstance(handler, (ast.Call, ast.Starred)): - ignored_handlers.append(handler) - else: - bad_handlers.append(handler) - 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)) - 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)) + + old_b040_caught_exception = self.b040_caught_exception + if node.name is None: + self.b040_caught_exception = None else: - maybe_error = _check_redundant_excepthandlers(names, node) - if maybe_error is not None: - self.errors.append(maybe_error) + self.b040_caught_exception = B040CaughtException(node.name, False) + + names = self.check_for_b013_b029_b030(node) + if ( "BaseException" in names and not ExceptBaseExceptionVisitor(node).re_raised() @@ -471,6 +457,13 @@ def visit_ExceptHandler(self, node): self.generic_visit(node) + if ( + self.b040_caught_exception is not None + and self.b040_caught_exception.has_note + ): + self.errors.append(B040(node.lineno, node.col_offset)) + self.b040_caught_exception = old_b040_caught_exception + def visit_UAdd(self, node): trailing_nodes = list(map(type, self.node_window[-4:])) if trailing_nodes == [ast.UnaryOp, ast.UAdd, ast.UnaryOp, ast.UAdd]: @@ -479,8 +472,10 @@ def visit_UAdd(self, node): self.generic_visit(node) def visit_Call(self, node): + is_b040_add_note = False if isinstance(node.func, ast.Attribute): self.check_for_b005(node) + is_b040_add_note = self.check_for_b040_add_note(node.func) else: with suppress(AttributeError, IndexError): if ( @@ -509,14 +504,28 @@ def visit_Call(self, node): self.check_for_b034(node) self.check_for_b039(node) self.check_for_b905(node) + + # no need for copying, if used in nested calls it will be set to None + current_b040_caught_exception = self.b040_caught_exception + if not is_b040_add_note: + self.check_for_b040_usage(node.args) + self.check_for_b040_usage(node.keywords) + self.generic_visit(node) + if is_b040_add_note: + # Avoid nested calls within the parameter list using the variable itself. + # e.g. `e.add_note(str(e))` + self.b040_caught_exception = current_b040_caught_exception + def visit_Module(self, node): self.generic_visit(node) - def visit_Assign(self, node): + def visit_Assign(self, node: ast.Assign) -> None: + self.check_for_b040_usage(node.value) if len(node.targets) == 1: t = node.targets[0] + if isinstance(t, ast.Attribute) and isinstance(t.value, ast.Name): if (t.value.id, t.attr) == ("os", "environ"): self.errors.append(B003(node.lineno, node.col_offset)) @@ -588,7 +597,12 @@ def visit_Compare(self, node): self.check_for_b015(node) self.generic_visit(node) - def visit_Raise(self, node): + def visit_Raise(self, node: ast.Raise): + if node.exc is None: + self.b040_caught_exception = None + else: + self.check_for_b040_usage(node.exc) + self.check_for_b040_usage(node.cause) self.check_for_b016(node) self.check_for_b904(node) self.generic_visit(node) @@ -605,6 +619,7 @@ def visit_JoinedStr(self, node): def visit_AnnAssign(self, node): self.check_for_b032(node) + self.check_for_b040_usage(node.value) self.generic_visit(node) def visit_Import(self, node): @@ -719,6 +734,40 @@ def _loop(node, bad_node_types): 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]: + handlers: Iterable[ast.expr | None] = _flatten_excepthandler(node.type) + names: list[str] = [] + bad_handlers: list[object] = [] + ignored_handlers: list[ast.Name | ast.Attribute | ast.Call | ast.Starred] = [] + + for handler in handlers: + if isinstance(handler, (ast.Name, ast.Attribute)): + name = _to_name_str(handler) + if name is None: + ignored_handlers.append(handler) + else: + names.append(name) + elif isinstance(handler, (ast.Call, ast.Starred)): + ignored_handlers.append(handler) + else: + bad_handlers.append(handler) + 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)) + 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)) + else: + maybe_error = _check_redundant_excepthandlers(names, node) + if maybe_error is not None: + self.errors.append(maybe_error) + return names + def check_for_b015(self, node): if isinstance(self.node_stack[-2], ast.Expr): self.errors.append(B015(node.lineno, node.col_offset)) @@ -1081,6 +1130,33 @@ def check_for_b035(self, node: ast.DictComp): B035(node.key.lineno, node.key.col_offset, vars=(node.key.id,)) ) + def check_for_b040_add_note(self, node: ast.Attribute) -> bool: + if ( + node.attr == "add_note" + and isinstance(node.value, ast.Name) + and self.b040_caught_exception + and node.value.id == self.b040_caught_exception.name + ): + self.b040_caught_exception.has_note = True + return True + return False + + def check_for_b040_usage(self, node: ast.expr | None) -> None: + def superwalk(node: ast.AST | list[ast.AST]): + if isinstance(node, list): + for n in node: + yield from ast.walk(n) + else: + yield from ast.walk(node) + + if not self.b040_caught_exception or node is None: + return + + for n in superwalk(node): + if isinstance(n, ast.Name) and n.id == self.b040_caught_exception.name: + self.b040_caught_exception = None + break + def _get_assigned_names(self, loop_node): loop_targets = (ast.For, ast.AsyncFor, ast.comprehension) for node in children_in_scope(loop_node): @@ -1766,7 +1842,7 @@ class NameFinder(ast.NodeVisitor): key is name string, value is the node (useful for location purposes). """ - names: Dict[str, List[ast.Name]] = attr.ib(default=attr.Factory(dict)) + names: Dict[str, List[ast.Name]] = attr.ib(factory=dict) def visit_Name( # noqa: B906 # names don't contain other names self, node: ast.Name @@ -1791,7 +1867,7 @@ class NamedExprFinder(ast.NodeVisitor): key is name string, value is the node (useful for location purposes). """ - names: Dict[str, List[ast.Name]] = attr.ib(default=attr.Factory(dict)) + names: Dict[str, List[ast.Name]] = attr.ib(factory=dict) def visit_NamedExpr(self, node: ast.NamedExpr): self.names.setdefault(node.target.id, []).append(node.target) @@ -2218,6 +2294,10 @@ def visit_Lambda(self, node): ) ) +B040 = Error( + message="B040 Exception with added note not used. Did you forget to raise it?" +) + # Warnings disabled by default. B901 = Error( message=( diff --git a/tests/b040.py b/tests/b040.py new file mode 100644 index 0000000..1f5d6ec --- /dev/null +++ b/tests/b040.py @@ -0,0 +1,184 @@ +def arbitrary_fun(*args, **kwargs): ... + + +# classic case +try: + ... +except Exception as e: + e.add_note("...") # error + +try: + ... +except Exception as e: # safe (other linters will catch this) + pass + +try: + ... +except Exception as e: + e.add_note("...") + raise # safe + +# other exception raised +try: + ... +except Exception as e: + f = ValueError() + e.add_note("...") # error + raise f + +# raised as cause +try: + ... +except Exception as e: + f = ValueError() + e.add_note("...") # safe + raise f from e + +# assigned to other variable +try: + ... +except Exception as e: + e.add_note("...") # safe + foo = e + +# "used" in function call +try: + ... +except Exception as e: + e.add_note("...") # safe + # not that printing the exception is actually using it, but we treat + # it being used as a parameter to any function as "using" it + print(e) + +try: + ... +except Exception as e: + e.add_note("...") # safe + list(e) + +# kwarg +try: + ... +except Exception as e: + e.add_note("...") # safe + arbitrary_fun(kwarg=e) + +# stararg +try: + ... +except Exception as e: + e.add_note("...") # safe + arbitrary_fun(*(e,)) + +try: + ... +except Exception as e: + e.add_note("...") # safe + arbitrary_fun(**{"e": e}) + + +# multiple ExceptHandlers +try: + ... +except ValueError as e: + e.add_note("") # error +except TypeError as e: + raise e + +# exception variable used before `add_note` +mylist = [] +try: + ... +except Exception as e: # safe + mylist.append(e) + e.add_note("") + + +# AnnAssign +try: + ... +except Exception as e: # safe + e.add_note("") + ann_assign_target: Exception = e + +# special case: e is only used in the `add_note` call itself +try: + ... +except Exception as e: # error + e.add_note(str(e)) + e.add_note(str(e)) + +# check nesting +try: + ... +except Exception as e: # error + e.add_note("") + try: + ... + except ValueError as e: + raise + +# questionable if this should error +try: + ... +except Exception as e: + e.add_note("") + e = ValueError() + +# *** unhandled cases *** + + +# This should ideally error +def exc_add_note_not_in_except(): + exc = ValueError() + exc.add_note("") + + +# but not this +def exc_add_note_not_in_except_safe(exc: ValueError): + exc.add_note("") + + +# we currently only check the target of the except handler, even though this clearly +# is hitting the same general pattern. But handling this case without a lot of false +# alarms is very tricky without more infrastructure. +try: + ... +except Exception as e: + e2 = ValueError() # should error + e2.add_note(str(e)) + + +def foo_add_note_in_excepthandler(): + e2 = ValueError() + try: + ... + except Exception as e: + e2.add_note(str(e)) # safe + raise e2 + + +# We don't currently handle lambdas or function definitions inside the exception +# handler. This isn't conceptually difficult, but not really worth it with current +# infrastructure +try: + ... +except Exception as e: # should error + e.add_note("") + f = lambda e: e + +try: + ... +except Exception as e: # should error + e.add_note("") + + def habla(e): + raise e + + +# We also don't track other variables +try: + ... +except Exception as e: + e3 = e + e3.add_note("") # should error diff --git a/tests/test_bugbear.py b/tests/test_bugbear.py index 249484e..2de9f3b 100644 --- a/tests/test_bugbear.py +++ b/tests/test_bugbear.py @@ -47,6 +47,7 @@ B036, B037, B039, + B040, B901, B902, B903, @@ -650,6 +651,20 @@ def test_b039(self) -> None: ) self.assertEqual(errors, expected) + def test_b040(self) -> None: + filename = Path(__file__).absolute().parent / "b040.py" + bbc = BugBearChecker(filename=str(filename)) + errors = list(bbc.run()) + expected = self.errors( + B040(7, 0), + B040(24, 0), + B040(83, 0), + B040(107, 0), + B040(114, 0), + B040(124, 0), + ) + self.assertEqual(errors, expected) + def test_b908(self): filename = Path(__file__).absolute().parent / "b908.py" bbc = BugBearChecker(filename=str(filename))