flake8-bugbear
flake8-bugbear copied to clipboard
Suggestion: New lint to forbid `except BaseException:`
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.
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.
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.
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
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.
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?
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!