Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add b040: exception with note added not reraised or used #477

Merged
merged 6 commits into from
Jun 28, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,10 @@ MIT
Change Log
----------

Future
~~~~~~
* Add B040: Exception with added note not reraised. (#474)

24.4.26
~~~~~~~

Expand Down
75 changes: 71 additions & 4 deletions bugbear.py
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,7 @@ class BugBearVisitor(ast.NodeVisitor):
errors = attr.ib(default=attr.Factory(list))
futures = attr.ib(default=attr.Factory(set))
contexts = attr.ib(default=attr.Factory(list))
exceptions_tracked: dict[str, bool | None] = attr.ib(default=attr.Factory(dict))
Copy link
Collaborator

@cooperlees cooperlees Jun 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This highlights we should type annotate all this code. #478


NODE_WINDOW_SIZE = 4
_b023_seen = attr.ib(factory=set, init=False)
Expand Down Expand Up @@ -428,11 +429,16 @@ def visit(self, node):

self.check_for_b018(node)

def visit_ExceptHandler(self, node):
def visit_ExceptHandler(self, node): # noqa: C901 # too complex
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonder if it's worth creating an issue to break this up ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is quite messy, with code for B013, B029, B030, B036 and B040 interwoven. Fairly straightforward to split out B013, B029 and B030 though - so I'll just go ahead and do that.

if node.type is None:
self.errors.append(B001(node.lineno, node.col_offset))
self.generic_visit(node)
return

# used by B040 to check for add_note exceptions that are unraised
old_exceptions_tracked = self.exceptions_tracked
self.exceptions_tracked = {node.name: None}

handlers = _flatten_excepthandler(node.type)
names = []
bad_handlers = []
Expand Down Expand Up @@ -471,6 +477,15 @@ def visit_ExceptHandler(self, node):

self.generic_visit(node)

for _exc_name, exc_status in self.exceptions_tracked.items():
if exc_status is None:
continue
# TODO: if we only track the caught exception, we can directly
# point to `node.name.lineno`. If we track more exceptions we should
# store the node of the variable in `exceptions_tracked` to point to here.
self.errors.append(B040(node.lineno, node.col_offset))
self.exceptions_tracked = old_exceptions_tracked

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]:
Expand All @@ -479,8 +494,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 (
Expand Down Expand Up @@ -508,14 +525,27 @@ def visit_Call(self, node):
self.check_for_b028(node)
self.check_for_b034(node)
self.check_for_b905(node)
self.generic_visit(node)

if not is_b040_add_note:
self.check_for_b040_usage(node.args)
self.check_for_b040_usage(node.keywords)
self.generic_visit(node)
else:
# TODO: temp dirty hack to avoid nested calls within the parameter list
# using the variable itself. (i.e. `e.add_note(str(e))`)
# will clean up, complexity depends on decision of scope of the check
current_exceptions_tracked = self.exceptions_tracked.copy()
self.generic_visit(node)
self.exceptions_tracked = current_exceptions_tracked

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))
Expand Down Expand Up @@ -587,7 +617,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.exceptions_tracked.clear()
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)
Expand All @@ -604,6 +639,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):
Expand Down Expand Up @@ -1054,6 +1090,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 node.value.id in self.exceptions_tracked
):
self.exceptions_tracked[node.value.id] = 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.exceptions_tracked or node is None:
return
for name in (
n.id
for n in superwalk(node)
if isinstance(n, ast.Name) and n.id in self.exceptions_tracked
):
del self.exceptions_tracked[name]

def _get_assigned_names(self, loop_node):
loop_targets = (ast.For, ast.AsyncFor, ast.comprehension)
for node in children_in_scope(loop_node):
Expand Down Expand Up @@ -2166,6 +2229,10 @@ def visit_Lambda(self, node):
message="B037 Class `__init__` methods must not return or yield and any values."
)

B040 = Error(
message="B040 Exception with added note not used. Did you forget to raise it?"
)

# Warnings disabled by default.
B901 = Error(
message=(
Expand Down
143 changes: 143 additions & 0 deletions tests/b040.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
def arbitrary_fun(*args, **kwargs):
...

try:
...
except Exception as e:
e.add_note("...") # error

try:
...
except Exception as e: # safe (handled by most type checkers)
pass

try:
...
except Exception as e:
e.add_note("...")
raise # safe

try:
...
except Exception as e:
e.add_note("...")
raise e # safe

try:
...
except Exception as e:
f = ValueError()
e.add_note("...") # error
raise f

try:
...
except Exception as e:
f = ValueError()
e.add_note("...") # safe
raise f from e

try:
...
except Exception as e:
e.add_note("...") # safe
foo = e

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)

try:
...
except Exception as e:
e.add_note("...") # safe
arbitrary_fun(kwarg=e)

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})


try:
...
except ValueError as e:
e.add_note("") # error
except TypeError as e:
raise e

mylist = []
try:
...
except Exception as e:
mylist.append(e)
e.add_note("")


def exc_add_note_not_in_except():
exc = ValueError()
exc.add_note("") # should maybe error?

try:
...
except Exception as e:
e2 = ValueError()
e2.add_note("") # should maybe error?
raise e

try:
...
except Exception as e:
e.add_note("")
e = ValueError()

try:
...
except Exception as e: # should error, but we don't handle lambdas
e.add_note("")
f = lambda e: e

try:
...
except Exception as e: # should error, but we don't handle nested functions
e.add_note("")
def habla(e):
raise e

try:
...
except Exception as e: # safe
e.add_note("")
ann_assign_target: Exception = e

try:
...
except Exception as e: # error
e.add_note(str(e))

# check nesting
try:
...
except Exception as e: # error
e.add_note("")
try:
...
except ValueError as f:
pass
15 changes: 15 additions & 0 deletions tests/test_bugbear.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
B035,
B036,
B037,
B040,
B901,
B902,
B903,
Expand Down Expand Up @@ -636,6 +637,20 @@ def test_b037(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(6, 0),
B040(28, 0),
B040(81, 0),
B040(107, 0),
B040(132, 0),
B040(138, 0),
)
self.assertEqual(errors, expected)

def test_b908(self):
filename = Path(__file__).absolute().parent / "b908.py"
bbc = BugBearChecker(filename=str(filename))
Expand Down