BLE001: conditional handling of errors not detected

[bentheiii]

keywords: BLE001, log, exception

The following code snippet does not trigger the BLE001 rule:

from logging import exception


try:
    pass
except Exception:
    exception("An error occurred")

but this one does:

from logging import exception

try:
    pass
except Exception:
    if True:
        exception("An error occurred")
    else:
        exception("An error occurred")

This is because ruff doesn't seem to consider the case the both branches of an if handle the error.

The reason we run into this is because sometimes we use code like the following:

from logging import exception

try:
    pass
except Exception as ex:
    error_data = gather_diagnostic_data(...)
    if isinstance(ex, SomespecificError):
        exception("A specific error occurred", extra=error_data)
    else:
        exception("A generic error occurred", extra=error_data)

[JonathanPlasse]

• Something similar was discussed previously in [Question] Re-raising errors and BLE001 #1119 (comment).

[rahulssheth]

We were recently investigating the 0.4.4 upgrade and found that snippets like this:

def method() -> None:
    try:
           raise Exception()
    except Exception:
           if False:
               raise
           else:
                  return None

would no longer raise BLE001 (Ruff playground for reference: https://play.ruff.rs/f65629f5-eda0-43f0-a85f-c05a0be1cb9c) because there's a conditional handling the exception. This seems a bit problematic, given that the original issue was about cases where both (or rather, all) branches of a conditional handle the exception, but that isn't the case here. Totally understandable if this is out of scope, just wanted to put it on the radar. Thanks (apologies if this should be filed in another issue)!

[JonathanPlasse]

A more complex visitor would need to be implemented like in flake8-trio.
#1119 (comment)

[charliermarsh]

Thanks @rahulssheth. My overall feeling was that if you're re-raising even within a branch, then the use of the catch-all exception was probably intentional, and so likely to be a false positive. Are you seeing real cases where you do want it to be flagged, despite a re-raise?

[rahulssheth]

@charliermarsh good point! In most scenarios, if there's some sort of explicit handling with a re-raise, even if there isn't always a re-raise, it likely isn't an problem. I mostly flagged this issue because I think there's a case to be made that false positives with an ignore should be preferred to false negatives here, but I haven't seen any real cases that merit a change of approach. Appreciate the quick feedback!