From 92143afeee088551b3074c4660d6b1640f21f7af Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Thu, 31 Aug 2023 09:19:45 +0200 Subject: [PATCH] Group binary operators with same precedence only (#7010) --- .../src/expression/expr_bin_op.rs | 6 +- .../src/expression/mod.rs | 80 +++++++++++-------- ...atibility@simple_cases__expression.py.snap | 46 +---------- ...expression__binary_implicit_string.py.snap | 3 +- 4 files changed, 55 insertions(+), 80 deletions(-) diff --git a/crates/ruff_python_formatter/src/expression/expr_bin_op.rs b/crates/ruff_python_formatter/src/expression/expr_bin_op.rs index e25aaeaaa4..2241d014cb 100644 --- a/crates/ruff_python_formatter/src/expression/expr_bin_op.rs +++ b/crates/ruff_python_formatter/src/expression/expr_bin_op.rs @@ -18,6 +18,7 @@ use crate::expression::parentheses::{ NeedsParentheses, OptionalParentheses, }; use crate::expression::string::StringLayout; +use crate::expression::OperatorPrecedence; use crate::prelude::*; #[derive(Default)] @@ -67,10 +68,13 @@ impl FormatNodeRule for FormatExprBinOp { BinOpLayout::Default => { let format_inner = format_with(|f: &mut PyFormatter| { let source = f.context().source(); + let precedence = OperatorPrecedence::from(item.op); let binary_chain: SmallVec<[&ExprBinOp; 4]> = iter::successors(Some(item), |parent| { parent.left.as_bin_op_expr().and_then(|bin_expression| { - if is_expression_parenthesized(bin_expression.into(), source) { + if OperatorPrecedence::from(bin_expression.op) != precedence + || is_expression_parenthesized(bin_expression.into(), source) + { None } else { Some(bin_expression) diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index 8904e470e4..9594ab08f6 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -363,9 +363,9 @@ impl<'ast> IntoFormat> for Expr { /// Tests if it is safe to omit the optional parentheses. /// /// We prefer parentheses at least in the following cases: -/// * The expression contains more than one unparenthesized expression with the same priority. For example, +/// * The expression contains more than one unparenthesized expression with the same precedence. For example, /// the expression `a * b * c` contains two multiply operations. We prefer parentheses in that case. -/// `(a * b) * c` or `a * b + c` are okay, because the subexpression is parenthesized, or the expression uses operands with a lower priority +/// `(a * b) * c` or `a * b + c` are okay, because the subexpression is parenthesized, or the expression uses operands with a lower precedence /// * The expression contains at least one parenthesized sub expression (optimization to avoid unnecessary work) /// /// This mimics Black's [`_maybe_split_omitting_optional_parens`](https://github.com/psf/black/blob/d1248ca9beaf0ba526d265f4108836d89cf551b7/src/black/linegen.py#L746-L820) @@ -373,11 +373,11 @@ fn can_omit_optional_parentheses(expr: &Expr, context: &PyFormatContext) -> bool let mut visitor = CanOmitOptionalParenthesesVisitor::new(context); visitor.visit_subexpression(expr); - if visitor.max_priority == OperatorPriority::None { + if visitor.max_precedence == OperatorPrecedence::None { true - } else if visitor.max_priority_count > 1 { + } else if visitor.pax_precedence_count > 1 { false - } else if visitor.max_priority == OperatorPriority::Attribute { + } else if visitor.max_precedence == OperatorPrecedence::Attribute { true } else if !visitor.any_parenthesized_expressions { // Only use the more complex IR when there is any expression that we can possibly split by @@ -397,8 +397,8 @@ fn can_omit_optional_parentheses(expr: &Expr, context: &PyFormatContext) -> bool #[derive(Clone, Debug)] struct CanOmitOptionalParenthesesVisitor<'input> { - max_priority: OperatorPriority, - max_priority_count: u32, + max_precedence: OperatorPrecedence, + pax_precedence_count: u32, any_parenthesized_expressions: bool, last: Option<&'input Expr>, first: Option<&'input Expr>, @@ -409,26 +409,26 @@ impl<'input> CanOmitOptionalParenthesesVisitor<'input> { fn new(context: &'input PyFormatContext) -> Self { Self { context, - max_priority: OperatorPriority::None, - max_priority_count: 0, + max_precedence: OperatorPrecedence::None, + pax_precedence_count: 0, any_parenthesized_expressions: false, last: None, first: None, } } - fn update_max_priority(&mut self, current_priority: OperatorPriority) { - self.update_max_priority_with_count(current_priority, 1); + fn update_max_precedence(&mut self, precedence: OperatorPrecedence) { + self.update_max_precedence_with_count(precedence, 1); } - fn update_max_priority_with_count(&mut self, current_priority: OperatorPriority, count: u32) { - match self.max_priority.cmp(¤t_priority) { + fn update_max_precedence_with_count(&mut self, precedence: OperatorPrecedence, count: u32) { + match self.max_precedence.cmp(&precedence) { Ordering::Less => { - self.max_priority_count = count; - self.max_priority = current_priority; + self.pax_precedence_count = count; + self.max_precedence = precedence; } Ordering::Equal => { - self.max_priority_count += count; + self.pax_precedence_count += count; } Ordering::Greater => {} } @@ -455,8 +455,8 @@ impl<'input> CanOmitOptionalParenthesesVisitor<'input> { range: _, op: _, values, - }) => self.update_max_priority_with_count( - OperatorPriority::BooleanOperation, + }) => self.update_max_precedence_with_count( + OperatorPrecedence::BooleanOperation, values.len().saturating_sub(1) as u32, ), Expr::BinOp(ast::ExprBinOp { @@ -464,11 +464,11 @@ impl<'input> CanOmitOptionalParenthesesVisitor<'input> { left: _, right: _, range: _, - }) => self.update_max_priority(OperatorPriority::from(*op)), + }) => self.update_max_precedence(OperatorPrecedence::from(*op)), Expr::IfExp(_) => { // + 1 for the if and one for the else - self.update_max_priority_with_count(OperatorPriority::Conditional, 2); + self.update_max_precedence_with_count(OperatorPrecedence::Conditional, 2); } // It's impossible for a file smaller or equal to 4GB to contain more than 2^32 comparisons @@ -480,7 +480,10 @@ impl<'input> CanOmitOptionalParenthesesVisitor<'input> { ops, comparators: _, }) => { - self.update_max_priority_with_count(OperatorPriority::Comparator, ops.len() as u32); + self.update_max_precedence_with_count( + OperatorPrecedence::Comparator, + ops.len() as u32, + ); } Expr::Call(ast::ExprCall { range: _, @@ -506,7 +509,7 @@ impl<'input> CanOmitOptionalParenthesesVisitor<'input> { operand: _, }) => { if op.is_invert() { - self.update_max_priority(OperatorPriority::BitwiseInversion); + self.update_max_precedence(OperatorPrecedence::BitwiseInversion); } } @@ -519,7 +522,7 @@ impl<'input> CanOmitOptionalParenthesesVisitor<'input> { }) => { self.visit_expr(value); if has_parentheses(value, self.context).is_some() { - self.update_max_priority(OperatorPriority::Attribute); + self.update_max_precedence(OperatorPrecedence::Attribute); } self.last = Some(expr); return; @@ -541,7 +544,7 @@ impl<'input> CanOmitOptionalParenthesesVisitor<'input> { implicit_concatenated: true, .. }) => { - self.update_max_priority(OperatorPriority::String); + self.update_max_precedence(OperatorPrecedence::String); } Expr::NamedExpr(_) @@ -777,38 +780,45 @@ pub(crate) fn has_own_parentheses( } } +/// The precedence of [python operators](https://docs.python.org/3/reference/expressions.html#operator-precedence) from +/// lowest to highest priority. +/// +/// Ruff uses the operator precedence to decide in which order to split operators: +/// Operators with a lower precedence split before higher-precedence operators. +/// Splitting by precedence ensures that the visual grouping reflects the precedence. #[derive(Debug, Copy, Clone, Ord, PartialOrd, Eq, PartialEq)] -enum OperatorPriority { +enum OperatorPrecedence { None, Attribute, - Comparator, Exponential, BitwiseInversion, Multiplicative, Additive, Shift, BitwiseAnd, - BitwiseOr, BitwiseXor, + BitwiseOr, + Comparator, + // Implicit string concatenation String, BooleanOperation, Conditional, } -impl From for OperatorPriority { +impl From for OperatorPrecedence { fn from(value: Operator) -> Self { match value { - Operator::Add | Operator::Sub => OperatorPriority::Additive, + Operator::Add | Operator::Sub => OperatorPrecedence::Additive, Operator::Mult | Operator::MatMult | Operator::Div | Operator::Mod - | Operator::FloorDiv => OperatorPriority::Multiplicative, - Operator::Pow => OperatorPriority::Exponential, - Operator::LShift | Operator::RShift => OperatorPriority::Shift, - Operator::BitOr => OperatorPriority::BitwiseOr, - Operator::BitXor => OperatorPriority::BitwiseXor, - Operator::BitAnd => OperatorPriority::BitwiseAnd, + | Operator::FloorDiv => OperatorPrecedence::Multiplicative, + Operator::Pow => OperatorPrecedence::Exponential, + Operator::LShift | Operator::RShift => OperatorPrecedence::Shift, + Operator::BitOr => OperatorPrecedence::BitwiseOr, + Operator::BitXor => OperatorPrecedence::BitwiseXor, + Operator::BitAnd => OperatorPrecedence::BitwiseAnd, } } } diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__expression.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__expression.py.snap index e55f593032..d8e7464160 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__expression.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__expression.py.snap @@ -311,38 +311,6 @@ last_call() sync(async_xxxx_xxx_xxxx_xxxxx_xxxx_xxx.__func__) ) xxxx_xxx_xxxx_xxxxx_xxxx_xxx: Callable[..., List[SomeClass]] = classmethod( -@@ -328,13 +331,18 @@ - ): - return True - if ( -- ~aaaa.a + aaaa.b - aaaa.c * aaaa.d / aaaa.e -+ ~aaaa.a -+ + aaaa.b -+ - aaaa.c * aaaa.d / aaaa.e - | aaaa.f & aaaa.g % aaaa.h ^ aaaa.i << aaaa.k >> aaaa.l**aaaa.m // aaaa.n - ): - return True - if ( -- ~aaaaaaaa.a + aaaaaaaa.b - aaaaaaaa.c @ aaaaaaaa.d / aaaaaaaa.e -- | aaaaaaaa.f & aaaaaaaa.g % aaaaaaaa.h -+ ~aaaaaaaa.a -+ + aaaaaaaa.b -+ - aaaaaaaa.c @ aaaaaaaa.d / aaaaaaaa.e -+ | aaaaaaaa.f -+ & aaaaaaaa.g % aaaaaaaa.h - ^ aaaaaaaa.i << aaaaaaaa.k >> aaaaaaaa.l**aaaaaaaa.m // aaaaaaaa.n - ): - return True -@@ -342,7 +350,8 @@ - ~aaaaaaaaaaaaaaaa.a - + aaaaaaaaaaaaaaaa.b - - aaaaaaaaaaaaaaaa.c * aaaaaaaaaaaaaaaa.d @ aaaaaaaaaaaaaaaa.e -- | aaaaaaaaaaaaaaaa.f & aaaaaaaaaaaaaaaa.g % aaaaaaaaaaaaaaaa.h -+ | aaaaaaaaaaaaaaaa.f -+ & aaaaaaaaaaaaaaaa.g % aaaaaaaaaaaaaaaa.h - ^ aaaaaaaaaaaaaaaa.i - << aaaaaaaaaaaaaaaa.k - >> aaaaaaaaaaaaaaaa.l**aaaaaaaaaaaaaaaa.m // aaaaaaaaaaaaaaaa.n ``` ## Ruff Output @@ -681,18 +649,13 @@ if ( ): return True if ( - ~aaaa.a - + aaaa.b - - aaaa.c * aaaa.d / aaaa.e + ~aaaa.a + aaaa.b - aaaa.c * aaaa.d / aaaa.e | aaaa.f & aaaa.g % aaaa.h ^ aaaa.i << aaaa.k >> aaaa.l**aaaa.m // aaaa.n ): return True if ( - ~aaaaaaaa.a - + aaaaaaaa.b - - aaaaaaaa.c @ aaaaaaaa.d / aaaaaaaa.e - | aaaaaaaa.f - & aaaaaaaa.g % aaaaaaaa.h + ~aaaaaaaa.a + aaaaaaaa.b - aaaaaaaa.c @ aaaaaaaa.d / aaaaaaaa.e + | aaaaaaaa.f & aaaaaaaa.g % aaaaaaaa.h ^ aaaaaaaa.i << aaaaaaaa.k >> aaaaaaaa.l**aaaaaaaa.m // aaaaaaaa.n ): return True @@ -700,8 +663,7 @@ if ( ~aaaaaaaaaaaaaaaa.a + aaaaaaaaaaaaaaaa.b - aaaaaaaaaaaaaaaa.c * aaaaaaaaaaaaaaaa.d @ aaaaaaaaaaaaaaaa.e - | aaaaaaaaaaaaaaaa.f - & aaaaaaaaaaaaaaaa.g % aaaaaaaaaaaaaaaa.h + | aaaaaaaaaaaaaaaa.f & aaaaaaaaaaaaaaaa.g % aaaaaaaaaaaaaaaa.h ^ aaaaaaaaaaaaaaaa.i << aaaaaaaaaaaaaaaa.k >> aaaaaaaaaaaaaaaa.l**aaaaaaaaaaaaaaaa.m // aaaaaaaaaaaaaaaa.n diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__binary_implicit_string.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__binary_implicit_string.py.snap index 207b8772ba..8cb1927ad4 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__binary_implicit_string.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__binary_implicit_string.py.snap @@ -215,8 +215,7 @@ self._assert_skipping( ( "aaaaaaaaaaaaaaaaaaaaaaaaaaaaa" "bbbbbbbbbbbbbbbbbbbbbbbbbbbbb" - "cccccccccccccccccccccccccc" - % aaaaaaaaaaaa + "cccccccccccccccccccccccccc" % aaaaaaaaaaaa + x )