[ruff] Needless else clause (RUF047) (#15051)

Co-authored-by: Micha Reiser <micha@reiser.io>
This commit is contained in:
InSync 2025-01-21 15:21:19 +07:00 committed by GitHub
parent 4cfa355519
commit c616650dfa
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
14 changed files with 876 additions and 7 deletions

View file

@ -0,0 +1,49 @@
### Errors
for _ in range(0):
loop_body_is_not_checked()
break
else:
pass
for this in comment:
belongs_to() # `for`
else:
...
for of in course():
this()
else:
...
# this comment does not belong to the else
### No errors
for this in second_comment:
belongs() # to
# `else`
else:
pass
for _and in so:
does()
# this
else:
pass
for of in course():
this()
else:
... # too
for of in course():
this()
else:
...
# too

View file

@ -0,0 +1,84 @@
### Errors
if False:
condition_is_not_evaluated()
else:
pass
if this_comment():
belongs_to() # `if`
else:
...
if elif_is():
treated()
elif the_same():
as_if()
else:
pass
if this_second_comment():
belongs() # to
# `if`
else:
pass
if this_second_comment():
belongs() # to
# `if`
else:
pass
if of_course():
this()
else:
...
# this comment doesn't belong to the if
if of_course: this()
else: ...
if of_course:
this() # comment
else: ...
def nested():
if a:
b()
else:
...
### No errors
if this_second_comment():
belongs() # to
# `else`
else:
pass
if of_course():
this()
else:
... # too
if of_course():
this()
else:
...
# comment
if of_course:
this() # comment
else: ... # trailing

View file

@ -0,0 +1,76 @@
### Errors
try:
raise try_body_is_not_checked()
except:
pass
else:
pass
try:
this()
except comment:
belongs()
except:
to() # `except`
else:
...
try:
of_course()
except:
this()
else:
...
# This comment belongs to finally
finally:
pass
try:
of_course()
except:
this()
else:
...
# This comment belongs to the statement coming after the else
### No errors
try:
this()
except (second, comment):
belongs() # to
# `else`
else:
pass
try:
and_so()
except:
does()
# this
else:
...
try:
of_course()
except:
this()
else:
... # too
try:
of_course()
except:
this()
else:
...
# This comment belongs to else
finally:
pass

View file

@ -0,0 +1,49 @@
### No errors
while True:
loop_body_is_not_checked()
break
else:
pass
while this_comment:
belongs_to() # `for`
else:
...
while of_course():
this()
else:
...
# this comment belongs to the statement coming after the else
### No errors
while this_second_comment:
belongs() # to
# `else`
else:
pass
while and_so:
does()
# this
else:
...
while of_course():
this()
else:
... # too
while of_course():
this()
else:
...
# this comment belongs to the else

View file

@ -1240,6 +1240,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::IfKeyInDictDel) { if checker.enabled(Rule::IfKeyInDictDel) {
ruff::rules::if_key_in_dict_del(checker, if_); ruff::rules::if_key_in_dict_del(checker, if_);
} }
if checker.enabled(Rule::NeedlessElse) {
ruff::rules::needless_else(checker, if_.into());
}
} }
Stmt::Assert( Stmt::Assert(
assert_stmt @ ast::StmtAssert { assert_stmt @ ast::StmtAssert {
@ -1349,6 +1352,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::AsyncBusyWait) { if checker.enabled(Rule::AsyncBusyWait) {
flake8_async::rules::async_busy_wait(checker, while_stmt); flake8_async::rules::async_busy_wait(checker, while_stmt);
} }
if checker.enabled(Rule::NeedlessElse) {
ruff::rules::needless_else(checker, while_stmt.into());
}
} }
Stmt::For( Stmt::For(
for_stmt @ ast::StmtFor { for_stmt @ ast::StmtFor {
@ -1438,14 +1444,19 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
refurb::rules::for_loop_writes(checker, for_stmt); refurb::rules::for_loop_writes(checker, for_stmt);
} }
} }
if checker.enabled(Rule::NeedlessElse) {
ruff::rules::needless_else(checker, for_stmt.into());
}
} }
Stmt::Try(ast::StmtTry { Stmt::Try(
body, try_stmt @ ast::StmtTry {
handlers, body,
orelse, handlers,
finalbody, orelse,
.. finalbody,
}) => { ..
},
) => {
if checker.enabled(Rule::TooManyNestedBlocks) { if checker.enabled(Rule::TooManyNestedBlocks) {
pylint::rules::too_many_nested_blocks(checker, stmt); pylint::rules::too_many_nested_blocks(checker, stmt);
} }
@ -1512,6 +1523,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::ErrorInsteadOfException) { if checker.enabled(Rule::ErrorInsteadOfException) {
tryceratops::rules::error_instead_of_exception(checker, handlers); tryceratops::rules::error_instead_of_exception(checker, handlers);
} }
if checker.enabled(Rule::NeedlessElse) {
ruff::rules::needless_else(checker, try_stmt.into());
}
} }
Stmt::Assign(assign @ ast::StmtAssign { targets, value, .. }) => { Stmt::Assign(assign @ ast::StmtAssign { targets, value, .. }) => {
if checker.enabled(Rule::SelfOrClsAssignment) { if checker.enabled(Rule::SelfOrClsAssignment) {

View file

@ -997,6 +997,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Ruff, "041") => (RuleGroup::Preview, rules::ruff::rules::UnnecessaryNestedLiteral), (Ruff, "041") => (RuleGroup::Preview, rules::ruff::rules::UnnecessaryNestedLiteral),
(Ruff, "043") => (RuleGroup::Preview, rules::ruff::rules::PytestRaisesAmbiguousPattern), (Ruff, "043") => (RuleGroup::Preview, rules::ruff::rules::PytestRaisesAmbiguousPattern),
(Ruff, "046") => (RuleGroup::Preview, rules::ruff::rules::UnnecessaryCastToInt), (Ruff, "046") => (RuleGroup::Preview, rules::ruff::rules::UnnecessaryCastToInt),
(Ruff, "047") => (RuleGroup::Preview, rules::ruff::rules::NeedlessElse),
(Ruff, "048") => (RuleGroup::Preview, rules::ruff::rules::MapIntVersionParsing), (Ruff, "048") => (RuleGroup::Preview, rules::ruff::rules::MapIntVersionParsing),
(Ruff, "049") => (RuleGroup::Preview, rules::ruff::rules::DataclassEnum), (Ruff, "049") => (RuleGroup::Preview, rules::ruff::rules::DataclassEnum),
(Ruff, "051") => (RuleGroup::Preview, rules::ruff::rules::IfKeyInDictDel), (Ruff, "051") => (RuleGroup::Preview, rules::ruff::rules::IfKeyInDictDel),

View file

@ -79,6 +79,10 @@ mod tests {
#[test_case(Rule::InvalidAssertMessageLiteralArgument, Path::new("RUF040.py"))] #[test_case(Rule::InvalidAssertMessageLiteralArgument, Path::new("RUF040.py"))]
#[test_case(Rule::UnnecessaryNestedLiteral, Path::new("RUF041.py"))] #[test_case(Rule::UnnecessaryNestedLiteral, Path::new("RUF041.py"))]
#[test_case(Rule::UnnecessaryNestedLiteral, Path::new("RUF041.pyi"))] #[test_case(Rule::UnnecessaryNestedLiteral, Path::new("RUF041.pyi"))]
#[test_case(Rule::NeedlessElse, Path::new("RUF047_if.py"))]
#[test_case(Rule::NeedlessElse, Path::new("RUF047_for.py"))]
#[test_case(Rule::NeedlessElse, Path::new("RUF047_while.py"))]
#[test_case(Rule::NeedlessElse, Path::new("RUF047_try.py"))]
#[test_case(Rule::IfKeyInDictDel, Path::new("RUF051.py"))] #[test_case(Rule::IfKeyInDictDel, Path::new("RUF051.py"))]
#[test_case(Rule::UsedDummyVariable, Path::new("RUF052.py"))] #[test_case(Rule::UsedDummyVariable, Path::new("RUF052.py"))]
#[test_case(Rule::FalsyDictGetFallback, Path::new("RUF056.py"))] #[test_case(Rule::FalsyDictGetFallback, Path::new("RUF056.py"))]

View file

@ -21,6 +21,7 @@ pub(crate) use missing_fstring_syntax::*;
pub(crate) use mutable_class_default::*; pub(crate) use mutable_class_default::*;
pub(crate) use mutable_dataclass_default::*; pub(crate) use mutable_dataclass_default::*;
pub(crate) use mutable_fromkeys_value::*; pub(crate) use mutable_fromkeys_value::*;
pub(crate) use needless_else::*;
pub(crate) use never_union::*; pub(crate) use never_union::*;
pub(crate) use none_not_at_end_of_union::*; pub(crate) use none_not_at_end_of_union::*;
pub(crate) use parenthesize_chained_operators::*; pub(crate) use parenthesize_chained_operators::*;
@ -75,6 +76,7 @@ mod missing_fstring_syntax;
mod mutable_class_default; mod mutable_class_default;
mod mutable_dataclass_default; mod mutable_dataclass_default;
mod mutable_fromkeys_value; mod mutable_fromkeys_value;
mod needless_else;
mod never_union; mod never_union;
mod none_not_at_end_of_union; mod none_not_at_end_of_union;
mod parenthesize_chained_operators; mod parenthesize_chained_operators;

View file

@ -0,0 +1,304 @@
use std::cmp::Ordering;
use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, ViolationMetadata};
use ruff_python_ast::helpers::comment_indentation_after;
use ruff_python_ast::whitespace::indentation;
use ruff_python_ast::{Stmt, StmtExpr, StmtFor, StmtIf, StmtTry, StmtWhile};
use ruff_python_parser::{TokenKind, Tokens};
use ruff_source_file::LineRanges;
use ruff_text_size::{Ranged, TextLen, TextRange, TextSize};
use crate::checkers::ast::Checker;
/// ## What it does
/// Checks for `else` clauses that only contains `pass` and `...` statements.
///
/// ## Why is this bad?
/// Such an else clause does nothing and can be removed.
///
/// ## Example
/// ```python
/// if foo:
/// bar()
/// else:
/// pass
/// ```
///
/// Use instead:
/// ```python
/// if foo:
/// bar()
/// ```
#[derive(ViolationMetadata)]
pub(crate) struct NeedlessElse;
impl AlwaysFixableViolation for NeedlessElse {
#[derive_message_formats]
fn message(&self) -> String {
"Empty `else` clause".to_string()
}
fn fix_title(&self) -> String {
"Remove the `else` clause".to_string()
}
}
/// RUF047
pub(crate) fn needless_else(checker: &mut Checker, stmt: AnyNodeWithOrElse) {
let source = checker.source();
let tokens = checker.tokens();
let else_body = stmt.else_body();
if !body_is_no_op(else_body) {
return;
}
let Some(else_range) = stmt.else_range(tokens) else {
return;
};
if else_contains_comments(stmt, else_range, checker) {
return;
}
let else_line_start = source.line_start(else_range.start());
let else_full_end = source.full_line_end(else_range.end());
let remove_range = TextRange::new(else_line_start, else_full_end);
let edit = Edit::range_deletion(remove_range);
let fix = Fix::safe_edit(edit);
let diagnostic = Diagnostic::new(NeedlessElse, else_range);
checker.diagnostics.push(diagnostic.with_fix(fix));
}
/// Whether `body` contains only one `pass` or `...` statement.
fn body_is_no_op(body: &[Stmt]) -> bool {
match body {
[Stmt::Pass(_)] => true,
[Stmt::Expr(StmtExpr { value, .. })] => value.is_ellipsis_literal_expr(),
_ => false,
}
}
fn else_contains_comments(
stmt: AnyNodeWithOrElse,
else_range: TextRange,
checker: &Checker,
) -> bool {
let else_full_end = checker.source().full_line_end(else_range.end());
let commentable_range = TextRange::new(else_range.start(), else_full_end);
// A comment after the `else` keyword or after the dummy statement.
//
// ```python
// if ...:
// ...
// else: # comment
// pass # comment
// ```
if checker.comment_ranges().intersects(commentable_range) {
return true;
}
let Some(preceding_stmt) = stmt.body_before_else().last() else {
return false;
};
let Some(else_last_stmt) = stmt.else_body().last() else {
return false;
};
else_branch_has_preceding_comment(preceding_stmt, else_range, checker)
|| else_branch_has_trailing_comment(else_last_stmt, else_full_end, checker)
}
/// Returns `true` if the `else` clause header has a leading own-line comment.
///
/// ```python
/// if ...:
/// ...
/// # some comment
/// else:
/// pass
/// ```
fn else_branch_has_preceding_comment(
preceding_stmt: &Stmt,
else_range: TextRange,
checker: &Checker,
) -> bool {
let (tokens, source) = (checker.tokens(), checker.source());
let before_else_full_end = source.full_line_end(preceding_stmt.end());
let preceding_indentation = indentation(source, &preceding_stmt)
.unwrap_or_default()
.text_len();
for token in tokens.in_range(TextRange::new(before_else_full_end, else_range.start())) {
if token.kind() != TokenKind::Comment {
continue;
}
let comment_indentation =
comment_indentation_after(preceding_stmt.into(), token.range(), source);
match comment_indentation.cmp(&preceding_indentation) {
// Comment belongs to preceding statement.
Ordering::Greater | Ordering::Equal => continue,
Ordering::Less => return true,
}
}
false
}
/// Returns `true` if the `else` branch has a trailing own line comment:
///
/// ```python
/// if ...:
/// ...
/// else:
/// pass
/// # some comment
/// ```
fn else_branch_has_trailing_comment(
last_else_stmt: &Stmt,
else_full_end: TextSize,
checker: &Checker,
) -> bool {
let (tokens, source) = (checker.tokens(), checker.source());
let preceding_indentation = indentation(source, &last_else_stmt)
.unwrap_or_default()
.text_len();
for token in tokens.after(else_full_end) {
match token.kind() {
TokenKind::Comment => {
let comment_indentation =
comment_indentation_after(last_else_stmt.into(), token.range(), source);
match comment_indentation.cmp(&preceding_indentation) {
Ordering::Greater | Ordering::Equal => return true,
Ordering::Less => break,
}
}
TokenKind::NonLogicalNewline
| TokenKind::Newline
| TokenKind::Indent
| TokenKind::Dedent => {}
_ => break,
}
}
false
}
#[derive(Copy, Clone, Debug)]
pub(crate) enum AnyNodeWithOrElse<'a> {
While(&'a StmtWhile),
For(&'a StmtFor),
Try(&'a StmtTry),
If(&'a StmtIf),
}
impl<'a> AnyNodeWithOrElse<'a> {
/// Returns the range from the `else` keyword to the last statement in its block.
fn else_range(self, tokens: &Tokens) -> Option<TextRange> {
match self {
Self::For(_) | Self::While(_) | Self::Try(_) => {
let before_else = self.body_before_else();
let else_body = self.else_body();
let end = else_body.last()?.end();
let start = tokens
.in_range(TextRange::new(before_else.last()?.end(), end))
.iter()
.find(|token| token.kind() == TokenKind::Else)?
.start();
Some(TextRange::new(start, end))
}
Self::If(StmtIf {
elif_else_clauses, ..
}) => elif_else_clauses
.last()
.filter(|clause| clause.test.is_none())
.map(Ranged::range),
}
}
/// Returns the suite before the else block.
fn body_before_else(self) -> &'a [Stmt] {
match self {
Self::Try(StmtTry { body, handlers, .. }) => handlers
.last()
.and_then(|handler| handler.as_except_handler())
.map(|handler| &handler.body)
.unwrap_or(body),
Self::While(StmtWhile { body, .. }) | Self::For(StmtFor { body, .. }) => body,
Self::If(StmtIf {
body,
elif_else_clauses,
..
}) => elif_else_clauses
.iter()
.rev()
.find(|clause| clause.test.is_some())
.map(|clause| &*clause.body)
.unwrap_or(body),
}
}
/// Returns the `else` suite.
/// Defaults to an empty suite if the statement has no `else` block.
fn else_body(self) -> &'a [Stmt] {
match self {
Self::While(StmtWhile { orelse, .. })
| Self::For(StmtFor { orelse, .. })
| Self::Try(StmtTry { orelse, .. }) => orelse,
Self::If(StmtIf {
elif_else_clauses, ..
}) => elif_else_clauses
.last()
.filter(|clause| clause.test.is_none())
.map(|clause| &*clause.body)
.unwrap_or_default(),
}
}
}
impl<'a> From<&'a StmtFor> for AnyNodeWithOrElse<'a> {
fn from(value: &'a StmtFor) -> Self {
Self::For(value)
}
}
impl<'a> From<&'a StmtWhile> for AnyNodeWithOrElse<'a> {
fn from(value: &'a StmtWhile) -> Self {
Self::While(value)
}
}
impl<'a> From<&'a StmtIf> for AnyNodeWithOrElse<'a> {
fn from(value: &'a StmtIf) -> Self {
Self::If(value)
}
}
impl<'a> From<&'a StmtTry> for AnyNodeWithOrElse<'a> {
fn from(value: &'a StmtTry) -> Self {
Self::Try(value)
}
}

