Skip to content

Commit

Permalink
[pylint] Add fix for useless-else-on-loop (PLW0120) (#9590)
Browse files Browse the repository at this point in the history
## Summary

adds fix for `useless-else-on-loop` / `PLW0120`.

## Test Plan

`cargo test`
  • Loading branch information
diceroll123 committed Jan 21, 2024
1 parent 9dc59cb commit 9e5f3f1
Show file tree
Hide file tree
Showing 6 changed files with 332 additions and 23 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -135,3 +135,14 @@ def test_break_in_match():
else:
return True
return False


def test_retain_comment():
"""Retain the comment within the `else` block"""
for j in range(10):
pass
else:
# [useless-else-on-loop]
print("fat chance")
for j in range(10):
break
21 changes: 20 additions & 1 deletion crates/ruff_linter/src/rules/pylint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,13 @@ mod tests {
use rustc_hash::FxHashSet;
use test_case::test_case;

use crate::assert_messages;
use crate::registry::Rule;
use crate::rules::pylint;
use crate::settings::types::PreviewMode;
use crate::settings::types::PythonVersion;
use crate::settings::LinterSettings;
use crate::test::test_path;
use crate::{assert_messages, settings};

#[test_case(Rule::AndOrTernary, Path::new("and_or_ternary.py"))]
#[test_case(Rule::AssertOnStringLiteral, Path::new("assert_on_string_literal.py"))]
Expand Down Expand Up @@ -191,6 +192,24 @@ mod tests {
Ok(())
}

#[test_case(Rule::UselessElseOnLoop, Path::new("useless_else_on_loop.py"))]
fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!(
"preview__{}_{}",
rule_code.noqa_code(),
path.to_string_lossy()
);
let diagnostics = test_path(
Path::new("pylint").join(path).as_path(),
&settings::LinterSettings {
preview: PreviewMode::Enabled,
..settings::LinterSettings::for_rule(rule_code)
},
)?;
assert_messages!(snapshot, diagnostics);
Ok(())
}

#[test]
fn repeated_isinstance_calls() -> Result<()> {
let diagnostics = test_path(
Expand Down
102 changes: 88 additions & 14 deletions crates/ruff_linter/src/rules/pylint/rules/useless_else_on_loop.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,16 @@
use ruff_python_ast::{self as ast, ExceptHandler, MatchCase, Stmt};
use anyhow::Result;

use ruff_diagnostics::{Diagnostic, Violation};
use ast::whitespace::indentation;
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::identifier;
use ruff_python_ast::{self as ast, ExceptHandler, MatchCase, Stmt};
use ruff_python_codegen::Stylist;
use ruff_source_file::Locator;
use ruff_text_size::{Ranged, TextRange};

use crate::checkers::ast::Checker;
use crate::rules::pyupgrade::fixes::adjust_indentation;

/// ## What it does
/// Checks for `else` clauses on loops without a `break` statement.
Expand Down Expand Up @@ -42,15 +48,50 @@ use crate::checkers::ast::Checker;
pub struct UselessElseOnLoop;

impl Violation for UselessElseOnLoop {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;
#[derive_message_formats]
fn message(&self) -> String {
format!(
"`else` clause on loop without a `break` statement; remove the `else` and de-indent all the \
code inside it"
"`else` clause on loop without a `break` statement; remove the `else` and dedent its contents"
)
}

fn fix_title(&self) -> Option<String> {
Some("Remove `else`".to_string())
}
}

/// PLW0120
pub(crate) fn useless_else_on_loop(
checker: &mut Checker,
stmt: &Stmt,
body: &[Stmt],
orelse: &[Stmt],
) {
if orelse.is_empty() || loop_exits_early(body) {
return;
}

let else_range = identifier::else_(stmt, checker.locator().contents()).expect("else clause");

let mut diagnostic = Diagnostic::new(UselessElseOnLoop, else_range);

if checker.settings.preview.is_enabled() {
diagnostic.try_set_fix(|| {
remove_else(
stmt,
orelse,
else_range,
checker.locator(),
checker.stylist(),
)
});
}

checker.diagnostics.push(diagnostic);
}

/// Returns `true` if the given body contains a `break` statement.
fn loop_exits_early(body: &[Stmt]) -> bool {
body.iter().any(|stmt| match stmt {
Stmt::If(ast::StmtIf {
Expand Down Expand Up @@ -91,17 +132,50 @@ fn loop_exits_early(body: &[Stmt]) -> bool {
})
}

/// PLW0120
pub(crate) fn useless_else_on_loop(
checker: &mut Checker,
/// Generate a [`Fix`] to remove the `else` clause from the given statement.
fn remove_else(
stmt: &Stmt,
body: &[Stmt],
orelse: &[Stmt],
) {
if !orelse.is_empty() && !loop_exits_early(body) {
checker.diagnostics.push(Diagnostic::new(
UselessElseOnLoop,
identifier::else_(stmt, checker.locator().contents()).unwrap(),
));
else_range: TextRange,
locator: &Locator,
stylist: &Stylist,
) -> Result<Fix> {
let Some(start) = orelse.first() else {
return Err(anyhow::anyhow!("Empty `else` clause"));
};
let Some(end) = orelse.last() else {
return Err(anyhow::anyhow!("Empty `else` clause"));
};

let start_indentation = indentation(locator, start);
if start_indentation.is_none() {
// Inline `else` block (e.g., `else: x = 1`).
Ok(Fix::safe_edit(Edit::deletion(
else_range.start(),
start.start(),
)))
} else {
// Identify the indentation of the loop itself (e.g., the `while` or `for`).
let Some(desired_indentation) = indentation(locator, stmt) else {
return Err(anyhow::anyhow!("Compound statement cannot be inlined"));
};

// Dedent the content from the end of the `else` to the end of the loop.
let indented = adjust_indentation(
TextRange::new(
locator.full_line_end(else_range.start()),
locator.full_line_end(end.end()),
),
desired_indentation,
locator,
stylist,
)?;

// Replace the content from the start of the `else` to the end of the loop.
Ok(Fix::safe_edit(Edit::replacement(
indented,
locator.line_start(else_range.start()),
locator.full_line_end(end.end()),
)))
}
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
---
source: crates/ruff_linter/src/rules/pylint/mod.rs
---
useless_else_on_loop.py:9:5: PLW0120 `else` clause on loop without a `break` statement; remove the `else` and de-indent all the code inside it
useless_else_on_loop.py:9:5: PLW0120 `else` clause on loop without a `break` statement; remove the `else` and dedent its contents
|
7 | if i % 2:
8 | return i
Expand All @@ -10,8 +10,9 @@ useless_else_on_loop.py:9:5: PLW0120 `else` clause on loop without a `break` sta
10 | print("math is broken")
11 | return None
|
= help: Remove `else`

useless_else_on_loop.py:18:5: PLW0120 `else` clause on loop without a `break` statement; remove the `else` and de-indent all the code inside it
useless_else_on_loop.py:18:5: PLW0120 `else` clause on loop without a `break` statement; remove the `else` and dedent its contents
|
16 | while True:
17 | return 1
Expand All @@ -20,26 +21,29 @@ useless_else_on_loop.py:18:5: PLW0120 `else` clause on loop without a `break` st
19 | print("math is broken")
20 | return None
|
= help: Remove `else`

useless_else_on_loop.py:30:1: PLW0120 `else` clause on loop without a `break` statement; remove the `else` and de-indent all the code inside it
useless_else_on_loop.py:30:1: PLW0120 `else` clause on loop without a `break` statement; remove the `else` and dedent its contents
|
28 | break
29 |
30 | else: # [useless-else-on-loop]
| ^^^^ PLW0120
31 | print("or else!")
|
= help: Remove `else`

useless_else_on_loop.py:37:1: PLW0120 `else` clause on loop without a `break` statement; remove the `else` and de-indent all the code inside it
useless_else_on_loop.py:37:1: PLW0120 `else` clause on loop without a `break` statement; remove the `else` and dedent its contents
|
35 | while False:
36 | break
37 | else: # [useless-else-on-loop]
| ^^^^ PLW0120
38 | print("or else!")
|
= help: Remove `else`

useless_else_on_loop.py:42:1: PLW0120 `else` clause on loop without a `break` statement; remove the `else` and de-indent all the code inside it
useless_else_on_loop.py:42:1: PLW0120 `else` clause on loop without a `break` statement; remove the `else` and dedent its contents
|
40 | for j in range(10):
41 | pass
Expand All @@ -48,8 +52,9 @@ useless_else_on_loop.py:42:1: PLW0120 `else` clause on loop without a `break` st
43 | print("fat chance")
44 | for j in range(10):
|
= help: Remove `else`

useless_else_on_loop.py:88:5: PLW0120 `else` clause on loop without a `break` statement; remove the `else` and de-indent all the code inside it
useless_else_on_loop.py:88:5: PLW0120 `else` clause on loop without a `break` statement; remove the `else` and dedent its contents
|
86 | else:
87 | print("all right")
Expand All @@ -58,8 +63,9 @@ useless_else_on_loop.py:88:5: PLW0120 `else` clause on loop without a `break` st
89 | return True
90 | return False
|
= help: Remove `else`

useless_else_on_loop.py:98:9: PLW0120 `else` clause on loop without a `break` statement; remove the `else` and de-indent all the code inside it
useless_else_on_loop.py:98:9: PLW0120 `else` clause on loop without a `break` statement; remove the `else` and dedent its contents
|
96 | for _ in range(3):
97 | pass
Expand All @@ -68,5 +74,17 @@ useless_else_on_loop.py:98:9: PLW0120 `else` clause on loop without a `break` st
99 | if 1 < 2: # pylint: disable=comparison-of-constants
100 | break
|
= help: Remove `else`

useless_else_on_loop.py:144:5: PLW0120 `else` clause on loop without a `break` statement; remove the `else` and dedent its contents
|
142 | for j in range(10):
143 | pass
144 | else:
| ^^^^ PLW0120
145 | # [useless-else-on-loop]
146 | print("fat chance")
|
= help: Remove `else`


0 comments on commit 9e5f3f1

Please sign in to comment.