broad-exception-caught (W0718) is too strict

[RuRo]

Bug description

I believe that the following piece of code should not cause a broad-exception-caught warning:

# pylint: disable=missing-module-docstring,missing-function-docstring
import logging


def try_get_result_or_default(func, default):
    try:
        return func()
    except Exception as exc:
        logging.info(
            "Couldn't get the value from %r. Using the default value of %r",
            func,
            default,
            exc_info=exc,
        )
        return default

The documentation for broad-exception-caught doesn't actually provide strong justification that catching Exception is always wrong. It links to a stack overflow post, but that post mainly discusses bare except: clauses, not except Exception as e:. Bare except: is ALWAYS wrong, but there are a lot of cases where except Exception is perfectly valid (even without rethrowing).

I believe that the above code is perfectly reasonable and should not produce a warning. The func function might raise arbitrary exceptions, so there is no way to rewrite this above code. Additionally, PEP8 (when talking about bare except: clauses) states that broad exception handling is valid if "the exception handler will be printing out or logging the traceback" which is the case here.

Configuration

No response

Command used

pylint example.py

Pylint output

************* Module example
example.py:8:11: W0718: Catching too general exception Exception (broad-exception-caught)

Expected behavior

(no warnings)

Pylint version

pylint 2.17.5
astroid 2.15.6
Python 3.11.3 (main, Jun  5 2023, 09:32:32) [GCC 13.1.1 20230429]

OS / Environment

No response

Additional dependencies

No response

[Pierre-Sassoulas]

I'm -1 on this let me explain why. Quoting the doc:

If you use a naked except Exception: clause, you might end up catching exceptions other than the ones you expect to catch. This can hide bugs or make it harder to debug programs when unrelated errors are hidden.

In the case of your example if there's something like this in func:

def func():
    distance = request.get("...")
    time = request.get("...")
    return distance / time

Then catching all Exception will hide the ZeroDivisionError that are a problem in the implementation of func and not with the actual retrieval of values. It will make things harder to debug because now the zero division error fail silently. If you know you just want to catch everything because func is perfect then you can disable locally imo.

[RuRo]

@Pierre-Sassoulas I disagree with your assessment. Even in the example that you gave, what makes you think that ZeroDivisionError was not intended to be caught in this case? You don't know where func is defined or what kind of contract it follows. It's a completely arbitrary function. It might be defined by the user. It might even raise some CustomException that inherits directly from Exception.

The intent is pretty clear here - try to run the function, but fall back to a default value if anything goes wrong. Additionally, it doesn't necessarily "hide" anything. Would you still say that it hides the ZeroDivisionError if it used logging.error instead of .info? Or more generally - if it just returned the caught Exception instead of the default.

Also note, that we aren't necessarily talking about whether any particular example with except Exception: is valid, but more about whether all (or at least most) such pieces of code are invalid. How often does a (non-newbie) python programmer write except Exception as e: because they are just lazy to narrow down the correct Exception type rather than actually meaning "yes, catch ALL exception types (ignoring SystemExit and friends)".

[DanielNoord]

I think there is a significant chance that a lot of newbies write this code without knowing about the issues with broad except. That's why I think it is still a valid message. If have seen very little cases where a broad except was really useful in production code and shouldn't have been replaced with specific excepts.

If you really "know what you're doing" you can just disable the message, but in general I would just add a local disable for the specific cases where applicable. A linter is opinionated by design: it warns about patterns that it thinks are problematic. If you don't agree with it just disable it?

[alumuko]

Can I disable "except Exception" but enable "except BaseException"?

[DanielNoord]

Yes!

See this configuration option:
overgeneral-exception

[Pierre-Sassoulas]

To be explicit, overgeneral-exceptions = ["builtins.Exception"] would do what you want. Although it feel like a hack because it doesn't make semantic sense to use it like that (as BaseException is broader than exception), it is very unlikely that we're going to change the check to raise for exception class broader than the one included in the settings, it's only a list of hard coded values that should raise,

[DiskCrasher]

On a related note, is the intent of this warning to persuade the developer to never use catch Exception? I might understand this if it's the only exception being caught but if it follows one or more other catch statements then shouldn't that be sufficient? I want to say with C# you get a similar warning but only when you use catch Exception exclusively.

[DanielNoord]

Yes it is. Or opt in by explicitly disabling the linter, which can be sensible in the outermost try ... except of an application.

[DiskCrasher]

That seems rather draconian and obtrusive when the language obviously supports this, especially when using multiple catch statements. Looks like best option is to just globally disable this check so your code doesn't become littered with linter ignores.

[DanielNoord]

In my personal experience I have never had to put too many ignore in the codebase. This is also the purpose of a linter: warn you about features the language supports but that you should not use. This makes a linter opinionated by definition and therefore not all warnings will be to your liking.

[DiskCrasher]

I get it, but there also comes a point where a linter starts complaining too much. Sometimes you just need to trust that the dev knows what he/she is doing. It's not uncommon to see code that catches a top level exception instead of catching each individual one, which requires a lot more research and coding to accomplish (especially in Python where they don't make it obvious what exceptions may be thrown).

[DanielNoord]

I think you're describing the issue that pylint is warning you about perfectly. To write the most robust code, which is what a linter is nudging you towards, you should that investigation to make sure you're not catching and potentially swallowing something you shouldn't. Of course, there are situations where that is fine and there you can explicitly disable the check.

Anyway, I don't think pylint will change default behaviour here and we provide a configuration option to get the behaviour you desire. @Pierre-Sassoulas shall we close this? I think the discussion has been successful in discovering all arguments and coming up with solutions and alternatives.