View file

@ -0,0 +1,42 @@
---
source: crates/ruff_linter/src/rules/ruff/mod.rs
---
RUF047_for.py:6:1: RUF047 [*] Empty `else` clause
|
4 | loop_body_is_not_checked()
5 | break
6 | / else:
7 | | pass
| |________^ RUF047
|
= help: Remove the `else` clause
Safe fix
3 3 | for _ in range(0):
4 4 | loop_body_is_not_checked()
5 5 | break
6 |-else:
7 |- pass
8 6 |
9 7 |
10 8 | for this in comment:
RUF047_for.py:12:1: RUF047 [*] Empty `else` clause
|
10 | for this in comment:
11 | belongs_to() # `for`
12 | / else:
13 | | ...
| |_______^ RUF047
|
= help: Remove the `else` clause
Safe fix
9 9 |
10 10 | for this in comment:
11 11 | belongs_to() # `for`
12 |-else:
13 |- ...
14 12 |
15 13 |
16 14 | for of in course():

View file

@ -0,0 +1,159 @@
---
source: crates/ruff_linter/src/rules/ruff/mod.rs
---
RUF047_if.py:5:1: RUF047 [*] Empty `else` clause
|
3 | if False:
4 | condition_is_not_evaluated()
5 | / else:
6 | | pass
| |________^ RUF047
|
= help: Remove the `else` clause
Safe fix
2 2 |
3 3 | if False:
4 4 | condition_is_not_evaluated()
5 |-else:
6 |- pass
7 5 |
8 6 |
9 7 | if this_comment():
RUF047_if.py:11:1: RUF047 [*] Empty `else` clause
|
9 | if this_comment():
10 | belongs_to() # `if`
11 | / else:
12 | | ...
| |_______^ RUF047
|
= help: Remove the `else` clause
Safe fix
8 8 |
9 9 | if this_comment():
10 10 | belongs_to() # `if`
11 |-else:
12 |- ...
13 11 |
14 12 |
15 13 | if elif_is():
RUF047_if.py:19:1: RUF047 [*] Empty `else` clause
|
17 | elif the_same():
18 | as_if()
19 | / else:
20 | | pass
| |________^ RUF047
|
= help: Remove the `else` clause
Safe fix
16 16 | treated()
17 17 | elif the_same():
18 18 | as_if()
19 |-else:
20 |- pass
21 19 |
22 20 |
23 21 | if this_second_comment():
RUF047_if.py:26:1: RUF047 [*] Empty `else` clause
|
24 | belongs() # to
25 | # `if`
26 | / else:
27 | | pass
| |________^ RUF047
28 |
29 | if this_second_comment():
|
= help: Remove the `else` clause
Safe fix
23 23 | if this_second_comment():
24 24 | belongs() # to
25 25 | # `if`
26 |-else:
27 |- pass
28 26 |
29 27 | if this_second_comment():
30 28 | belongs() # to
RUF047_if.py:32:1: RUF047 [*] Empty `else` clause
|
30 | belongs() # to
31 | # `if`
32 | / else:
33 | | pass
| |________^ RUF047
|
= help: Remove the `else` clause
Safe fix
29 29 | if this_second_comment():
30 30 | belongs() # to
31 31 | # `if`
32 |-else:
33 |- pass
34 32 |
35 33 |
36 34 | if of_course():
RUF047_if.py:44:1: RUF047 [*] Empty `else` clause
|
43 | if of_course: this()
44 | else: ...
| ^^^^^^^^^ RUF047
|
= help: Remove the `else` clause
Safe fix
41 41 |
42 42 |
43 43 | if of_course: this()
44 |-else: ...
45 44 |
46 45 |
47 46 | if of_course:
RUF047_if.py:49:1: RUF047 [*] Empty `else` clause
|
47 | if of_course:
48 | this() # comment
49 | else: ...
| ^^^^^^^^^ RUF047
|
= help: Remove the `else` clause
Safe fix
46 46 |
47 47 | if of_course:
48 48 | this() # comment
49 |-else: ...
50 49 |
51 50 |
52 51 | def nested():
RUF047_if.py:55:5: RUF047 [*] Empty `else` clause
|
53 | if a:
54 | b()
55 | / else:
56 | | ...
| |___________^ RUF047
|
= help: Remove the `else` clause
Safe fix
52 52 | def nested():
53 53 | if a:
54 54 | b()
55 |- else:
56 |- ...
57 55 |
58 56 |
59 57 | ### No errors

