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] implement useless if-else (RUF034) #13218

Merged
merged 5 commits into from
Sep 4, 2024
Merged
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
11 changes: 11 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/ruff/RUF034.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# Valid
x = 1 if True else 2

# Invalid
x = 1 if True else 1

# Invalid
x = "a" if True else "a"

# Invalid
x = 0.1 if False else 0.1
3 changes: 3 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1404,6 +1404,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
if checker.enabled(Rule::IfExpInsteadOfOrOperator) {
refurb::rules::if_exp_instead_of_or_operator(checker, if_exp);
}
if checker.enabled(Rule::UselessIfElse) {
ruff::rules::useless_if_else(checker, if_exp);
}
}
Expr::ListComp(
comp @ ast::ExprListComp {
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 @@ -961,6 +961,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Ruff, "031") => (RuleGroup::Preview, rules::ruff::rules::IncorrectlyParenthesizedTupleInSubscript),
(Ruff, "032") => (RuleGroup::Preview, rules::ruff::rules::DecimalFromFloatLiteral),
(Ruff, "033") => (RuleGroup::Preview, rules::ruff::rules::PostInitDefault),
(Ruff, "034") => (RuleGroup::Preview, rules::ruff::rules::UselessIfElse),
(Ruff, "100") => (RuleGroup::Stable, rules::ruff::rules::UnusedNOQA),
(Ruff, "101") => (RuleGroup::Stable, rules::ruff::rules::RedirectedNOQA),

Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/ruff/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ mod tests {
#[test_case(Rule::AssertWithPrintMessage, Path::new("RUF030.py"))]
#[test_case(Rule::IncorrectlyParenthesizedTupleInSubscript, Path::new("RUF031.py"))]
#[test_case(Rule::DecimalFromFloatLiteral, Path::new("RUF032.py"))]
#[test_case(Rule::UselessIfElse, Path::new("RUF034.py"))]
#[test_case(Rule::RedirectedNOQA, Path::new("RUF101.py"))]
#[test_case(Rule::PostInitDefault, Path::new("RUF033.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
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 @@ -29,6 +29,7 @@ pub(crate) use unnecessary_iterable_allocation_for_first_element::*;
pub(crate) use unnecessary_key_check::*;
pub(crate) use unused_async::*;
pub(crate) use unused_noqa::*;
pub(crate) use useless_if_else::*;
pub(crate) use zip_instead_of_pairwise::*;

mod ambiguous_unicode_character;
Expand Down Expand Up @@ -66,6 +67,7 @@ mod unnecessary_iterable_allocation_for_first_element;
mod unnecessary_key_check;
mod unused_async;
mod unused_noqa;
mod useless_if_else;
mod zip_instead_of_pairwise;

#[derive(Clone, Copy)]
Expand Down
55 changes: 55 additions & 0 deletions crates/ruff_linter/src/rules/ruff/rules/useless_if_else.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
use crate::checkers::ast::Checker;
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast as ast;
use ruff_python_ast::comparable::ComparableExpr;

/// ## What it does
/// Checks for useless if-else conditions with identical arms.
///
/// ## Why is this bad?
/// Useless if-else conditions add unnecessary complexity to the code without
/// providing any logical benefit.
///
/// Assigning the value directly is clearer and more explicit, and
/// should be preferred.
///
/// ## Example
/// ```python
/// # Bad
/// foo = x if y else x
/// ```
///
/// Use instead:
/// ```python
/// # Good
/// foo = x
/// ```
#[violation]
pub struct UselessIfElse;

impl Violation for UselessIfElse {
#[derive_message_formats]
fn message(&self) -> String {
format!("Useless if-else condition")
}
}

/// RUF031
pub(crate) fn useless_if_else(checker: &mut Checker, if_expr: &ast::ExprIf) {
let ast::ExprIf {
body,
orelse,
range,
..
} = if_expr;

// Skip if the body and orelse are not the same
if ComparableExpr::from(body) != ComparableExpr::from(orelse) {
return;
}

checker
.diagnostics
.push(Diagnostic::new(UselessIfElse, *range));
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
---
source: crates/ruff_linter/src/rules/ruff/mod.rs
---
RUF034.py:5:5: RUF034 Useless if-else condition
|
4 | # Invalid
5 | x = 1 if True else 1
| ^^^^^^^^^^^^^^^^ RUF034
6 |
7 | # Invalid
|

RUF034.py:8:5: RUF034 Useless if-else condition
|
7 | # Invalid
8 | x = "a" if True else "a"
| ^^^^^^^^^^^^^^^^^^^^ RUF034
9 |
10 | # Invalid
|

RUF034.py:11:5: RUF034 Useless if-else condition
|
10 | # Invalid
11 | x = 0.1 if False else 0.1
| ^^^^^^^^^^^^^^^^^^^^^ RUF034
|
1 change: 1 addition & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading