Add rule raise-missing-from

14 files changed, 243 insertions, 10 deletions

diff --git a/CONTRIBUTORS.txt b/CONTRIBUTORS.txt
index 6c6a21a46..b8c0bedaf 100644
--- a/CONTRIBUTORS.txt
+++ b/CONTRIBUTORS.txt
@@ -391,3 +391,5 @@ contributors:
 * Shiv Venkatasubrahmanyam
 
 * Jochen Preusche (iilei): contributor
+
+* Ram Rachum (cool-RR)
\ No newline at end of file
diff --git a/ChangeLog b/ChangeLog
index 0406bc0ed..5f9939822 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -39,6 +39,8 @@ Release date: TBA
 
   Close #3564
 
+* Add `raise-missing-from` check for exceptions that should have a cause.
+
 
 What's New in Pylint 2.5.4?
 ===========================
diff --git a/doc/whatsnew/2.6.rst b/doc/whatsnew/2.6.rst
index dc659d656..4da6f6122 100644
--- a/doc/whatsnew/2.6.rst
+++ b/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
 =============
 
diff --git a/pylint/checkers/exceptions.py b/pylint/checkers/exceptions.py
index 354a73d59..957938454 100644
--- a/pylint/checkers/exceptions.py
+++ b/pylint/checkers/exceptions.py
@@ -152,6 +152,15 @@ MSGS = {
         "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 +288,16 @@ class ExceptionsChecker(checkers.BaseChecker):
         "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 +332,7 @@ class ExceptionsChecker(checkers.BaseChecker):
         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 +349,37 @@ class ExceptionsChecker(checkers.BaseChecker):
         ):
             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.
diff --git a/pylint/checkers/utils.py b/pylint/checkers/utils.py
index 941b0f69b..ed2c1478c 100644
--- a/pylint/checkers/utils.py
+++ b/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)
diff --git a/tests/functional/m/misplaced_bare_raise.py b/tests/functional/m/misplaced_bare_raise.py
index 3a11aaee8..6148733e2 100644
--- a/tests/functional/m/misplaced_bare_raise.py
+++ b/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:

diff --git a/tests/functional/n/no_else_raise.py b/tests/functional/n/no_else_raise.py
index b3f27a74e..ae807fae3 100644
--- a/tests/functional/n/no_else_raise.py
+++ b/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]
diff --git a/tests/functional/n/no_else_return.py b/tests/functional/n/no_else_return.py
index 0f90c153a..0a87551fa 100644
--- a/tests/functional/n/no_else_return.py
+++ b/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
diff --git a/tests/functional/r/raise_missing_from.py b/tests/functional/r/raise_missing_from.py
new file mode 100644
index 000000000..d23cefb5e
--- /dev/null
+++ b/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
diff --git a/tests/functional/r/raise_missing_from.txt b/tests/functional/r/raise_missing_from.txt
new file mode 100644
index 000000000..ee4f52889
--- /dev/null
+++ b/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
diff --git a/tests/functional/r/regression_3416_unused_argument_raise.py b/tests/functional/r/regression_3416_unused_argument_raise.py
index 2d5087288..9bc761701 100644
--- a/tests/functional/r/regression_3416_unused_argument_raise.py
+++ b/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"""
diff --git a/tests/functional/r/regression_3416_unused_argument_raise.txt b/tests/functional/r/regression_3416_unused_argument_raise.txt
index ecfd97fbe..3595cc339 100644
--- a/tests/functional/r/regression_3416_unused_argument_raise.txt
+++ b/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'
diff --git a/tests/functional/s/stop_iteration_inside_generator.py b/tests/functional/s/stop_iteration_inside_generator.py
index 12f049483..659932439 100644
--- a/tests/functional/s/stop_iteration_inside_generator.py
+++ b/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):
diff --git a/tests/functional/t/try_except_raise.py b/tests/functional/t/try_except_raise.py
index c78852b81..006a29bf9 100644
--- a/tests/functional/t/try_except_raise.py
+++ b/tests/functional/t/try_except_raise.py
@@ -1,5 +1,5 @@
 # pylint:disable=missing-docstring, unreachable, bad-except-order, bare-except, unnecessary-pass
-# pylint: disable=undefined-variable, broad-except
+# pylint: disable=undefined-variable, broad-except, raise-missing-from
 try:
     int("9a")
 except:  # [try-except-raise]