View file

@ -0,0 +1,42 @@
---
source: crates/ruff_linter/src/rules/ruff/mod.rs
---
RUF047_try.py:7:1: RUF047 [*] Empty `else` clause
|
5 | except:
6 | pass
7 | / else:
8 | | pass
| |________^ RUF047
|
= help: Remove the `else` clause
Safe fix
4 4 | raise try_body_is_not_checked()
5 5 | except:
6 6 | pass
7 |-else:
8 |- pass
9 7 |
10 8 |
11 9 | try:
RUF047_try.py:17:1: RUF047 [*] Empty `else` clause
|
15 | except:
16 | to() # `except`
17 | / else:
18 | | ...
| |_______^ RUF047
|
= help: Remove the `else` clause
Safe fix
14 14 | belongs()
15 15 | except:
16 16 | to() # `except`
17 |-else:
18 |- ...
19 17 |
20 18 |
21 19 | try:

View file

@ -0,0 +1,42 @@
---
source: crates/ruff_linter/src/rules/ruff/mod.rs
---
RUF047_while.py:6:1: RUF047 [*] Empty `else` clause
|
4 | loop_body_is_not_checked()
5 | break
6 | / else:
7 | | pass
| |________^ RUF047
|
= help: Remove the `else` clause
Safe fix
3 3 | while True:
4 4 | loop_body_is_not_checked()
5 5 | break
6 |-else:
7 |- pass
8 6 |
9 7 |
10 8 | while this_comment:
RUF047_while.py:12:1: RUF047 [*] Empty `else` clause
|
10 | while this_comment:
11 | belongs_to() # `for`
12 | / else:
13 | | ...
| |_______^ RUF047
|
= help: Remove the `else` clause
Safe fix
9 9 |
10 10 | while this_comment:
11 11 | belongs_to() # `for`
12 |-else:
13 |- ...
14 12 |
15 13 |
16 14 | while of_course():

1
ruff.schema.json generated
View file

@ -3921,6 +3921,7 @@
"RUF041", "RUF041",
"RUF043", "RUF043",
"RUF046", "RUF046",
"RUF047",
"RUF048", "RUF048",
"RUF049", "RUF049",
"RUF05", "RUF05",