Skip to content

Commit e070166

Browse files
harupyharupy
authored
Implement useless-else-on-loop (#1031)
1 parent 0ae6890 commit e070166

File tree

8 files changed

+245
-4
lines changed

8 files changed

+245
-4
lines changed

‎README.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -772,6 +772,7 @@ For more, see [Pylint](https://pypi.org/project/pylint/2.15.7/) on PyPI.
772772
| PLR0206 | PropertyWithParameters | Cannot have defined parameters for properties | |
773773
| PLR0402 | ConsiderUsingFromImport | Consider using `from ... import ...` | |
774774
| PLR1701 | ConsiderMergingIsinstance | Consider merging these isinstance calls: `isinstance(..., (...))` | |
775+
| PLW0120 | UselessElseOnLoop | Else clause on loop without a break statement, remove the else and de-indent all the code inside it. | |
775776
776777
### Ruff-specific rules
777778

‎resources/test/fixtures/pylint/useless_else_on_loop.py

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
"""Check for else branches on loops with break and return only."""
2+
3+
4+
def test_return_for():
5+
"""else + return is not acceptable."""
6+
for i in range(10):
7+
if i % 2:
8+
return i
9+
else: # [useless-else-on-loop]
10+
print("math is broken")
11+
return None
12+
13+
14+
def test_return_while():
15+
"""else + return is not acceptable."""
16+
while True:
17+
return 1
18+
else: # [useless-else-on-loop]
19+
print("math is broken")
20+
return None
21+
22+
23+
while True:
24+
25+
def short_fun():
26+
"""A function with a loop."""
27+
for _ in range(10):
28+
break
29+
30+
else: # [useless-else-on-loop]
31+
print("or else!")
32+
33+
34+
while True:
35+
while False:
36+
break
37+
else: # [useless-else-on-loop]
38+
print("or else!")
39+
40+
for j in range(10):
41+
pass
42+
else: # [useless-else-on-loop]
43+
print("fat chance")
44+
for j in range(10):
45+
break
46+
47+
48+
def test_return_for2():
49+
"""no false positive for break in else
50+
51+
https://bitbucket.org/logilab/pylint/issue/117/useless-else-on-loop-false-positives
52+
"""
53+
for i in range(10):
54+
for _ in range(i):
55+
if i % 2:
56+
break
57+
else:
58+
break
59+
else:
60+
print("great math")
61+
62+
63+
def test_break_in_orelse_deep():
64+
"""no false positive for break in else deeply nested"""
65+
for _ in range(10):
66+
if 1 < 2: # pylint: disable=comparison-of-constants
67+
for _ in range(3):
68+
if 3 < 2: # pylint: disable=comparison-of-constants
69+
break
70+
else:
71+
break
72+
else:
73+
return True
74+
return False
75+
76+
77+
def test_break_in_orelse_deep2():
78+
"""should rise a useless-else-on-loop message, as the break statement is only
79+
for the inner for loop
80+
"""
81+
for _ in range(10):
82+
if 1 < 2: # pylint: disable=comparison-of-constants
83+
for _ in range(3):
84+
if 3 < 2: # pylint: disable=comparison-of-constants
85+
break
86+
else:
87+
print("all right")
88+
else: # [useless-else-on-loop]
89+
return True
90+
return False
91+
92+
93+
def test_break_in_orelse_deep3():
94+
"""no false positive for break deeply nested in else"""
95+
for _ in range(10):
96+
for _ in range(3):
97+
pass
98+
else:
99+
if 1 < 2: # pylint: disable=comparison-of-constants
100+
break
101+
else:
102+
return True
103+
return False

‎src/check_ast.rs

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1009,16 +1009,27 @@ where
10091009
flake8_bugbear::plugins::assert_raises_exception(self, stmt, items);
10101010
}
10111011
}
1012-
StmtKind::While { .. } => {
1012+
StmtKind::While { body, orelse, .. } => {
10131013
if self.settings.enabled.contains(&CheckCode::B023) {
10141014
flake8_bugbear::plugins::function_uses_loop_variable(self, &Node::Stmt(stmt));
10151015
}
1016+
if self.settings.enabled.contains(&CheckCode::PLW0120) {
1017+
pylint::plugins::useless_else_on_loop(self, stmt, body, orelse);
1018+
}
10161019
}
10171020
StmtKind::For {
1018-
target, body, iter, ..
1021+
target,
1022+
body,
1023+
iter,
1024+
orelse,
1025+
..
10191026
}
10201027
| StmtKind::AsyncFor {
1021-
target, body, iter, ..
1028+
target,
1029+
body,
1030+
iter,
1031+
orelse,
1032+
..
10221033
} => {
10231034
if self.settings.enabled.contains(&CheckCode::B007) {
10241035
flake8_bugbear::plugins::unused_loop_control_variable(self, target, body);
@@ -1029,6 +1040,9 @@ where
10291040
if self.settings.enabled.contains(&CheckCode::B023) {
10301041
flake8_bugbear::plugins::function_uses_loop_variable(self, &Node::Stmt(stmt));
10311042
}
1043+
if self.settings.enabled.contains(&CheckCode::PLW0120) {
1044+
pylint::plugins::useless_else_on_loop(self, stmt, body, orelse);
1045+
}
10321046
}
10331047
StmtKind::Try { handlers, .. } => {
10341048
if self.settings.enabled.contains(&CheckCode::F707) {

‎src/checks.rs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,7 @@ pub enum CheckCode {
9999
PLR0206,
100100
PLR0402,
101101
PLR1701,
102+
PLW0120,
102103
// flake8-builtins
103104
A001,
104105
A002,
@@ -579,6 +580,7 @@ pub enum CheckKind {
579580
PropertyWithParameters,
580581
ConsiderUsingFromImport(String, String),
581582
AwaitOutsideAsync,
583+
UselessElseOnLoop,
582584
// flake8-builtins
583585
BuiltinVariableShadowing(String),
584586
BuiltinArgumentShadowing(String),
@@ -881,6 +883,7 @@ impl CheckCode {
881883
CheckCode::PLR1701 => {
882884
CheckKind::ConsiderMergingIsinstance("...".to_string(), vec!["...".to_string()])
883885
}
886+
CheckCode::PLW0120 => CheckKind::UselessElseOnLoop,
884887
// flake8-builtins
885888
CheckCode::A001 => CheckKind::BuiltinVariableShadowing("...".to_string()),
886889
CheckCode::A002 => CheckKind::BuiltinArgumentShadowing("...".to_string()),
@@ -1305,6 +1308,7 @@ impl CheckCode {
13051308
CheckCode::PLR0206 => CheckCategory::Pylint,
13061309
CheckCode::PLR0402 => CheckCategory::Pylint,
13071310
CheckCode::PLR1701 => CheckCategory::Pylint,
1311+
CheckCode::PLW0120 => CheckCategory::Pylint,
13081312
CheckCode::Q000 => CheckCategory::Flake8Quotes,
13091313
CheckCode::Q001 => CheckCategory::Flake8Quotes,
13101314
CheckCode::Q002 => CheckCategory::Flake8Quotes,
@@ -1432,6 +1436,7 @@ impl CheckKind {
14321436
CheckKind::ConsiderMergingIsinstance(..) => &CheckCode::PLR1701,
14331437
CheckKind::PropertyWithParameters => &CheckCode::PLR0206,
14341438
CheckKind::ConsiderUsingFromImport(..) => &CheckCode::PLR0402,
1439+
CheckKind::UselessElseOnLoop => &CheckCode::PLW0120,
14351440
// flake8-builtins
14361441
CheckKind::BuiltinVariableShadowing(_) => &CheckCode::A001,
14371442
CheckKind::BuiltinArgumentShadowing(_) => &CheckCode::A002,
@@ -1831,6 +1836,9 @@ impl CheckKind {
18311836
CheckKind::AwaitOutsideAsync => {
18321837
"`await` should be used within an async function".to_string()
18331838
}
1839+
CheckKind::UselessElseOnLoop => "Else clause on loop without a break statement, \
1840+
remove the else and de-indent all the code inside it"
1841+
.to_string(),
18341842
// flake8-builtins
18351843
CheckKind::BuiltinVariableShadowing(name) => {
18361844
format!("Variable `{name}` is shadowing a python builtin")

‎src/checks_gen.rs

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -305,6 +305,11 @@ pub enum CheckCodePrefix {
305305
PLR17,
306306
PLR170,
307307
PLR1701,
308+
PLW,
309+
PLW0,
310+
PLW01,
311+
PLW012,
312+
PLW0120,
308313
Q,
309314
Q0,
310315
Q00,
@@ -1228,6 +1233,11 @@ impl CheckCodePrefix {
12281233
CheckCodePrefix::PLR17 => vec![CheckCode::PLR1701],
12291234
CheckCodePrefix::PLR170 => vec![CheckCode::PLR1701],
12301235
CheckCodePrefix::PLR1701 => vec![CheckCode::PLR1701],
1236+
CheckCodePrefix::PLW => vec![CheckCode::PLW0120],
1237+
CheckCodePrefix::PLW0 => vec![CheckCode::PLW0120],
1238+
CheckCodePrefix::PLW01 => vec![CheckCode::PLW0120],
1239+
CheckCodePrefix::PLW012 => vec![CheckCode::PLW0120],
1240+
CheckCodePrefix::PLW0120 => vec![CheckCode::PLW0120],
12311241
CheckCodePrefix::Q => vec![
12321242
CheckCode::Q000,
12331243
CheckCode::Q001,
@@ -1942,6 +1952,11 @@ impl CheckCodePrefix {
19421952
CheckCodePrefix::PLR17 => SuffixLength::Two,
19431953
CheckCodePrefix::PLR170 => SuffixLength::Three,
19441954
CheckCodePrefix::PLR1701 => SuffixLength::Four,
1955+
CheckCodePrefix::PLW => SuffixLength::Zero,
1956+
CheckCodePrefix::PLW0 => SuffixLength::One,
1957+
CheckCodePrefix::PLW01 => SuffixLength::Two,
1958+
CheckCodePrefix::PLW012 => SuffixLength::Three,
1959+
CheckCodePrefix::PLW0120 => SuffixLength::Four,
19451960
CheckCodePrefix::Q => SuffixLength::Zero,
19461961
CheckCodePrefix::Q0 => SuffixLength::One,
19471962
CheckCodePrefix::Q00 => SuffixLength::Two,
@@ -2068,6 +2083,7 @@ pub const CATEGORIES: &[CheckCodePrefix] = &[
20682083
CheckCodePrefix::PLC,
20692084
CheckCodePrefix::PLE,
20702085
CheckCodePrefix::PLR,
2086+
CheckCodePrefix::PLW,
20712087
CheckCodePrefix::Q,
20722088
CheckCodePrefix::RET,
20732089
CheckCodePrefix::RUF,

‎src/pylint/mod.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ mod tests {
1717
#[test_case(CheckCode::PLR0206, Path::new("property_with_parameters.py"); "PLR0206")]
1818
#[test_case(CheckCode::PLR0402, Path::new("import_aliasing.py"); "PLR0402")]
1919
#[test_case(CheckCode::PLR1701, Path::new("consider_merging_isinstance.py"); "PLR1701")]
20+
#[test_case(CheckCode::PLW0120, Path::new("useless_else_on_loop.py"); "PLW0120")]
2021
fn checks(check_code: CheckCode, path: &Path) -> Result<()> {
2122
let snapshot = format!("{}", path.to_string_lossy());
2223
let mut checks = test_path(

‎src/pylint/plugins.rs

Lines changed: 38 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
use itertools::Itertools;
22
use rustc_hash::{FxHashMap, FxHashSet};
3-
use rustpython_ast::{Alias, Arguments, Boolop, Cmpop, Expr, ExprKind, Stmt};
3+
use rustpython_ast::{
4+
Alias, Arguments, Boolop, Cmpop, ExcepthandlerKind, Expr, ExprKind, Stmt, StmtKind,
5+
};
46

57
use crate::ast::types::{FunctionScope, Range, ScopeKind};
68
use crate::autofix::Fix;
@@ -162,3 +164,38 @@ pub fn consider_merging_isinstance(
162164
}
163165
}
164166
}
167+
168+
fn loop_exits_early(body: &[Stmt]) -> bool {
169+
body.iter().any(|stmt| match &stmt.node {
170+
StmtKind::If { body, .. } => loop_exits_early(body),
171+
StmtKind::Try {
172+
body,
173+
handlers,
174+
orelse,
175+
finalbody,
176+
..
177+
} => {
178+
loop_exits_early(body)
179+
|| handlers.iter().any(|handler| match &handler.node {
180+
ExcepthandlerKind::ExceptHandler { body, .. } => loop_exits_early(body),
181+
})
182+
|| loop_exits_early(orelse)
183+
|| loop_exits_early(finalbody)
184+
}
185+
StmtKind::For { orelse, .. }
186+
| StmtKind::AsyncFor { orelse, .. }
187+
| StmtKind::While { orelse, .. } => loop_exits_early(orelse),
188+
StmtKind::Break { .. } => true,
189+
_ => false,
190+
})
191+
}
192+
193+
/// PLW0120
194+
pub fn useless_else_on_loop(checker: &mut Checker, stmt: &Stmt, body: &[Stmt], orelse: &[Stmt]) {
195+
if !orelse.is_empty() && !loop_exits_early(body) {
196+
checker.add_check(Check::new(
197+
CheckKind::UselessElseOnLoop,
198+
Range::from_located(stmt),
199+
));
200+
}
201+
}

‎src/pylint/snapshots/ruff__pylint__tests__useless_else_on_loop.py.snap

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
---
2+
source: src/pylint/mod.rs
3+
expression: checks
4+
---
5+
- kind: UselessElseOnLoop
6+
location:
7+
row: 6
8+
column: 4
9+
end_location:
10+
row: 11
11+
column: 4
12+
fix: ~
13+
- kind: UselessElseOnLoop
14+
location:
15+
row: 16
16+
column: 4
17+
end_location:
18+
row: 20
19+
column: 4
20+
fix: ~
21+
- kind: UselessElseOnLoop
22+
location:
23+
row: 23
24+
column: 0
25+
end_location:
26+
row: 34
27+
column: 0
28+
fix: ~
29+
- kind: UselessElseOnLoop
30+
location:
31+
row: 34
32+
column: 0
33+
end_location:
34+
row: 40
35+
column: 0
36+
fix: ~
37+
- kind: UselessElseOnLoop
38+
location:
39+
row: 40
40+
column: 0
41+
end_location:
42+
row: 48
43+
column: 0
44+
fix: ~
45+
- kind: UselessElseOnLoop
46+
location:
47+
row: 81
48+
column: 4
49+
end_location:
50+
row: 90
51+
column: 4
52+
fix: ~
53+
- kind: UselessElseOnLoop
54+
location:
55+
row: 96
56+
column: 8
57+
end_location:
58+
row: 101
59+
column: 4
60+
fix: ~
61+

0 commit comments

Comments
 (0)