Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ruff] Needless else clause (RUF047) #15051

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/ruff/RUF047_for.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
for _ in range(0):
loop_body_is_not_checked()
break
else:
pass


for this in comment:
belongs_to() # `for`
else:
...


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

for of in course():
this()
else:
...
# this comment does not belong to the else
60 changes: 60 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/ruff/RUF047_if.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
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
# `else`
else:
pass

Comment on lines +21 to +26
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should detect that this else branch is useless because the comment belongs to the if block


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()
else:
...
# this comment doesn't belong to the if

if of_course: this()
else: ...

if of_course:
this() # comment
else: ...

if of_course:
this() # comment
else: ... # trailing
71 changes: 71 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/ruff/RUF047_try.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
try:
raise try_body_is_not_checked()
except:
pass
else:
pass


try:
this()
except comment:
belongs()
except:
to() # `except`
else:
...


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

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
44 changes: 44 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/ruff/RUF047_while.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
while True:
loop_body_is_not_checked()
break
else:
pass


while this_comment:
belongs_to() # `for`
else:
...


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

while of_course():
this()
else:
...
# this comment belongs to the statement coming after the else

28 changes: 21 additions & 7 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1238,6 +1238,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::IfKeyInDictDel) {
ruff::rules::if_key_in_dict_del(checker, if_);
}
if checker.enabled(Rule::NeedlessElse) {
ruff::rules::needless_else(checker, if_.into());
}
}
Stmt::Assert(
assert_stmt @ ast::StmtAssert {
Expand Down Expand Up @@ -1344,6 +1347,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::AsyncBusyWait) {
flake8_async::rules::async_busy_wait(checker, while_stmt);
}
if checker.enabled(Rule::NeedlessElse) {
ruff::rules::needless_else(checker, while_stmt.into());
}
}
Stmt::For(
for_stmt @ ast::StmtFor {
Expand Down Expand Up @@ -1430,14 +1436,19 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
refurb::rules::for_loop_set_mutations(checker, for_stmt);
}
}
if checker.enabled(Rule::NeedlessElse) {
ruff::rules::needless_else(checker, for_stmt.into());
}
}
Stmt::Try(ast::StmtTry {
body,
handlers,
orelse,
finalbody,
..
}) => {
Stmt::Try(
try_stmt @ ast::StmtTry {
body,
handlers,
orelse,
finalbody,
..
},
) => {
if checker.enabled(Rule::TooManyNestedBlocks) {
pylint::rules::too_many_nested_blocks(checker, stmt);
}
Expand Down Expand Up @@ -1504,6 +1515,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::ErrorInsteadOfException) {
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, .. }) => {
if checker.enabled(Rule::SelfOrClsAssignment) {
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -987,6 +987,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Ruff, "041") => (RuleGroup::Preview, rules::ruff::rules::UnnecessaryNestedLiteral),
(Ruff, "043") => (RuleGroup::Preview, rules::ruff::rules::PytestRaisesAmbiguousPattern),
(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, "051") => (RuleGroup::Preview, rules::ruff::rules::IfKeyInDictDel),
(Ruff, "052") => (RuleGroup::Preview, rules::ruff::rules::UsedDummyVariable),
Expand Down
4 changes: 4 additions & 0 deletions crates/ruff_linter/src/rules/ruff/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,10 @@ mod tests {
#[test_case(Rule::UnnecessaryRegularExpression, Path::new("RUF055_1.py"))]
#[test_case(Rule::UnnecessaryCastToInt, Path::new("RUF046.py"))]
#[test_case(Rule::PytestRaisesAmbiguousPattern, Path::new("RUF043.py"))]
#[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"))]
Comment on lines +419 to +422
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The needless-else rule has no mode.is_preview checks. We can add it to the normal rules test, which simplifies promoting them to stable.

fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!(
"preview__{}_{}",
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/ruff/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ pub(crate) use missing_fstring_syntax::*;
pub(crate) use mutable_class_default::*;
pub(crate) use mutable_dataclass_default::*;
pub(crate) use mutable_fromkeys_value::*;
pub(crate) use needless_else::*;
pub(crate) use never_union::*;
pub(crate) use none_not_at_end_of_union::*;
pub(crate) use parenthesize_chained_operators::*;
Expand Down Expand Up @@ -68,6 +69,7 @@ mod missing_fstring_syntax;
mod mutable_class_default;
mod mutable_dataclass_default;
mod mutable_fromkeys_value;
mod needless_else;
mod never_union;
mod none_not_at_end_of_union;
mod parenthesize_chained_operators;
Expand Down
Loading
Loading