flake8-bugbear icon indicating copy to clipboard operation
flake8-bugbear copied to clipboard

Suggestion: New lint to forbid `except BaseException:`

Open untitaker opened this issue 3 years ago • 6 comments

This goes further than the existing lint against except:. The rationale is that in many codebases (celery task bodies and request handlers, in our case) it's extremely unlikely that except BaseException: is ever a valid way to handle errors. In our org we've seen people (usually ones who are not writing Python daily) introduce except BaseException: as per a suggestion from the lint against except: without diving deeper into the topic, and causing all kinds of problems. With such a new lint forbidding BaseException as well they'd be forced to think more about what exceptions they really want to handle, and hopefully they'll reconsider when writing except (Exception, SystemExit, KeyboardInterrupt, ...).

If y'all would accept such an opt-in lint I'll start working on it immediately.

untitaker avatar Jun 29 '21 15:06 untitaker

I agree this is dangerous and never needed. I am even open to having the release this makes it into default to warning on this. What do others think here. Please speak up if you don't want this on by default.

Please just have good unittests + nice explanation in the errors raised + README.md. Bonus points, add to CHANGES to please.

cooperlees avatar Jun 29 '21 16:06 cooperlees

FWIW I'd personally rather see this as opt-in rather than default. I find myself using the following pattern on a rare occasion

try:
    ...
except BaseException:
    # some teardown or cleanup code here
    raise

As long as the re-raise is there and the teardown code doesn't raise subclasses of Exception, I don't see this being that bad. And since a bare except: is prohibited, one already has to be very explicit about doing it.

If the re-raise is missing I definitely think a linter warning is justified.

hukkin avatar Aug 26 '21 15:08 hukkin

I think still allowing it as long as the exception is unconditionally reraised makes sense. I didn't think about how complicated that would be to implement yet

untitaker avatar Aug 26 '21 17:08 untitaker

It's not easy to check that every code path in the except-block re-raises the exception, but we've done it in flake8-trio fwiw.

Zac-HD avatar Nov 30 '22 06:11 Zac-HD

I'm happy to implement this - would the consensus be default or opt-in?

I was also looking for similar checks for plain Exception similar to pylint's broad-exception-raised and broad-exception-caught - these should probably be opt-in. Would be happy to implement these as well, any interest?

r-downing avatar Dec 02 '23 23:12 r-downing

So long as we only trigger on blocks that don't re-raise, I think default enabled is reasonable. Thanks for volunteering to help with this!

Zac-HD avatar Dec 03 '23 00:12 Zac-HD