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

Implement B036 check for except BaseException without re-raising #438

Merged
merged 3 commits into from
Dec 18, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 2 additions & 0 deletions README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we also catch a bare except here and error?


Opinionated warnings
~~~~~~~~~~~~~~~~~~~~

Expand Down
13 changes: 11 additions & 2 deletions bugbear.py
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,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 any(
isinstance(subnode, ast.Raise) and subnode.exc is None
for subnode in node.body
):
self.errors.append(B036(node.lineno, node.col_offset))

self.generic_visit(node)

def visit_UAdd(self, node):
Expand Down Expand Up @@ -668,7 +674,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)
)
)
)
Expand All @@ -677,7 +683,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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just making tuples since they don't need to be immutable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually not a tuple, just parenthesizing the generator:

>>> x = [i for i in range(3)] 
>>> x
[0, 1, 2]
>>> x = (i for i in range(3))
>>> x
<generator object <genexpr> at 0x0000021C2DFCAA80>

Just a tiny efficiency improvement, rather than having iterate the generator to construct a list, then iterate through the list to find the thing... just iterate the generator directly

)
)
and len(item_context.args) == 1
Expand Down Expand Up @@ -1953,6 +1959,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(
Expand Down
35 changes: 35 additions & 0 deletions tests/b036.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@

try:
pass
except BaseException: # bad
print("aaa")
pass
Comment on lines +2 to +6
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add and make this work easily? If not, no worries.

Suggested change
try:
pass
except BaseException: # bad
print("aaa")
pass
try:
pass
except: # bad
print("aaa")
pass

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Look like bare except already caught by both flake8 and bugbear:

try:
    pass
except:
    pass

gives both:

  • 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:.
  • E722 do not use bare 'except'

Should this be consolidated with B001, rather than a new error?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm - that's a good question. I'd like that, but I wonder if that would upset people?

I vote yes, but lets see if any other contributors / maintainers weigh in here ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess keeping separate also gives you the flexibility to enable/disable either one? Maybe that's the safer option?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah ok, I like this now the more I think about it.



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
13 changes: 13 additions & 0 deletions tests/test_bugbear.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
B033,
B034,
B035,
B036,
B901,
B902,
B903,
Expand Down Expand Up @@ -597,6 +598,18 @@ 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),
)
self.assertEqual(errors, expected)

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