diff options
author | Dani Alcala <112832187+clavedeluna@users.noreply.github.com> | 2022-12-12 15:00:30 -0300 |
---|---|---|
committer | GitHub <noreply@github.com> | 2022-12-12 19:00:30 +0100 |
commit | 1b3922b3d34dbd526e1a182bfb705aedda6ea3d1 (patch) | |
tree | dd240f14c0055ec65b31161d7f1b5d754fd1942f | |
parent | b9b77c2f3f550a50bb8fec4b842ed3ff648966b4 (diff) | |
download | pylint-git-1b3922b3d34dbd526e1a182bfb705aedda6ea3d1.tar.gz |
Fix ``no-else-return false Negative for try/except/else pattern (#7888)
Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
-rw-r--r-- | doc/whatsnew/fragments/7788.false_negative | 3 | ||||
-rw-r--r-- | pylint/checkers/refactoring/refactoring_checker.py | 43 | ||||
-rw-r--r-- | pylint/lint/pylinter.py | 4 | ||||
-rw-r--r-- | tests/functional/n/no/no_else_break.txt | 14 | ||||
-rw-r--r-- | tests/functional/n/no/no_else_continue.txt | 14 | ||||
-rw-r--r-- | tests/functional/n/no/no_else_raise.txt | 14 | ||||
-rw-r--r-- | tests/functional/n/no/no_else_return.py | 84 | ||||
-rw-r--r-- | tests/functional/n/no/no_else_return.txt | 21 | ||||
-rw-r--r-- | tests/functional/r/raise_missing_from.py | 2 |
9 files changed, 161 insertions, 38 deletions
diff --git a/doc/whatsnew/fragments/7788.false_negative b/doc/whatsnew/fragments/7788.false_negative new file mode 100644 index 000000000..f04311a6b --- /dev/null +++ b/doc/whatsnew/fragments/7788.false_negative @@ -0,0 +1,3 @@ +``no-else-return`` or ``no-else-raise`` will be emitted if ``except`` block always returns or raises. + +Closes #7788 diff --git a/pylint/checkers/refactoring/refactoring_checker.py b/pylint/checkers/refactoring/refactoring_checker.py index 6a79ce55a..361238592 100644 --- a/pylint/checkers/refactoring/refactoring_checker.py +++ b/pylint/checkers/refactoring/refactoring_checker.py @@ -73,6 +73,16 @@ def _if_statement_is_always_returning( return any(isinstance(node, returning_node_class) for node in if_node.body) +def _except_statement_is_always_returning( + node: nodes.TryExcept, returning_node_class: nodes.NodeNG +) -> bool: + """Detect if all except statements return.""" + return all( + any(isinstance(child, returning_node_class) for child in handler.body) + for handler in node.handlers + ) + + def _is_trailing_comma(tokens: list[tokenize.TokenInfo], index: int) -> bool: """Check if the given token is a trailing comma. @@ -531,7 +541,7 @@ class RefactoringChecker(checkers.BaseTokenChecker): node.value.value, bool ) - def _is_actual_elif(self, node: nodes.If) -> bool: + def _is_actual_elif(self, node: nodes.If | nodes.TryExcept) -> bool: """Check if the given node is an actual elif. This is a problem we're having with the builtin ast module, @@ -642,10 +652,14 @@ class RefactoringChecker(checkers.BaseTokenChecker): ) self._init() - @utils.only_required_for_messages("too-many-nested-blocks") - def visit_tryexcept(self, node: nodes.TryExcept) -> None: + @utils.only_required_for_messages("too-many-nested-blocks", "no-else-return") + def visit_tryexcept(self, node: nodes.TryExcept | nodes.TryFinally) -> None: self._check_nested_blocks(node) + if isinstance(node, nodes.TryExcept): + self._check_superfluous_else_return(node) + self._check_superfluous_else_raise(node) + visit_tryfinally = visit_tryexcept visit_while = visit_tryexcept @@ -707,23 +721,38 @@ class RefactoringChecker(checkers.BaseTokenChecker): self._check_redefined_argument_from_local(name) def _check_superfluous_else( - self, node: nodes.If, msg_id: str, returning_node_class: nodes.NodeNG + self, + node: nodes.If | nodes.TryExcept, + msg_id: str, + returning_node_class: nodes.NodeNG, ) -> None: + if isinstance(node, nodes.TryExcept) and isinstance( + node.parent, nodes.TryFinally + ): + # Not interested in try/except/else/finally statements. + return + if not node.orelse: - # Not interested in if statements without else. + # Not interested in if/try statements without else. return if self._is_actual_elif(node): # Not interested in elif nodes; only if return - if _if_statement_is_always_returning(node, returning_node_class): + if ( + isinstance(node, nodes.If) + and _if_statement_is_always_returning(node, returning_node_class) + ) or ( + isinstance(node, nodes.TryExcept) + and _except_statement_is_always_returning(node, returning_node_class) + ): orelse = node.orelse[0] if (orelse.lineno, orelse.col_offset) in self._elifs: args = ("elif", 'remove the leading "el" from "elif"') else: args = ("else", 'remove the "else" and de-indent the code inside it') - self.add_message(msg_id, node=node, args=args) + self.add_message(msg_id, node=node, args=args, confidence=HIGH) def _check_superfluous_else_return(self, node: nodes.If) -> None: return self._check_superfluous_else( diff --git a/pylint/lint/pylinter.py b/pylint/lint/pylinter.py index 70f27c7c0..2f96a9fda 100644 --- a/pylint/lint/pylinter.py +++ b/pylint/lint/pylinter.py @@ -454,8 +454,8 @@ class PyLinter( reporter_class = _load_reporter_by_class(reporter_name) except (ImportError, AttributeError, AssertionError) as e: raise exceptions.InvalidReporterError(name) from e - else: - return reporter_class() + + return reporter_class() def set_reporter( self, reporter: reporters.BaseReporter | reporters.MultiReporter diff --git a/tests/functional/n/no/no_else_break.txt b/tests/functional/n/no/no_else_break.txt index cf045f367..1e5bee1eb 100644 --- a/tests/functional/n/no/no_else_break.txt +++ b/tests/functional/n/no/no_else_break.txt @@ -1,7 +1,7 @@ -no-else-break:8:8:11:17:foo1:"Unnecessary ""else"" after ""break"", remove the ""else"" and de-indent the code inside it":UNDEFINED -no-else-break:16:8:21:17:foo2:"Unnecessary ""elif"" after ""break"", remove the leading ""el"" from ""elif""":UNDEFINED -no-else-break:28:12:33:21:foo3:"Unnecessary ""else"" after ""break"", remove the ""else"" and de-indent the code inside it":UNDEFINED -no-else-break:41:8:48:17:foo4:"Unnecessary ""else"" after ""break"", remove the ""else"" and de-indent the code inside it":UNDEFINED -no-else-break:54:8:63:17:foo5:"Unnecessary ""elif"" after ""break"", remove the leading ""el"" from ""elif""":UNDEFINED -no-else-break:70:12:74:21:foo6:"Unnecessary ""else"" after ""break"", remove the ""else"" and de-indent the code inside it":UNDEFINED -no-else-break:110:8:116:21:bar4:"Unnecessary ""else"" after ""break"", remove the ""else"" and de-indent the code inside it":UNDEFINED +no-else-break:8:8:11:17:foo1:"Unnecessary ""else"" after ""break"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-break:16:8:21:17:foo2:"Unnecessary ""elif"" after ""break"", remove the leading ""el"" from ""elif""":HIGH +no-else-break:28:12:33:21:foo3:"Unnecessary ""else"" after ""break"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-break:41:8:48:17:foo4:"Unnecessary ""else"" after ""break"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-break:54:8:63:17:foo5:"Unnecessary ""elif"" after ""break"", remove the leading ""el"" from ""elif""":HIGH +no-else-break:70:12:74:21:foo6:"Unnecessary ""else"" after ""break"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-break:110:8:116:21:bar4:"Unnecessary ""else"" after ""break"", remove the ""else"" and de-indent the code inside it":HIGH diff --git a/tests/functional/n/no/no_else_continue.txt b/tests/functional/n/no/no_else_continue.txt index f0e813e40..7b0dde4e9 100644 --- a/tests/functional/n/no/no_else_continue.txt +++ b/tests/functional/n/no/no_else_continue.txt @@ -1,7 +1,7 @@ -no-else-continue:8:8:11:17:foo1:"Unnecessary ""else"" after ""continue"", remove the ""else"" and de-indent the code inside it":UNDEFINED -no-else-continue:16:8:21:17:foo2:"Unnecessary ""elif"" after ""continue"", remove the leading ""el"" from ""elif""":UNDEFINED -no-else-continue:28:12:33:24:foo3:"Unnecessary ""else"" after ""continue"", remove the ""else"" and de-indent the code inside it":UNDEFINED -no-else-continue:41:8:48:17:foo4:"Unnecessary ""else"" after ""continue"", remove the ""else"" and de-indent the code inside it":UNDEFINED -no-else-continue:54:8:63:17:foo5:"Unnecessary ""elif"" after ""continue"", remove the leading ""el"" from ""elif""":UNDEFINED -no-else-continue:70:12:74:21:foo6:"Unnecessary ""else"" after ""continue"", remove the ""else"" and de-indent the code inside it":UNDEFINED -no-else-continue:110:8:116:24:bar4:"Unnecessary ""else"" after ""continue"", remove the ""else"" and de-indent the code inside it":UNDEFINED +no-else-continue:8:8:11:17:foo1:"Unnecessary ""else"" after ""continue"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-continue:16:8:21:17:foo2:"Unnecessary ""elif"" after ""continue"", remove the leading ""el"" from ""elif""":HIGH +no-else-continue:28:12:33:24:foo3:"Unnecessary ""else"" after ""continue"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-continue:41:8:48:17:foo4:"Unnecessary ""else"" after ""continue"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-continue:54:8:63:17:foo5:"Unnecessary ""elif"" after ""continue"", remove the leading ""el"" from ""elif""":HIGH +no-else-continue:70:12:74:21:foo6:"Unnecessary ""else"" after ""continue"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-continue:110:8:116:24:bar4:"Unnecessary ""else"" after ""continue"", remove the ""else"" and de-indent the code inside it":HIGH diff --git a/tests/functional/n/no/no_else_raise.txt b/tests/functional/n/no/no_else_raise.txt index 72f8f8376..9e22f9a43 100644 --- a/tests/functional/n/no/no_else_raise.txt +++ b/tests/functional/n/no/no_else_raise.txt @@ -1,7 +1,7 @@ -no-else-raise:6:4:11:26:foo1:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":UNDEFINED -no-else-raise:15:4:23:26:foo2:"Unnecessary ""elif"" after ""raise"", remove the leading ""el"" from ""elif""":UNDEFINED -no-else-raise:29:8:34:30:foo3:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":UNDEFINED -no-else-raise:41:4:48:13:foo4:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":UNDEFINED -no-else-raise:53:4:62:13:foo5:"Unnecessary ""elif"" after ""raise"", remove the leading ""el"" from ""elif""":UNDEFINED -no-else-raise:68:8:72:17:foo6:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":UNDEFINED -no-else-raise:104:4:110:33:bar4:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":UNDEFINED +no-else-raise:6:4:11:26:foo1:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-raise:15:4:23:26:foo2:"Unnecessary ""elif"" after ""raise"", remove the leading ""el"" from ""elif""":HIGH +no-else-raise:29:8:34:30:foo3:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-raise:41:4:48:13:foo4:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-raise:53:4:62:13:foo5:"Unnecessary ""elif"" after ""raise"", remove the leading ""el"" from ""elif""":HIGH +no-else-raise:68:8:72:17:foo6:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-raise:104:4:110:33:bar4:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":HIGH diff --git a/tests/functional/n/no/no_else_return.py b/tests/functional/n/no/no_else_return.py index 0a87551fa..eb4b05d87 100644 --- a/tests/functional/n/no/no_else_return.py +++ b/tests/functional/n/no/no_else_return.py @@ -108,3 +108,87 @@ def bar4(x): return False except ValueError: return None + +# pylint: disable = bare-except +def try_one_except() -> bool: + try: # [no-else-return] + print('asdf') + except: + print("bad") + return False + else: + return True + + +def try_multiple_except() -> bool: + try: # [no-else-return] + print('asdf') + except TypeError: + print("bad") + return False + except ValueError: + print("bad second") + return False + else: + return True + +def try_not_all_except_return() -> bool: # [inconsistent-return-statements] + try: + print('asdf') + except TypeError: + print("bad") + return False + except ValueError: + val = "something" + else: + return True + +# pylint: disable = raise-missing-from +def try_except_raises() -> bool: + try: # [no-else-raise] + print('asdf') + except: + raise ValueError + else: + return True + +def try_except_raises2() -> bool: + try: # [no-else-raise] + print('asdf') + except TypeError: + raise ValueError + except ValueError: + raise TypeError + else: + return True + +def test() -> bool: # [inconsistent-return-statements] + try: + print('asdf') + except RuntimeError: + return False + finally: + print("in finally") + + +def try_finally_return() -> bool: # [inconsistent-return-statements] + try: + print('asdf') + except RuntimeError: + return False + else: + print("inside else") + finally: + print("in finally") + +def try_finally_raise(): + current_tags = {} + try: + yield current_tags + except Exception: + current_tags["result"] = "failure" + raise + else: + current_tags["result"] = "success" + finally: + print("inside finally") diff --git a/tests/functional/n/no/no_else_return.txt b/tests/functional/n/no/no_else_return.txt index c73b177da..a2c64f73c 100644 --- a/tests/functional/n/no/no_else_return.txt +++ b/tests/functional/n/no/no_else_return.txt @@ -1,7 +1,14 @@ -no-else-return:6:4:11:16:foo1:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":UNDEFINED -no-else-return:15:4:23:16:foo2:"Unnecessary ""elif"" after ""return"", remove the leading ""el"" from ""elif""":UNDEFINED -no-else-return:29:8:34:20:foo3:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":UNDEFINED -no-else-return:41:4:48:13:foo4:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":UNDEFINED -no-else-return:53:4:62:13:foo5:"Unnecessary ""elif"" after ""return"", remove the leading ""el"" from ""elif""":UNDEFINED -no-else-return:68:8:72:17:foo6:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":UNDEFINED -no-else-return:104:4:110:23:bar4:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":UNDEFINED +no-else-return:6:4:11:16:foo1:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-return:15:4:23:16:foo2:"Unnecessary ""elif"" after ""return"", remove the leading ""el"" from ""elif""":HIGH +no-else-return:29:8:34:20:foo3:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-return:41:4:48:13:foo4:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-return:53:4:62:13:foo5:"Unnecessary ""elif"" after ""return"", remove the leading ""el"" from ""elif""":HIGH +no-else-return:68:8:72:17:foo6:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-return:104:4:110:23:bar4:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-return:114:4:120:19:try_one_except:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-return:124:4:133:19:try_multiple_except:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":HIGH +inconsistent-return-statements:135:0:135:29:try_not_all_except_return:Either all return statements in a function should return an expression, or none of them should.:UNDEFINED +no-else-raise:148:4:153:19:try_except_raises:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-raise:156:4:163:19:try_except_raises2:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":HIGH +inconsistent-return-statements:165:0:165:8:test:Either all return statements in a function should return an expression, or none of them should.:UNDEFINED +inconsistent-return-statements:174:0:174:22:try_finally_return:Either all return statements in a function should return an expression, or none of them should.:UNDEFINED diff --git a/tests/functional/r/raise_missing_from.py b/tests/functional/r/raise_missing_from.py index 897a0e9c4..3b66609c8 100644 --- a/tests/functional/r/raise_missing_from.py +++ b/tests/functional/r/raise_missing_from.py @@ -1,5 +1,5 @@ # 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 +# pylint:disable=try-except-raise, undefined-variable, too-few-public-methods, superfluous-parens, no-else-raise try: 1 / 0 |