diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB110.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB110.py index 57da77344763ee..44960b6b74ef16 100644 --- a/crates/ruff_linter/resources/test/fixtures/refurb/FURB110.py +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB110.py @@ -6,3 +6,25 @@ 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 97624a9aaff526..8809ce9904f4ea 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -1363,8 +1363,8 @@ 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); + if checker.enabled(Rule::IfExpInsteadOfOr) { + refurb::rules::if_exp_instead_of_or(checker, if_exp); } } Expr::ListComp( diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index a8a828a2a4fb5f..95fca377e03b19 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -1037,7 +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), + (Refurb, "110") => (RuleGroup::Preview, rules::refurb::rules::IfExpInsteadOfOr), #[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 16415cc9d4bdec..3befce107075fd 100644 --- a/crates/ruff_linter/src/rules/refurb/mod.rs +++ b/crates/ruff_linter/src/rules/refurb/mod.rs @@ -16,7 +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::IfExpInsteadOfOr, 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.rs b/crates/ruff_linter/src/rules/refurb/rules/if_exp_instead_of_or.rs new file mode 100644 index 00000000000000..a458be4c68f886 --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/rules/if_exp_instead_of_or.rs @@ -0,0 +1,100 @@ +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 it returns a truthy value), but only once in `foo() or bar()`. +#[violation] +pub struct IfExpInsteadOfOr; + +impl Violation for IfExpInsteadOfOr { + 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(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(IfExpInsteadOfOr, *range); + + // Tokenize from the end of the `body`. + 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()); + + 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 fe1161a8b04b9a..b0c47115c02297 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::*; pub(crate) use if_expr_min_max::*; pub(crate) use implicit_cwd::*; pub(crate) use int_on_sliced_str::*; @@ -10,7 +11,6 @@ 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::*; @@ -31,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; mod if_expr_min_max; mod implicit_cwd; mod int_on_sliced_str; @@ -38,7 +39,6 @@ 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; diff --git a/crates/ruff_linter/src/rules/refurb/rules/or_oper.rs b/crates/ruff_linter/src/rules/refurb/rules/or_oper.rs deleted file mode 100644 index 5d7c9bf359d498..00000000000000 --- a/crates/ruff_linter/src/rules/refurb/rules/or_oper.rs +++ /dev/null @@ -1,101 +0,0 @@ -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 { - 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); -} 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 index 76cf26f28aa61f..c49c21b90f4670 100644 --- 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 @@ -1,23 +1,23 @@ --- source: crates/ruff_linter/src/rules/refurb/mod.rs --- -FURB110.py:1:5: FURB110 [*] Replace `x if x or y` with `x or y` +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: Use `or` expression + = help: Replace with `or` operator -ℹ Unsafe fix +ℹ 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 `x if x or y` with `x or y` +FURB110.py:3:5: FURB110 [*] Replace ternary `if` expression with `or` operator | 1 | z = x if x else y # FURB110 2 | @@ -28,9 +28,9 @@ FURB110.py:3:5: FURB110 [*] Replace `x if x or y` with `x or y` 5 | 6 | z = x if x \ | - = help: Use `or` expression + = help: Replace with `or` operator -ℹ Unsafe fix +ℹ Safe fix 1 1 | z = x if x else y # FURB110 2 2 | 3 |-z = x \ @@ -40,24 +40,111 @@ FURB110.py:3:5: FURB110 [*] Replace `x if x or y` with `x or y` 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 +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 -ℹ Unsafe fix +ℹ Safe 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 +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 | )