Add rule raise-missing-from

Author: cool-RR

CONTRIBUTORS.txt

@@ -391,3 +391,5 @@ contributors:
 * Shiv Venkatasubrahmanyam
 
 * Jochen Preusche (iilei): contributor
+
+* Ram Rachum (cool-RR)

ChangeLog

@@ -34,6 +34,8 @@ Release date: TBA
 
 * Fix `pre-commit` config that could lead to undetected duplicate lines of code
 
+* Add `raise-missing-from` check for exceptions that should have a cause.
+
 
 What's New in Pylint 2.5.4?
 ===========================

doc/whatsnew/2.6.rst

@@ -15,6 +15,8 @@ New checkers
 
 * Add `super-with-arguments` check for flagging instances of Python 2 style super calls.
 
+* Add `raise-missing-from` check for exceptions that should have a cause.
+
 Other Changes
 =============
 

pylint/checkers/exceptions.py

@@ -76,6 +76,7 @@ def _is_raising(body: typing.List) -> bool:
     return False
 
 
+
 OVERGENERAL_EXCEPTIONS = ("BaseException", "Exception")
 BUILTINS_NAME = builtins.__name__
 
@@ -152,6 +153,15 @@ def _is_raising(body: typing.List) -> bool:
         "immediately. Remove the raise operator or the entire "
         "try-except-raise block!",
     ),
+    "W0707": (
+        "Consider explicitly re-raising using the 'from' keyword",
+        "raise-missing-from",
+        "Python 3's exception chaining means it shows the traceback of the "
+        "current exception, but also the original exception. Not using `raise "
+        "from` makes the traceback inaccurate, because the message implies "
+        "there is a bug in the exception-handling code itself, which is a "
+        "separate situation than wrapping an exception.",
+    ),
     "W0711": (
         'Exception to catch is the result of a binary "%s" operation',
         "binary-op-exception",
@@ -279,13 +289,16 @@ def open(self):
         "notimplemented-raised",
         "bad-exception-context",
         "raising-format-tuple",
+        "raise-missing-from",
     )
     def visit_raise(self, node):
         if node.exc is None:
             self._check_misplaced_bare_raise(node)
             return
 
-        if node.cause:
+        if node.cause is None:
+            self._check_raise_missing_from(node)
+        else:
             self._check_bad_exception_context(node)
 
         expr = node.exc
@@ -320,7 +333,7 @@ def _check_misplaced_bare_raise(self, node):
         if not current or not isinstance(current.parent, expected):
             self.add_message("misplaced-bare-raise", node=node)
 
