From c67ba2a8ffbe0e8fb7c3ed913d7ce249f7fd5fb2 Mon Sep 17 00:00:00 2001 From: Steve C Date: Sun, 31 Mar 2024 23:01:14 -0400 Subject: [PATCH 1/3] [`refurb`] - add `or-oper` (`FURB110`) --- .../resources/test/fixtures/refurb/FURB110.py | 8 ++ .../src/checkers/ast/analyze/expression.rs | 3 + crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/refurb/mod.rs | 1 + .../ruff_linter/src/rules/refurb/rules/mod.rs | 2 + .../src/rules/refurb/rules/or_oper.rs | 101 ++++++++++++++++++ ...es__refurb__tests__FURB110_FURB110.py.snap | 63 +++++++++++ ruff.schema.json | 1 + 8 files changed, 180 insertions(+) create mode 100644 crates/ruff_linter/resources/test/fixtures/refurb/FURB110.py create mode 100644 crates/ruff_linter/src/rules/refurb/rules/or_oper.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..57da77344763e --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB110.py @@ -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 diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 9e85625e87eba..350bb7fcfcfa9 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -1364,6 +1364,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 { diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index a6db70d943302..446829fd122b6 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -1035,6 +1035,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), diff --git a/crates/ruff_linter/src/rules/refurb/mod.rs b/crates/ruff_linter/src/rules/refurb/mod.rs index 67e1022600426..992c89e948211 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::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"))] diff --git a/crates/ruff_linter/src/rules/refurb/rules/mod.rs b/crates/ruff_linter/src/rules/refurb/rules/mod.rs index cbb8d1fc7f0bb..9211e7083334c 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/mod.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/mod.rs @@ -9,6 +9,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::*; @@ -35,6 +36,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; diff --git a/crates/ruff_linter/src/rules/refurb/rules/or_oper.rs b/crates/ruff_linter/src/rules/refurb/rules/or_oper.rs new file mode 100644 index 0000000000000..5d7c9bf359d49 --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/rules/or_oper.rs @@ -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 { + 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 new file mode 100644 index 0000000000000..76cf26f28aa61 --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB110_FURB110.py.snap @@ -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 diff --git a/ruff.schema.json b/ruff.schema.json index 1c26154c2d19f..a821bcea599c6 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3043,6 +3043,7 @@ "FURB101", "FURB105", "FURB11", + "FURB110", "FURB113", "FURB118", "FURB12", From a52c531e14f410cccb15cca2c23b45f8cbb237a8 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 6 Apr 2024 12:30:14 -0400 Subject: [PATCH 2/3] Tweak message, add more tests --- .../resources/test/fixtures/refurb/FURB110.py | 22 +++ .../src/checkers/ast/analyze/expression.rs | 4 +- crates/ruff_linter/src/codes.rs | 2 +- crates/ruff_linter/src/rules/refurb/mod.rs | 2 +- .../rules/if_exp_instead_of_or_operator.rs | 100 ++++++++++++++ .../ruff_linter/src/rules/refurb/rules/mod.rs | 4 +- .../src/rules/refurb/rules/or_oper.rs | 101 -------------- ...es__refurb__tests__FURB110_FURB110.py.snap | 129 +++++++++++++++--- 8 files changed, 236 insertions(+), 128 deletions(-) create mode 100644 crates/ruff_linter/src/rules/refurb/rules/if_exp_instead_of_or_operator.rs delete mode 100644 crates/ruff_linter/src/rules/refurb/rules/or_oper.rs diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB110.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB110.py index 57da77344763e..44960b6b74ef1 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 97624a9aaff52..f55c7195d083a 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::IfExpInsteadOfOrOperator) { + refurb::rules::if_exp_instead_of_or_operator(checker, if_exp); } } Expr::ListComp( diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index a8a828a2a4fb5..5e6a0ead3d765 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::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 16415cc9d4bde..dbf556c629b5d 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::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..4a2c2f2307e95 --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/rules/if_exp_instead_of_or_operator.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 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 fe1161a8b04b9..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::*; @@ -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_operator; 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 5d7c9bf359d49..0000000000000 --- 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 76cf26f28aa61..c49c21b90f467 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 | ) From 06db6cf2715e8cafdd41af813102a08ce98a7193 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 6 Apr 2024 12:32:22 -0400 Subject: [PATCH 3/3] Tweak docs --- .../src/rules/refurb/rules/if_exp_instead_of_or_operator.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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 index 4a2c2f2307e95..3096f9a62b0b7 100644 --- 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 @@ -31,7 +31,8 @@ use crate::checkers::ast::Checker; /// `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()`. +/// (assuming `foo()` returns a truthy value), but only once in +/// `foo() or bar()`. #[violation] pub struct IfExpInsteadOfOrOperator; @@ -89,7 +90,7 @@ pub(crate) fn if_exp_instead_of_or_operator(checker: &mut Checker, if_expr: &ast ), if_expr.range(), ), - if contains_effect(&body, |id| checker.semantic().is_builtin(id)) { + if contains_effect(body, |id| checker.semantic().is_builtin(id)) { Applicability::Unsafe } else { Applicability::Safe