From a73585740398052ccd6c67d33869d9049a27e8c7 Mon Sep 17 00:00:00 2001 From: Ram Rachum Date: Tue, 16 Jun 2020 20:06:08 +0300 Subject: Add rule raise-missing-from --- CONTRIBUTORS.txt | 2 + ChangeLog | 2 + doc/whatsnew/2.6.rst | 2 + pylint/checkers/exceptions.py | 47 +++++- pylint/checkers/utils.py | 18 +++ tests/functional/m/misplaced_bare_raise.py | 2 +- tests/functional/n/no_else_raise.py | 2 +- tests/functional/n/no_else_return.py | 2 +- tests/functional/r/raise_missing_from.py | 157 +++++++++++++++++++++ tests/functional/r/raise_missing_from.txt | 7 + .../r/regression_3416_unused_argument_raise.py | 2 + .../r/regression_3416_unused_argument_raise.txt | 6 +- .../s/stop_iteration_inside_generator.py | 2 +- tests/functional/t/try_except_raise.py | 2 +- 14 files changed, 243 insertions(+), 10 deletions(-) create mode 100644 tests/functional/r/raise_missing_from.py create mode 100644 tests/functional/r/raise_missing_from.txt 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] -- cgit v1.2.1