Skip to content

Commit 1b3922b

Browse files
clavedelunaPierre-Sassoulas
clavedeluna
and
Pierre-Sassoulas
authored
Fix ``no-else-return false Negative for try/except/else pattern (#7888)
 Co-authored-by: Pierre Sassoulas <[email protected]>
1 parent b9b77c2 commit 1b3922b

File tree

9 files changed

+161
-38
lines changed

9 files changed

+161
-38
lines changed

‎doc/whatsnew/fragments/7788.false_negative

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
``no-else-return`` or ``no-else-raise`` will be emitted if ``except`` block always returns or raises.
2+
3+
Closes #7788

‎pylint/checkers/refactoring/refactoring_checker.py

Lines changed: 36 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,16 @@ def _if_statement_is_always_returning(
7373
return any(isinstance(node, returning_node_class) for node in if_node.body)
7474

7575

76+
def _except_statement_is_always_returning(
77+
node: nodes.TryExcept, returning_node_class: nodes.NodeNG
78+
) -> bool:
79+
"""Detect if all except statements return."""
80+
return all(
81+
any(isinstance(child, returning_node_class) for child in handler.body)
82+
for handler in node.handlers
83+
)
84+
85+
7686
def _is_trailing_comma(tokens: list[tokenize.TokenInfo], index: int) -> bool:
7787
"""Check if the given token is a trailing comma.
7888
@@ -531,7 +541,7 @@ def _is_bool_const(node: nodes.Return | nodes.Assign) -> bool:
531541
node.value.value, bool
532542
)
533543

534-
def _is_actual_elif(self, node: nodes.If) -> bool:
544+
def _is_actual_elif(self, node: nodes.If | nodes.TryExcept) -> bool:
535545
"""Check if the given node is an actual elif.
536546
537547
This is a problem we're having with the builtin ast module,
@@ -642,10 +652,14 @@ def leave_module(self, _: nodes.Module) -> None:
642652
)
643653
self._init()
644654

645-
@utils.only_required_for_messages("too-many-nested-blocks")
646-
def visit_tryexcept(self, node: nodes.TryExcept) -> None:
655+
@utils.only_required_for_messages("too-many-nested-blocks", "no-else-return")
656+
def visit_tryexcept(self, node: nodes.TryExcept | nodes.TryFinally) -> None:
647657
self._check_nested_blocks(node)
648658

659+
if isinstance(node, nodes.TryExcept):
660+
self._check_superfluous_else_return(node)
661+
self._check_superfluous_else_raise(node)
662+
649663
visit_tryfinally = visit_tryexcept
650664
visit_while = visit_tryexcept
651665

@@ -707,23 +721,38 @@ def visit_with(self, node: nodes.With) -> None:
707721
self._check_redefined_argument_from_local(name)
708722

709723
def _check_superfluous_else(
710-
self, node: nodes.If, msg_id: str, returning_node_class: nodes.NodeNG
724+
self,
725+
node: nodes.If | nodes.TryExcept,
726+
msg_id: str,
727+
returning_node_class: nodes.NodeNG,
711728
) -> None:
729+
if isinstance(node, nodes.TryExcept) and isinstance(
730+
node.parent, nodes.TryFinally
731+
):
732+
# Not interested in try/except/else/finally statements.
733+
return
734+
712735
if not node.orelse:
713-
# Not interested in if statements without else.
736+
# Not interested in if/try statements without else.
714737
return
715738

716739
if self._is_actual_elif(node):
717740
# Not interested in elif nodes; only if
718741
return
719742

720-
if _if_statement_is_always_returning(node, returning_node_class):
743+
if (
744+
isinstance(node, nodes.If)
745+
and _if_statement_is_always_returning(node, returning_node_class)
746+
) or (
747+
isinstance(node, nodes.TryExcept)
748+
and _except_statement_is_always_returning(node, returning_node_class)
749+
):
721750
orelse = node.orelse[0]
722751
if (orelse.lineno, orelse.col_offset) in self._elifs:
723752
args = ("elif", 'remove the leading "el" from "elif"')
724753
else:
725754
args = ("else", 'remove the "else" and de-indent the code inside it')
726-
self.add_message(msg_id, node=node, args=args)
755+
self.add_message(msg_id, node=node, args=args, confidence=HIGH)
727756

728757
def _check_superfluous_else_return(self, node: nodes.If) -> None:
729758
return self._check_superfluous_else(

‎pylint/lint/pylinter.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -454,8 +454,8 @@ def _load_reporter_by_name(self, reporter_name: str) -> reporters.BaseReporter:
454454
reporter_class = _load_reporter_by_class(reporter_name)
455455
except (ImportError, AttributeError, AssertionError) as e:
456456
raise exceptions.InvalidReporterError(name) from e
457-
else:
458-
return reporter_class()
457+
458+
return reporter_class()
459459

460460
def set_reporter(
461461
self, reporter: reporters.BaseReporter | reporters.MultiReporter

‎tests/functional/n/no/no_else_break.txt

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
1-
no-else-break:8:8:11:17:foo1:"Unnecessary ""else"" after ""break"", remove the ""else"" and de-indent the code inside it":UNDEFINED
2-
no-else-break:16:8:21:17:foo2:"Unnecessary ""elif"" after ""break"", remove the leading ""el"" from ""elif""":UNDEFINED
3-
no-else-break:28:12:33:21:foo3:"Unnecessary ""else"" after ""break"", remove the ""else"" and de-indent the code inside it":UNDEFINED
4-
no-else-break:41:8:48:17:foo4:"Unnecessary ""else"" after ""break"", remove the ""else"" and de-indent the code inside it":UNDEFINED
5-
no-else-break:54:8:63:17:foo5:"Unnecessary ""elif"" after ""break"", remove the leading ""el"" from ""elif""":UNDEFINED
6-
no-else-break:70:12:74:21:foo6:"Unnecessary ""else"" after ""break"", remove the ""else"" and de-indent the code inside it":UNDEFINED
7-
no-else-break:110:8:116:21:bar4:"Unnecessary ""else"" after ""break"", remove the ""else"" and de-indent the code inside it":UNDEFINED
1+
no-else-break:8:8:11:17:foo1:"Unnecessary ""else"" after ""break"", remove the ""else"" and de-indent the code inside it":HIGH
2+
no-else-break:16:8:21:17:foo2:"Unnecessary ""elif"" after ""break"", remove the leading ""el"" from ""elif""":HIGH
3+
no-else-break:28:12:33:21:foo3:"Unnecessary ""else"" after ""break"", remove the ""else"" and de-indent the code inside it":HIGH
4+
no-else-break:41:8:48:17:foo4:"Unnecessary ""else"" after ""break"", remove the ""else"" and de-indent the code inside it":HIGH
5+
no-else-break:54:8:63:17:foo5:"Unnecessary ""elif"" after ""break"", remove the leading ""el"" from ""elif""":HIGH
6+
no-else-break:70:12:74:21:foo6:"Unnecessary ""else"" after ""break"", remove the ""else"" and de-indent the code inside it":HIGH
7+
no-else-break:110:8:116:21:bar4:"Unnecessary ""else"" after ""break"", remove the ""else"" and de-indent the code inside it":HIGH

‎tests/functional/n/no/no_else_continue.txt

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
1-
no-else-continue:8:8:11:17:foo1:"Unnecessary ""else"" after ""continue"", remove the ""else"" and de-indent the code inside it":UNDEFINED
2-
no-else-continue:16:8:21:17:foo2:"Unnecessary ""elif"" after ""continue"", remove the leading ""el"" from ""elif""":UNDEFINED
3-
no-else-continue:28:12:33:24:foo3:"Unnecessary ""else"" after ""continue"", remove the ""else"" and de-indent the code inside it":UNDEFINED
4-
no-else-continue:41:8:48:17:foo4:"Unnecessary ""else"" after ""continue"", remove the ""else"" and de-indent the code inside it":UNDEFINED
5-
no-else-continue:54:8:63:17:foo5:"Unnecessary ""elif"" after ""continue"", remove the leading ""el"" from ""elif""":UNDEFINED
6-
no-else-continue:70:12:74:21:foo6:"Unnecessary ""else"" after ""continue"", remove the ""else"" and de-indent the code inside it":UNDEFINED
7-
no-else-continue:110:8:116:24:bar4:"Unnecessary ""else"" after ""continue"", remove the ""else"" and de-indent the code inside it":UNDEFINED
1+
no-else-continue:8:8:11:17:foo1:"Unnecessary ""else"" after ""continue"", remove the ""else"" and de-indent the code inside it":HIGH
2+
no-else-continue:16:8:21:17:foo2:"Unnecessary ""elif"" after ""continue"", remove the leading ""el"" from ""elif""":HIGH
3+
no-else-continue:28:12:33:24:foo3:"Unnecessary ""else"" after ""continue"", remove the ""else"" and de-indent the code inside it":HIGH
4+
no-else-continue:41:8:48:17:foo4:"Unnecessary ""else"" after ""continue"", remove the ""else"" and de-indent the code inside it":HIGH
5+
no-else-continue:54:8:63:17:foo5:"Unnecessary ""elif"" after ""continue"", remove the leading ""el"" from ""elif""":HIGH
6+
no-else-continue:70:12:74:21:foo6:"Unnecessary ""else"" after ""continue"", remove the ""else"" and de-indent the code inside it":HIGH
7+
no-else-continue:110:8:116:24:bar4:"Unnecessary ""else"" after ""continue"", remove the ""else"" and de-indent the code inside it":HIGH

‎tests/functional/n/no/no_else_raise.txt

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
1-
no-else-raise:6:4:11:26:foo1:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":UNDEFINED
2-
no-else-raise:15:4:23:26:foo2:"Unnecessary ""elif"" after ""raise"", remove the leading ""el"" from ""elif""":UNDEFINED
3-
no-else-raise:29:8:34:30:foo3:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":UNDEFINED
4-
no-else-raise:41:4:48:13:foo4:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":UNDEFINED
5-
no-else-raise:53:4:62:13:foo5:"Unnecessary ""elif"" after ""raise"", remove the leading ""el"" from ""elif""":UNDEFINED
6-
no-else-raise:68:8:72:17:foo6:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":UNDEFINED
7-
no-else-raise:104:4:110:33:bar4:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":UNDEFINED
1+
no-else-raise:6:4:11:26:foo1:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":HIGH
2+
no-else-raise:15:4:23:26:foo2:"Unnecessary ""elif"" after ""raise"", remove the leading ""el"" from ""elif""":HIGH
3+
no-else-raise:29:8:34:30:foo3:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":HIGH
4+
no-else-raise:41:4:48:13:foo4:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":HIGH
5+
no-else-raise:53:4:62:13:foo5:"Unnecessary ""elif"" after ""raise"", remove the leading ""el"" from ""elif""":HIGH
6+
no-else-raise:68:8:72:17:foo6:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":HIGH
7+
no-else-raise:104:4:110:33:bar4:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":HIGH

‎tests/functional/n/no/no_else_return.py

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,3 +108,87 @@ def bar4(x):
108108
return False
109109
except ValueError:
110110
return None
111+
112+
# pylint: disable = bare-except
113+
def try_one_except() -> bool:
114+
try: # [no-else-return]
115+
print('asdf')
116+
except:
117+
print("bad")
118+
return False
119+
else:
120+
return True
121+
122+
123+
def try_multiple_except() -> bool:
124+
try: # [no-else-return]
125+
print('asdf')
126+
except TypeError:
127+
print("bad")
128+
return False
129+
except ValueError:
130+
print("bad second")
131+
return False
132+
else:
133+
return True
134+
135+
def try_not_all_except_return() -> bool: # [inconsistent-return-statements]
136+
try:
137+
print('asdf')
138+
except TypeError:
139+
print("bad")
140+
return False
141+
except ValueError:
142+
val = "something"
143+
else:
144+
return True
145+
146+
# pylint: disable = raise-missing-from
147+
def try_except_raises() -> bool:
148+
try: # [no-else-raise]
149+
print('asdf')
150+
except:
151+
raise ValueError
152+
else:
153+
return True
154+
155+
def try_except_raises2() -> bool:
156+
try: # [no-else-raise]
157+
print('asdf')
158+
except TypeError:
159+
raise ValueError
160+
except ValueError:
161+
raise TypeError
162+
else:
163+
return True
164+
165+
def test() -> bool: # [inconsistent-return-statements]
166+
try:
167+
print('asdf')
168+
except RuntimeError:
169+
return False
170+
finally:
171+
print("in finally")
172+
173+
174+
def try_finally_return() -> bool: # [inconsistent-return-statements]
175+
try:
176+
print('asdf')
177+
except RuntimeError:
178+
return False
179+
else:
180+
print("inside else")
181+
finally:
182+
print("in finally")
183+
184+
def try_finally_raise():
185+
current_tags = {}
186+
try:
187+
yield current_tags
188+
except Exception:
189+
current_tags["result"] = "failure"
190+
raise
191+
else:
192+
current_tags["result"] = "success"
193+
finally:
194+
print("inside finally")

‎tests/functional/n/no/no_else_return.txt

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,14 @@
1-
no-else-return:6:4:11:16:foo1:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":UNDEFINED
2-
no-else-return:15:4:23:16:foo2:"Unnecessary ""elif"" after ""return"", remove the leading ""el"" from ""elif""":UNDEFINED
3-
no-else-return:29:8:34:20:foo3:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":UNDEFINED
4-
no-else-return:41:4:48:13:foo4:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":UNDEFINED
5-
no-else-return:53:4:62:13:foo5:"Unnecessary ""elif"" after ""return"", remove the leading ""el"" from ""elif""":UNDEFINED
6-
no-else-return:68:8:72:17:foo6:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":UNDEFINED
7-
no-else-return:104:4:110:23:bar4:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":UNDEFINED
1+
no-else-return:6:4:11:16:foo1:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":HIGH
2+
no-else-return:15:4:23:16:foo2:"Unnecessary ""elif"" after ""return"", remove the leading ""el"" from ""elif""":HIGH
3+
no-else-return:29:8:34:20:foo3:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":HIGH
4+
no-else-return:41:4:48:13:foo4:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":HIGH
5+
no-else-return:53:4:62:13:foo5:"Unnecessary ""elif"" after ""return"", remove the leading ""el"" from ""elif""":HIGH
6+
no-else-return:68:8:72:17:foo6:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":HIGH
7+
no-else-return:104:4:110:23:bar4:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":HIGH
8+
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
9+
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
10+
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
11+
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
12+
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
13+
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
14+
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

‎tests/functional/r/raise_missing_from.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
# pylint:disable=missing-docstring, unreachable, using-constant-test, invalid-name, bare-except
2-
# pylint:disable=try-except-raise, undefined-variable, too-few-public-methods, superfluous-parens
2+
# pylint:disable=try-except-raise, undefined-variable, too-few-public-methods, superfluous-parens, no-else-raise
33

44
try:
55
1 / 0

0 commit comments

Comments
 (0)