From 91df23905fa3cdafde7bbc25a66132e22ffa1a6b Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Sat, 8 Jul 2023 10:01:04 +0200 Subject: [PATCH 01/10] Use new IR to format expressions --- crates/ruff_python_formatter/src/builders.rs | 8 +- .../src/comments/format.rs | 4 +- crates/ruff_python_formatter/src/context.rs | 9 +- .../src/expression/binary_like.rs | 216 ------------------ .../src/expression/expr_bin_op.rs | 55 +---- .../src/expression/expr_bool_op.rs | 107 +++------ .../src/expression/expr_compare.rs | 73 ++---- .../src/expression/mod.rs | 74 ++++-- .../src/expression/parentheses.rs | 60 ++++- crates/ruff_python_formatter/src/lib.rs | 17 +- .../src/other/arguments.rs | 3 +- ...tibility@simple_cases__composition.py.snap | 12 +- ...ses__composition_no_trailing_comma.py.snap | 12 +- ...tibility@simple_cases__empty_lines.py.snap | 131 ++--------- ...@simple_cases__remove_await_parens.py.snap | 97 ++------ ...ompatibility@simple_cases__torture.py.snap | 83 ++----- ...s__trailing_comma_optional_parens1.py.snap | 56 +---- .../format@expression__binary.py.snap | 6 +- .../format@expression__compare.py.snap | 53 ++--- .../format@expression__unary.py.snap | 11 +- 20 files changed, 303 insertions(+), 784 deletions(-) delete mode 100644 crates/ruff_python_formatter/src/expression/binary_like.rs diff --git a/crates/ruff_python_formatter/src/builders.rs b/crates/ruff_python_formatter/src/builders.rs index 4541acda740ee..1cf0a03b40404 100644 --- a/crates/ruff_python_formatter/src/builders.rs +++ b/crates/ruff_python_formatter/src/builders.rs @@ -24,7 +24,7 @@ impl<'ast> Format> for OptionalParentheses<'_, 'ast> { group(&format_args![ if_group_breaks(&text("(")), soft_block_indent(&Arguments::from(&self.inner)), - if_group_breaks(&text(")")) + if_group_breaks(&text(")")), ]) .fmt(f) } @@ -113,7 +113,9 @@ impl<'fmt, 'ast, 'buf> JoinNodesBuilder<'fmt, 'ast, 'buf> { 0 | 1 => hard_line_break().fmt(self.fmt), _ => empty_line().fmt(self.fmt), }, - NodeLevel::Expression => hard_line_break().fmt(self.fmt), + NodeLevel::Expression(_) | NodeLevel::ParenthesizedExpression => { + hard_line_break().fmt(self.fmt) + } }?; } @@ -353,7 +355,7 @@ no_leading_newline = 30"# // Removes all empty lines #[test] fn ranged_builder_parenthesized_level() { - let printed = format_ranged(NodeLevel::Expression); + let printed = format_ranged(NodeLevel::Expression(None)); assert_eq!( &printed, diff --git a/crates/ruff_python_formatter/src/comments/format.rs b/crates/ruff_python_formatter/src/comments/format.rs index 31c7e42cd133a..c0a0a2a5bd3fc 100644 --- a/crates/ruff_python_formatter/src/comments/format.rs +++ b/crates/ruff_python_formatter/src/comments/format.rs @@ -318,7 +318,9 @@ impl Format> for FormatEmptyLines { }, // Remove all whitespace in parenthesized expressions - NodeLevel::Expression => write!(f, [hard_line_break()]), + NodeLevel::Expression(_) | NodeLevel::ParenthesizedExpression => { + write!(f, [hard_line_break()]) + } } } } diff --git a/crates/ruff_python_formatter/src/context.rs b/crates/ruff_python_formatter/src/context.rs index cdf587c35fd9a..2683641dd861a 100644 --- a/crates/ruff_python_formatter/src/context.rs +++ b/crates/ruff_python_formatter/src/context.rs @@ -1,6 +1,6 @@ use crate::comments::Comments; use crate::PyFormatOptions; -use ruff_formatter::{FormatContext, SourceCode}; +use ruff_formatter::{FormatContext, GroupId, SourceCode}; use ruff_python_ast::source_code::Locator; use std::fmt::{Debug, Formatter}; @@ -78,6 +78,9 @@ pub(crate) enum NodeLevel { /// (`if`, `while`, `match`, etc.). CompoundStatement, - /// Formatting nodes that are enclosed in a parenthesized expression. - Expression, + /// The root or any sub-expression. + Expression(Option), + + /// Formatting nodes that are enclosed by a parenthesized (any `[]`, `{}` or `()`) expression. + ParenthesizedExpression, } diff --git a/crates/ruff_python_formatter/src/expression/binary_like.rs b/crates/ruff_python_formatter/src/expression/binary_like.rs deleted file mode 100644 index c934322e55a64..0000000000000 --- a/crates/ruff_python_formatter/src/expression/binary_like.rs +++ /dev/null @@ -1,216 +0,0 @@ -//! This module provides helper utilities to format an expression that has a left side, an operator, -//! and a right side (binary like). - -use rustpython_parser::ast::{self, Expr}; - -use ruff_formatter::{format_args, write}; - -use crate::expression::parentheses::{is_expression_parenthesized, Parentheses}; -use crate::prelude::*; - -/// Trait to implement a binary like syntax that has a left operand, an operator, and a right operand. -pub(super) trait FormatBinaryLike<'ast> { - /// The type implementing the formatting of the operator. - type FormatOperator: Format>; - - /// Formats the binary like expression to `f`. - fn fmt_binary( - &self, - parentheses: Option, - f: &mut PyFormatter<'ast, '_>, - ) -> FormatResult<()> { - let left = self.left()?; - let operator = self.operator(); - let right = self.right()?; - - let layout = if parentheses == Some(Parentheses::Custom) { - self.binary_layout(f.context().contents()) - } else { - BinaryLayout::Default - }; - - match layout { - BinaryLayout::Default => self.fmt_default(f), - BinaryLayout::ExpandLeft => { - let left = left.format().memoized(); - let right = right.format().memoized(); - write!( - f, - [best_fitting![ - // Everything on a single line - format_args![group(&left), space(), operator, space(), right], - // Break the left over multiple lines, keep the right flat - format_args![ - group(&left).should_expand(true), - space(), - operator, - space(), - right - ], - // The content doesn't fit, indent the content and break before the operator. - format_args![ - text("("), - block_indent(&format_args![ - left, - hard_line_break(), - operator, - space(), - right - ]), - text(")") - ] - ] - .with_mode(BestFittingMode::AllLines)] - ) - } - BinaryLayout::ExpandRight => { - let left_group = f.group_id("BinaryLeft"); - - write!( - f, - [ - // Wrap the left in a group and gives it an id. The printer first breaks the - // right side if `right` contains any line break because the printer breaks - // sequences of groups from right to left. - // Indents the left side if the group breaks. - group(&format_args![ - if_group_breaks(&text("(")), - indent_if_group_breaks( - &format_args![ - soft_line_break(), - left.format(), - soft_line_break_or_space(), - operator, - space() - ], - left_group - ) - ]) - .with_group_id(Some(left_group)), - // Wrap the right in a group and indents its content but only if the left side breaks - group(&indent_if_group_breaks(&right.format(), left_group)), - // If the left side breaks, insert a hard line break to finish the indent and close the open paren. - if_group_breaks(&format_args![hard_line_break(), text(")")]) - .with_group_id(Some(left_group)) - ] - ) - } - BinaryLayout::ExpandRightThenLeft => { - // The formatter expands group-sequences from right to left, and expands both if - // there isn't enough space when expanding only one of them. - write!( - f, - [ - group(&left.format()), - space(), - operator, - space(), - group(&right.format()) - ] - ) - } - } - } - - /// Determines which binary layout to use. - fn binary_layout(&self, source: &str) -> BinaryLayout { - if let (Ok(left), Ok(right)) = (self.left(), self.right()) { - BinaryLayout::from_left_right(left, right, source) - } else { - BinaryLayout::Default - } - } - - /// Formats the node according to the default layout. - fn fmt_default(&self, f: &mut PyFormatter<'ast, '_>) -> FormatResult<()>; - - /// Returns the left operator - fn left(&self) -> FormatResult<&Expr>; - - /// Returns the right operator. - fn right(&self) -> FormatResult<&Expr>; - - /// Returns the object that formats the operator. - fn operator(&self) -> Self::FormatOperator; -} - -fn can_break_expr(expr: &Expr, source: &str) -> bool { - let can_break = match expr { - Expr::Tuple(ast::ExprTuple { - elts: expressions, .. - }) - | Expr::List(ast::ExprList { - elts: expressions, .. - }) - | Expr::Set(ast::ExprSet { - elts: expressions, .. - }) - | Expr::Dict(ast::ExprDict { - values: expressions, - .. - }) => !expressions.is_empty(), - Expr::Call(ast::ExprCall { args, keywords, .. }) => { - !(args.is_empty() && keywords.is_empty()) - } - Expr::ListComp(_) | Expr::SetComp(_) | Expr::DictComp(_) | Expr::GeneratorExp(_) => true, - Expr::UnaryOp(ast::ExprUnaryOp { operand, .. }) => can_break_expr(operand.as_ref(), source), - _ => false, - }; - - can_break || is_expression_parenthesized(expr.into(), source) -} - -#[derive(Copy, Clone, Debug, Eq, PartialEq)] -pub(super) enum BinaryLayout { - /// Put each operand on their own line if either side expands - Default, - - /// Try to expand the left to make it fit. Add parentheses if the left or right don't fit. - /// - ///```python - /// [ - /// a, - /// b - /// ] & c - ///``` - ExpandLeft, - - /// Try to expand the right to make it fix. Add parentheses if the left or right don't fit. - /// - /// ```python - /// a & [ - /// b, - /// c - /// ] - /// ``` - ExpandRight, - - /// Both the left and right side can be expanded. Try in the following order: - /// * expand the right side - /// * expand the left side - /// * expand both sides - /// - /// to make the expression fit - /// - /// ```python - /// [ - /// a, - /// b - /// ] & [ - /// c, - /// d - /// ] - /// ``` - ExpandRightThenLeft, -} - -impl BinaryLayout { - pub(super) fn from_left_right(left: &Expr, right: &Expr, source: &str) -> BinaryLayout { - match (can_break_expr(left, source), can_break_expr(right, source)) { - (false, false) => BinaryLayout::Default, - (true, false) => BinaryLayout::ExpandLeft, - (false, true) => BinaryLayout::ExpandRight, - (true, true) => BinaryLayout::ExpandRightThenLeft, - } - } -} 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 859e03ce4f2d3..9307ed204320d 100644 --- a/crates/ruff_python_formatter/src/expression/expr_bin_op.rs +++ b/crates/ruff_python_formatter/src/expression/expr_bin_op.rs @@ -1,8 +1,7 @@ use crate::comments::{trailing_comments, trailing_node_comments, Comments}; -use crate::expression::binary_like::{BinaryLayout, FormatBinaryLike}; use crate::expression::parentheses::{ - default_expression_needs_parentheses, is_expression_parenthesized, NeedsParentheses, - Parenthesize, + default_expression_needs_parentheses, in_parentheses_only_group, is_expression_parenthesized, + NeedsParentheses, Parenthesize, }; use crate::expression::Parentheses; use crate::prelude::*; @@ -31,24 +30,11 @@ impl FormatRuleWithOptions> for FormatExprBinOp { impl FormatNodeRule for FormatExprBinOp { fn fmt_fields(&self, item: &ExprBinOp, f: &mut PyFormatter) -> FormatResult<()> { - item.fmt_binary(self.parentheses, f) - } - - fn fmt_dangling_comments(&self, _node: &ExprBinOp, _f: &mut PyFormatter) -> FormatResult<()> { - // Handled inside of `fmt_fields` - Ok(()) - } -} - -impl<'ast> FormatBinaryLike<'ast> for ExprBinOp { - type FormatOperator = FormatOwnedWithRule>; - - fn fmt_default(&self, f: &mut PyFormatter<'ast, '_>) -> FormatResult<()> { let comments = f.context().comments().clone(); let format_inner = format_with(|f: &mut PyFormatter| { let source = f.context().contents(); - let binary_chain: SmallVec<[&ExprBinOp; 4]> = iter::successors(Some(self), |parent| { + 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.as_any_node_ref(), source) { None @@ -63,7 +49,7 @@ impl<'ast> FormatBinaryLike<'ast> for ExprBinOp { let left_most = binary_chain.last().unwrap(); // Format the left most expression - group(&left_most.left.format()).fmt(f)?; + in_parentheses_only_group(&left_most.left.format()).fmt(f)?; // Iterate upwards in the binary expression tree and, for each level, format the operator // and the right expression. @@ -100,13 +86,13 @@ impl<'ast> FormatBinaryLike<'ast> for ExprBinOp { space().fmt(f)?; } - group(&right.format()).fmt(f)?; + in_parentheses_only_group(&right.format()).fmt(f)?; // It's necessary to format the trailing comments because the code bypasses // `FormatNodeRule::fmt` for the nested binary expressions. // Don't call the formatting function for the most outer binary expression because // these comments have already been formatted. - if current != self { + if current != item { trailing_node_comments(current).fmt(f)?; } } @@ -114,19 +100,12 @@ impl<'ast> FormatBinaryLike<'ast> for ExprBinOp { Ok(()) }); - group(&format_inner).fmt(f) + in_parentheses_only_group(&format_inner).fmt(f) } - fn left(&self) -> FormatResult<&Expr> { - Ok(&self.left) - } - - fn right(&self) -> FormatResult<&Expr> { - Ok(&self.right) - } - - fn operator(&self) -> Self::FormatOperator { - self.op.into_format() + fn fmt_dangling_comments(&self, _node: &ExprBinOp, _f: &mut PyFormatter) -> FormatResult<()> { + // Handled inside of `fmt_fields` + Ok(()) } } @@ -200,18 +179,6 @@ impl NeedsParentheses for ExprBinOp { source: &str, comments: &Comments, ) -> Parentheses { - match default_expression_needs_parentheses(self.into(), parenthesize, source, comments) { - Parentheses::Optional => { - if self.binary_layout(source) == BinaryLayout::Default - || comments.has_leading_comments(self.right.as_ref()) - || comments.has_dangling_comments(self) - { - Parentheses::Optional - } else { - Parentheses::Custom - } - } - parentheses => parentheses, - } + default_expression_needs_parentheses(self.into(), parenthesize, source, comments) } } diff --git a/crates/ruff_python_formatter/src/expression/expr_bool_op.rs b/crates/ruff_python_formatter/src/expression/expr_bool_op.rs index 8fb9b42eb2207..5a350645095ce 100644 --- a/crates/ruff_python_formatter/src/expression/expr_bool_op.rs +++ b/crates/ruff_python_formatter/src/expression/expr_bool_op.rs @@ -1,13 +1,11 @@ use crate::comments::{leading_comments, Comments}; -use crate::expression::binary_like::{BinaryLayout, FormatBinaryLike}; use crate::expression::parentheses::{ - default_expression_needs_parentheses, NeedsParentheses, Parentheses, Parenthesize, + default_expression_needs_parentheses, in_parentheses_only_group, NeedsParentheses, Parentheses, + Parenthesize, }; use crate::prelude::*; -use ruff_formatter::{ - write, FormatError, FormatOwnedWithRule, FormatRefWithRule, FormatRuleWithOptions, -}; -use rustpython_parser::ast::{BoolOp, Expr, ExprBoolOp}; +use ruff_formatter::{write, FormatOwnedWithRule, FormatRefWithRule, FormatRuleWithOptions}; +use rustpython_parser::ast::{BoolOp, ExprBoolOp}; #[derive(Default)] pub struct FormatExprBoolOp { @@ -24,64 +22,48 @@ impl FormatRuleWithOptions> for FormatExprBoolOp impl FormatNodeRule for FormatExprBoolOp { fn fmt_fields(&self, item: &ExprBoolOp, f: &mut PyFormatter) -> FormatResult<()> { - item.fmt_binary(self.parentheses, f) - } -} - -impl<'ast> FormatBinaryLike<'ast> for ExprBoolOp { - type FormatOperator = FormatOwnedWithRule>; - - fn binary_layout(&self, source: &str) -> BinaryLayout { - match self.values.as_slice() { - [left, right] => BinaryLayout::from_left_right(left, right, source), - [..] => BinaryLayout::Default, - } - } - - fn fmt_default(&self, f: &mut PyFormatter<'ast, '_>) -> FormatResult<()> { let ExprBoolOp { range: _, op, values, - } = self; - - let mut values = values.iter(); - let comments = f.context().comments().clone(); - - let Some(first) = values.next() else { - return Ok(()); - }; - - write!(f, [group(&first.format())])?; + } = item; + + let inner = format_with(|f: &mut PyFormatter| { + let mut values = values.iter(); + let comments = f.context().comments().clone(); + + let Some(first) = values.next() else { + return Ok(()); + }; + + write!(f, [in_parentheses_only_group(&first.format())])?; + + for value in values { + let leading_value_comments = comments.leading_comments(value); + // Format the expressions leading comments **before** the operator + if leading_value_comments.is_empty() { + write!(f, [soft_line_break_or_space()])?; + } else { + write!( + f, + [hard_line_break(), leading_comments(leading_value_comments)] + )?; + } - for value in values { - let leading_value_comments = comments.leading_comments(value); - // Format the expressions leading comments **before** the operator - if leading_value_comments.is_empty() { - write!(f, [soft_line_break_or_space()])?; - } else { write!( f, - [hard_line_break(), leading_comments(leading_value_comments)] + [ + op.format(), + space(), + in_parentheses_only_group(&value.format()) + ] )?; } - write!(f, [op.format(), space(), group(&value.format())])?; - } + Ok(()) + }); - Ok(()) - } - - fn left(&self) -> FormatResult<&Expr> { - self.values.first().ok_or(FormatError::SyntaxError) - } - - fn right(&self) -> FormatResult<&Expr> { - self.values.last().ok_or(FormatError::SyntaxError) - } - - fn operator(&self) -> Self::FormatOperator { - self.op.into_format() + in_parentheses_only_group(&inner).fmt(f) } } @@ -92,24 +74,7 @@ impl NeedsParentheses for ExprBoolOp { source: &str, comments: &Comments, ) -> Parentheses { - match default_expression_needs_parentheses(self.into(), parenthesize, source, comments) { - Parentheses::Optional => match self.binary_layout(source) { - BinaryLayout::Default => Parentheses::Optional, - - BinaryLayout::ExpandRight - | BinaryLayout::ExpandLeft - | BinaryLayout::ExpandRightThenLeft - if self - .values - .last() - .map_or(false, |right| comments.has_leading_comments(right)) => - { - Parentheses::Optional - } - _ => Parentheses::Custom, - }, - parentheses => parentheses, - } + default_expression_needs_parentheses(self.into(), parenthesize, source, comments) } } diff --git a/crates/ruff_python_formatter/src/expression/expr_compare.rs b/crates/ruff_python_formatter/src/expression/expr_compare.rs index 094344be61492..10bffe948b056 100644 --- a/crates/ruff_python_formatter/src/expression/expr_compare.rs +++ b/crates/ruff_python_formatter/src/expression/expr_compare.rs @@ -1,14 +1,11 @@ use crate::comments::{leading_comments, Comments}; -use crate::expression::binary_like::{BinaryLayout, FormatBinaryLike}; use crate::expression::parentheses::{ - default_expression_needs_parentheses, NeedsParentheses, Parentheses, Parenthesize, + default_expression_needs_parentheses, in_parentheses_only_group, NeedsParentheses, Parentheses, + Parenthesize, }; use crate::prelude::*; use crate::FormatNodeRule; -use ruff_formatter::{ - write, FormatError, FormatOwnedWithRule, FormatRefWithRule, FormatRuleWithOptions, -}; -use rustpython_parser::ast::Expr; +use ruff_formatter::{write, FormatOwnedWithRule, FormatRefWithRule, FormatRuleWithOptions}; use rustpython_parser::ast::{CmpOp, ExprCompare}; #[derive(Default)] @@ -27,35 +24,16 @@ impl FormatRuleWithOptions> for FormatExprCompa impl FormatNodeRule for FormatExprCompare { fn fmt_fields(&self, item: &ExprCompare, f: &mut PyFormatter) -> FormatResult<()> { - item.fmt_binary(self.parentheses, f) - } -} - -impl<'ast> FormatBinaryLike<'ast> for ExprCompare { - type FormatOperator = FormatOwnedWithRule>; - - fn binary_layout(&self, source: &str) -> BinaryLayout { - if self.ops.len() == 1 { - match self.comparators.as_slice() { - [right] => BinaryLayout::from_left_right(&self.left, right, source), - [..] => BinaryLayout::Default, - } - } else { - BinaryLayout::Default - } - } - - fn fmt_default(&self, f: &mut PyFormatter<'ast, '_>) -> FormatResult<()> { let ExprCompare { range: _, left, ops, comparators, - } = self; + } = item; let comments = f.context().comments().clone(); - write!(f, [group(&left.format())])?; + write!(f, [in_parentheses_only_group(&left.format())])?; assert_eq!(comparators.len(), ops.len()); @@ -74,24 +52,18 @@ impl<'ast> FormatBinaryLike<'ast> for ExprCompare { )?; } - write!(f, [operator.format(), space(), group(&comparator.format())])?; + write!( + f, + [ + operator.format(), + space(), + in_parentheses_only_group(&comparator.format()) + ] + )?; } Ok(()) } - - fn left(&self) -> FormatResult<&Expr> { - Ok(self.left.as_ref()) - } - - fn right(&self) -> FormatResult<&Expr> { - self.comparators.last().ok_or(FormatError::SyntaxError) - } - - fn operator(&self) -> Self::FormatOperator { - let op = *self.ops.first().unwrap(); - op.into_format() - } } impl NeedsParentheses for ExprCompare { @@ -101,24 +73,7 @@ impl NeedsParentheses for ExprCompare { source: &str, comments: &Comments, ) -> Parentheses { - match default_expression_needs_parentheses(self.into(), parenthesize, source, comments) { - parentheses @ Parentheses::Optional => match self.binary_layout(source) { - BinaryLayout::Default => parentheses, - - BinaryLayout::ExpandRight - | BinaryLayout::ExpandLeft - | BinaryLayout::ExpandRightThenLeft - if self - .comparators - .last() - .map_or(false, |right| comments.has_leading_comments(right)) => - { - parentheses - } - _ => Parentheses::Custom, - }, - parentheses => parentheses, - } + default_expression_needs_parentheses(self.into(), parenthesize, source, comments) } } diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index ac3a4d682c637..a9042fe27aaa8 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -1,16 +1,16 @@ -use crate::builders::optional_parentheses; +use rustpython_parser::ast::Expr; + +use ruff_formatter::{ + format_args, FormatOwnedWithRule, FormatRefWithRule, FormatRule, FormatRuleWithOptions, +}; + use crate::comments::Comments; use crate::context::NodeLevel; use crate::expression::expr_tuple::TupleParentheses; -use crate::expression::parentheses::{NeedsParentheses, Parentheses, Parenthesize}; +use crate::expression::parentheses::{parenthesized, NeedsParentheses, Parentheses, Parenthesize}; use crate::expression::string::StringLayout; use crate::prelude::*; -use ruff_formatter::{ - format_args, write, FormatOwnedWithRule, FormatRefWithRule, FormatRule, FormatRuleWithOptions, -}; -use rustpython_parser::ast::Expr; -pub(crate) mod binary_like; pub(crate) mod expr_attribute; pub(crate) mod expr_await; pub(crate) mod expr_bin_op; @@ -99,26 +99,52 @@ impl FormatRule> for FormatExpr { Expr::Slice(expr) => expr.format().fmt(f), }); - let saved_level = f.context().node_level(); - f.context_mut().set_node_level(NodeLevel::Expression); - let result = match parentheses { - Parentheses::Always => { - write!( - f, - [group(&format_args![ - text("("), - soft_block_indent(&format_expr), - text(")") - ])] - ) - } + Parentheses::Always => parenthesized("(", &format_expr, ")").fmt(f), // Add optional parentheses. Ignore if the item renders parentheses itself. - Parentheses::Optional => optional_parentheses(&format_expr).fmt(f), - Parentheses::Custom | Parentheses::Never => Format::fmt(&format_expr, f), - }; + Parentheses::Optional => { + let saved_level = f.context().node_level(); - f.context_mut().set_node_level(saved_level); + let parens_id = f.group_id("optional_parentheses"); + + f.context_mut() + .set_node_level(NodeLevel::Expression(Some(parens_id))); + + let result = group(&format_args![ + if_group_breaks(&text("(")), + indent_if_group_breaks( + &format_args![soft_line_break(), format_expr], + parens_id + ), + soft_line_break(), + if_group_breaks(&text(")")) + ]) + .with_group_id(Some(parens_id)) + .fmt(f); + + f.context_mut().set_node_level(saved_level); + + result + } + Parentheses::Custom | Parentheses::Never => { + let saved_level = f.context().node_level(); + + let new_level = match saved_level { + NodeLevel::TopLevel | NodeLevel::CompoundStatement => { + NodeLevel::Expression(None) + } + level @ (NodeLevel::Expression(_) | NodeLevel::ParenthesizedExpression) => { + level + } + }; + + f.context_mut().set_node_level(new_level); + + let result = Format::fmt(&format_expr, f); + f.context_mut().set_node_level(saved_level); + result + } + }; result } diff --git a/crates/ruff_python_formatter/src/expression/parentheses.rs b/crates/ruff_python_formatter/src/expression/parentheses.rs index c73b5773ce408..a3087eebe43cd 100644 --- a/crates/ruff_python_formatter/src/expression/parentheses.rs +++ b/crates/ruff_python_formatter/src/expression/parentheses.rs @@ -1,7 +1,9 @@ use crate::comments::Comments; +use crate::context::NodeLevel; use crate::prelude::*; use crate::trivia::{first_non_trivia_token, first_non_trivia_token_rev, Token, TokenKind}; -use ruff_formatter::{format_args, write, Argument, Arguments}; +use ruff_formatter::prelude::tag::Condition; +use ruff_formatter::{format_args, Argument, Arguments}; use ruff_python_ast::node::AnyNodeRef; use rustpython_parser::ast::Ranged; @@ -145,13 +147,59 @@ pub(crate) struct FormatParenthesized<'content, 'ast> { impl<'ast> Format> for FormatParenthesized<'_, 'ast> { fn fmt(&self, f: &mut Formatter>) -> FormatResult<()> { - write!( - f, - [group(&format_args![ + let inner = format_with(|f| { + group(&format_args![ text(self.left), &soft_block_indent(&Arguments::from(&self.content)), text(self.right) - ])] - ) + ]) + .fmt(f) + }); + + let current_level = f.context().node_level(); + + f.context_mut() + .set_node_level(NodeLevel::ParenthesizedExpression); + + let result = if let NodeLevel::Expression(Some(group_id)) = current_level { + fits_expanded(&inner) + .with_condition(Some(Condition::if_group_fits_on_line(group_id))) + .fmt(f) + } else { + inner.fmt(f) + }; + + f.context_mut().set_node_level(current_level); + + result + } +} + +pub(crate) fn in_parentheses_only_group<'content, 'ast, Content>( + content: &'content Content, +) -> FormatInParenthesesOnlyGroup<'content, 'ast> +where + Content: Format>, +{ + FormatInParenthesesOnlyGroup { + content: Argument::new(content), + } +} + +pub(crate) struct FormatInParenthesesOnlyGroup<'content, 'ast> { + content: Argument<'content, PyFormatContext<'ast>>, +} + +impl<'ast> Format> for FormatInParenthesesOnlyGroup<'_, 'ast> { + fn fmt(&self, f: &mut Formatter>) -> FormatResult<()> { + if let NodeLevel::Expression(Some(group_id)) = f.context().node_level() { + conditional_group( + &Arguments::from(&self.content), + Condition::if_group_breaks(group_id), + ) + .fmt(f) + } else { + group(&Arguments::from(&self.content)).fmt(f) + } } } diff --git a/crates/ruff_python_formatter/src/lib.rs b/crates/ruff_python_formatter/src/lib.rs index eebcf056640f2..1795480a98f57 100644 --- a/crates/ruff_python_formatter/src/lib.rs +++ b/crates/ruff_python_formatter/src/lib.rs @@ -280,12 +280,9 @@ if True: #[test] fn quick_test() { let src = r#" -if [ - aaaaaa, - BBBB,ccccccccc,ddddddd,eeeeeeeeee,ffffff -] & bbbbbbbbbbbbbbbbbbddddddddddddddddddddddddddddbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb: - ... -"#; +[ + AAAAAAAAAAAAA +] + SHARED_AAAAAAAAAAAAA + USER_AAAAAAAAAAAAA + AAAAAAAAAAAAA + AAAAAAAAAAAAAAAAASAAAAAAAA"#; // Tokenize once let mut tokens = Vec::new(); let mut comment_ranges = CommentRangesBuilder::default(); @@ -312,10 +309,10 @@ if [ // Uncomment the `dbg` to print the IR. // Use `dbg_write!(f, []) instead of `write!(f, [])` in your formatting code to print some IR // inside of a `Format` implementation - // use ruff_formatter::FormatContext; - // dbg!(formatted - // .document() - // .display(formatted.context().source_code())); + use ruff_formatter::FormatContext; + dbg!(formatted + .document() + .display(formatted.context().source_code())); // // dbg!(formatted // .context() diff --git a/crates/ruff_python_formatter/src/other/arguments.rs b/crates/ruff_python_formatter/src/other/arguments.rs index e6d91f7b69fb1..0682a25632c48 100644 --- a/crates/ruff_python_formatter/src/other/arguments.rs +++ b/crates/ruff_python_formatter/src/other/arguments.rs @@ -31,7 +31,8 @@ impl FormatNodeRule for FormatArguments { } = item; let saved_level = f.context().node_level(); - f.context_mut().set_node_level(NodeLevel::Expression); + f.context_mut() + .set_node_level(NodeLevel::ParenthesizedExpression); let comments = f.context().comments().clone(); let dangling = comments.dangling_comments(item); diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__composition.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__composition.py.snap index 399e4ab91541f..d6de11c9cae1f 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__composition.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__composition.py.snap @@ -203,7 +203,7 @@ class C: print(i) xxxxxxxxxxxxxxxx = Yyyy2YyyyyYyyyyy( push_manager=context.request.resource_manager, -@@ -47,113 +47,46 @@ +@@ -47,10 +47,20 @@ def omitting_trailers(self) -> None: get_collection( hey_this_is_a_very_long_call, it_has_funny_attributes, really=True @@ -226,10 +226,7 @@ class C: d[0][1][2][3][4][5][6][7][8][9][10][11][12][13][14][15][16][17][18][19][20][21][ 22 ] - assignment = ( -- some.rather.elaborate.rule() and another.rule.ending_with.index[123] -+ some.rather.elaborate.rule() -+ and another.rule.ending_with.index[123] +@@ -59,101 +69,23 @@ ) def easy_asserts(self) -> None: @@ -340,7 +337,7 @@ class C: %3d 0 LOAD_FAST 1 (x) 2 LOAD_CONST 1 (1) 4 COMPARE_OP 2 (==) -@@ -161,21 +94,8 @@ +@@ -161,21 +93,8 @@ 8 STORE_ATTR 0 (x) 10 LOAD_CONST 0 (None) 12 RETURN_VALUE @@ -437,8 +434,7 @@ class C: 22 ] assignment = ( - some.rather.elaborate.rule() - and another.rule.ending_with.index[123] + some.rather.elaborate.rule() and another.rule.ending_with.index[123] ) def easy_asserts(self) -> None: diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__composition_no_trailing_comma.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__composition_no_trailing_comma.py.snap index 69415159cec31..d4ca2fcc125c1 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__composition_no_trailing_comma.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__composition_no_trailing_comma.py.snap @@ -203,7 +203,7 @@ class C: print(i) xxxxxxxxxxxxxxxx = Yyyy2YyyyyYyyyyy( push_manager=context.request.resource_manager, -@@ -47,113 +47,46 @@ +@@ -47,10 +47,20 @@ def omitting_trailers(self) -> None: get_collection( hey_this_is_a_very_long_call, it_has_funny_attributes, really=True @@ -226,10 +226,7 @@ class C: d[0][1][2][3][4][5][6][7][8][9][10][11][12][13][14][15][16][17][18][19][20][21][ 22 ] - assignment = ( -- some.rather.elaborate.rule() and another.rule.ending_with.index[123] -+ some.rather.elaborate.rule() -+ and another.rule.ending_with.index[123] +@@ -59,101 +69,23 @@ ) def easy_asserts(self) -> None: @@ -340,7 +337,7 @@ class C: %3d 0 LOAD_FAST 1 (x) 2 LOAD_CONST 1 (1) 4 COMPARE_OP 2 (==) -@@ -161,21 +94,8 @@ +@@ -161,21 +93,8 @@ 8 STORE_ATTR 0 (x) 10 LOAD_CONST 0 (None) 12 RETURN_VALUE @@ -437,8 +434,7 @@ class C: 22 ] assignment = ( - some.rather.elaborate.rule() - and another.rule.ending_with.index[123] + some.rather.elaborate.rule() and another.rule.ending_with.index[123] ) def easy_asserts(self) -> None: diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__empty_lines.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__empty_lines.py.snap index 598235141bf6e..4cfde0d1a6179 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__empty_lines.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__empty_lines.py.snap @@ -113,61 +113,15 @@ def g(): prev = leaf.prev_sibling if not prev: -@@ -25,23 +25,31 @@ - return NO - - if prevp.type == token.EQUAL: -- if prevp.parent and prevp.parent.type in { -- syms.typedargslist, -- syms.varargslist, -- syms.parameters, -- syms.arglist, -- syms.argument, -- }: -+ if ( -+ prevp.parent -+ and prevp.parent.type -+ in { -+ syms.typedargslist, -+ syms.varargslist, -+ syms.parameters, -+ syms.arglist, -+ syms.argument, -+ } -+ ): - return NO - - elif prevp.type == token.DOUBLESTAR: -- if prevp.parent and prevp.parent.type in { -- syms.typedargslist, -- syms.varargslist, -- syms.parameters, -- syms.arglist, -- syms.dictsetmaker, -- }: -+ if ( -+ prevp.parent -+ and prevp.parent.type -+ in { -+ syms.typedargslist, -+ syms.varargslist, -+ syms.parameters, -+ syms.arglist, -+ syms.dictsetmaker, -+ } -+ ): - return NO - - -@@ -49,7 +57,6 @@ +@@ -48,7 +48,6 @@ + ############################################################################### # SECTION BECAUSE SECTIONS ############################################################################### - - + def g(): NO = "" - SPACE = " " -@@ -67,7 +74,7 @@ +@@ -67,7 +66,7 @@ return DOUBLESPACE # Another comment because more comments @@ -176,29 +130,6 @@ def g(): prev = leaf.prev_sibling if not prev: -@@ -79,11 +86,15 @@ - return NO - - if prevp.type == token.EQUAL: -- if prevp.parent and prevp.parent.type in { -- syms.typedargslist, -- syms.varargslist, -- syms.parameters, -- syms.arglist, -- syms.argument, -- }: -+ if ( -+ prevp.parent -+ and prevp.parent.type -+ in { -+ syms.typedargslist, -+ syms.varargslist, -+ syms.parameters, -+ syms.arglist, -+ syms.argument, -+ } -+ ): - return NO ``` ## Ruff Output @@ -231,31 +162,23 @@ def f(): return NO if prevp.type == token.EQUAL: - if ( - prevp.parent - and prevp.parent.type - in { - syms.typedargslist, - syms.varargslist, - syms.parameters, - syms.arglist, - syms.argument, - } - ): + if prevp.parent and prevp.parent.type in { + syms.typedargslist, + syms.varargslist, + syms.parameters, + syms.arglist, + syms.argument, + }: return NO elif prevp.type == token.DOUBLESTAR: - if ( - prevp.parent - and prevp.parent.type - in { - syms.typedargslist, - syms.varargslist, - syms.parameters, - syms.arglist, - syms.dictsetmaker, - } - ): + if prevp.parent and prevp.parent.type in { + syms.typedargslist, + syms.varargslist, + syms.parameters, + syms.arglist, + syms.dictsetmaker, + }: return NO @@ -292,17 +215,13 @@ def g(): return NO if prevp.type == token.EQUAL: - if ( - prevp.parent - and prevp.parent.type - in { - syms.typedargslist, - syms.varargslist, - syms.parameters, - syms.arglist, - syms.argument, - } - ): + if prevp.parent and prevp.parent.type in { + syms.typedargslist, + syms.varargslist, + syms.parameters, + syms.arglist, + syms.argument, + }: return NO ``` diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__remove_await_parens.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__remove_await_parens.py.snap index d529f18f49088..2468093c5fa95 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__remove_await_parens.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__remove_await_parens.py.snap @@ -93,7 +93,7 @@ async def main(): ```diff --- Black +++ Ruff -@@ -8,59 +8,70 @@ +@@ -8,28 +8,33 @@ # Remove brackets for short coroutine/task async def main(): @@ -122,10 +122,8 @@ async def main(): async def main(): - await asyncio.sleep(1) # Hello -+ ( -+ await ( -+ asyncio.sleep(1) # Hello -+ ) ++ await ( ++ asyncio.sleep(1) # Hello + ) @@ -135,50 +133,7 @@ async def main(): # Long lines - async def main(): -- await asyncio.gather( -- asyncio.sleep(1), -- asyncio.sleep(1), -- asyncio.sleep(1), -- asyncio.sleep(1), -- asyncio.sleep(1), -- asyncio.sleep(1), -- asyncio.sleep(1), -+ ( -+ await asyncio.gather( -+ asyncio.sleep(1), -+ asyncio.sleep(1), -+ asyncio.sleep(1), -+ asyncio.sleep(1), -+ asyncio.sleep(1), -+ asyncio.sleep(1), -+ asyncio.sleep(1), -+ ) - ) - - - # Same as above but with magic trailing comma in function - async def main(): -- await asyncio.gather( -- asyncio.sleep(1), -- asyncio.sleep(1), -- asyncio.sleep(1), -- asyncio.sleep(1), -- asyncio.sleep(1), -- asyncio.sleep(1), -- asyncio.sleep(1), -+ ( -+ await asyncio.gather( -+ asyncio.sleep(1), -+ asyncio.sleep(1), -+ asyncio.sleep(1), -+ asyncio.sleep(1), -+ asyncio.sleep(1), -+ asyncio.sleep(1), -+ asyncio.sleep(1), -+ ) - ) - +@@ -60,7 +65,7 @@ # Cr@zY Br@ck3Tz async def main(): @@ -187,7 +142,7 @@ async def main(): # Keep brackets around non power operations and nested awaits -@@ -78,16 +89,16 @@ +@@ -78,16 +83,16 @@ async def main(): @@ -243,10 +198,8 @@ async def main(): async def main(): - ( - await ( - asyncio.sleep(1) # Hello - ) + await ( + asyncio.sleep(1) # Hello ) @@ -256,31 +209,27 @@ async def main(): # Long lines async def main(): - ( - await asyncio.gather( - asyncio.sleep(1), - asyncio.sleep(1), - asyncio.sleep(1), - asyncio.sleep(1), - asyncio.sleep(1), - asyncio.sleep(1), - asyncio.sleep(1), - ) + await asyncio.gather( + asyncio.sleep(1), + asyncio.sleep(1), + asyncio.sleep(1), + asyncio.sleep(1), + asyncio.sleep(1), + asyncio.sleep(1), + asyncio.sleep(1), ) # Same as above but with magic trailing comma in function async def main(): - ( - await asyncio.gather( - asyncio.sleep(1), - asyncio.sleep(1), - asyncio.sleep(1), - asyncio.sleep(1), - asyncio.sleep(1), - asyncio.sleep(1), - asyncio.sleep(1), - ) + await asyncio.gather( + asyncio.sleep(1), + asyncio.sleep(1), + asyncio.sleep(1), + asyncio.sleep(1), + asyncio.sleep(1), + asyncio.sleep(1), + asyncio.sleep(1), ) diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__torture.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__torture.py.snap index 246fcc69bc0ee..058819773d264 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__torture.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__torture.py.snap @@ -64,7 +64,7 @@ assert ( importA 0 -@@ -25,34 +15,33 @@ +@@ -25,9 +15,7 @@ class A: def foo(self): for _ in range(10): @@ -75,46 +75,7 @@ assert ( def test(self, othr): -- return 1 == 2 and ( -- name, -- description, -- self.default, -- self.selected, -- self.auto_generated, -- self.parameters, -- self.meta_data, -- self.schedule, -- ) == ( -- name, -- description, -- othr.default, -- othr.selected, -- othr.auto_generated, -- othr.parameters, -- othr.meta_data, -- othr.schedule, -+ return ( -+ 1 == 2 -+ and ( -+ name, -+ description, -+ self.default, -+ self.selected, -+ self.auto_generated, -+ self.parameters, -+ self.meta_data, -+ self.schedule, -+ ) -+ == ( -+ name, -+ description, -+ othr.default, -+ othr.selected, -+ othr.auto_generated, -+ othr.parameters, -+ othr.meta_data, -+ othr.schedule, -+ ) +@@ -52,7 +40,4 @@ ) @@ -149,28 +110,24 @@ class A: def test(self, othr): - return ( - 1 == 2 - and ( - name, - description, - self.default, - self.selected, - self.auto_generated, - self.parameters, - self.meta_data, - self.schedule, - ) - == ( - name, - description, - othr.default, - othr.selected, - othr.auto_generated, - othr.parameters, - othr.meta_data, - othr.schedule, - ) + return 1 == 2 and ( + name, + description, + self.default, + self.selected, + self.auto_generated, + self.parameters, + self.meta_data, + self.schedule, + ) == ( + name, + description, + othr.default, + othr.selected, + othr.auto_generated, + othr.parameters, + othr.meta_data, + othr.schedule, ) diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__trailing_comma_optional_parens1.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__trailing_comma_optional_parens1.py.snap index 35f9fc85aa1d2..7fb41572f30e8 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__trailing_comma_optional_parens1.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__trailing_comma_optional_parens1.py.snap @@ -37,17 +37,7 @@ class A: ```diff --- Black +++ Ruff -@@ -1,18 +1,16 @@ --if e1234123412341234.winerror not in ( -- _winapi.ERROR_SEM_TIMEOUT, -- _winapi.ERROR_PIPE_BUSY, --) or _check_timeout(t): -+if ( -+ e1234123412341234.winerror -+ not in (_winapi.ERROR_SEM_TIMEOUT, _winapi.ERROR_PIPE_BUSY) -+ or _check_timeout(t) -+): - pass +@@ -6,13 +6,10 @@ if x: if y: @@ -65,36 +55,15 @@ class A: class X: -@@ -26,9 +24,14 @@ - - class A: - def b(self): -- if self.connection.mysql_is_mariadb and ( -- 10, -- 4, -- 3, -- ) < self.connection.mysql_version < (10, 5, 2): -+ if ( -+ self.connection.mysql_is_mariadb -+ and ( -+ 10, -+ 4, -+ 3, -+ ) -+ < self.connection.mysql_version -+ < (10, 5, 2) -+ ): - pass ``` ## Ruff Output ```py -if ( - e1234123412341234.winerror - not in (_winapi.ERROR_SEM_TIMEOUT, _winapi.ERROR_PIPE_BUSY) - or _check_timeout(t) -): +if e1234123412341234.winerror not in ( + _winapi.ERROR_SEM_TIMEOUT, + _winapi.ERROR_PIPE_BUSY, +) or _check_timeout(t): pass if x: @@ -116,16 +85,11 @@ class X: class A: def b(self): - if ( - self.connection.mysql_is_mariadb - and ( - 10, - 4, - 3, - ) - < self.connection.mysql_version - < (10, 5, 2) - ): + if self.connection.mysql_is_mariadb and ( + 10, + 4, + 3, + ) < self.connection.mysql_version < (10, 5, 2): pass ``` diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__binary.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__binary.py.snap index 890bfd41cb613..9dea01bb94863 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__binary.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__binary.py.snap @@ -476,9 +476,9 @@ if ( # Unstable formatting in https://github.com/realtyem/synapse-unraid/blob/unraid_develop/synapse/handlers/presence.py -for user_id in set( - target_user_ids -) - {NOT_IMPLEMENTED_set_value for value in NOT_IMPLEMENTED_set}: +for user_id in ( + set(target_user_ids) - {NOT_IMPLEMENTED_set_value for value in NOT_IMPLEMENTED_set} +): updates.append(UserPresenceState.default(user_id)) # Keeps parenthesized left hand sides diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__compare.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__compare.py.snap index 03d75d057bf06..3e67442fe6eda 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__compare.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__compare.py.snap @@ -100,38 +100,29 @@ a < b > c == d == ddddddddddddddddddddd ) -( - aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa - < [ - bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb, - ff, - ] - < [ccccccccccccccccccccccccccccc, dddd] - < ddddddddddddddddddddddddddddddddddddddddddd -) +aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa < [ + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb, + ff, +] < [ccccccccccccccccccccccccccccc, dddd] < ddddddddddddddddddddddddddddddddddddddddddd -return ( - 1 == 2 - and ( - name, - description, - self_default, - self_selected, - self_auto_generated, - self_parameters, - self_meta_data, - self_schedule, - ) - == ( - name, - description, - othr_default, - othr_selected, - othr_auto_generated, - othr_parameters, - othr_meta_data, - othr_schedule, - ) +return 1 == 2 and ( + name, + description, + self_default, + self_selected, + self_auto_generated, + self_parameters, + self_meta_data, + self_schedule, +) == ( + name, + description, + othr_default, + othr_selected, + othr_auto_generated, + othr_parameters, + othr_meta_data, + othr_schedule, ) ( diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__unary.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__unary.py.snap index c6f1534b318fb..e0325c897bd0b 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__unary.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__unary.py.snap @@ -256,13 +256,10 @@ if aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa & ( ): pass -if ( - not ( - aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa - + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb - ) - & aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa -): +if not ( + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb +) & aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa: pass From d40b868398047cff1af7ed6b19f63ee0b5107feb Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Sun, 9 Jul 2023 10:41:58 +0200 Subject: [PATCH 02/10] Commit regressions --- crates/ruff_python_formatter/src/builders.rs | 13 +++- crates/ruff_python_formatter/src/lib.rs | 11 ++- ...patibility@simple_cases__comments2.py.snap | 12 +-- ...patibility@simple_cases__comments6.py.snap | 12 +-- ...atibility@simple_cases__expression.py.snap | 29 ++++--- ...ple_cases__function_trailing_comma.py.snap | 64 ++++++++------- ...s__trailing_comma_optional_parens2.py.snap | 52 ------------- ...s__trailing_comma_optional_parens3.py.snap | 77 +++++++++++++++++++ 8 files changed, 161 insertions(+), 109 deletions(-) delete mode 100644 crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__trailing_comma_optional_parens2.py.snap create mode 100644 crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__trailing_comma_optional_parens3.py.snap diff --git a/crates/ruff_python_formatter/src/builders.rs b/crates/ruff_python_formatter/src/builders.rs index 1cf0a03b40404..d39ff8a46dbde 100644 --- a/crates/ruff_python_formatter/src/builders.rs +++ b/crates/ruff_python_formatter/src/builders.rs @@ -21,12 +21,21 @@ pub(crate) struct OptionalParentheses<'a, 'ast> { impl<'ast> Format> for OptionalParentheses<'_, 'ast> { fn fmt(&self, f: &mut Formatter>) -> FormatResult<()> { - group(&format_args![ + let saved_level = f.context().node_level(); + + f.context_mut() + .set_node_level(NodeLevel::ParenthesizedExpression); + + let result = group(&format_args![ if_group_breaks(&text("(")), soft_block_indent(&Arguments::from(&self.inner)), if_group_breaks(&text(")")), ]) - .fmt(f) + .fmt(f); + + f.context_mut().set_node_level(saved_level); + + result } } diff --git a/crates/ruff_python_formatter/src/lib.rs b/crates/ruff_python_formatter/src/lib.rs index 1795480a98f57..5b569e1fda4dd 100644 --- a/crates/ruff_python_formatter/src/lib.rs +++ b/crates/ruff_python_formatter/src/lib.rs @@ -280,9 +280,14 @@ if True: #[test] fn quick_test() { let src = r#" -[ - AAAAAAAAAAAAA -] + SHARED_AAAAAAAAAAAAA + USER_AAAAAAAAAAAAA + AAAAAAAAAAAAA + AAAAAAAAAAAAAAAAASAAAAAAAA"#; +if True: + if True: + if True: + return _( + "qweasdzxcqweasdzxcqweasdzxcqweasdz" + + "qweasdzxcqweasdzxcqweasdzxcqweas", + "qweasdzxcqweasdzxcqweasdzxcqweasdz", + ) % {"reported_username": reported_username, "report_reason": report_reason}"#; // Tokenize once let mut tokens = Vec::new(); let mut comment_ranges = CommentRangesBuilder::default(); diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments2.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments2.py.snap index ef5229fdb1879..ac22aa3613e2a 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments2.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments2.py.snap @@ -260,7 +260,7 @@ instruction()#comment with bad spacing ] while True: if False: -@@ -143,7 +160,10 @@ +@@ -143,22 +160,25 @@ # let's return return Node( syms.simple_stmt, @@ -459,13 +459,9 @@ short ) -CONFIG_FILES = ( - [ - CONFIG_FILE, - ] - + SHARED_CONFIG_FILES - + USER_CONFIG_FILES -) # type: Final +CONFIG_FILES = [ + CONFIG_FILE, +] + SHARED_CONFIG_FILES + USER_CONFIG_FILES # type: Final class Test: diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments6.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments6.py.snap index 6be2a77bc13c3..979b67eb97e17 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments6.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments6.py.snap @@ -162,9 +162,9 @@ aaaaaaaaaaaaa, bbbbbbbbb = map(list, map(itertools.chain.from_iterable, zip(*ite +result = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" # aaa -AAAAAAAAAAAAA = [AAAAAAAAAAAAA] + SHARED_AAAAAAAAAAAAA + USER_AAAAAAAAAAAAA + AAAAAAAAAAAAA # type: ignore -+AAAAAAAAAAAAA = ( -+ [AAAAAAAAAAAAA] + SHARED_AAAAAAAAAAAAA + USER_AAAAAAAAAAAAA + AAAAAAAAAAAAA -+) # type: ignore ++AAAAAAAAAAAAA = [ ++ AAAAAAAAAAAAA ++] + SHARED_AAAAAAAAAAAAA + USER_AAAAAAAAAAAAA + AAAAAAAAAAAAA # type: ignore call_to_some_function_asdf( foo, @@ -295,9 +295,9 @@ def func( result = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" # aaa -AAAAAAAAAAAAA = ( - [AAAAAAAAAAAAA] + SHARED_AAAAAAAAAAAAA + USER_AAAAAAAAAAAAA + AAAAAAAAAAAAA -) # type: ignore +AAAAAAAAAAAAA = [ + AAAAAAAAAAAAA +] + SHARED_AAAAAAAAAAAAA + USER_AAAAAAAAAAAAA + AAAAAAAAAAAAA # type: ignore call_to_some_function_asdf( foo, 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 8fc2eb367f515..33d7af17ee9b1 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 @@ -451,7 +451,7 @@ last_call() - models.Customer.id.asc(), - ) - .all() --) + ) +result = session.query(models.Customer.id).filter( + models.Customer.account_id == account_id, models.Customer.email == email_address +).order_by(models.Customer.id.asc()).all() @@ -531,7 +531,7 @@ last_call() ^ aaaaaaaa.i << aaaaaaaa.k >> aaaaaaaa.l**aaaaaaaa.m // aaaaaaaa.n ): return True -@@ -342,7 +335,8 @@ +@@ -342,19 +335,16 @@ ~aaaaaaaaaaaaaaaa.a + aaaaaaaaaaaaaaaa.b - aaaaaaaaaaaaaaaa.c * aaaaaaaaaaaaaaaa.d @ aaaaaaaaaaaaaaaa.e @@ -541,6 +541,21 @@ last_call() ^ aaaaaaaaaaaaaaaa.i << aaaaaaaaaaaaaaaa.k >> aaaaaaaaaaaaaaaa.l**aaaaaaaaaaaaaaaa.m // aaaaaaaaaaaaaaaa.n + ): + return True +-( +- aaaaaaaaaaaaaaaa +- + aaaaaaaaaaaaaaaa +- - aaaaaaaaaaaaaaaa +- * (aaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaa) +- / (aaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaa) +-) ++aaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaa - aaaaaaaaaaaaaaaa * ( ++ aaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaa ++) / (aaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaa) + aaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaa + ( + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa ``` ## Ruff Output @@ -890,13 +905,9 @@ if ( >> aaaaaaaaaaaaaaaa.l**aaaaaaaaaaaaaaaa.m // aaaaaaaaaaaaaaaa.n ): return True -( - aaaaaaaaaaaaaaaa - + aaaaaaaaaaaaaaaa - - aaaaaaaaaaaaaaaa - * (aaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaa) - / (aaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaa) -) +aaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaa - aaaaaaaaaaaaaaaa * ( + aaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaa +) / (aaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaa) aaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaa ( aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__function_trailing_comma.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__function_trailing_comma.py.snap index fcb6ee100ba23..bfbf6fba905ea 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__function_trailing_comma.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__function_trailing_comma.py.snap @@ -73,26 +73,37 @@ some_module.some_function( ```diff --- Black +++ Ruff -@@ -35,7 +35,9 @@ +@@ -35,26 +35,25 @@ x = { "a": 1, "b": 2, - }["a"] +- if ( +- a +- == { +- "a": 1, +- "b": 2, +- "c": 3, +- "d": 4, +- "e": 5, +- "f": 6, +- "g": 7, +- "h": 8, +- }["a"] +- ): + }[ + "a" + ] - if ( - a - == { -@@ -47,14 +49,16 @@ - "f": 6, - "g": 7, - "h": 8, -- }["a"] -+ }[ -+ "a" -+ ] - ): ++ if a == { ++ "a": 1, ++ "b": 2, ++ "c": 3, ++ "d": 4, ++ "e": 5, ++ "f": 6, ++ "g": 7, ++ "h": 8, ++ }["a"]: pass @@ -105,7 +116,7 @@ some_module.some_function( json = { "k": { "k2": { -@@ -80,18 +84,14 @@ +@@ -80,18 +79,14 @@ pass @@ -173,21 +184,16 @@ def f( }[ "a" ] - if ( - a - == { - "a": 1, - "b": 2, - "c": 3, - "d": 4, - "e": 5, - "f": 6, - "g": 7, - "h": 8, - }[ - "a" - ] - ): + if a == { + "a": 1, + "b": 2, + "c": 3, + "d": 4, + "e": 5, + "f": 6, + "g": 7, + "h": 8, + }["a"]: pass diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__trailing_comma_optional_parens2.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__trailing_comma_optional_parens2.py.snap deleted file mode 100644 index bc155f8982bff..0000000000000 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__trailing_comma_optional_parens2.py.snap +++ /dev/null @@ -1,52 +0,0 @@ ---- -source: crates/ruff_python_formatter/tests/fixtures.rs -input_file: crates/ruff_python_formatter/resources/test/fixtures/black/simple_cases/trailing_comma_optional_parens2.py ---- -## Input - -```py -if (e123456.get_tk_patchlevel() >= (8, 6, 0, 'final') or - (8, 5, 8) <= get_tk_patchlevel() < (8, 6)): - pass -``` - -## Black Differences - -```diff ---- Black -+++ Ruff -@@ -1,6 +1,5 @@ --if e123456.get_tk_patchlevel() >= (8, 6, 0, "final") or ( -- 8, -- 5, -- 8, --) <= get_tk_patchlevel() < (8, 6): -+if ( -+ e123456.get_tk_patchlevel() >= (8, 6, 0, "final") -+ or (8, 5, 8) <= get_tk_patchlevel() < (8, 6) -+): - pass -``` - -## Ruff Output - -```py -if ( - e123456.get_tk_patchlevel() >= (8, 6, 0, "final") - or (8, 5, 8) <= get_tk_patchlevel() < (8, 6) -): - pass -``` - -## Black Output - -```py -if e123456.get_tk_patchlevel() >= (8, 6, 0, "final") or ( - 8, - 5, - 8, -) <= get_tk_patchlevel() < (8, 6): - pass -``` - - diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__trailing_comma_optional_parens3.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__trailing_comma_optional_parens3.py.snap new file mode 100644 index 0000000000000..d13dff5ee49a9 --- /dev/null +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__trailing_comma_optional_parens3.py.snap @@ -0,0 +1,77 @@ +--- +source: crates/ruff_python_formatter/tests/fixtures.rs +input_file: crates/ruff_python_formatter/resources/test/fixtures/black/simple_cases/trailing_comma_optional_parens3.py +--- +## Input + +```py +if True: + if True: + if True: + return _( + "qweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweas " + + "qweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqwegqweasdzxcqweasdzxc.", + "qweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqwe", + ) % {"reported_username": reported_username, "report_reason": report_reason} +``` + +## Black Differences + +```diff +--- Black ++++ Ruff +@@ -1,8 +1,14 @@ + if True: + if True: + if True: +- return _( +- "qweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweas " +- + "qweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqwegqweasdzxcqweasdzxc.", +- "qweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqwe", +- ) % {"reported_username": reported_username, "report_reason": report_reason} ++ return ( ++ _( ++ "qweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweas " ++ + "qweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqwegqweasdzxcqweasdzxc.", ++ "qweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqwe", ++ ) ++ % { ++ "reported_username": reported_username, ++ "report_reason": report_reason, ++ } ++ ) +``` + +## Ruff Output + +```py +if True: + if True: + if True: + return ( + _( + "qweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweas " + + "qweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqwegqweasdzxcqweasdzxc.", + "qweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqwe", + ) + % { + "reported_username": reported_username, + "report_reason": report_reason, + } + ) +``` + +## Black Output + +```py +if True: + if True: + if True: + return _( + "qweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweas " + + "qweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqwegqweasdzxcqweasdzxc.", + "qweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqwe", + ) % {"reported_username": reported_username, "report_reason": report_reason} +``` + + From eaa9491fa6313afabf2bbc5a512a7851bd1b2cb0 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Sun, 9 Jul 2023 10:43:28 +0200 Subject: [PATCH 03/10] Revert quicktest changes --- crates/ruff_python_formatter/src/lib.rs | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/crates/ruff_python_formatter/src/lib.rs b/crates/ruff_python_formatter/src/lib.rs index 5b569e1fda4dd..eebcf056640f2 100644 --- a/crates/ruff_python_formatter/src/lib.rs +++ b/crates/ruff_python_formatter/src/lib.rs @@ -280,14 +280,12 @@ if True: #[test] fn quick_test() { let src = r#" -if True: - if True: - if True: - return _( - "qweasdzxcqweasdzxcqweasdzxcqweasdz" - + "qweasdzxcqweasdzxcqweasdzxcqweas", - "qweasdzxcqweasdzxcqweasdzxcqweasdz", - ) % {"reported_username": reported_username, "report_reason": report_reason}"#; +if [ + aaaaaa, + BBBB,ccccccccc,ddddddd,eeeeeeeeee,ffffff +] & bbbbbbbbbbbbbbbbbbddddddddddddddddddddddddddddbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb: + ... +"#; // Tokenize once let mut tokens = Vec::new(); let mut comment_ranges = CommentRangesBuilder::default(); @@ -314,10 +312,10 @@ if True: // Uncomment the `dbg` to print the IR. // Use `dbg_write!(f, []) instead of `write!(f, [])` in your formatting code to print some IR // inside of a `Format` implementation - use ruff_formatter::FormatContext; - dbg!(formatted - .document() - .display(formatted.context().source_code())); + // use ruff_formatter::FormatContext; + // dbg!(formatted + // .document() + // .display(formatted.context().source_code())); // // dbg!(formatted // .context() From d90f4d447eb46f7591a2ae0873c86dac9769c7a8 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Sun, 9 Jul 2023 12:49:58 +0200 Subject: [PATCH 04/10] Add partial implementation of `can_omit_optional_parentheses` --- .../src/expression/expr_if_exp.rs | 12 +- .../src/expression/expr_subscript.rs | 30 +- .../src/expression/mod.rs | 273 ++++++++++++++++-- crates/ruff_python_formatter/src/lib.rs | 6 +- ...patibility@simple_cases__comments2.py.snap | 12 +- ...patibility@simple_cases__comments6.py.snap | 12 +- ...tibility@simple_cases__composition.py.snap | 41 +-- ...ses__composition_no_trailing_comma.py.snap | 41 +-- ...atibility@simple_cases__expression.py.snap | 29 +- ...ple_cases__function_trailing_comma.py.snap | 61 +--- ...__trailing_commas_in_leading_parts.py.snap | 22 +- .../snapshots/format@statement__raise.py.snap | 6 +- 12 files changed, 337 insertions(+), 208 deletions(-) diff --git a/crates/ruff_python_formatter/src/expression/expr_if_exp.rs b/crates/ruff_python_formatter/src/expression/expr_if_exp.rs index 0a2ffcf584f38..ae142e7122277 100644 --- a/crates/ruff_python_formatter/src/expression/expr_if_exp.rs +++ b/crates/ruff_python_formatter/src/expression/expr_if_exp.rs @@ -1,10 +1,11 @@ use crate::comments::{leading_comments, Comments}; use crate::expression::parentheses::{ - default_expression_needs_parentheses, NeedsParentheses, Parentheses, Parenthesize, + default_expression_needs_parentheses, in_parentheses_only_group, NeedsParentheses, Parentheses, + Parenthesize, }; -use crate::{AsFormat, FormatNodeRule, PyFormatter}; -use ruff_formatter::prelude::{group, soft_line_break_or_space, space, text}; -use ruff_formatter::{format_args, write, Buffer, FormatResult}; +use crate::prelude::*; +use crate::FormatNodeRule; +use ruff_formatter::{format_args, write}; use rustpython_parser::ast::ExprIfExp; #[derive(Default)] @@ -19,12 +20,13 @@ impl FormatNodeRule for FormatExprIfExp { orelse, } = item; let comments = f.context().comments().clone(); + // We place `if test` and `else orelse` on a single line, so the `test` and `orelse` leading // comments go on the line before the `if` or `else` instead of directly ahead `test` or // `orelse` write!( f, - [group(&format_args![ + [in_parentheses_only_group(&format_args![ body.format(), soft_line_break_or_space(), leading_comments(comments.leading_comments(test.as_ref())), diff --git a/crates/ruff_python_formatter/src/expression/expr_subscript.rs b/crates/ruff_python_formatter/src/expression/expr_subscript.rs index 55f8d22490f49..abdef84df7f72 100644 --- a/crates/ruff_python_formatter/src/expression/expr_subscript.rs +++ b/crates/ruff_python_formatter/src/expression/expr_subscript.rs @@ -1,12 +1,16 @@ +use rustpython_parser::ast::ExprSubscript; + +use ruff_formatter::{format_args, write}; +use ruff_python_ast::node::AstNode; + use crate::comments::{trailing_comments, Comments}; +use crate::context::NodeLevel; use crate::expression::parentheses::{ - default_expression_needs_parentheses, NeedsParentheses, Parentheses, Parenthesize, + default_expression_needs_parentheses, in_parentheses_only_group, NeedsParentheses, Parentheses, + Parenthesize, }; -use crate::{AsFormat, FormatNodeRule, PyFormatter}; -use ruff_formatter::prelude::{group, soft_block_indent, text}; -use ruff_formatter::{format_args, write, Buffer, FormatResult}; -use ruff_python_ast::node::AstNode; -use rustpython_parser::ast::ExprSubscript; +use crate::prelude::*; +use crate::FormatNodeRule; #[derive(Default)] pub struct FormatExprSubscript; @@ -27,10 +31,20 @@ impl FormatNodeRule for FormatExprSubscript { "The subscript expression must have at most a single comment, the one after the bracket" ); + if let NodeLevel::Expression(Some(group_id)) = f.context().node_level() { + // Enforce the optional parentheses for parenthesized values. + f.context_mut().set_node_level(NodeLevel::Expression(None)); + let result = value.format().fmt(f); + f.context_mut() + .set_node_level(NodeLevel::Expression(Some(group_id))); + result?; + } else { + value.format().fmt(f)?; + } + write!( f, - [group(&format_args![ - value.format(), + [in_parentheses_only_group(&format_args![ text("["), trailing_comments(dangling_comments), soft_block_indent(&slice.format()), diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index a9042fe27aaa8..8e4a8b97c9399 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -1,13 +1,20 @@ -use rustpython_parser::ast::Expr; +use rustpython_parser::ast; +use rustpython_parser::ast::{Expr, Operator}; +use std::cmp::Ordering; +use crate::builders::optional_parentheses; use ruff_formatter::{ format_args, FormatOwnedWithRule, FormatRefWithRule, FormatRule, FormatRuleWithOptions, }; +use ruff_python_ast::node::AnyNodeRef; +use ruff_python_ast::visitor::preorder::{walk_expr, PreorderVisitor}; use crate::comments::Comments; use crate::context::NodeLevel; use crate::expression::expr_tuple::TupleParentheses; -use crate::expression::parentheses::{parenthesized, NeedsParentheses, Parentheses, Parenthesize}; +use crate::expression::parentheses::{ + is_expression_parenthesized, parenthesized, NeedsParentheses, Parentheses, Parenthesize, +}; use crate::expression::string::StringLayout; use crate::prelude::*; @@ -103,28 +110,32 @@ impl FormatRule> for FormatExpr { Parentheses::Always => parenthesized("(", &format_expr, ")").fmt(f), // Add optional parentheses. Ignore if the item renders parentheses itself. Parentheses::Optional => { - let saved_level = f.context().node_level(); + if can_omit_optional_parentheses(item, f.context()) { + let saved_level = f.context().node_level(); - let parens_id = f.group_id("optional_parentheses"); + let parens_id = f.group_id("optional_parentheses"); - f.context_mut() - .set_node_level(NodeLevel::Expression(Some(parens_id))); + f.context_mut() + .set_node_level(NodeLevel::Expression(Some(parens_id))); - let result = group(&format_args![ - if_group_breaks(&text("(")), - indent_if_group_breaks( - &format_args![soft_line_break(), format_expr], - parens_id - ), - soft_line_break(), - if_group_breaks(&text(")")) - ]) - .with_group_id(Some(parens_id)) - .fmt(f); + let result = group(&format_args![ + if_group_breaks(&text("(")), + indent_if_group_breaks( + &format_args![soft_line_break(), format_expr], + parens_id + ), + soft_line_break(), + if_group_breaks(&text(")")) + ]) + .with_group_id(Some(parens_id)) + .fmt(f); - f.context_mut().set_node_level(saved_level); + f.context_mut().set_node_level(saved_level); - result + result + } else { + optional_parentheses(&format_expr).fmt(f) + } } Parentheses::Custom | Parentheses::Never => { let saved_level = f.context().node_level(); @@ -204,3 +215,227 @@ impl<'ast> IntoFormat> for Expr { FormatOwnedWithRule::new(self, FormatExpr::default()) } } + +/// 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 `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 +/// * The expression contains at least one parenthesized sub expression (optimization to avoid unnecessary work) +fn can_omit_optional_parentheses(expr: &Expr, context: &PyFormatContext) -> bool { + let mut visitor = MaxOperatorPriorityVisitor::new(context.contents()); + + visitor.visit_subexpression(expr); + + let (max_operator_priority, operation_count, any_parenthesized_expression) = visitor.finish(); + + if operation_count > 1 { + false + } else if max_operator_priority == OperatorPriority::Attribute { + true + } else { + // Only use the more complex IR when there is any expression that we can possibly split by + any_parenthesized_expression + } +} + +#[derive(Clone, Debug)] +struct MaxOperatorPriorityVisitor<'input> { + max_priority: OperatorPriority, + max_priority_count: u32, + any_parenthesized_expressions: bool, + source: &'input str, +} + +impl<'input> MaxOperatorPriorityVisitor<'input> { + fn new(source: &'input str) -> Self { + Self { + source, + max_priority: OperatorPriority::None, + max_priority_count: 0, + any_parenthesized_expressions: false, + } + } + + fn update_max_priority(&mut self, current_priority: OperatorPriority) { + match self.max_priority.cmp(¤t_priority) { + Ordering::Less => { + self.max_priority_count = 1; + self.max_priority = current_priority; + } + Ordering::Equal => { + self.max_priority_count += 1; + } + Ordering::Greater => {} + } + } + + // Visits a subexpression, ignoring whether it is parenthesized or not + fn visit_subexpression(&mut self, expr: &'input Expr) { + match expr { + Expr::Dict(_) | Expr::List(_) | Expr::Tuple(_) | Expr::Set(_) => { + self.any_parenthesized_expressions = true; + // The values are always parenthesized, don't visit. + return; + } + Expr::ListComp(_) | Expr::SetComp(_) | Expr::DictComp(_) => { + self.any_parenthesized_expressions = true; + self.update_max_priority(OperatorPriority::Comprehension); + return; + } + Expr::BoolOp(ast::ExprBoolOp { + range: _, + op: _, + values: _, + }) => self.update_max_priority(OperatorPriority::BooleanOperation), + Expr::BinOp(ast::ExprBinOp { + op, + left: _, + right: _, + range: _, + }) => self.update_max_priority(OperatorPriority::from(*op)), + + Expr::IfExp(ast::ExprIfExp { + range: _, + test, + body: _, + orelse: _, + }) => { + self.update_max_priority(OperatorPriority::Conditional); + + // Nested if else expressions are always parenthesized. Ignore parentheses in this case + if let Expr::IfExp(_) = test.as_ref() { + self.update_max_priority(OperatorPriority::Conditional); + } + } + + Expr::Compare(_) => self.update_max_priority(OperatorPriority::Comparator), + Expr::Call(ast::ExprCall { + range: _, + func, + args: _, + keywords: _, + }) => { + self.any_parenthesized_expressions = true; + // Only walk the function, the arguments are always parenthesized + self.visit_expr(func); + return; + } + Expr::Subscript(_) => { + // Don't walk the value. Splitting before the value looks weird. + // Don't walk the slice, because the slice is always parenthesized. + return; + } + Expr::UnaryOp(ast::ExprUnaryOp { + range: _, + op, + operand: _, + }) => { + if op.is_invert() { + self.update_max_priority(OperatorPriority::BitwiseInversion); + } + } + + // `[a, b].test[300].dot` + Expr::Attribute(ast::ExprAttribute { + range: _, + value, + attr: _, + ctx: _, + }) => { + if has_parentheses(&value, self.source) { + self.update_max_priority(OperatorPriority::Attribute); + } + } + + Expr::NamedExpr(_) + | Expr::GeneratorExp(_) + | Expr::Lambda(_) + | Expr::Await(_) + | Expr::Yield(_) + | Expr::YieldFrom(_) + | Expr::FormattedValue(_) + | Expr::JoinedStr(_) + | Expr::Constant(_) + | Expr::Starred(_) + | Expr::Name(_) + | Expr::Slice(_) => {} + }; + + walk_expr(self, expr); + } + + fn finish(self) -> (OperatorPriority, u32, bool) { + ( + self.max_priority, + self.max_priority_count, + self.any_parenthesized_expressions, + ) + } +} + +impl<'input> PreorderVisitor<'input> for MaxOperatorPriorityVisitor<'input> { + fn visit_expr(&mut self, expr: &'input Expr) { + // Rule only applies for non-parenthesized expressions. + if is_expression_parenthesized(AnyNodeRef::from(expr), self.source) { + self.any_parenthesized_expressions = true; + } else { + self.visit_subexpression(expr); + } + } +} + +fn has_parentheses(expr: &Expr, source: &str) -> bool { + matches!( + expr, + Expr::Dict(_) + | Expr::List(_) + | Expr::Tuple(_) + | Expr::Set(_) + | Expr::ListComp(_) + | Expr::SetComp(_) + | Expr::DictComp(_) + | Expr::Call(_) + | Expr::Subscript(_) + ) || is_expression_parenthesized(AnyNodeRef::from(expr), source) +} + +#[derive(Debug, Copy, Clone, Ord, PartialOrd, Eq, PartialEq)] +enum OperatorPriority { + None, + Attribute, + Comparator, + Exponential, + BitwiseInversion, + Multiplicative, + Additive, + Shift, + BitwiseAnd, + BitwiseOr, + BitwiseXor, + // TODO(micha) + #[allow(unused)] + String, + BooleanOperation, + Conditional, + Comprehension, +} + +impl From for OperatorPriority { + fn from(value: Operator) -> Self { + match value { + Operator::Add | Operator::Sub => OperatorPriority::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, + } + } +} diff --git a/crates/ruff_python_formatter/src/lib.rs b/crates/ruff_python_formatter/src/lib.rs index eebcf056640f2..9ee89e2679d25 100644 --- a/crates/ruff_python_formatter/src/lib.rs +++ b/crates/ruff_python_formatter/src/lib.rs @@ -280,11 +280,7 @@ if True: #[test] fn quick_test() { let src = r#" -if [ - aaaaaa, - BBBB,ccccccccc,ddddddd,eeeeeeeeee,ffffff -] & bbbbbbbbbbbbbbbbbbddddddddddddddddddddddddddddbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb: - ... +CONFIG_FILES = [CONFIG_FILE, ] + SHARED_CONFIG_FILES + USER_CONFIG_FILES # type: Final "#; // Tokenize once let mut tokens = Vec::new(); diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments2.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments2.py.snap index ac22aa3613e2a..ef5229fdb1879 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments2.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments2.py.snap @@ -260,7 +260,7 @@ instruction()#comment with bad spacing ] while True: if False: -@@ -143,22 +160,25 @@ +@@ -143,7 +160,10 @@ # let's return return Node( syms.simple_stmt, @@ -459,9 +459,13 @@ short ) -CONFIG_FILES = [ - CONFIG_FILE, -] + SHARED_CONFIG_FILES + USER_CONFIG_FILES # type: Final +CONFIG_FILES = ( + [ + CONFIG_FILE, + ] + + SHARED_CONFIG_FILES + + USER_CONFIG_FILES +) # type: Final class Test: diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments6.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments6.py.snap index 979b67eb97e17..6be2a77bc13c3 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments6.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments6.py.snap @@ -162,9 +162,9 @@ aaaaaaaaaaaaa, bbbbbbbbb = map(list, map(itertools.chain.from_iterable, zip(*ite +result = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" # aaa -AAAAAAAAAAAAA = [AAAAAAAAAAAAA] + SHARED_AAAAAAAAAAAAA + USER_AAAAAAAAAAAAA + AAAAAAAAAAAAA # type: ignore -+AAAAAAAAAAAAA = [ -+ AAAAAAAAAAAAA -+] + SHARED_AAAAAAAAAAAAA + USER_AAAAAAAAAAAAA + AAAAAAAAAAAAA # type: ignore ++AAAAAAAAAAAAA = ( ++ [AAAAAAAAAAAAA] + SHARED_AAAAAAAAAAAAA + USER_AAAAAAAAAAAAA + AAAAAAAAAAAAA ++) # type: ignore call_to_some_function_asdf( foo, @@ -295,9 +295,9 @@ def func( result = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" # aaa -AAAAAAAAAAAAA = [ - AAAAAAAAAAAAA -] + SHARED_AAAAAAAAAAAAA + USER_AAAAAAAAAAAAA + AAAAAAAAAAAAA # type: ignore +AAAAAAAAAAAAA = ( + [AAAAAAAAAAAAA] + SHARED_AAAAAAAAAAAAA + USER_AAAAAAAAAAAAA + AAAAAAAAAAAAA +) # type: ignore call_to_some_function_asdf( foo, diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__composition.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__composition.py.snap index d6de11c9cae1f..71b76f27cd760 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__composition.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__composition.py.snap @@ -203,30 +203,7 @@ class C: print(i) xxxxxxxxxxxxxxxx = Yyyy2YyyyyYyyyyy( push_manager=context.request.resource_manager, -@@ -47,10 +47,20 @@ - def omitting_trailers(self) -> None: - get_collection( - hey_this_is_a_very_long_call, it_has_funny_attributes, really=True -- )[OneLevelIndex] -+ )[ -+ OneLevelIndex -+ ] - get_collection( - hey_this_is_a_very_long_call, it_has_funny_attributes, really=True -- )[OneLevelIndex][TwoLevelIndex][ThreeLevelIndex][FourLevelIndex] -+ )[ -+ OneLevelIndex -+ ][ -+ TwoLevelIndex -+ ][ -+ ThreeLevelIndex -+ ][ -+ FourLevelIndex -+ ] - d[0][1][2][3][4][5][6][7][8][9][10][11][12][13][14][15][16][17][18][19][20][21][ - 22 - ] -@@ -59,101 +69,23 @@ +@@ -59,101 +59,23 @@ ) def easy_asserts(self) -> None: @@ -337,7 +314,7 @@ class C: %3d 0 LOAD_FAST 1 (x) 2 LOAD_CONST 1 (1) 4 COMPARE_OP 2 (==) -@@ -161,21 +93,8 @@ +@@ -161,21 +83,8 @@ 8 STORE_ATTR 0 (x) 10 LOAD_CONST 0 (None) 12 RETURN_VALUE @@ -416,20 +393,10 @@ class C: def omitting_trailers(self) -> None: get_collection( hey_this_is_a_very_long_call, it_has_funny_attributes, really=True - )[ - OneLevelIndex - ] + )[OneLevelIndex] get_collection( hey_this_is_a_very_long_call, it_has_funny_attributes, really=True - )[ - OneLevelIndex - ][ - TwoLevelIndex - ][ - ThreeLevelIndex - ][ - FourLevelIndex - ] + )[OneLevelIndex][TwoLevelIndex][ThreeLevelIndex][FourLevelIndex] d[0][1][2][3][4][5][6][7][8][9][10][11][12][13][14][15][16][17][18][19][20][21][ 22 ] diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__composition_no_trailing_comma.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__composition_no_trailing_comma.py.snap index d4ca2fcc125c1..975059e7a0062 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__composition_no_trailing_comma.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__composition_no_trailing_comma.py.snap @@ -203,30 +203,7 @@ class C: print(i) xxxxxxxxxxxxxxxx = Yyyy2YyyyyYyyyyy( push_manager=context.request.resource_manager, -@@ -47,10 +47,20 @@ - def omitting_trailers(self) -> None: - get_collection( - hey_this_is_a_very_long_call, it_has_funny_attributes, really=True -- )[OneLevelIndex] -+ )[ -+ OneLevelIndex -+ ] - get_collection( - hey_this_is_a_very_long_call, it_has_funny_attributes, really=True -- )[OneLevelIndex][TwoLevelIndex][ThreeLevelIndex][FourLevelIndex] -+ )[ -+ OneLevelIndex -+ ][ -+ TwoLevelIndex -+ ][ -+ ThreeLevelIndex -+ ][ -+ FourLevelIndex -+ ] - d[0][1][2][3][4][5][6][7][8][9][10][11][12][13][14][15][16][17][18][19][20][21][ - 22 - ] -@@ -59,101 +69,23 @@ +@@ -59,101 +59,23 @@ ) def easy_asserts(self) -> None: @@ -337,7 +314,7 @@ class C: %3d 0 LOAD_FAST 1 (x) 2 LOAD_CONST 1 (1) 4 COMPARE_OP 2 (==) -@@ -161,21 +93,8 @@ +@@ -161,21 +83,8 @@ 8 STORE_ATTR 0 (x) 10 LOAD_CONST 0 (None) 12 RETURN_VALUE @@ -416,20 +393,10 @@ class C: def omitting_trailers(self) -> None: get_collection( hey_this_is_a_very_long_call, it_has_funny_attributes, really=True - )[ - OneLevelIndex - ] + )[OneLevelIndex] get_collection( hey_this_is_a_very_long_call, it_has_funny_attributes, really=True - )[ - OneLevelIndex - ][ - TwoLevelIndex - ][ - ThreeLevelIndex - ][ - FourLevelIndex - ] + )[OneLevelIndex][TwoLevelIndex][ThreeLevelIndex][FourLevelIndex] d[0][1][2][3][4][5][6][7][8][9][10][11][12][13][14][15][16][17][18][19][20][21][ 22 ] 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 33d7af17ee9b1..8fc2eb367f515 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 @@ -451,7 +451,7 @@ last_call() - models.Customer.id.asc(), - ) - .all() - ) +-) +result = session.query(models.Customer.id).filter( + models.Customer.account_id == account_id, models.Customer.email == email_address +).order_by(models.Customer.id.asc()).all() @@ -531,7 +531,7 @@ last_call() ^ aaaaaaaa.i << aaaaaaaa.k >> aaaaaaaa.l**aaaaaaaa.m // aaaaaaaa.n ): return True -@@ -342,19 +335,16 @@ +@@ -342,7 +335,8 @@ ~aaaaaaaaaaaaaaaa.a + aaaaaaaaaaaaaaaa.b - aaaaaaaaaaaaaaaa.c * aaaaaaaaaaaaaaaa.d @ aaaaaaaaaaaaaaaa.e @@ -541,21 +541,6 @@ last_call() ^ aaaaaaaaaaaaaaaa.i << aaaaaaaaaaaaaaaa.k >> aaaaaaaaaaaaaaaa.l**aaaaaaaaaaaaaaaa.m // aaaaaaaaaaaaaaaa.n - ): - return True --( -- aaaaaaaaaaaaaaaa -- + aaaaaaaaaaaaaaaa -- - aaaaaaaaaaaaaaaa -- * (aaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaa) -- / (aaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaa) --) -+aaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaa - aaaaaaaaaaaaaaaa * ( -+ aaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaa -+) / (aaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaa) - aaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaa - ( - aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa ``` ## Ruff Output @@ -905,9 +890,13 @@ if ( >> aaaaaaaaaaaaaaaa.l**aaaaaaaaaaaaaaaa.m // aaaaaaaaaaaaaaaa.n ): return True -aaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaa - aaaaaaaaaaaaaaaa * ( - aaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaa -) / (aaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaa) +( + aaaaaaaaaaaaaaaa + + aaaaaaaaaaaaaaaa + - aaaaaaaaaaaaaaaa + * (aaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaa) + / (aaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaa) +) aaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaa ( aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__function_trailing_comma.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__function_trailing_comma.py.snap index bfbf6fba905ea..388d0c3d4e82e 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__function_trailing_comma.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__function_trailing_comma.py.snap @@ -73,37 +73,7 @@ some_module.some_function( ```diff --- Black +++ Ruff -@@ -35,26 +35,25 @@ - x = { - "a": 1, - "b": 2, -- }["a"] -- if ( -- a -- == { -- "a": 1, -- "b": 2, -- "c": 3, -- "d": 4, -- "e": 5, -- "f": 6, -- "g": 7, -- "h": 8, -- }["a"] -- ): -+ }[ -+ "a" -+ ] -+ if a == { -+ "a": 1, -+ "b": 2, -+ "c": 3, -+ "d": 4, -+ "e": 5, -+ "f": 6, -+ "g": 7, -+ "h": 8, -+ }["a"]: +@@ -52,9 +52,9 @@ pass @@ -116,7 +86,7 @@ some_module.some_function( json = { "k": { "k2": { -@@ -80,18 +79,14 @@ +@@ -80,18 +80,14 @@ pass @@ -181,19 +151,20 @@ def f( x = { "a": 1, "b": 2, - }[ - "a" - ] - if a == { - "a": 1, - "b": 2, - "c": 3, - "d": 4, - "e": 5, - "f": 6, - "g": 7, - "h": 8, - }["a"]: + }["a"] + if ( + a + == { + "a": 1, + "b": 2, + "c": 3, + "d": 4, + "e": 5, + "f": 6, + "g": 7, + "h": 8, + }["a"] + ): pass diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__trailing_commas_in_leading_parts.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__trailing_commas_in_leading_parts.py.snap index b50a6c5ff75a6..5084e5caa424c 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__trailing_commas_in_leading_parts.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__trailing_commas_in_leading_parts.py.snap @@ -56,19 +56,7 @@ assert xxxxxxxxx.xxxxxxxxx.xxxxxxxxx( # Example from https://github.com/psf/black/issues/3229 -@@ -32,19 +30,18 @@ - "refreshToken": refresh_token, - }, - api_key=api_key, -- )["extensions"]["sdk"]["token"] -+ )[ -+ "extensions" -+ ][ -+ "sdk" -+ ][ -+ "token" -+ ] - +@@ -37,14 +35,7 @@ # Edge case where a bug in a working-in-progress version of # https://github.com/psf/black/pull/3370 causes an infinite recursion. @@ -122,13 +110,7 @@ def refresh_token(self, device_family, refresh_token, api_key): "refreshToken": refresh_token, }, api_key=api_key, - )[ - "extensions" - ][ - "sdk" - ][ - "token" - ] + )["extensions"]["sdk"]["token"] # Edge case where a bug in a working-in-progress version of diff --git a/crates/ruff_python_formatter/tests/snapshots/format@statement__raise.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@statement__raise.py.snap index d36d48fbfb1b6..f9e18087589d4 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@statement__raise.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@statement__raise.py.snap @@ -143,8 +143,10 @@ raise ( + cccccccccccccccccccccc + ddddddddddddddddddddddddd ) -raise aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbb + ( - cccccccccccccccccccccc + ddddddddddddddddddddddddd +raise ( + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + + bbbbbbbbbbbbbbbbbbbbbbbbbb + + (cccccccccccccccccccccc + ddddddddddddddddddddddddd) ) raise ( aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa From f344531fe1b9f36f91fcc59018a8cf4b4522de0d Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Mon, 10 Jul 2023 11:32:18 +0200 Subject: [PATCH 05/10] Fix boolean and compare op count --- .../src/expression/mod.rs | 24 +++++++++++++++---- crates/ruff_python_formatter/src/lib.rs | 9 ++++++- .../format@expression__compare.py.snap | 13 ++++++---- 3 files changed, 36 insertions(+), 10 deletions(-) diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index 8e4a8b97c9399..0c43db5868bc5 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -259,13 +259,17 @@ impl<'input> MaxOperatorPriorityVisitor<'input> { } fn update_max_priority(&mut self, current_priority: OperatorPriority) { + self.update_max_priority_with_count(current_priority, 1) + } + + fn update_max_priority_with_count(&mut self, current_priority: OperatorPriority, count: u32) { match self.max_priority.cmp(¤t_priority) { Ordering::Less => { - self.max_priority_count = 1; + self.max_priority_count = count; self.max_priority = current_priority; } Ordering::Equal => { - self.max_priority_count += 1; + self.max_priority_count += count; } Ordering::Greater => {} } @@ -287,8 +291,11 @@ impl<'input> MaxOperatorPriorityVisitor<'input> { Expr::BoolOp(ast::ExprBoolOp { range: _, op: _, - values: _, - }) => self.update_max_priority(OperatorPriority::BooleanOperation), + values, + }) => self.update_max_priority_with_count( + OperatorPriority::BooleanOperation, + values.len().saturating_sub(1) as u32, + ), Expr::BinOp(ast::ExprBinOp { op, left: _, @@ -310,7 +317,14 @@ impl<'input> MaxOperatorPriorityVisitor<'input> { } } - Expr::Compare(_) => self.update_max_priority(OperatorPriority::Comparator), + Expr::Compare(ast::ExprCompare { + range: _, + left: _, + ops, + comparators: _, + }) => { + self.update_max_priority_with_count(OperatorPriority::Comparator, ops.len() as u32) + } Expr::Call(ast::ExprCall { range: _, func, diff --git a/crates/ruff_python_formatter/src/lib.rs b/crates/ruff_python_formatter/src/lib.rs index 9ee89e2679d25..611b5c4735b1e 100644 --- a/crates/ruff_python_formatter/src/lib.rs +++ b/crates/ruff_python_formatter/src/lib.rs @@ -280,7 +280,14 @@ if True: #[test] fn quick_test() { let src = r#" -CONFIG_FILES = [CONFIG_FILE, ] + SHARED_CONFIG_FILES + USER_CONFIG_FILES # type: Final + +if ( + field.is_relation + and not field.many_to_many + and hasattr(field, "attname") + and field.attname == name +): + pass "#; // Tokenize once let mut tokens = Vec::new(); diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__compare.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__compare.py.snap index 3e67442fe6eda..9535adb8a0c00 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__compare.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__compare.py.snap @@ -100,10 +100,15 @@ a < b > c == d == ddddddddddddddddddddd ) -aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa < [ - bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb, - ff, -] < [ccccccccccccccccccccccccccccc, dddd] < ddddddddddddddddddddddddddddddddddddddddddd +( + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + < [ + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb, + ff, + ] + < [ccccccccccccccccccccccccccccc, dddd] + < ddddddddddddddddddddddddddddddddddddddddddd +) return 1 == 2 and ( name, From 2194dae160d46c4c8383a572525a4541abb4cb27 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Mon, 10 Jul 2023 11:50:30 +0200 Subject: [PATCH 06/10] Count conditional expressions as two operations --- .../ruff_python_formatter/src/expression/mod.rs | 15 +++------------ crates/ruff_python_formatter/src/lib.rs | 12 +++++------- 2 files changed, 8 insertions(+), 19 deletions(-) diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index 0c43db5868bc5..171a61a3b8c42 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -303,18 +303,9 @@ impl<'input> MaxOperatorPriorityVisitor<'input> { range: _, }) => self.update_max_priority(OperatorPriority::from(*op)), - Expr::IfExp(ast::ExprIfExp { - range: _, - test, - body: _, - orelse: _, - }) => { - self.update_max_priority(OperatorPriority::Conditional); - - // Nested if else expressions are always parenthesized. Ignore parentheses in this case - if let Expr::IfExp(_) = test.as_ref() { - self.update_max_priority(OperatorPriority::Conditional); - } + Expr::IfExp(_) => { + // + 1 for the if and one for the else + self.update_max_priority_with_count(OperatorPriority::Conditional, 2); } Expr::Compare(ast::ExprCompare { diff --git a/crates/ruff_python_formatter/src/lib.rs b/crates/ruff_python_formatter/src/lib.rs index 611b5c4735b1e..0a03c2f0440a9 100644 --- a/crates/ruff_python_formatter/src/lib.rs +++ b/crates/ruff_python_formatter/src/lib.rs @@ -281,13 +281,11 @@ if True: fn quick_test() { let src = r#" -if ( - field.is_relation - and not field.many_to_many - and hasattr(field, "attname") - and field.attname == name -): - pass +def test3(): + if True: + field = ( + model._meta.pk if from_field is None else model._meta.get_field(from_field) + ) "#; // Tokenize once let mut tokens = Vec::new(); From 1ffaf261e4608f25ccd608dc38e54c1b99f9439e Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Mon, 10 Jul 2023 13:14:41 +0200 Subject: [PATCH 07/10] Always use `group` for subscript --- crates/ruff_python_formatter/src/expression/expr_subscript.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff_python_formatter/src/expression/expr_subscript.rs b/crates/ruff_python_formatter/src/expression/expr_subscript.rs index abdef84df7f72..e880d69e7af76 100644 --- a/crates/ruff_python_formatter/src/expression/expr_subscript.rs +++ b/crates/ruff_python_formatter/src/expression/expr_subscript.rs @@ -44,7 +44,7 @@ impl FormatNodeRule for FormatExprSubscript { write!( f, - [in_parentheses_only_group(&format_args![ + [group(&format_args![ text("["), trailing_comments(dangling_comments), soft_block_indent(&slice.format()), From d08a62ae6e451353cf08807b564cbc69a2e8064e Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Mon, 10 Jul 2023 13:18:50 +0200 Subject: [PATCH 08/10] Add some documentation --- crates/ruff_python_formatter/src/expression/parentheses.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/crates/ruff_python_formatter/src/expression/parentheses.rs b/crates/ruff_python_formatter/src/expression/parentheses.rs index a3087eebe43cd..670293e744eb6 100644 --- a/crates/ruff_python_formatter/src/expression/parentheses.rs +++ b/crates/ruff_python_formatter/src/expression/parentheses.rs @@ -124,6 +124,9 @@ pub(crate) fn is_expression_parenthesized(expr: AnyNodeRef, contents: &str) -> b ) } +/// Formats `content` enclosed by the `left` and `right` parentheses. The implementation also ensures +/// that expanding the parenthesized expression (or any of its children) doesn't enforce the +/// optional parentheses around the outer-most expression to materialize. pub(crate) fn parenthesized<'content, 'ast, Content>( left: &'static str, content: &'content Content, @@ -175,6 +178,8 @@ impl<'ast> Format> for FormatParenthesized<'_, 'ast> { } } +/// Makes `content` a group, but only if the outer expression is parenthesized (a list, parenthesized expression, dict, ...) +/// or if the expression gets parenthesized because it expands over multiple lines. pub(crate) fn in_parentheses_only_group<'content, 'ast, Content>( content: &'content Content, ) -> FormatInParenthesesOnlyGroup<'content, 'ast> From 6ef1b8441d61f95d9c42c33931fd43eafe43ffa5 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Mon, 10 Jul 2023 13:41:47 +0200 Subject: [PATCH 09/10] Clippy, restore `lib.rs` --- .../src/expression/expr_subscript.rs | 3 +-- crates/ruff_python_formatter/src/expression/mod.rs | 12 +++++++++--- crates/ruff_python_formatter/src/lib.rs | 11 +++++------ 3 files changed, 15 insertions(+), 11 deletions(-) diff --git a/crates/ruff_python_formatter/src/expression/expr_subscript.rs b/crates/ruff_python_formatter/src/expression/expr_subscript.rs index e880d69e7af76..7010af60d6ff1 100644 --- a/crates/ruff_python_formatter/src/expression/expr_subscript.rs +++ b/crates/ruff_python_formatter/src/expression/expr_subscript.rs @@ -6,8 +6,7 @@ use ruff_python_ast::node::AstNode; use crate::comments::{trailing_comments, Comments}; use crate::context::NodeLevel; use crate::expression::parentheses::{ - default_expression_needs_parentheses, in_parentheses_only_group, NeedsParentheses, Parentheses, - Parenthesize, + default_expression_needs_parentheses, NeedsParentheses, Parentheses, Parenthesize, }; use crate::prelude::*; use crate::FormatNodeRule; diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index 171a61a3b8c42..0178f0e9b4a8b 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -259,7 +259,7 @@ impl<'input> MaxOperatorPriorityVisitor<'input> { } fn update_max_priority(&mut self, current_priority: OperatorPriority) { - self.update_max_priority_with_count(current_priority, 1) + self.update_max_priority_with_count(current_priority, 1); } fn update_max_priority_with_count(&mut self, current_priority: OperatorPriority, count: u32) { @@ -288,6 +288,9 @@ impl<'input> MaxOperatorPriorityVisitor<'input> { self.update_max_priority(OperatorPriority::Comprehension); return; } + // It's impossible for a file smaller or equal to 4GB to contain more than 2^32 comparisons + // because each comparison requires a left operand, and `n` `operands` and right sides. + #[allow(clippy::cast_possible_truncation)] Expr::BoolOp(ast::ExprBoolOp { range: _, op: _, @@ -308,13 +311,16 @@ impl<'input> MaxOperatorPriorityVisitor<'input> { self.update_max_priority_with_count(OperatorPriority::Conditional, 2); } + // It's impossible for a file smaller or equal to 4GB to contain more than 2^32 comparisons + // because each comparison requires a left operand, and `n` `operands` and right sides. + #[allow(clippy::cast_possible_truncation)] Expr::Compare(ast::ExprCompare { range: _, left: _, ops, comparators: _, }) => { - self.update_max_priority_with_count(OperatorPriority::Comparator, ops.len() as u32) + self.update_max_priority_with_count(OperatorPriority::Comparator, ops.len() as u32); } Expr::Call(ast::ExprCall { range: _, @@ -349,7 +355,7 @@ impl<'input> MaxOperatorPriorityVisitor<'input> { attr: _, ctx: _, }) => { - if has_parentheses(&value, self.source) { + if has_parentheses(value, self.source) { self.update_max_priority(OperatorPriority::Attribute); } } diff --git a/crates/ruff_python_formatter/src/lib.rs b/crates/ruff_python_formatter/src/lib.rs index 0a03c2f0440a9..eebcf056640f2 100644 --- a/crates/ruff_python_formatter/src/lib.rs +++ b/crates/ruff_python_formatter/src/lib.rs @@ -280,12 +280,11 @@ if True: #[test] fn quick_test() { let src = r#" - -def test3(): - if True: - field = ( - model._meta.pk if from_field is None else model._meta.get_field(from_field) - ) +if [ + aaaaaa, + BBBB,ccccccccc,ddddddd,eeeeeeeeee,ffffff +] & bbbbbbbbbbbbbbbbbbddddddddddddddddddddddddddddbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb: + ... "#; // Tokenize once let mut tokens = Vec::new(); From 830edb6576a75e609054a7a4a92f4fb15535dac6 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Tue, 11 Jul 2023 13:27:45 +0200 Subject: [PATCH 10/10] Code review feedback --- crates/ruff_python_formatter/src/expression/mod.rs | 9 +++++++++ .../ruff_python_formatter/src/expression/parentheses.rs | 6 ++++++ 2 files changed, 15 insertions(+) diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index 0178f0e9b4a8b..cc395311de954 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -113,11 +113,18 @@ impl FormatRule> for FormatExpr { if can_omit_optional_parentheses(item, f.context()) { let saved_level = f.context().node_level(); + // The group id is used as a condition in [`in_parentheses_only`] to create a conditional group + // that is only active if the optional parentheses group expands. let parens_id = f.group_id("optional_parentheses"); f.context_mut() .set_node_level(NodeLevel::Expression(Some(parens_id))); + // We can't use `soft_block_indent` here because that would always increment the indent, + // even if the group does not break (the indent is not soft). This would result in + // too deep indentations if a `parenthesized` group expands. Using `indent_if_group_breaks` + // gives us the desired *soft* indentation that is only present if the optional parentheses + // are shown. let result = group(&format_args![ if_group_breaks(&text("(")), indent_if_group_breaks( @@ -223,6 +230,8 @@ impl<'ast> IntoFormat> for Expr { /// 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 /// * 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) fn can_omit_optional_parentheses(expr: &Expr, context: &PyFormatContext) -> bool { let mut visitor = MaxOperatorPriorityVisitor::new(context.contents()); diff --git a/crates/ruff_python_formatter/src/expression/parentheses.rs b/crates/ruff_python_formatter/src/expression/parentheses.rs index 670293e744eb6..444697213f698 100644 --- a/crates/ruff_python_formatter/src/expression/parentheses.rs +++ b/crates/ruff_python_formatter/src/expression/parentheses.rs @@ -165,10 +165,13 @@ impl<'ast> Format> for FormatParenthesized<'_, 'ast> { .set_node_level(NodeLevel::ParenthesizedExpression); let result = if let NodeLevel::Expression(Some(group_id)) = current_level { + // Use fits expanded if there's an enclosing group that adds the optional parentheses. + // This ensures that expanding this parenthesized expression does not expand the optional parentheses group. fits_expanded(&inner) .with_condition(Some(Condition::if_group_fits_on_line(group_id))) .fmt(f) } else { + // It's not necessary to wrap the content if it is not inside of an optional_parentheses group. inner.fmt(f) }; @@ -198,12 +201,15 @@ pub(crate) struct FormatInParenthesesOnlyGroup<'content, 'ast> { impl<'ast> Format> for FormatInParenthesesOnlyGroup<'_, 'ast> { fn fmt(&self, f: &mut Formatter>) -> FormatResult<()> { if let NodeLevel::Expression(Some(group_id)) = f.context().node_level() { + // If this content is enclosed by a group that adds the optional parentheses, then *disable* + // this group *except* if the optional parentheses are shown. conditional_group( &Arguments::from(&self.content), Condition::if_group_breaks(group_id), ) .fmt(f) } else { + // Unconditionally group the content if it is not enclosed by an optional parentheses group. group(&Arguments::from(&self.content)).fmt(f) } }