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

r-downing · 2023-12-13T03:04:57Z

Started implementing this new check per #174

Currently just looking for BaseException - I figured Exception should maybe be a separate, optional one.

It only checks for re-raises at the top-level of the except. I thought maybe checking all branches of of a complex except block to see they all re-raise might be overkill - WDYT?

This does contradict the advice (and failing test-cases) of 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:.Flake8(B001)

So those would also need to be updated, and should probably change that wording - what do you think should be suggested instead? Or just "use a more specific exception" or something like that?

cooperlees

I figured Exception should maybe be a separate, optional one.

I agree here, this will be much more controversial.

It only checks for re-raises at the top-level of the except. I thought maybe checking all branches of of a complex except block to see they all re-raise might be overkill - WDYT?

I'm happy to start simple here and we can add to it if people complain about to much noise ... But covering as many re-raises as we can will be key.

So those would also need to be updated, and should probably change that wording - what do you think should be suggested instead? Or just "use a more specific exception" or something like that?

I'm happy with "please use a more specific exception" in the wording. Using BaseException shouldn't really be needed in any non core cpython/runtime libraries imo.

Please fix the unit tests. Did black break it?

cooperlees · 2023-12-15T01:53:01Z

README.rst

@@ -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.


Do we also catch a bare except here and error?

cooperlees · 2023-12-15T01:54:00Z

bugbear.py

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


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

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

cooperlees · 2023-12-15T01:54:44Z

tests/b036.py

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


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

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?

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 ...

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

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

r-downing · 2023-12-15T02:15:01Z

Please fix the unit tests. Did black break it?

Oh I think the unittests broken currently because the B001 test is now throwing new B036 errors in addition to the expected ones. I might change the "good" examples in the B001 test to something other than BaseException since that's the other thing we're trying to discourage

r-downing · 2023-12-15T02:33:58Z

Ok fixed up the unittests and wording. Regarding finding the re-raise - I'm thinking I can just do a recursive search and see if there's a raise in any branch? Rather than the top-level? Shouldn't be too hard to implement...

I could imagine some weird intentional case where you might have something like

except BaseException as e:
    if ...:
        raise e
    else:
        # don't raise

WDYT?

cooperlees · 2023-12-15T03:00:01Z

That sounds good to me for a first effort finding re-raises.

Did you check out what flake8-trio did that was mention on the issue? Could maybe steal some logic.

r-downing · 2023-12-15T03:52:38Z

Did you check out what flake8-trio did that was mention on the issue? Could maybe steal some logic.

Took a look at it, seems like they're trying to check all paths. I just added a basic recursive search for a corresponding raise on any path, to start with. I think this might make more sense actually, or at least be a little more forgiving.

It ignores raise within a nested except, as I think it should. Check out the b036.py for examples

cooperlees

Sounds a good start to me. Thanks for working through and iterating!

Implement B036 check for except BaseException without re-raising

8e17d43

cooperlees requested changes Dec 15, 2023

View reviewed changes

handle case where exception is re-raised by name. Fix unittests

e7eb28e

r-downing marked this pull request as draft December 15, 2023 02:34

recursively search for re-raise

30b5ec5

r-downing marked this pull request as ready for review December 15, 2023 03:52

r-downing requested a review from cooperlees December 15, 2023 03:52

cooperlees approved these changes Dec 18, 2023

View reviewed changes

cooperlees merged commit dd4fec5 into PyCQA:main Dec 18, 2023
6 checks passed

dejvidq mentioned this pull request Sep 4, 2024

Suggestion: New lint to forbid except BaseException: #174

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

r-downing commented Dec 13, 2023

cooperlees left a comment

cooperlees Dec 15, 2023

cooperlees Dec 15, 2023

r-downing Dec 15, 2023

cooperlees Dec 15, 2023

r-downing Dec 15, 2023

cooperlees Dec 15, 2023

r-downing Dec 15, 2023

cooperlees Dec 15, 2023

r-downing commented Dec 15, 2023

r-downing commented Dec 15, 2023

cooperlees commented Dec 15, 2023

r-downing commented Dec 15, 2023

cooperlees left a comment

		@@ -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.

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

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

Conversation

r-downing commented Dec 13, 2023

cooperlees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-downing commented Dec 15, 2023

r-downing commented Dec 15, 2023

cooperlees commented Dec 15, 2023

r-downing commented Dec 15, 2023

cooperlees left a comment

Choose a reason for hiding this comment

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

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