Use qualified name when checking for overgeneral exceptions (#7497)

* Update tests * Use qualified name when checking for overgeneral exceptions * WIP: Add deprecation warning * Add changelog fragment * Use qualified name in test case * spell check fix * Update changelog fragment with suggested fixes Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com> * Move OVERGENERAL_EXCEPTIONS directly to the default value in dict * Mark as TODO for pylint 3.0 * Properly warn on each occurrence of name without dots * Update the warning per the review * Rephrase the warning to mention pylint 3.0 * Remove unnecessary nesting of the if condition * Quote the exception name in deprecation warning * Use config value for overgeneral exceptions in broad-exception-raised * Infer qualified name of the exception in broad-exception-raised * e.g. -> maybe? Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com> * Suppress missing class docstrings * Add few more tests for broad-exception-raised Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com> * Fix unexpected missing-raise-from * Revert "Fix unexpected missing-raise-from" This reverts commit d796e72. * Revert "Add few more tests for broad-exception-raised" This reverts commit e5a193e. * Change confidence of broad-exception-raised from HIGH to INFERENCE * Only trigger broad-exception-raised for raise with new exc instance * Update overgeneral-exceptions definition in example pylintrc file * Update pylint/checkers/exceptions.py Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com> Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
pylint-dev · Nov 13, 2022 · ac65bdc · ac65bdc
1 parent 88701fa
commit ac65bdc
Show file tree

Hide file tree

Showing 13 changed files with 134 additions and 28 deletions.
diff --git a/doc/user_guide/configuration/all-options.rst b/doc/user_guide/configuration/all-options.rst
@@ -812,7 +812,7 @@ Standard Checkers
 .. code-block:: toml
 
    [tool.pylint.exceptions]
-   overgeneral-exceptions = ["BaseException", "Exception"]
+   overgeneral-exceptions = ["builtins.BaseException", "builtins.Exception"]
 
 
 

diff --git a/doc/whatsnew/fragments/7495.bugfix b/doc/whatsnew/fragments/7495.bugfix
@@ -0,0 +1,4 @@
+Allow specifying non-builtin exceptions in the ``overgeneral-exception`` option
+using an exception's qualified name.
+
+Closes #7495
diff --git a/examples/pylintrc b/examples/pylintrc
@@ -307,8 +307,8 @@ min-public-methods=2
 [EXCEPTIONS]
 
 # Exceptions that will emit a warning when caught.
-overgeneral-exceptions=BaseException,
-                       Exception
+overgeneral-exceptions=builtins.BaseException,
+                       builtins.Exception
 
 
 [FORMAT]

diff --git a/examples/pyproject.toml b/examples/pyproject.toml
@@ -264,7 +264,7 @@ min-public-methods = 2
 
 [tool.pylint.exceptions]
 # Exceptions that will emit a warning when caught.
-overgeneral-exceptions = ["BaseException", "Exception"]
+overgeneral-exceptions = ["builtins.BaseException", "builtins.Exception"]
 
 [tool.pylint.format]
 # Expected format of line ending, e.g. empty (any line ending), LF or CRLF.

diff --git a/pylint/checkers/exceptions.py b/pylint/checkers/exceptions.py
@@ -8,6 +8,7 @@
 
 import builtins
 import inspect
+import warnings
 from collections.abc import Generator
 from typing import TYPE_CHECKING, Any
 
@@ -59,8 +60,6 @@ def _is_raising(body: list[nodes.NodeNG]) -> bool:
     return any(isinstance(node, nodes.Raise) for node in body)
 
 
-OVERGENERAL_EXCEPTIONS = ("BaseException", "Exception")
-
 MSGS: dict[str, MessageDefinitionTuple] = {
     "E0701": (
         "Bad except clauses order (%s)",
@@ -201,13 +200,23 @@ def visit_name(self, node: nodes.Name) -> None:
             self._checker.add_message(
                 "notimplemented-raised", node=self._node, confidence=HIGH
             )
-        elif node.name in OVERGENERAL_EXCEPTIONS:
-            self._checker.add_message(
-                "broad-exception-raised",
-                args=node.name,
-                node=self._node,
-                confidence=HIGH,
-            )
+            return
+
+        try:
+            exceptions = list(_annotated_unpack_infer(node))
+        except astroid.InferenceError:
+            return
+
+        for _, exception in exceptions:
+            if isinstance(
+                exception, nodes.ClassDef
+            ) and self._checker._is_overgeneral_exception(exception):
+                self._checker.add_message(
+                    "broad-exception-raised",
+                    args=exception.name,
+                    node=self._node,
+                    confidence=INFERENCE,
+                )
 
     def visit_call(self, node: nodes.Call) -> None:
         if isinstance(node.func, nodes.Name):
@@ -278,7 +287,7 @@ class ExceptionsChecker(checkers.BaseChecker):
         (
             "overgeneral-exceptions",
             {
-                "default": OVERGENERAL_EXCEPTIONS,
+                "default": ("builtins.BaseException", "builtins.Exception"),
                 "type": "csv",
                 "metavar": "<comma-separated class names>",
                 "help": "Exceptions that will emit a warning when caught.",
@@ -288,6 +297,18 @@ class ExceptionsChecker(checkers.BaseChecker):
 
     def open(self) -> None:
         self._builtin_exceptions = _builtin_exceptions()
+        for exc_name in self.linter.config.overgeneral_exceptions:
+            if "." not in exc_name:
+                warnings.warn_explicit(
+                    "Specifying exception names in the overgeneral-exceptions option"
+                    " without module name is deprecated and support for it"
+                    " will be removed in pylint 3.0."
+                    f" Use fully qualified name (maybe 'builtins.{exc_name}' ?) instead.",
+                    category=UserWarning,
+                    filename="pylint: Command line or configuration file",
+                    lineno=1,
+                    module="pylint",
+                )
         super().open()
 
     @utils.only_required_for_messages(
@@ -592,10 +613,8 @@ def visit_tryexcept(self, node: nodes.TryExcept) -> None:
                                 args=msg,
                                 confidence=INFERENCE,
                             )
-                    if (
-                        exception.name in self.linter.config.overgeneral_exceptions
-                        and exception.root().name == utils.EXCEPTIONS_MODULE
-                        and not _is_raising(handler.body)
+                    if self._is_overgeneral_exception(exception) and not _is_raising(
+                        handler.body
                     ):
                         self.add_message(
                             "broad-exception-caught",
@@ -614,6 +633,15 @@ def visit_tryexcept(self, node: nodes.TryExcept) -> None:
 
                 exceptions_classes += [exc for _, exc in exceptions]
 
+    def _is_overgeneral_exception(self, exception: nodes.ClassDef) -> bool:
+        return (
+            exception.qname() in self.linter.config.overgeneral_exceptions
+            # TODO: 3.0: not a qualified name, deprecated
+            or "." not in exception.name
+            and exception.name in self.linter.config.overgeneral_exceptions
+            and exception.root().name == utils.EXCEPTIONS_MODULE
+        )
+
 
 def register(linter: PyLinter) -> None:
     linter.register_checker(ExceptionsChecker(linter))
diff --git a/pylintrc b/pylintrc
@@ -510,7 +510,7 @@ preferred-modules=
 
 # Exceptions that will emit a warning when being caught. Defaults to
 # "Exception"
-overgeneral-exceptions=Exception
+overgeneral-exceptions=builtins.Exception
 
 
 [TYPING]

diff --git a/tests/functional/b/bad_exception_cause.txt b/tests/functional/b/bad_exception_cause.txt
@@ -3,4 +3,4 @@ bad-exception-cause:16:4:16:34:test:Exception cause set to something which is no
 bad-exception-cause:22:4:22:36:test:Exception cause set to something which is not an exception, nor None:INFERENCE
 catching-non-exception:30:7:30:15::"Catching an exception which doesn't inherit from Exception: function":UNDEFINED
 bad-exception-cause:31:4:31:28::Exception cause set to something which is not an exception, nor None:INFERENCE
-broad-exception-raised:31:4:31:28::"Raising too general exception: Exception":HIGH
+broad-exception-raised:31:4:31:28::"Raising too general exception: Exception":INFERENCE
diff --git a/tests/functional/b/broad_exception_caught.py b/tests/functional/b/broad_exception_caught.py
@@ -1,6 +1,14 @@
 # pylint: disable=missing-docstring
 __revision__ = 0
 
+class CustomBroadException(Exception):
+    pass
+
+
+class CustomNarrowException(CustomBroadException):
+    pass
+
+
 try:
     __revision__ += 1
 except Exception: # [broad-exception-caught]
@@ -11,3 +19,21 @@
     __revision__ += 1
 except BaseException: # [broad-exception-caught]
     print('error')
+
+
+try:
+    __revision__ += 1
+except ValueError:
+    print('error')
+
+
+try:
+    __revision__ += 1
+except CustomBroadException: # [broad-exception-caught]
+    print('error')
+
+
+try:
+    __revision__ += 1
+except CustomNarrowException:
+    print('error')
diff --git a/tests/functional/b/broad_exception_caught.rc b/tests/functional/b/broad_exception_caught.rc
@@ -0,0 +1,4 @@
+[EXCEPTIONS]
+overgeneral-exceptions=builtins.BaseException,
+                       builtins.Exception,
+                       functional.b.broad_exception_caught.CustomBroadException
diff --git a/tests/functional/b/broad_exception_caught.txt b/tests/functional/b/broad_exception_caught.txt
@@ -1,2 +1,3 @@
-broad-exception-caught:6:7:6:16::Catching too general exception Exception:INFERENCE
-broad-exception-caught:12:7:12:20::Catching too general exception BaseException:INFERENCE
+broad-exception-caught:14:7:14:16::Catching too general exception Exception:INFERENCE
+broad-exception-caught:20:7:20:20::Catching too general exception BaseException:INFERENCE
+broad-exception-caught:32:7:32:27::Catching too general exception CustomBroadException:INFERENCE
diff --git a/tests/functional/b/broad_exception_raised.py b/tests/functional/b/broad_exception_raised.py
@@ -1,4 +1,14 @@
-# pylint: disable=missing-module-docstring, missing-function-docstring, unreachable
+# pylint: disable=missing-docstring, unreachable
+
+ExceptionAlias = Exception
+
+class CustomBroadException(Exception):
+    pass
+
+
+class CustomNarrowException(CustomBroadException):
+    pass
+
 
 def exploding_apple(apple):
     print(f"{apple} is about to explode")
@@ -11,6 +21,32 @@ def raise_and_catch():
     except Exception as ex:  # [broad-exception-caught]
         print(ex)
 
+
+def raise_catch_reraise():
+    try:
+        exploding_apple("apple")
+    except Exception as ex:
+        print(ex)
+        raise ex
+
+
+def raise_catch_raise():
+    try:
+        exploding_apple("apple")
+    except Exception as ex:
+        print(ex)
+        raise Exception() from None  # [broad-exception-raised]
+
+
+def raise_catch_raise_using_alias():
+    try:
+        exploding_apple("apple")
+    except Exception as ex:
+        print(ex)
+        raise ExceptionAlias() from None  # [broad-exception-raised]
+
 raise Exception()  # [broad-exception-raised]
 raise BaseException()  # [broad-exception-raised]
+raise CustomBroadException()  # [broad-exception-raised]
 raise IndexError from None
+raise CustomNarrowException() from None
diff --git a/tests/functional/b/broad_exception_raised.rc b/tests/functional/b/broad_exception_raised.rc
@@ -0,0 +1,4 @@
+[EXCEPTIONS]
+overgeneral-exceptions=builtins.BaseException,
+                       builtins.Exception,
+                       functional.b.broad_exception_raised.CustomBroadException
diff --git a/tests/functional/b/broad_exception_raised.txt b/tests/functional/b/broad_exception_raised.txt
@@ -1,5 +1,8 @@
-broad-exception-raised:5:4:5:41:exploding_apple:"Raising too general exception: Exception":HIGH
-broad-exception-raised:10:8:10:34:raise_and_catch:"Raising too general exception: Exception":HIGH
-broad-exception-caught:11:11:11:20:raise_and_catch:Catching too general exception Exception:INFERENCE
-broad-exception-raised:14:0:14:17::"Raising too general exception: Exception":HIGH
-broad-exception-raised:15:0:15:21::"Raising too general exception: BaseException":HIGH
+broad-exception-raised:15:4:15:41:exploding_apple:"Raising too general exception: Exception":INFERENCE
+broad-exception-raised:20:8:20:34:raise_and_catch:"Raising too general exception: Exception":INFERENCE
+broad-exception-caught:21:11:21:20:raise_and_catch:Catching too general exception Exception:INFERENCE
+broad-exception-raised:38:8:38:35:raise_catch_raise:"Raising too general exception: Exception":INFERENCE
+broad-exception-raised:46:8:46:40:raise_catch_raise_using_alias:"Raising too general exception: Exception":INFERENCE
+broad-exception-raised:48:0:48:17::"Raising too general exception: Exception":INFERENCE
+broad-exception-raised:49:0:49:21::"Raising too general exception: BaseException":INFERENCE
+broad-exception-raised:50:0:50:28::"Raising too general exception: CustomBroadException":INFERENCE