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

[refurb] Implement if-expr-instead-of-or-operator (FURB110) #10687

Merged
merged 4 commits into from
Apr 6, 2024
Merged
Show file tree
Hide file tree
Changes from 2 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
8 changes: 8 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/refurb/FURB110.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
z = x if x else y # FURB110

z = x \
if x else y # FURB110

z = x if x \
else \
y # FURB110
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 @@ -1363,6 +1363,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
if checker.enabled(Rule::IfExprMinMax) {
refurb::rules::if_expr_min_max(checker, if_exp);
}
if checker.enabled(Rule::OrOper) {
refurb::rules::or_oper(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 @@ -1037,6 +1037,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
// refurb
(Refurb, "101") => (RuleGroup::Preview, rules::refurb::rules::ReadWholeFile),
(Refurb, "105") => (RuleGroup::Preview, rules::refurb::rules::PrintEmptyString),
(Refurb, "110") => (RuleGroup::Preview, rules::refurb::rules::OrOper),
#[allow(deprecated)]
(Refurb, "113") => (RuleGroup::Nursery, rules::refurb::rules::RepeatedAppend),
(Refurb, "118") => (RuleGroup::Preview, rules::refurb::rules::ReimplementedOperator),
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/refurb/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ mod tests {

#[test_case(Rule::ReadWholeFile, Path::new("FURB101.py"))]
#[test_case(Rule::RepeatedAppend, Path::new("FURB113.py"))]
#[test_case(Rule::OrOper, Path::new("FURB110.py"))]
#[test_case(Rule::ReimplementedOperator, Path::new("FURB118.py"))]
#[test_case(Rule::ReadlinesInFor, Path::new("FURB129.py"))]
#[test_case(Rule::DeleteFullSlice, Path::new("FURB131.py"))]
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/refurb/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ pub(crate) use isinstance_type_none::*;
pub(crate) use list_reverse_copy::*;
pub(crate) use math_constant::*;
pub(crate) use metaclass_abcmeta::*;
pub(crate) use or_oper::*;
pub(crate) use print_empty_string::*;
pub(crate) use read_whole_file::*;
pub(crate) use readlines_in_for::*;
Expand Down Expand Up @@ -37,6 +38,7 @@ mod isinstance_type_none;
mod list_reverse_copy;
mod math_constant;
mod metaclass_abcmeta;
mod or_oper;
mod print_empty_string;
mod read_whole_file;
mod readlines_in_for;
Expand Down
101 changes: 101 additions & 0 deletions crates/ruff_linter/src/rules/refurb/rules/or_oper.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast};
use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer};
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;
use crate::fix::snippet::SourceCodeSnippet;

/// ## What it does
/// Checks for ternary `if` expressions that can be replaced with
/// `or` expressions.
///
/// ## Why is this bad?
/// Ternary if statements are more verbose than `or` expressions, and
/// generally have the same functionality.
///
/// ## Example
/// ```python
/// z = x if x else y
/// ```
///
/// Use instead:
/// ```python
/// z = x or y
/// ```
///
/// Note: if `x` depends on side-effects, then this check should be ignored.
#[violation]
pub struct OrOper {
if_true: SourceCodeSnippet,
if_false: SourceCodeSnippet,
}

impl Violation for OrOper {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;

#[derive_message_formats]
fn message(&self) -> String {
let OrOper { if_true, if_false } = self;

match (if_true.full_display(), if_false.full_display()) {
(_, None) | (None, _) => {
format!("Replace ternary `if` expression with `or` expression")
}
(Some(if_true), Some(if_false)) => {
format!(
"Replace `{if_true} if {if_true} or {if_false}` with `{if_true} or {if_false}`"
)
}
}
}

fn fix_title(&self) -> Option<String> {
Some(format!("Use `or` expression"))
}
}

/// FURB110
pub(crate) fn or_oper(checker: &mut Checker, if_expr: &ast::ExprIf) {
let ast::ExprIf {
test,
body,
orelse,
range,
} = if_expr;

let if_true = body.as_ref();
let if_true_code = checker.locator().slice(if_true);
let test_expr_code = checker.locator().slice(test.as_ref());

if test_expr_code != if_true_code {
return;
}

let if_false = orelse.as_ref();

let mut diagnostic = Diagnostic::new(
OrOper {
if_true: SourceCodeSnippet::from_str(if_true_code),
if_false: SourceCodeSnippet::from_str(checker.locator().slice(if_false)),
},
*range,
);

let mut tokenizer = SimpleTokenizer::starts_at(if_true.end(), checker.locator().contents());

// find the `else` token to replace with `or`
let else_token = tokenizer
.find(|tok| tok.kind() == SimpleTokenKind::Else)
.expect("else token to exist");

let fix = Fix::unsafe_edits(
Edit::range_replacement("or".to_string(), else_token.range()),
[Edit::deletion(if_true.start(), test.start())],
);

diagnostic.set_fix(fix);

checker.diagnostics.push(diagnostic);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
---
source: crates/ruff_linter/src/rules/refurb/mod.rs
---
FURB110.py:1:5: FURB110 [*] Replace `x if x or y` with `x or y`
|
1 | z = x if x else y # FURB110
| ^^^^^^^^^^^^^ FURB110
2 |
3 | z = x \
|
= help: Use `or` expression

ℹ Unsafe fix
1 |-z = x if x else y # FURB110
1 |+z = x or y # FURB110
2 2 |
3 3 | z = x \
4 4 | if x else y # FURB110

FURB110.py:3:5: FURB110 [*] Replace `x if x or y` with `x or y`
|
1 | z = x if x else y # FURB110
2 |
3 | z = x \
| _____^
4 | | if x else y # FURB110
| |_______________^ FURB110
5 |
6 | z = x if x \
|
= help: Use `or` expression

ℹ Unsafe fix
1 1 | z = x if x else y # FURB110
2 2 |
3 |-z = x \
4 |- if x else y # FURB110
3 |+z = x or y # FURB110
5 4 |
6 5 | z = x if x \
7 6 | else \

FURB110.py:6:5: FURB110 [*] Replace `x if x or y` with `x or y`
|
4 | if x else y # FURB110
5 |
6 | z = x if x \
| _____^
7 | | else \
8 | | y # FURB110
| |_________^ FURB110
|
= help: Use `or` expression

ℹ Unsafe fix
3 3 | z = x \
4 4 | if x else y # FURB110
5 5 |
6 |-z = x if x \
7 |- else \
6 |+z = x \
7 |+ or \
8 8 | y # FURB110
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