Skip to content

overgeneral-exceptions doesn't work for exceptions outside builtins #7495

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Jackenmen opened this issue Sep 19, 2022 · 3 comments · Fixed by #7497
Closed

overgeneral-exceptions doesn't work for exceptions outside builtins #7495

Jackenmen opened this issue Sep 19, 2022 · 3 comments · Fixed by #7497
Labels
Bug 🪲 Configuration Related to configuration Enhancement ✨ Improvement to a component Good first issue Friendly and approachable by new contributors Needs PR This issue is accepted, sufficiently specified and now needs an implementation

Comments

@Jackenmen
Copy link
Contributor

Jackenmen commented Sep 19, 2022

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 Jackenmen added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label Sep 19, 2022
@DanielNoord
Copy link
Collaborator

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 DanielNoord added Bug 🪲 Enhancement ✨ Improvement to a component Configuration Related to configuration Needs PR This issue is accepted, sufficiently specified and now needs an implementation Good first issue Friendly and approachable by new contributors and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Sep 19, 2022
@Jackenmen
Copy link
Contributor Author

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 😄)

@DanielNoord
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🪲 Configuration Related to configuration Enhancement ✨ Improvement to a component Good first issue Friendly and approachable by new contributors Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants