-
Notifications
You must be signed in to change notification settings - Fork 109
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
Changes from 3 commits
abb732c
48e3dff
d749c3d
3260555
755a08d
30c7c91
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)) | ||
|
||
NODE_WINDOW_SIZE = 4 | ||
_b023_seen = attr.ib(factory=set, init=False) | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wonder if it's worth creating an issue to break this up ... There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 = [] | ||
|
@@ -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]: | ||
|
@@ -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 ( | ||
|
@@ -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)) | ||
|
@@ -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) | ||
|
@@ -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): | ||
|
@@ -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): | ||
|
@@ -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=( | ||
|
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 |
There was a problem hiding this comment.
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