search

PyCQA / flake8-bugbear

A plugin for Flake8 finding likely bugs and design problems in your program. Contains warnings that don't belong in pyflakes and pycodestyle.
MIT License
1.05k stars 103 forks source link

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

Closed r-downing closed 6 months ago

r-downing commented 6 months ago

Started implementing this new check per https://github.com/PyCQA/flake8-bugbear/issues/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?

r-downing commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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