Apply suggestions from code review

Author: cool-RR

CONTRIBUTORS.txt

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

pylint/checkers/exceptions.py

@@ -76,6 +76,13 @@ def _is_raising(body: typing.List) -> bool:
     return False
 
 
+def iterate_parents(node: NodeNG) -> typing.Iterator[NodeNG]:
+    current = node.parent
+    while current is not None:
+        yield current
+        current = current.parent
+
+
 OVERGENERAL_EXCEPTIONS = ("BaseException", "Exception")
 BUILTINS_NAME = builtins.__name__
 
@@ -152,6 +159,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,14 +295,15 @@ 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:
-            self._check_bad_exception_context(node)
+        self._check_bad_exception_cause(node)
+        self._check_raise_missing_from(node)
 
         expr = node.exc
         ExceptionRaiseRefVisitor(self, node).visit(expr)
@@ -320,15 +337,16 @@ 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):
-        """Verify that the exception context is properly set.
+    def _check_bad_exception_cause(self, node: astroid.Raise) -> None:
+        """Verify that the exception cause is properly set.
 
-        An exception context can be only `None` or an exception.
+        An exception cause can be only `None` or an exception.
         """
+        if not node.cause:
+            return
         cause = utils.safe_infer(node.cause)
         if cause in (astroid.Uninferable, None):
             return
-
         if isinstance(cause, astroid.Const):
             if cause.value is not None:
                 self.add_message("bad-exception-context", node=node)
@@ -337,6 +355,46 @@ 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.cause is not None:
+            return
+        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:
+        for parent in iterate_parents(node):
+            if isinstance(parent, astroid.ExceptHandler):
+                except_parent = parent
+                # 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 except_parent.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 != except_parent.name.name
+                ):
+                    # We have a `raise SomeException`.
+                    self.add_message("raise-missing-from", node=node)
+                break
+            if isinstance(parent, astroid.scoped_nodes.LocalsDictNodeNG):
+                # If we're inside a function/class definition, we don't want to keep checking
+                # higher parents 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.
+                break
+
     def _check_catching_non_exception(self, handler, exc, part):
         if isinstance(exc, astroid.Tuple):
             # Check if it is a tuple of exceptions.

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::Exception needs to be raised with cause
+raise-missing-from:19::Exception needs to be raised with cause
+raise-missing-from:25::Exception needs to be raised with cause
+raise-missing-from:31::Exception needs to be raised with cause
+raise-missing-from:45::Exception needs to be raised with cause
+raise-missing-from:53::Exception needs to be raised with cause
+raise-missing-from:59::Exception needs to be raised with cause

tests/functional/r/regression_3416_unused_argument_raise.py

@@ -3,6 +3,8 @@
 https://github.com/PyCQA/pylint/issues/3416
 """
 
+# pylint:disable=raise-missing-from
+
 # +1: [unused-argument, unused-argument, unused-argument]
 def fun(arg_a, arg_b, arg_c) -> None:
     """Routine docstring"""

tests/functional/r/regression_3416_unused_argument_raise.txt

@@ -1,3 +1,3 @@
-unused-argument:7:fun:Unused argument 'arg_a'
-unused-argument:7:fun:Unused argument 'arg_b'
-unused-argument:7:fun:Unused argument 'arg_c'
+unused-argument:9:fun:Unused argument 'arg_a'
+unused-argument:9:fun:Unused argument 'arg_b'
+unused-argument:9:fun:Unused argument 'arg_c'

tests/functional/s/stop_iteration_inside_generator.py

@@ -1,7 +1,7 @@
 """
 Test that no StopIteration is raised inside a generator
 """
-# pylint: disable=missing-docstring,invalid-name,import-error, try-except-raise, wrong-import-position,not-callable
+# pylint: disable=missing-docstring,invalid-name,import-error, try-except-raise, wrong-import-position,not-callable,raise-missing-from
 import asyncio
 
 class RebornStopIteration(StopIteration):