Skip to content

Commit

Permalink
[refurb] Implement if-expr-instead-of-or-operator (FURB110) (#1…
Browse files Browse the repository at this point in the history
  • Loading branch information
diceroll123 authored Apr 6, 2024
1 parent 1dc9310 commit 44459f9
Show file tree
Hide file tree
Showing 8 changed files with 289 additions and 0 deletions.
30 changes: 30 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,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
)
)
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 @@ -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 {
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::IfExpInsteadOfOrOperator),
#[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::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"))]
Expand Down
Original file line number Diff line number Diff line change
@@ -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<String> {
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);
}
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 @@ -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::*;
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
@@ -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 | )
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.

0 comments on commit 44459f9

Please sign in to comment.