From 44459f92efdcdaf2287d773dfb64f31590fb4078 Mon Sep 17 00:00:00 2001 From: Steve C Date: Sat, 6 Apr 2024 12:39:40 -0400 Subject: [PATCH] [`refurb`] Implement `if-expr-instead-of-or-operator` (`FURB110`) (#10687) ## Summary Add [`FURB110`](https://github.com/dosisod/refurb/blob/master/refurb/checks/logical/use_or.py) See: #1348 ## Test Plan `cargo test` --- .../resources/test/fixtures/refurb/FURB110.py | 30 ++++ .../src/checkers/ast/analyze/expression.rs | 3 + crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/refurb/mod.rs | 1 + .../rules/if_exp_instead_of_or_operator.rs | 101 ++++++++++++ .../ruff_linter/src/rules/refurb/rules/mod.rs | 2 + ...es__refurb__tests__FURB110_FURB110.py.snap | 150 ++++++++++++++++++ ruff.schema.json | 1 + 8 files changed, 289 insertions(+) create mode 100644 crates/ruff_linter/resources/test/fixtures/refurb/FURB110.py create mode 100644 crates/ruff_linter/src/rules/refurb/rules/if_exp_instead_of_or_operator.rs create mode 100644 crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB110_FURB110.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB110.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB110.py new file mode 100644 index 0000000000000..44960b6b74ef1 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB110.py @@ -0,0 +1,30 @@ +z = x if x else y # FURB110 + +z = x \ + if x else y # FURB110 + +z = x if x \ + else \ + y # FURB110 + +z = x() if x() else y() # FURB110 + +# FURB110 +z = x if ( + # Test for x. + x +) else ( + # Test for y. + y +) + +# FURB110 +z = ( + x if ( + # Test for x. + x + ) else ( + # Test for y. + y + ) +) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index e46dad03b722e..72a293d3acc52 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -1358,6 +1358,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::IfExpInsteadOfOrOperator) { + refurb::rules::if_exp_instead_of_or_operator(checker, if_exp); + } } Expr::ListComp( comp @ ast::ExprListComp { diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index eb8553d155817..5e6a0ead3d765 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -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::IfExpInsteadOfOrOperator), #[allow(deprecated)] (Refurb, "113") => (RuleGroup::Nursery, rules::refurb::rules::RepeatedAppend), (Refurb, "118") => (RuleGroup::Preview, rules::refurb::rules::ReimplementedOperator), diff --git a/crates/ruff_linter/src/rules/refurb/mod.rs b/crates/ruff_linter/src/rules/refurb/mod.rs index f27f2291d1135..dbf556c629b5d 100644 --- a/crates/ruff_linter/src/rules/refurb/mod.rs +++ b/crates/ruff_linter/src/rules/refurb/mod.rs @@ -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::IfExpInsteadOfOrOperator, 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"))] diff --git a/crates/ruff_linter/src/rules/refurb/rules/if_exp_instead_of_or_operator.rs b/crates/ruff_linter/src/rules/refurb/rules/if_exp_instead_of_or_operator.rs new file mode 100644 index 0000000000000..3096f9a62b0b7 --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/rules/if_exp_instead_of_or_operator.rs @@ -0,0 +1,101 @@ +use ruff_diagnostics::{Applicability, Diagnostic, Edit, Fix, FixAvailability, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast as ast; +use ruff_python_ast::comparable::ComparableExpr; +use ruff_python_ast::helpers::contains_effect; +use ruff_python_ast::parenthesize::parenthesized_range; +use ruff_text_size::Ranged; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for ternary `if` expressions that can be replaced with the `or` +/// operator. +/// +/// ## Why is this bad? +/// Ternary `if` expressions are more verbose than `or` expressions while +/// providing the same functionality. +/// +/// ## Example +/// ```python +/// z = x if x else y +/// ``` +/// +/// Use instead: +/// ```python +/// z = x or y +/// ``` +/// +/// ## Fix safety +/// This rule's fix is marked as unsafe in the event that the body of the +/// `if` expression contains side effects. +/// +/// For example, `foo` will be called twice in `foo() if foo() else bar()` +/// (assuming `foo()` returns a truthy value), but only once in +/// `foo() or bar()`. +#[violation] +pub struct IfExpInsteadOfOrOperator; + +impl Violation for IfExpInsteadOfOrOperator { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; + + #[derive_message_formats] + fn message(&self) -> String { + format!("Replace ternary `if` expression with `or` operator") + } + + fn fix_title(&self) -> Option { + Some(format!("Replace with `or` operator")) + } +} + +/// FURB110 +pub(crate) fn if_exp_instead_of_or_operator(checker: &mut Checker, if_expr: &ast::ExprIf) { + let ast::ExprIf { + test, + body, + orelse, + range, + } = if_expr; + + if ComparableExpr::from(test) != ComparableExpr::from(body) { + return; + } + + let mut diagnostic = Diagnostic::new(IfExpInsteadOfOrOperator, *range); + + // Grab the range of the `test` and `orelse` expressions. + let left = parenthesized_range( + test.into(), + if_expr.into(), + checker.indexer().comment_ranges(), + checker.locator().contents(), + ) + .unwrap_or(test.range()); + let right = parenthesized_range( + orelse.into(), + if_expr.into(), + checker.indexer().comment_ranges(), + checker.locator().contents(), + ) + .unwrap_or(orelse.range()); + + // Replace with `{test} or {orelse}`. + diagnostic.set_fix(Fix::applicable_edit( + Edit::range_replacement( + format!( + "{} or {}", + checker.locator().slice(left), + checker.locator().slice(right), + ), + if_expr.range(), + ), + if contains_effect(body, |id| checker.semantic().is_builtin(id)) { + Applicability::Unsafe + } else { + Applicability::Safe + }, + )); + + checker.diagnostics.push(diagnostic); +} diff --git a/crates/ruff_linter/src/rules/refurb/rules/mod.rs b/crates/ruff_linter/src/rules/refurb/rules/mod.rs index c18f2350ecd4d..bff4b082c2d88 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/mod.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/mod.rs @@ -3,6 +3,7 @@ pub(crate) use check_and_remove_from_set::*; pub(crate) use delete_full_slice::*; pub(crate) use for_loop_set_mutations::*; pub(crate) use hashlib_digest_hex::*; +pub(crate) use if_exp_instead_of_or_operator::*; pub(crate) use if_expr_min_max::*; pub(crate) use implicit_cwd::*; pub(crate) use int_on_sliced_str::*; @@ -30,6 +31,7 @@ mod check_and_remove_from_set; mod delete_full_slice; mod for_loop_set_mutations; mod hashlib_digest_hex; +mod if_exp_instead_of_or_operator; mod if_expr_min_max; mod implicit_cwd; mod int_on_sliced_str; diff --git a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB110_FURB110.py.snap b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB110_FURB110.py.snap new file mode 100644 index 0000000000000..c49c21b90f467 --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB110_FURB110.py.snap @@ -0,0 +1,150 @@ +--- +source: crates/ruff_linter/src/rules/refurb/mod.rs +--- +FURB110.py:1:5: FURB110 [*] Replace ternary `if` expression with `or` operator + | +1 | z = x if x else y # FURB110 + | ^^^^^^^^^^^^^ FURB110 +2 | +3 | z = x \ + | + = help: Replace with `or` operator + +ℹ Safe 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 ternary `if` expression with `or` operator + | +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: Replace with `or` operator + +ℹ Safe 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 ternary `if` expression with `or` operator + | + 4 | if x else y # FURB110 + 5 | + 6 | z = x if x \ + | _____^ + 7 | | else \ + 8 | | y # FURB110 + | |_________^ FURB110 + 9 | +10 | z = x() if x() else y() # FURB110 + | + = help: Replace with `or` operator + +ℹ Safe fix +3 3 | z = x \ +4 4 | if x else y # FURB110 +5 5 | +6 |-z = x if x \ +7 |- else \ +8 |- y # FURB110 + 6 |+z = x or y # FURB110 +9 7 | +10 8 | z = x() if x() else y() # FURB110 +11 9 | + +FURB110.py:10:5: FURB110 [*] Replace ternary `if` expression with `or` operator + | + 8 | y # FURB110 + 9 | +10 | z = x() if x() else y() # FURB110 + | ^^^^^^^^^^^^^^^^^^^ FURB110 +11 | +12 | # FURB110 + | + = help: Replace with `or` operator + +ℹ Unsafe fix +7 7 | else \ +8 8 | y # FURB110 +9 9 | +10 |-z = x() if x() else y() # FURB110 + 10 |+z = x() or y() # FURB110 +11 11 | +12 12 | # FURB110 +13 13 | z = x if ( + +FURB110.py:13:5: FURB110 [*] Replace ternary `if` expression with `or` operator + | +12 | # FURB110 +13 | z = x if ( + | _____^ +14 | | # Test for x. +15 | | x +16 | | ) else ( +17 | | # Test for y. +18 | | y +19 | | ) + | |_^ FURB110 +20 | +21 | # FURB110 + | + = help: Replace with `or` operator + +ℹ Safe fix +10 10 | z = x() if x() else y() # FURB110 +11 11 | +12 12 | # FURB110 +13 |-z = x if ( + 13 |+z = ( +14 14 | # Test for x. +15 15 | x +16 |-) else ( + 16 |+) or ( +17 17 | # Test for y. +18 18 | y +19 19 | ) + +FURB110.py:23:5: FURB110 [*] Replace ternary `if` expression with `or` operator + | +21 | # FURB110 +22 | z = ( +23 | x if ( + | _____^ +24 | | # Test for x. +25 | | x +26 | | ) else ( +27 | | # Test for y. +28 | | y +29 | | ) + | |_____^ FURB110 +30 | ) + | + = help: Replace with `or` operator + +ℹ Safe fix +20 20 | +21 21 | # FURB110 +22 22 | z = ( +23 |- x if ( + 23 |+ ( +24 24 | # Test for x. +25 25 | x +26 |- ) else ( + 26 |+ ) or ( +27 27 | # Test for y. +28 28 | y +29 29 | ) diff --git a/ruff.schema.json b/ruff.schema.json index b88212860e164..ea85c0ce15c40 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3043,6 +3043,7 @@ "FURB101", "FURB105", "FURB11", + "FURB110", "FURB113", "FURB118", "FURB12",