-    def _check_bad_exception_context(self, node):
+    def _check_bad_exception_context(self, node: astroid.Raise) -> None:
         """Verify that the exception context is properly set.
 
         An exception context can be only `None` or an exception.
@@ -337,6 +350,37 @@ def _check_bad_exception_context(self, node):
         ):
             self.add_message("bad-exception-context", node=node)
 
+    def _check_raise_missing_from(self, node: astroid.Raise) -> None:
+        if node.exc is None:
+            # This is a plain `raise`, raising the previously-caught exception. No need for a
+            # cause.
+            return
+        # We'd like to check whether we're inside an `except` clause:
+        containing_except_node = utils.find_except_wrapper_node_in_scope(node)
+        if not containing_except_node:
+            return
+        # We found a surrounding `except`! We're almost done proving there's a
+        # `raise-missing-from` here. The only thing we need to protect against is that maybe
+        # the `raise` is raising the exception that was caught, possibly with some shenanigans
+        # like `exc.with_traceback(whatever)`. We won't analyze these, we'll just assume
+        # there's a violation on two simple cases: `raise SomeException(whatever)` and `raise
+        # SomeException`.
+        if containing_except_node.name is None:
+            # The `except` doesn't have an `as exception:` part, meaning there's no way that
+            # the `raise` is raising the same exception.
+            self.add_message("raise-missing-from", node=node)
+        elif isinstance(node.exc, astroid.Call) and isinstance(
+            node.exc.func, astroid.Name
+        ):
+            # We have a `raise SomeException(whatever)`.
+            self.add_message("raise-missing-from", node=node)
+        elif (
+            isinstance(node.exc, astroid.Name)
+            and node.exc.name != containing_except_node.name.name
+        ):
+            # We have a `raise SomeException`.
+            self.add_message("raise-missing-from", node=node)
+
     def _check_catching_non_exception(self, handler, exc, part):
         if isinstance(exc, astroid.Tuple):
             # Check if it is a tuple of exceptions.

pylint/checkers/utils.py

@@ -859,6 +859,24 @@ def find_try_except_wrapper_node(
     return None
 
 
+def find_except_wrapper_node_in_scope(
+    node: astroid.node_classes.NodeNG,
+) -> Optional[Union[astroid.ExceptHandler, astroid.TryExcept]]:
+    """Return the ExceptHandler in which the node is, without going out of scope."""
+    current = node
+    while current.parent is not None:
+        current = current.parent
+        if isinstance(current, astroid.scoped_nodes.LocalsDictNodeNG):
+            # If we're inside a function/class definition, we don't want to keep checking
+            # higher ancestors for `except` clauses, because if these exist, it means our
+            # function/class was defined in an `except` clause, rather than the current code
+            # actually running in an `except` clause.
+            return None
+        if isinstance(current, astroid.ExceptHandler):
+            return current
+    return None
+
+
 def is_from_fallback_block(node: astroid.node_classes.NodeNG) -> bool:
     """Check if the given node is from a fallback import block."""
     context = find_try_except_wrapper_node(node)

tests/functional/m/misplaced_bare_raise.py

@@ -1,4 +1,4 @@
-# pylint: disable=missing-docstring, broad-except, unreachable, try-except-raise
+# pylint: disable=missing-docstring, broad-except, unreachable, try-except-raise, raise-missing-from
 # pylint: disable=unused-variable, too-few-public-methods, invalid-name, useless-object-inheritance
 
 try:

tests/functional/n/no_else_raise.py

@@ -1,6 +1,6 @@
 """ Test that superfluous else return are detected. """
 
-# pylint:disable=invalid-name,missing-docstring,unused-variable
+# pylint:disable=invalid-name,missing-docstring,unused-variable,raise-missing-from
 
 def foo1(x, y, z):
     if x:  # [no-else-raise]

tests/functional/n/no_else_return.py

@@ -1,7 +1,7 @@
 """ Test that superfluous else return are detected. """
 
-# pylint:disable=invalid-name,missing-docstring,unused-variable
 
+# pylint:disable=invalid-name,missing-docstring,unused-variable
 def foo1(x, y, z):
     if x:  # [no-else-return]
         a = 1

tests/functional/r/raise_missing_from.py

@@ -0,0 +1,157 @@
+# pylint:disable=missing-docstring, unreachable, using-constant-test, invalid-name, bare-except
+# pylint:disable=try-except-raise, undefined-variable, too-few-public-methods, superfluous-parens
+
+################################################################################
+# Positives:
+
+try:
+    1 / 0
+except ZeroDivisionError:
+    # +1: [raise-missing-from]
+    raise KeyError
+
+try:
+    1 / 0
+except ZeroDivisionError:
+    # Our algorithm doesn't have to be careful about the complicated expression below, because
+    # the exception above wasn't bound to a name.
+    # +1: [raise-missing-from]
+    raise (foo + bar).baz
+
+try:
+    1 / 0
+except ZeroDivisionError as e:
+    # +1: [raise-missing-from]
+    raise KeyError
+
+try:
+    1 / 0
+except ZeroDivisionError as e:
+    # +1: [raise-missing-from]
+    raise KeyError
+else:
+    pass
+finally:
+    pass
+
+try:
+    1 / 0
+except ZeroDivisionError as e:
+    if 1:
+        if 1:
+            with whatever:
+                try:
+                    # +1: [raise-missing-from]
+                    raise KeyError
+                except:
+                    pass
+
+try:
+    1 / 0
+except ZeroDivisionError as e:
+    # +1: [raise-missing-from]
+    raise KeyError()
+
+try:
+    1 / 0
+except ZeroDivisionError as e:
+    # +1: [raise-missing-from]
+    raise KeyError(whatever, whatever=whatever)
+
+
+################################################################################
+# Negatives (Same cases as above, except with `from`):
+
+try:
+    1 / 0
+except ZeroDivisionError:
+    raise KeyError from foo
+
+try:
+    1 / 0
+except ZeroDivisionError:
+    raise (foo + bar).baz from foo
+
+try:
+    1 / 0
+except ZeroDivisionError as e:
+    raise KeyError from foo
+
+try:
+    1 / 0
+except ZeroDivisionError as e:
+    raise KeyError from foo
+else:
+    pass
+finally:
+    pass
+
+try:
+    1 / 0
+except ZeroDivisionError as e:
+    if 1:
+        if 1:
+            with whatever:
+                try:
+                    raise KeyError from foo
+                except:
+                    pass
+
+try:
+    1 / 0
+except ZeroDivisionError as e:
+    raise KeyError() from foo
+
+try:
+    1 / 0
+except ZeroDivisionError as e:
+    raise KeyError(whatever, whatever=whatever) from foo
+
+
+################################################################################
+# Other negatives:
+
+try:
+    1 / 0
+except ZeroDivisionError:
+    raise
+
+try:
+    1 / 0
+except ZeroDivisionError as e:
+    raise
+
+try:
+    1 / 0
+except ZeroDivisionError as e:
+    raise e
+
+try:
+    1 / 0
+except ZeroDivisionError as e:
+    if 1:
+        if 1:
+            if 1:
+                raise e
+
+try:
+    1 / 0
+except ZeroDivisionError as e:
+    raise e.with_traceback(e.__traceback__)
+
+try:
+    1 / 0
+except ZeroDivisionError as e:
+    raise (e + 7)
+
+try:
+    1 / 0
+except ZeroDivisionError as e:
+    def f():
+        raise KeyError
+
+try:
+    1 / 0
+except ZeroDivisionError as e:
+    class Foo:
+        raise KeyError

tests/functional/r/raise_missing_from.txt

@@ -0,0 +1,7 @@
+raise-missing-from:11::Consider explicitly re-raising using the 'from' keyword
+raise-missing-from:19::Consider explicitly re-raising using the 'from' keyword
+raise-missing-from:25::Consider explicitly re-raising using the 'from' keyword
+raise-missing-from:31::Consider explicitly re-raising using the 'from' keyword
+raise-missing-from:45::Consider explicitly re-raising using the 'from' keyword
+raise-missing-from:53::Consider explicitly re-raising using the 'from' keyword
+raise-missing-from:59::Consider explicitly re-raising using the 'from' keyword