diff --git a/README.md b/README.md index a2efb4d0b5..4700382ec5 100644 --- a/README.md +++ b/README.md @@ -772,6 +772,7 @@ For more, see [Pylint](https://pypi.org/project/pylint/2.15.7/) on PyPI. | PLR0206 | PropertyWithParameters | Cannot have defined parameters for properties | | | PLR0402 | ConsiderUsingFromImport | Consider using `from ... import ...` | | | PLR1701 | ConsiderMergingIsinstance | Consider merging these isinstance calls: `isinstance(..., (...))` | | +| PLW0120 | UselessElseOnLoop | Else clause on loop without a break statement, remove the else and de-indent all the code inside it. | | ### Ruff-specific rules diff --git a/resources/test/fixtures/pylint/useless_else_on_loop.py b/resources/test/fixtures/pylint/useless_else_on_loop.py new file mode 100644 index 0000000000..9d370dc12f --- /dev/null +++ b/resources/test/fixtures/pylint/useless_else_on_loop.py @@ -0,0 +1,103 @@ +"""Check for else branches on loops with break and return only.""" + + +def test_return_for(): + """else + return is not acceptable.""" + for i in range(10): + if i % 2: + return i + else: # [useless-else-on-loop] + print("math is broken") + return None + + +def test_return_while(): + """else + return is not acceptable.""" + while True: + return 1 + else: # [useless-else-on-loop] + print("math is broken") + return None + + +while True: + + def short_fun(): + """A function with a loop.""" + for _ in range(10): + break + +else: # [useless-else-on-loop] + print("or else!") + + +while True: + while False: + break +else: # [useless-else-on-loop] + print("or else!") + +for j in range(10): + pass +else: # [useless-else-on-loop] + print("fat chance") + for j in range(10): + break + + +def test_return_for2(): + """no false positive for break in else + + https://bitbucket.org/logilab/pylint/issue/117/useless-else-on-loop-false-positives + """ + for i in range(10): + for _ in range(i): + if i % 2: + break + else: + break + else: + print("great math") + + +def test_break_in_orelse_deep(): + """no false positive for break in else deeply nested""" + for _ in range(10): + if 1 < 2: # pylint: disable=comparison-of-constants + for _ in range(3): + if 3 < 2: # pylint: disable=comparison-of-constants + break + else: + break + else: + return True + return False + + +def test_break_in_orelse_deep2(): + """should rise a useless-else-on-loop message, as the break statement is only + for the inner for loop + """ + for _ in range(10): + if 1 < 2: # pylint: disable=comparison-of-constants + for _ in range(3): + if 3 < 2: # pylint: disable=comparison-of-constants + break + else: + print("all right") + else: # [useless-else-on-loop] + return True + return False + + +def test_break_in_orelse_deep3(): + """no false positive for break deeply nested in else""" + for _ in range(10): + for _ in range(3): + pass + else: + if 1 < 2: # pylint: disable=comparison-of-constants + break + else: + return True + return False diff --git a/src/check_ast.rs b/src/check_ast.rs index 7e8db15f8b..e607eedbe7 100644 --- a/src/check_ast.rs +++ b/src/check_ast.rs @@ -1009,16 +1009,27 @@ where flake8_bugbear::plugins::assert_raises_exception(self, stmt, items); } } - StmtKind::While { .. } => { + StmtKind::While { body, orelse, .. } => { if self.settings.enabled.contains(&CheckCode::B023) { flake8_bugbear::plugins::function_uses_loop_variable(self, &Node::Stmt(stmt)); } + if self.settings.enabled.contains(&CheckCode::PLW0120) { + pylint::plugins::useless_else_on_loop(self, stmt, body, orelse); + } } StmtKind::For { - target, body, iter, .. + target, + body, + iter, + orelse, + .. } | StmtKind::AsyncFor { - target, body, iter, .. + target, + body, + iter, + orelse, + .. } => { if self.settings.enabled.contains(&CheckCode::B007) { flake8_bugbear::plugins::unused_loop_control_variable(self, target, body); @@ -1029,6 +1040,9 @@ where if self.settings.enabled.contains(&CheckCode::B023) { flake8_bugbear::plugins::function_uses_loop_variable(self, &Node::Stmt(stmt)); } + if self.settings.enabled.contains(&CheckCode::PLW0120) { + pylint::plugins::useless_else_on_loop(self, stmt, body, orelse); + } } StmtKind::Try { handlers, .. } => { if self.settings.enabled.contains(&CheckCode::F707) { diff --git a/src/checks.rs b/src/checks.rs index 2b2fbfc1c8..bb5cb0552e 100644 --- a/src/checks.rs +++ b/src/checks.rs @@ -99,6 +99,7 @@ pub enum CheckCode { PLR0206, PLR0402, PLR1701, + PLW0120, // flake8-builtins A001, A002, @@ -579,6 +580,7 @@ pub enum CheckKind { PropertyWithParameters, ConsiderUsingFromImport(String, String), AwaitOutsideAsync, + UselessElseOnLoop, // flake8-builtins BuiltinVariableShadowing(String), BuiltinArgumentShadowing(String), @@ -881,6 +883,7 @@ impl CheckCode { CheckCode::PLR1701 => { CheckKind::ConsiderMergingIsinstance("...".to_string(), vec!["...".to_string()]) } + CheckCode::PLW0120 => CheckKind::UselessElseOnLoop, // flake8-builtins CheckCode::A001 => CheckKind::BuiltinVariableShadowing("...".to_string()), CheckCode::A002 => CheckKind::BuiltinArgumentShadowing("...".to_string()), @@ -1305,6 +1308,7 @@ impl CheckCode { CheckCode::PLR0206 => CheckCategory::Pylint, CheckCode::PLR0402 => CheckCategory::Pylint, CheckCode::PLR1701 => CheckCategory::Pylint, + CheckCode::PLW0120 => CheckCategory::Pylint, CheckCode::Q000 => CheckCategory::Flake8Quotes, CheckCode::Q001 => CheckCategory::Flake8Quotes, CheckCode::Q002 => CheckCategory::Flake8Quotes, @@ -1432,6 +1436,7 @@ impl CheckKind { CheckKind::ConsiderMergingIsinstance(..) => &CheckCode::PLR1701, CheckKind::PropertyWithParameters => &CheckCode::PLR0206, CheckKind::ConsiderUsingFromImport(..) => &CheckCode::PLR0402, + CheckKind::UselessElseOnLoop => &CheckCode::PLW0120, // flake8-builtins CheckKind::BuiltinVariableShadowing(_) => &CheckCode::A001, CheckKind::BuiltinArgumentShadowing(_) => &CheckCode::A002, @@ -1831,6 +1836,9 @@ impl CheckKind { CheckKind::AwaitOutsideAsync => { "`await` should be used within an async function".to_string() } + CheckKind::UselessElseOnLoop => "Else clause on loop without a break statement, \ + remove the else and de-indent all the code inside it" + .to_string(), // flake8-builtins CheckKind::BuiltinVariableShadowing(name) => { format!("Variable `{name}` is shadowing a python builtin") diff --git a/src/checks_gen.rs b/src/checks_gen.rs index 6f3d208ef5..8b9895f85a 100644 --- a/src/checks_gen.rs +++ b/src/checks_gen.rs @@ -305,6 +305,11 @@ pub enum CheckCodePrefix { PLR17, PLR170, PLR1701, + PLW, + PLW0, + PLW01, + PLW012, + PLW0120, Q, Q0, Q00, @@ -1228,6 +1233,11 @@ impl CheckCodePrefix { CheckCodePrefix::PLR17 => vec![CheckCode::PLR1701], CheckCodePrefix::PLR170 => vec![CheckCode::PLR1701], CheckCodePrefix::PLR1701 => vec![CheckCode::PLR1701], + CheckCodePrefix::PLW => vec![CheckCode::PLW0120], + CheckCodePrefix::PLW0 => vec![CheckCode::PLW0120], + CheckCodePrefix::PLW01 => vec![CheckCode::PLW0120], + CheckCodePrefix::PLW012 => vec![CheckCode::PLW0120], + CheckCodePrefix::PLW0120 => vec![CheckCode::PLW0120], CheckCodePrefix::Q => vec![ CheckCode::Q000, CheckCode::Q001, @@ -1942,6 +1952,11 @@ impl CheckCodePrefix { CheckCodePrefix::PLR17 => SuffixLength::Two, CheckCodePrefix::PLR170 => SuffixLength::Three, CheckCodePrefix::PLR1701 => SuffixLength::Four, + CheckCodePrefix::PLW => SuffixLength::Zero, + CheckCodePrefix::PLW0 => SuffixLength::One, + CheckCodePrefix::PLW01 => SuffixLength::Two, + CheckCodePrefix::PLW012 => SuffixLength::Three, + CheckCodePrefix::PLW0120 => SuffixLength::Four, CheckCodePrefix::Q => SuffixLength::Zero, CheckCodePrefix::Q0 => SuffixLength::One, CheckCodePrefix::Q00 => SuffixLength::Two, @@ -2068,6 +2083,7 @@ pub const CATEGORIES: &[CheckCodePrefix] = &[ CheckCodePrefix::PLC, CheckCodePrefix::PLE, CheckCodePrefix::PLR, + CheckCodePrefix::PLW, CheckCodePrefix::Q, CheckCodePrefix::RET, CheckCodePrefix::RUF, diff --git a/src/pylint/mod.rs b/src/pylint/mod.rs index d81b48389e..6a58c19440 100644 --- a/src/pylint/mod.rs +++ b/src/pylint/mod.rs @@ -17,6 +17,7 @@ mod tests { #[test_case(CheckCode::PLR0206, Path::new("property_with_parameters.py"); "PLR0206")] #[test_case(CheckCode::PLR0402, Path::new("import_aliasing.py"); "PLR0402")] #[test_case(CheckCode::PLR1701, Path::new("consider_merging_isinstance.py"); "PLR1701")] + #[test_case(CheckCode::PLW0120, Path::new("useless_else_on_loop.py"); "PLW0120")] fn checks(check_code: CheckCode, path: &Path) -> Result<()> { let snapshot = format!("{}", path.to_string_lossy()); let mut checks = test_path( diff --git a/src/pylint/plugins.rs b/src/pylint/plugins.rs index 0182153aa3..e72d12e8a4 100644 --- a/src/pylint/plugins.rs +++ b/src/pylint/plugins.rs @@ -1,6 +1,8 @@ use itertools::Itertools; use rustc_hash::{FxHashMap, FxHashSet}; -use rustpython_ast::{Alias, Arguments, Boolop, Cmpop, Expr, ExprKind, Stmt}; +use rustpython_ast::{ + Alias, Arguments, Boolop, Cmpop, ExcepthandlerKind, Expr, ExprKind, Stmt, StmtKind, +}; use crate::ast::types::{FunctionScope, Range, ScopeKind}; use crate::autofix::Fix; @@ -162,3 +164,38 @@ pub fn consider_merging_isinstance( } } } + +fn loop_exits_early(body: &[Stmt]) -> bool { + body.iter().any(|stmt| match &stmt.node { + StmtKind::If { body, .. } => loop_exits_early(body), + StmtKind::Try { + body, + handlers, + orelse, + finalbody, + .. + } => { + loop_exits_early(body) + || handlers.iter().any(|handler| match &handler.node { + ExcepthandlerKind::ExceptHandler { body, .. } => loop_exits_early(body), + }) + || loop_exits_early(orelse) + || loop_exits_early(finalbody) + } + StmtKind::For { orelse, .. } + | StmtKind::AsyncFor { orelse, .. } + | StmtKind::While { orelse, .. } => loop_exits_early(orelse), + StmtKind::Break { .. } => true, + _ => false, + }) +} + +/// PLW0120 +pub fn useless_else_on_loop(checker: &mut Checker, stmt: &Stmt, body: &[Stmt], orelse: &[Stmt]) { + if !orelse.is_empty() && !loop_exits_early(body) { + checker.add_check(Check::new( + CheckKind::UselessElseOnLoop, + Range::from_located(stmt), + )); + } +} diff --git a/src/pylint/snapshots/ruff__pylint__tests__useless_else_on_loop.py.snap b/src/pylint/snapshots/ruff__pylint__tests__useless_else_on_loop.py.snap new file mode 100644 index 0000000000..433f962316 --- /dev/null +++ b/src/pylint/snapshots/ruff__pylint__tests__useless_else_on_loop.py.snap @@ -0,0 +1,61 @@ +--- +source: src/pylint/mod.rs +expression: checks +--- +- kind: UselessElseOnLoop + location: + row: 6 + column: 4 + end_location: + row: 11 + column: 4 + fix: ~ +- kind: UselessElseOnLoop + location: + row: 16 + column: 4 + end_location: + row: 20 + column: 4 + fix: ~ +- kind: UselessElseOnLoop + location: + row: 23 + column: 0 + end_location: + row: 34 + column: 0 + fix: ~ +- kind: UselessElseOnLoop + location: + row: 34 + column: 0 + end_location: + row: 40 + column: 0 + fix: ~ +- kind: UselessElseOnLoop + location: + row: 40 + column: 0 + end_location: + row: 48 + column: 0 + fix: ~ +- kind: UselessElseOnLoop + location: + row: 81 + column: 4 + end_location: + row: 90 + column: 4 + fix: ~ +- kind: UselessElseOnLoop + location: + row: 96 + column: 8 + end_location: + row: 101 + column: 4 + fix: ~ +