pylint icon indicating copy to clipboard operation
pylint copied to clipboard

overgeneral-exceptions doesn't work for exceptions outside builtins

Open Jackenmen opened this issue 3 years ago • 3 comments

Bug description

# pylint: disable=missing-docstring

class GeneralException(Exception):
    pass


try:
    raise GeneralException('...')
except GeneralException:
    ...
# for comparison, pylint triggers on this
except Exception:
    ...

Configuration

No response

Command used

pylint repro.py --overgeneral-exceptions BaseException,Exception,GeneralException

Pylint output

************* Module repro
repro.py:12:7: W0703: Catching too general exception Exception (broad-except)

------------------------------------------------------------------
Your code has been rated at 8.75/10 (previous run: 8.75/10, +0.00)

Expected behavior

I expected to get broad-except message for line 9 (which is not happening regardless of whether except Exception is there or not).

This is also reproducible when making a whole package and referring to the exception class using package.GeneralException name (which I personally think would be the best way of specifying non-builtin exceptions).

This seems to vastly limit the usefulness of --overgeneral-exceptions.

Pylint version

pylint 2.15.3
astroid 2.12.10
Python 3.10.4 (main, Jun 29 2022, 12:14:53) [GCC 11.2.0]

OS / Environment

Kubuntu 22.04.1 LTS

Additional dependencies

No response

Jackenmen avatar Sep 19 '22 19:09 Jackenmen

For some reason we check whether the exception is in builtins here: https://github.com/PyCQA/pylint/blob/7ef145b9a7042ab884aa2d7b554fcdd96768815a/pylint/checkers/exceptions.py#L554

I'd be happy to review a PR that removes this check, it does indeed seem silly.

DanielNoord avatar Sep 19 '22 20:09 DanielNoord

Do you think pylint should only check the name, or should it try matching the module name if a dotted name is provided and built-in if not?

The only public usage I could find was my own (using the dotted name) and Home Assistant (using just the exception name), everyone else who specifies overgeneral-exceptions only specifies one or more of Exception, BaseException, and StandardError (which is surely just leftover from Python 1.x :smile:)

Jackenmen avatar Sep 19 '22 20:09 Jackenmen

Do you think pylint should only check the name, or should it try matching the module name if a dotted name is provided and built-in if not?

The only public usage I could find was my own (using the dotted name) and Home Assistant (using just the exception name), everyone else who specifies overgeneral-exceptions only specifies one or more of Exception, BaseException, and StandardError (which is surely just leftover from Python 1.x 😄)

Perhaps do:

if name in {"Exception", "BaseException"} and name in self.linter.config.overgeneral_exceptions and exception.root().name == utils.EXCEPTIONS_MODULE`
or exception.qname() in self.linter.config.overgeneral_exceptions

So, for Exception and BaseException allow not having them as qualified names and for all others make sure they should include the module name. This is backwards incompatible but since the current form is completely unusable we can get away with this I think. Personally I would even prefer if we could issue a DeprecationWarning for Exception and not builtins.Exception (so case 1 in the code) but that is perhaps a bit too much for someone just looking to quickly fix this.

DanielNoord avatar Sep 19 '22 20:09 DanielNoord