Skip to content

Commit

Permalink
Respect operator precedence in FURB110 (#11464)
Browse files Browse the repository at this point in the history
## Summary

Ensures that we parenthesize expressions (if necessary) to preserve
operator precedence in `FURB110`.

Closes #11398.
  • Loading branch information
charliermarsh authored May 19, 2024
1 parent 24899ef commit 48b0660
Show file tree
Hide file tree
Showing 3 changed files with 73 additions and 18 deletions.
9 changes: 9 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/refurb/FURB110.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,12 @@
else
y
)

# FURB110
z = (
x
if x
else y
if y > 0
else None
)
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
use std::borrow::Cow;

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_python_ast::Expr;
use ruff_python_index::Indexer;
use ruff_source_file::Locator;
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;
Expand Down Expand Up @@ -64,29 +69,13 @@ pub(crate) fn if_exp_instead_of_or_operator(checker: &mut Checker, if_expr: &ast

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),
parenthesize_test(test, if_expr, checker.indexer(), checker.locator()),
parenthesize_test(orelse, if_expr, checker.indexer(), checker.locator()),
),
if_expr.range(),
),
Expand All @@ -99,3 +88,30 @@ pub(crate) fn if_exp_instead_of_or_operator(checker: &mut Checker, if_expr: &ast

checker.diagnostics.push(diagnostic);
}

/// Parenthesize an expression for use in an `or` operator (e.g., parenthesize `x` in `x or y`),
/// if it's required to maintain the correct order of operations.
///
/// If the expression is already parenthesized, it will be returned as-is regardless of whether
/// the parentheses are required.
///
/// See: <https://docs.python.org/3/reference/expressions.html#operator-precedence>
fn parenthesize_test<'a>(
expr: &Expr,
if_expr: &ast::ExprIf,
indexer: &Indexer,
locator: &Locator<'a>,
) -> Cow<'a, str> {
if let Some(range) = parenthesized_range(
expr.into(),
if_expr.into(),
indexer.comment_ranges(),
locator.contents(),
) {
Cow::Borrowed(locator.slice(range))
} else if matches!(expr, Expr::If(_) | Expr::Lambda(_) | Expr::Named(_)) {
Cow::Owned(format!("({})", locator.slice(expr.range())))
} else {
Cow::Borrowed(locator.slice(expr.range()))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -177,3 +177,33 @@ FURB110.py:34:5: FURB110 [*] Replace ternary `if` expression with `or` operator
39 |- y
34 |+ x or y
40 35 | )
41 36 |
42 37 | # FURB110

FURB110.py:44:5: FURB110 [*] Replace ternary `if` expression with `or` operator
|
42 | # FURB110
43 | z = (
44 | x
| _____^
45 | | if x
46 | | else y
47 | | if y > 0
48 | | else None
| |_____________^ FURB110
49 | )
|
= help: Replace with `or` operator

Safe fix
41 41 |
42 42 | # FURB110
43 43 | z = (
44 |- x
45 |- if x
46 |- else y
44 |+ x or (y
47 45 | if y > 0
48 |- else None
46 |+ else None)
49 47 | )

0 comments on commit 48b0660

Please sign in to comment.