From dcdfe683f99c8b6510b2fece6baf96a2547833a2 Mon Sep 17 00:00:00 2001 From: konstin Date: Fri, 28 Jul 2023 15:16:44 +0200 Subject: [PATCH 01/15] Call chain formatting in fluent style --- .../test/fixtures/ruff/call_chains.py | 126 +++++++ .../test/fixtures/ruff/expression/call.py | 2 +- .../src/comments/placement.rs | 4 +- .../src/expression/expr_attribute.rs | 108 +++++- .../src/expression/expr_bin_op.rs | 18 +- .../src/expression/expr_call.rs | 30 +- .../src/expression/expr_subscript.rs | 35 +- .../src/expression/mod.rs | 186 ++++++++-- ...patibility@simple_cases__comments4.py.snap | 348 ------------------ ...atibility@simple_cases__expression.py.snap | 63 +--- ...mpatibility@simple_cases__fmtonoff.py.snap | 35 +- ...mpatibility@simple_cases__function.py.snap | 31 +- .../snapshots/format@call_chains.py.snap | 269 ++++++++++++++ .../format@expression__attribute.py.snap | 8 +- .../snapshots/format@expression__call.py.snap | 17 +- 15 files changed, 758 insertions(+), 522 deletions(-) create mode 100644 crates/ruff_python_formatter/resources/test/fixtures/ruff/call_chains.py delete mode 100644 crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments4.py.snap create mode 100644 crates/ruff_python_formatter/tests/snapshots/format@call_chains.py.snap diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/call_chains.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/call_chains.py new file mode 100644 index 0000000000000..31d946357825b --- /dev/null +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/call_chains.py @@ -0,0 +1,126 @@ +# Test cases for call chains and optional parentheses, with and without fluent style + +raise OsError("") from a.aaaaa( + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa +).a(aaaa) + +raise OsError( + "sökdjffffsldkfjlhsakfjhalsökafhsöfdahsödfjösaaksjdllllllllllllll" +) from a.aaaaa( + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa +).a( + aaaa +) + +a1 = Blog.objects.filter(entry__headline__contains="Lennon").filter( + entry__pub_date__year=2008 +) + +a2 = Blog.objects.filter( + entry__headline__contains="Lennon", +).filter( + entry__pub_date__year=2008, +) + +raise OsError("") from ( + Blog.objects.filter( + entry__headline__contains="Lennon", + ) + .filter( + entry__pub_date__year=2008, + ) + .filter( + entry__pub_date__year=2008, + ) +) + +raise OsError("sökdjffffsldkfjlhsakfjhalsökafhsöfdahsödfjösaaksjdllllllllllllll") from ( + Blog.objects.filter( + entry__headline__contains="Lennon", + ) + .filter( + entry__pub_date__year=2008, + ) + .filter( + entry__pub_date__year=2008, + ) +) + +# Break only after calls and indexing +b1 = ( + session.query(models.Customer.id) + .filter( + models.Customer.account_id == account_id, models.Customer.email == email_address + ) + .count() +) + +b2 = ( + Blog.objects.filter( + entry__headline__contains="Lennon", + ) + .limit_results[:10] + .filter( + entry__pub_date__month=10, + ) +) + +# Nested call chains +c1 = ( + Blog.objects.filter( + entry__headline__contains="Lennon", + ).filter( + entry__pub_date__year=2008, + ) + + Blog.objects.filter( + entry__headline__contains="McCartney", + ) + .limit_results[:10] + .filter( + entry__pub_date__year=2010, + ) +).all() + +# Test different cases with trailing end of line comments: +# * fluent style, fits: no parentheses -> ignore the expand_parent +# * fluent style, doesn't fit: break all soft line breaks +# * default, fits: no parentheses +# * default, doesn't fit: parentheses but no soft line breaks + +# Fits, either style +d11 = x.e().e().e() # +d12 = (x.e().e().e()) # +d13 = ( + x.e() # + .e() + .e() +) + +# Doesn't fit, default +d2 = ( + x.e().esadjkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkfsdddd() # +) + +# Doesn't fit, fluent style +d3 = ( + x.e() # + .esadjkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkk() + .esadjkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkk() +) + +# Don't drop the bin op parentheses +e1 = (1 + 2).w().t() +e2 = (1 + 2)().w().t() +e3 = (1 + 2)[1].w().t() + +# Treat preserved parentheses correctly +# TODO: The implementation assumes `Parentheses::Preserve`, what's with other +# parentheses? +f1 = a.w().t(1,) +f2 = (a).w().t(1,) + +# Indent in the parentheses without breaking +g1 = ( + queryset.distinct().order_by(field.name).values_list(field_name_flat_long_long=True) +) + diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/call.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/call.py index f5198c62ca652..a158a2ec4dfdc 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/call.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/call.py @@ -52,7 +52,7 @@ def f(*args, **kwargs): hey_this_is_a_very_long_call=1, it_has_funny_attributes_asdf_asdf=1, too_long_for_the_line=1, really=True ) -# TODO(konstin): Call chains/fluent interface (https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#call-chains) +# Call chains/fluent interface (https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#call-chains) result = ( session.query(models.Customer.id) .filter( diff --git a/crates/ruff_python_formatter/src/comments/placement.rs b/crates/ruff_python_formatter/src/comments/placement.rs index ddd0080cc36d2..14988291df099 100644 --- a/crates/ruff_python_formatter/src/comments/placement.rs +++ b/crates/ruff_python_formatter/src/comments/placement.rs @@ -1021,7 +1021,7 @@ fn handle_attribute_comment<'a>( .contains(comment.slice().start()) ); if comment.line_position().is_end_of_line() { - // Attach to node with b + // Attach as trailing comment to a. The specific placement is only relevant for fluent style // ```python // x322 = ( // a @@ -1029,7 +1029,7 @@ fn handle_attribute_comment<'a>( // b // ) // ``` - CommentPlacement::trailing(comment.enclosing_node(), comment) + CommentPlacement::trailing(attribute.value.as_ref(), comment) } else { CommentPlacement::dangling(attribute, comment) } diff --git a/crates/ruff_python_formatter/src/expression/expr_attribute.rs b/crates/ruff_python_formatter/src/expression/expr_attribute.rs index a577ae88d0610..31237597340a6 100644 --- a/crates/ruff_python_formatter/src/expression/expr_attribute.rs +++ b/crates/ruff_python_formatter/src/expression/expr_attribute.rs @@ -1,15 +1,27 @@ -use ruff_python_ast::{Constant, Expr, ExprAttribute, ExprConstant}; - -use ruff_formatter::write; +use ruff_formatter::{write, FormatRuleWithOptions}; use ruff_python_ast::node::AnyNodeRef; +use ruff_python_ast::{Constant, Expr, ExprAttribute, ExprConstant}; use crate::comments::{leading_comments, trailing_comments}; -use crate::expression::parentheses::{NeedsParentheses, OptionalParentheses, Parentheses}; +use crate::expression::parentheses::{ + is_expression_parenthesized, NeedsParentheses, OptionalParentheses, Parentheses, +}; use crate::prelude::*; use crate::FormatNodeRule; #[derive(Default)] -pub struct FormatExprAttribute; +pub struct FormatExprAttribute { + fluent_style: bool, +} + +impl FormatRuleWithOptions> for FormatExprAttribute { + type Options = bool; + + fn with_options(mut self, options: Self::Options) -> Self { + self.fluent_style = options; + self + } +} impl FormatNodeRule for FormatExprAttribute { fn fmt_fields(&self, item: &ExprAttribute, f: &mut PyFormatter) -> FormatResult<()> { @@ -37,11 +49,32 @@ impl FormatNodeRule for FormatExprAttribute { if needs_parentheses { value.format().with_options(Parentheses::Always).fmt(f)?; - } else if let Expr::Attribute(expr_attribute) = value.as_ref() { - // We're in a attribute chain (`a.b.c`). The outermost node adds parentheses if - // required, the inner ones don't need them so we skip the `Expr` formatting that - // normally adds the parentheses. - expr_attribute.format().fmt(f)?; + } else if self.fluent_style { + // Fluent style: We need to pass the parentheses on to inner attributes or call chains + match value.as_ref() { + Expr::Attribute(expr) => { + expr.format().with_options(self.fluent_style).fmt(f)?; + } + Expr::Call(expr) => { + expr.format().with_options(self.fluent_style).fmt(f)?; + // Format the dot on its own line + soft_line_break().fmt(f)?; + } + Expr::Subscript(expr) => { + expr.format().with_options(self.fluent_style).fmt(f)?; + // Format the dot on its own line + soft_line_break().fmt(f)?; + } + _ => { + value.format().fmt(f)?; + // TODO: This is inefficient, we already check this in `FormatExpr`. + // TODO 2: This assumes `Parentheses::Preserve`, what's with other parentheses + // kinds + if is_expression_parenthesized(value.as_ref().into(), f.context().source()) { + soft_line_break().fmt(f)?; + } + } + } } else { value.format().fmt(f)?; } @@ -50,16 +83,51 @@ impl FormatNodeRule for FormatExprAttribute { hard_line_break().fmt(f)?; } - write!( - f, - [ - text("."), - trailing_comments(trailing_dot_comments), - (!leading_attribute_comments.is_empty()).then_some(hard_line_break()), - leading_comments(leading_attribute_comments), - attr.format() - ] - ) + if self.fluent_style { + // Fluent style has line breaks before the dot + // ```python + // blogs3 = ( + // Blog.objects.filter( + // entry__headline__contains="Lennon", + // ) + // .filter( + // entry__pub_date__year=2008, + // ) + // .filter( + // entry__pub_date__year=2008, + // ) + // ) + // ``` + write!( + f, + [ + (!leading_attribute_comments.is_empty()).then_some(hard_line_break()), + leading_comments(leading_attribute_comments), + text("."), + trailing_comments(trailing_dot_comments), + attr.format() + ] + ) + } else { + // Regular style + // ```python + // blogs2 = Blog.objects.filter( + // entry__headline__contains="Lennon", + // ).filter( + // entry__pub_date__year=2008, + // ) + // ``` + write!( + f, + [ + text("."), + trailing_comments(trailing_dot_comments), + (!leading_attribute_comments.is_empty()).then_some(hard_line_break()), + leading_comments(leading_attribute_comments), + attr.format() + ] + ) + } } fn fmt_dangling_comments( 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 af24f6ea4afba..1a59bdb051236 100644 --- a/crates/ruff_python_formatter/src/expression/expr_bin_op.rs +++ b/crates/ruff_python_formatter/src/expression/expr_bin_op.rs @@ -5,9 +5,7 @@ use ruff_python_ast::{ }; use smallvec::SmallVec; -use ruff_formatter::{ - format_args, write, FormatOwnedWithRule, FormatRefWithRule, FormatRuleWithOptions, -}; +use ruff_formatter::{format_args, write, FormatOwnedWithRule, FormatRefWithRule}; use ruff_python_ast::node::{AnyNodeRef, AstNode}; use ruff_python_ast::str::is_implicit_concatenation; @@ -19,23 +17,11 @@ use crate::expression::parentheses::{ NeedsParentheses, OptionalParentheses, }; use crate::expression::string::StringLayout; -use crate::expression::Parentheses; use crate::prelude::*; use crate::FormatNodeRule; #[derive(Default)] -pub struct FormatExprBinOp { - parentheses: Option, -} - -impl FormatRuleWithOptions> for FormatExprBinOp { - type Options = Option; - - fn with_options(mut self, options: Self::Options) -> Self { - self.parentheses = options; - self - } -} +pub struct FormatExprBinOp; impl FormatNodeRule for FormatExprBinOp { fn fmt_fields(&self, item: &ExprBinOp, f: &mut PyFormatter) -> FormatResult<()> { diff --git a/crates/ruff_python_formatter/src/expression/expr_call.rs b/crates/ruff_python_formatter/src/expression/expr_call.rs index 2372b4a5776b9..3c266c3391f47 100644 --- a/crates/ruff_python_formatter/src/expression/expr_call.rs +++ b/crates/ruff_python_formatter/src/expression/expr_call.rs @@ -1,13 +1,24 @@ -use ruff_formatter::write; +use ruff_formatter::{write, FormatRuleWithOptions}; use ruff_python_ast::node::AnyNodeRef; -use ruff_python_ast::ExprCall; +use ruff_python_ast::{Expr, ExprCall}; use crate::expression::parentheses::{NeedsParentheses, OptionalParentheses}; use crate::prelude::*; use crate::FormatNodeRule; #[derive(Default)] -pub struct FormatExprCall; +pub struct FormatExprCall { + fluent_style: bool, +} + +impl FormatRuleWithOptions> for FormatExprCall { + type Options = bool; + + fn with_options(mut self, options: Self::Options) -> Self { + self.fluent_style = options; + self + } +} impl FormatNodeRule for FormatExprCall { fn fmt_fields(&self, item: &ExprCall, f: &mut PyFormatter) -> FormatResult<()> { @@ -17,7 +28,18 @@ impl FormatNodeRule for FormatExprCall { arguments, } = item; - write!(f, [func.format(), arguments.format()]) + if self.fluent_style { + match func.as_ref() { + Expr::Attribute(expr) => expr.format().with_options(self.fluent_style).fmt(f)?, + Expr::Call(expr) => expr.format().with_options(self.fluent_style).fmt(f)?, + Expr::Subscript(expr) => expr.format().with_options(self.fluent_style).fmt(f)?, + _ => func.format().fmt(f)?, + } + } else { + func.format().fmt(f)?; + } + + write!(f, [arguments.format()]) } } diff --git a/crates/ruff_python_formatter/src/expression/expr_subscript.rs b/crates/ruff_python_formatter/src/expression/expr_subscript.rs index 464028c2e1a60..4eafb9ed06aee 100644 --- a/crates/ruff_python_formatter/src/expression/expr_subscript.rs +++ b/crates/ruff_python_formatter/src/expression/expr_subscript.rs @@ -1,7 +1,6 @@ -use ruff_python_ast::{Expr, ExprSubscript}; - -use ruff_formatter::{format_args, write}; +use ruff_formatter::{format_args, write, FormatRuleWithOptions}; use ruff_python_ast::node::{AnyNodeRef, AstNode}; +use ruff_python_ast::{Expr, ExprSubscript}; use crate::comments::trailing_comments; use crate::context::PyFormatContext; @@ -12,7 +11,18 @@ use crate::prelude::*; use crate::FormatNodeRule; #[derive(Default)] -pub struct FormatExprSubscript; +pub struct FormatExprSubscript { + fluent_style: bool, +} + +impl FormatRuleWithOptions> for FormatExprSubscript { + type Options = bool; + + fn with_options(mut self, options: Self::Options) -> Self { + self.fluent_style = options; + self + } +} impl FormatNodeRule for FormatExprSubscript { fn fmt_fields(&self, item: &ExprSubscript, f: &mut PyFormatter) -> FormatResult<()> { @@ -30,12 +40,25 @@ impl FormatNodeRule for FormatExprSubscript { "A subscript expression can only have a single dangling comment, the one after the bracket" ); + let format_value = format_with(|f| { + if self.fluent_style { + match value.as_ref() { + Expr::Attribute(expr) => expr.format().with_options(self.fluent_style).fmt(f), + Expr::Call(expr) => expr.format().with_options(self.fluent_style).fmt(f), + Expr::Subscript(expr) => expr.format().with_options(self.fluent_style).fmt(f), + _ => value.format().fmt(f), + } + } else { + value.format().fmt(f) + } + }); + if let NodeLevel::Expression(Some(_)) = f.context().node_level() { // Enforce the optional parentheses for parenthesized values. let mut f = WithNodeLevel::new(NodeLevel::Expression(None), f); - write!(f, [value.format()])?; + write!(f, [format_value])?; } else { - value.format().fmt(f)?; + format_value.fmt(f)?; } let format_slice = format_with(|f: &mut PyFormatter| { diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index c96f9b59db085..326a2b7ddfc33 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -1,13 +1,12 @@ use std::cmp::Ordering; -use ruff_python_ast as ast; -use ruff_python_ast::{Expr, Operator}; - use ruff_formatter::{ write, FormatOwnedWithRule, FormatRefWithRule, FormatRule, FormatRuleWithOptions, }; +use ruff_python_ast as ast; use ruff_python_ast::node::AnyNodeRef; use ruff_python_ast::visitor::preorder::{walk_expr, PreorderVisitor}; +use ruff_python_ast::{Expr, Operator}; use crate::builders::parenthesize_if_expands; use crate::context::{NodeLevel, WithNodeLevel}; @@ -66,10 +65,15 @@ impl FormatRule> for FormatExpr { fn fmt(&self, expression: &Expr, f: &mut PyFormatter) -> FormatResult<()> { let parentheses = self.parentheses; + // Nested expressions that are in some outer expression's parentheses may be formatted in + // fluent style + let fluent_style = f.context().node_level() == NodeLevel::ParenthesizedExpression + && is_fluent_style_call_chain(expression); + let format_expr = format_with(|f| match expression { Expr::BoolOp(expr) => expr.format().with_options(Some(parentheses)).fmt(f), Expr::NamedExpr(expr) => expr.format().fmt(f), - Expr::BinOp(expr) => expr.format().with_options(Some(parentheses)).fmt(f), + Expr::BinOp(expr) => expr.format().fmt(f), Expr::UnaryOp(expr) => expr.format().fmt(f), Expr::Lambda(expr) => expr.format().fmt(f), Expr::IfExp(expr) => expr.format().fmt(f), @@ -83,12 +87,12 @@ impl FormatRule> for FormatExpr { Expr::Yield(expr) => expr.format().fmt(f), Expr::YieldFrom(expr) => expr.format().fmt(f), Expr::Compare(expr) => expr.format().with_options(Some(parentheses)).fmt(f), - Expr::Call(expr) => expr.format().fmt(f), + Expr::Call(expr) => expr.format().with_options(fluent_style).fmt(f), Expr::FormattedValue(expr) => expr.format().fmt(f), Expr::JoinedStr(expr) => expr.format().fmt(f), Expr::Constant(expr) => expr.format().fmt(f), - Expr::Attribute(expr) => expr.format().fmt(f), - Expr::Subscript(expr) => expr.format().fmt(f), + Expr::Attribute(expr) => expr.format().with_options(fluent_style).fmt(f), + Expr::Subscript(expr) => expr.format().with_options(fluent_style).fmt(f), Expr::Starred(expr) => expr.format().fmt(f), Expr::Name(expr) => expr.format().fmt(f), Expr::List(expr) => expr.format().fmt(f), @@ -99,9 +103,10 @@ impl FormatRule> for FormatExpr { let parenthesize = match parentheses { Parentheses::Preserve => { - is_expression_parenthesized(AnyNodeRef::from(expression), f.context().source()) + is_expression_parenthesized(expression.into(), f.context().source()) } Parentheses::Always => true, + // Fluent style means we already have parentheses Parentheses::Never => false, }; @@ -117,7 +122,17 @@ impl FormatRule> for FormatExpr { let mut f = WithNodeLevel::new(level, f); - write!(f, [format_expr]) + // Allow to indent the parentheses while + // ```python + // g1 = ( + // queryset.distinct().order_by(field.name).values_list(field_name_flat_long_long=True) + // ) + // ``` + if fluent_style { + write!(f, [group(&format_expr)]) + } else { + write!(f, [format_expr]) + } } } } @@ -154,6 +169,25 @@ impl Format> for MaybeParenthesizeExpression<'_> { parenthesize, } = self; + // Fluent style means we always add parentheses, so we don't need to check for existing + // parentheses, comments or `needs_parentheses` + let omit_optional_parentheses_result = + can_omit_optional_parentheses(expression, f.context()); + if omit_optional_parentheses_result == CanOmitOptionalParentheses::FluentStyle { + return match expression { + Expr::Attribute(expr) => { + parenthesize_if_expands(&group(&expr.format().with_options(true))).fmt(f) + } + Expr::Call(expr) => { + parenthesize_if_expands(&group(&expr.format().with_options(true))).fmt(f) + } + Expr::Subscript(expr) => { + parenthesize_if_expands(&group(&expr.format().with_options(true))).fmt(f) + } + _ => unreachable!("Fluent style can only exist with attribute, call and subscript"), + }; + } + let comments = f.context().comments(); let preserve_parentheses = parenthesize.is_optional() && is_expression_parenthesized(AnyNodeRef::from(*expression), f.context().source()); @@ -179,12 +213,15 @@ impl Format> for MaybeParenthesizeExpression<'_> { match needs_parentheses { OptionalParentheses::Multiline if *parenthesize != Parenthesize::IfRequired => { - if can_omit_optional_parentheses(expression, f.context()) { - optional_parentheses(&expression.format().with_options(Parentheses::Never)) - .fmt(f) - } else { - parenthesize_if_expands(&expression.format().with_options(Parentheses::Never)) - .fmt(f) + match omit_optional_parentheses_result { + CanOmitOptionalParentheses::Yes | CanOmitOptionalParentheses::FluentStyle => { + optional_parentheses(&expression.format().with_options(Parentheses::Never)) + .fmt(f) + } + CanOmitOptionalParentheses::No => parenthesize_if_expands( + &expression.format().with_options(Parentheses::Never), + ) + .fmt(f), } } OptionalParentheses::Always => { @@ -261,10 +298,38 @@ impl<'ast> IntoFormat> for Expr { /// * 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 { +fn can_omit_optional_parentheses( + expr: &Expr, + context: &PyFormatContext, +) -> CanOmitOptionalParentheses { let mut visitor = CanOmitOptionalParenthesesVisitor::new(context.source()); visitor.visit_subexpression(expr); - visitor.can_omit() + + if visitor.max_priority_count > 1 { + if visitor.max_priority == OperatorPriority::Attribute { + CanOmitOptionalParentheses::FluentStyle + } else { + CanOmitOptionalParentheses::No + } + } else if visitor.max_priority == OperatorPriority::Attribute { + CanOmitOptionalParentheses::Yes + } else if !visitor.any_parenthesized_expressions { + // Only use the more complex IR when there is any expression that we can possibly split by + CanOmitOptionalParentheses::No + } else { + // Only use the layout if the first or last expression has parentheses of some sort. + let first_parenthesized = visitor + .first + .is_some_and(|first| has_parentheses(first, visitor.source)); + let last_parenthesized = visitor + .last + .is_some_and(|last| has_parentheses(last, visitor.source)); + if first_parenthesized || last_parenthesized { + CanOmitOptionalParentheses::Yes + } else { + CanOmitOptionalParentheses::No + } + } } #[derive(Clone, Debug)] @@ -277,6 +342,13 @@ struct CanOmitOptionalParenthesesVisitor<'input> { source: &'input str, } +#[derive(Copy, Clone, Debug, Eq, PartialEq)] +pub(crate) enum CanOmitOptionalParentheses { + Yes, + No, + FluentStyle, +} + impl<'input> CanOmitOptionalParenthesesVisitor<'input> { fn new(source: &'input str) -> Self { Self { @@ -364,8 +436,10 @@ impl<'input> CanOmitOptionalParenthesesVisitor<'input> { self.last = Some(expr); return; } - Expr::Subscript(_) => { - // Don't walk the value. Splitting before the value looks weird. + Expr::Subscript(ast::ExprSubscript { value, .. }) => { + self.any_parenthesized_expressions = true; + // Only walk the function, the subscript is always parenthesized + self.visit_expr(value); // Don't walk the slice, because the slice is always parenthesized. return; } @@ -408,26 +482,6 @@ impl<'input> CanOmitOptionalParenthesesVisitor<'input> { walk_expr(self, expr); } - - fn can_omit(self) -> bool { - if self.max_priority_count > 1 { - false - } else if self.max_priority == OperatorPriority::Attribute { - true - } else if !self.any_parenthesized_expressions { - // Only use the more complex IR when there is any expression that we can possibly split by - false - } else { - // Only use the layout if the first or last expression has parentheses of some sort. - let first_parenthesized = self - .first - .is_some_and(|first| has_parentheses(first, self.source)); - let last_parenthesized = self - .last - .is_some_and(|last| has_parentheses(last, self.source)); - first_parenthesized || last_parenthesized - } - } } impl<'input> PreorderVisitor<'input> for CanOmitOptionalParenthesesVisitor<'input> { @@ -447,6 +501,60 @@ impl<'input> PreorderVisitor<'input> for CanOmitOptionalParenthesesVisitor<'inpu } } +/// Returns `true` if this expression is a call chain that should be formatted in fluent style. +/// +/// A call chain consists only of attribute access (`.` operator), function/method calls and +/// subscripts. We use fluent style for the call chain if there are at least two attribute dots +/// after call parentheses or subscript brackets. In case of fluent style the parentheses/bracket +/// will close on the previous line and the dot gets its own line, otherwise the line will start +/// with the closing parentheses/bracket and the dot follows immediately after. +/// +/// We only use this check if we're already in a parenthesized context, otherwise refer to +/// [`CanOmitOptionalParentheses`]. +/// +/// Below, the left hand side of addition has only a single attribute access after a call, the +/// second `.filter`. The first `.filter` is a call, but it doesn't follow a call. The right hand +/// side has two, the `.limit_results` after the call and the `.filter` after the subscript, so it +/// gets formatted in fluent style. The outer expression we assign to `blogs` has zero since the +/// `.all` follows attribute parentheses and not call parentheses. +/// +/// ```python +/// blogs = ( +/// Blog.objects.filter( +/// entry__headline__contains="Lennon", +/// ).filter( +/// entry__pub_date__year=2008, +/// ) +/// + Blog.objects.filter( +/// entry__headline__contains="McCartney", +/// ) +/// .limit_results[:10] +/// .filter( +/// entry__pub_date__year=2010, +/// ) +/// ).all() +/// ``` +fn is_fluent_style_call_chain(mut expr: &Expr) -> bool { + let mut attributes_after_parentheses = 0; + loop { + match expr { + Expr::Attribute(ast::ExprAttribute { value, .. }) => { + // `f().x` | `data[:100].T` + if matches!(value.as_ref(), Expr::Call(_) | Expr::Subscript(_)) { + attributes_after_parentheses += 1; + } + expr = value; + } + Expr::Call(ast::ExprCall { func: inner, .. }) + | Expr::Subscript(ast::ExprSubscript { value: inner, .. }) => { + expr = inner; + } + _ => break, + } + } + attributes_after_parentheses >= 2 +} + fn has_parentheses(expr: &Expr, source: &str) -> bool { has_own_parentheses(expr) || is_expression_parenthesized(AnyNodeRef::from(expr), source) } diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments4.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments4.py.snap deleted file mode 100644 index b7752423a8e47..0000000000000 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments4.py.snap +++ /dev/null @@ -1,348 +0,0 @@ ---- -source: crates/ruff_python_formatter/tests/fixtures.rs -input_file: crates/ruff_python_formatter/resources/test/fixtures/black/simple_cases/comments4.py ---- -## Input - -```py -from com.my_lovely_company.my_lovely_team.my_lovely_project.my_lovely_component import ( - MyLovelyCompanyTeamProjectComponent, # NOT DRY -) -from com.my_lovely_company.my_lovely_team.my_lovely_project.my_lovely_component import ( - MyLovelyCompanyTeamProjectComponent as component, # DRY -) - - -class C: - @pytest.mark.parametrize( - ("post_data", "message"), - [ - # metadata_version errors. - ( - {}, - "None is an invalid value for Metadata-Version. Error: This field is" - " required. see" - " https://packaging.python.org/specifications/core-metadata", - ), - ( - {"metadata_version": "-1"}, - "'-1' is an invalid value for Metadata-Version. Error: Unknown Metadata" - " Version see" - " https://packaging.python.org/specifications/core-metadata", - ), - # name errors. - ( - {"metadata_version": "1.2"}, - "'' is an invalid value for Name. Error: This field is required. see" - " https://packaging.python.org/specifications/core-metadata", - ), - ( - {"metadata_version": "1.2", "name": "foo-"}, - "'foo-' is an invalid value for Name. Error: Must start and end with a" - " letter or numeral and contain only ascii numeric and '.', '_' and" - " '-'. see https://packaging.python.org/specifications/core-metadata", - ), - # version errors. - ( - {"metadata_version": "1.2", "name": "example"}, - "'' is an invalid value for Version. Error: This field is required. see" - " https://packaging.python.org/specifications/core-metadata", - ), - ( - {"metadata_version": "1.2", "name": "example", "version": "dog"}, - "'dog' is an invalid value for Version. Error: Must start and end with" - " a letter or numeral and contain only ascii numeric and '.', '_' and" - " '-'. see https://packaging.python.org/specifications/core-metadata", - ), - ], - ) - def test_fails_invalid_post_data( - self, pyramid_config, db_request, post_data, message - ): - pyramid_config.testing_securitypolicy(userid=1) - db_request.POST = MultiDict(post_data) - - -def foo(list_a, list_b): - results = ( - User.query.filter(User.foo == "bar") - .filter( # Because foo. - db.or_(User.field_a.astext.in_(list_a), User.field_b.astext.in_(list_b)) - ) - .filter(User.xyz.is_(None)) - # Another comment about the filtering on is_quux goes here. - .filter(db.not_(User.is_pending.astext.cast(db.Boolean).is_(True))) - .order_by(User.created_at.desc()) - .with_for_update(key_share=True) - .all() - ) - return results - - -def foo2(list_a, list_b): - # Standalone comment reasonably placed. - return ( - User.query.filter(User.foo == "bar") - .filter( - db.or_(User.field_a.astext.in_(list_a), User.field_b.astext.in_(list_b)) - ) - .filter(User.xyz.is_(None)) - ) - - -def foo3(list_a, list_b): - return ( - # Standalone comment but weirdly placed. - User.query.filter(User.foo == "bar") - .filter( - db.or_(User.field_a.astext.in_(list_a), User.field_b.astext.in_(list_b)) - ) - .filter(User.xyz.is_(None)) - ) -``` - -## Black Differences - -```diff ---- Black -+++ Ruff -@@ -58,37 +58,28 @@ - - def foo(list_a, list_b): - results = ( -- User.query.filter(User.foo == "bar") -- .filter( # Because foo. -+ User.query.filter(User.foo == "bar").filter( # Because foo. - db.or_(User.field_a.astext.in_(list_a), User.field_b.astext.in_(list_b)) -- ) -- .filter(User.xyz.is_(None)) -+ ).filter(User.xyz.is_(None)). - # Another comment about the filtering on is_quux goes here. -- .filter(db.not_(User.is_pending.astext.cast(db.Boolean).is_(True))) -- .order_by(User.created_at.desc()) -- .with_for_update(key_share=True) -- .all() -+ filter(db.not_(User.is_pending.astext.cast(db.Boolean).is_(True))).order_by( -+ User.created_at.desc() -+ ).with_for_update(key_share=True).all() - ) - return results - - - def foo2(list_a, list_b): - # Standalone comment reasonably placed. -- return ( -- User.query.filter(User.foo == "bar") -- .filter( -- db.or_(User.field_a.astext.in_(list_a), User.field_b.astext.in_(list_b)) -- ) -- .filter(User.xyz.is_(None)) -- ) -+ return User.query.filter(User.foo == "bar").filter( -+ db.or_(User.field_a.astext.in_(list_a), User.field_b.astext.in_(list_b)) -+ ).filter(User.xyz.is_(None)) - - - def foo3(list_a, list_b): - return ( - # Standalone comment but weirdly placed. -- User.query.filter(User.foo == "bar") -- .filter( -+ User.query.filter(User.foo == "bar").filter( - db.or_(User.field_a.astext.in_(list_a), User.field_b.astext.in_(list_b)) -- ) -- .filter(User.xyz.is_(None)) -+ ).filter(User.xyz.is_(None)) - ) -``` - -## Ruff Output - -```py -from com.my_lovely_company.my_lovely_team.my_lovely_project.my_lovely_component import ( - MyLovelyCompanyTeamProjectComponent, # NOT DRY -) -from com.my_lovely_company.my_lovely_team.my_lovely_project.my_lovely_component import ( - MyLovelyCompanyTeamProjectComponent as component, # DRY -) - - -class C: - @pytest.mark.parametrize( - ("post_data", "message"), - [ - # metadata_version errors. - ( - {}, - "None is an invalid value for Metadata-Version. Error: This field is" - " required. see" - " https://packaging.python.org/specifications/core-metadata", - ), - ( - {"metadata_version": "-1"}, - "'-1' is an invalid value for Metadata-Version. Error: Unknown Metadata" - " Version see" - " https://packaging.python.org/specifications/core-metadata", - ), - # name errors. - ( - {"metadata_version": "1.2"}, - "'' is an invalid value for Name. Error: This field is required. see" - " https://packaging.python.org/specifications/core-metadata", - ), - ( - {"metadata_version": "1.2", "name": "foo-"}, - "'foo-' is an invalid value for Name. Error: Must start and end with a" - " letter or numeral and contain only ascii numeric and '.', '_' and" - " '-'. see https://packaging.python.org/specifications/core-metadata", - ), - # version errors. - ( - {"metadata_version": "1.2", "name": "example"}, - "'' is an invalid value for Version. Error: This field is required. see" - " https://packaging.python.org/specifications/core-metadata", - ), - ( - {"metadata_version": "1.2", "name": "example", "version": "dog"}, - "'dog' is an invalid value for Version. Error: Must start and end with" - " a letter or numeral and contain only ascii numeric and '.', '_' and" - " '-'. see https://packaging.python.org/specifications/core-metadata", - ), - ], - ) - def test_fails_invalid_post_data( - self, pyramid_config, db_request, post_data, message - ): - pyramid_config.testing_securitypolicy(userid=1) - db_request.POST = MultiDict(post_data) - - -def foo(list_a, list_b): - results = ( - User.query.filter(User.foo == "bar").filter( # Because foo. - db.or_(User.field_a.astext.in_(list_a), User.field_b.astext.in_(list_b)) - ).filter(User.xyz.is_(None)). - # Another comment about the filtering on is_quux goes here. - filter(db.not_(User.is_pending.astext.cast(db.Boolean).is_(True))).order_by( - User.created_at.desc() - ).with_for_update(key_share=True).all() - ) - return results - - -def foo2(list_a, list_b): - # Standalone comment reasonably placed. - return User.query.filter(User.foo == "bar").filter( - db.or_(User.field_a.astext.in_(list_a), User.field_b.astext.in_(list_b)) - ).filter(User.xyz.is_(None)) - - -def foo3(list_a, list_b): - return ( - # Standalone comment but weirdly placed. - User.query.filter(User.foo == "bar").filter( - db.or_(User.field_a.astext.in_(list_a), User.field_b.astext.in_(list_b)) - ).filter(User.xyz.is_(None)) - ) -``` - -## Black Output - -```py -from com.my_lovely_company.my_lovely_team.my_lovely_project.my_lovely_component import ( - MyLovelyCompanyTeamProjectComponent, # NOT DRY -) -from com.my_lovely_company.my_lovely_team.my_lovely_project.my_lovely_component import ( - MyLovelyCompanyTeamProjectComponent as component, # DRY -) - - -class C: - @pytest.mark.parametrize( - ("post_data", "message"), - [ - # metadata_version errors. - ( - {}, - "None is an invalid value for Metadata-Version. Error: This field is" - " required. see" - " https://packaging.python.org/specifications/core-metadata", - ), - ( - {"metadata_version": "-1"}, - "'-1' is an invalid value for Metadata-Version. Error: Unknown Metadata" - " Version see" - " https://packaging.python.org/specifications/core-metadata", - ), - # name errors. - ( - {"metadata_version": "1.2"}, - "'' is an invalid value for Name. Error: This field is required. see" - " https://packaging.python.org/specifications/core-metadata", - ), - ( - {"metadata_version": "1.2", "name": "foo-"}, - "'foo-' is an invalid value for Name. Error: Must start and end with a" - " letter or numeral and contain only ascii numeric and '.', '_' and" - " '-'. see https://packaging.python.org/specifications/core-metadata", - ), - # version errors. - ( - {"metadata_version": "1.2", "name": "example"}, - "'' is an invalid value for Version. Error: This field is required. see" - " https://packaging.python.org/specifications/core-metadata", - ), - ( - {"metadata_version": "1.2", "name": "example", "version": "dog"}, - "'dog' is an invalid value for Version. Error: Must start and end with" - " a letter or numeral and contain only ascii numeric and '.', '_' and" - " '-'. see https://packaging.python.org/specifications/core-metadata", - ), - ], - ) - def test_fails_invalid_post_data( - self, pyramid_config, db_request, post_data, message - ): - pyramid_config.testing_securitypolicy(userid=1) - db_request.POST = MultiDict(post_data) - - -def foo(list_a, list_b): - results = ( - User.query.filter(User.foo == "bar") - .filter( # Because foo. - db.or_(User.field_a.astext.in_(list_a), User.field_b.astext.in_(list_b)) - ) - .filter(User.xyz.is_(None)) - # Another comment about the filtering on is_quux goes here. - .filter(db.not_(User.is_pending.astext.cast(db.Boolean).is_(True))) - .order_by(User.created_at.desc()) - .with_for_update(key_share=True) - .all() - ) - return results - - -def foo2(list_a, list_b): - # Standalone comment reasonably placed. - return ( - User.query.filter(User.foo == "bar") - .filter( - db.or_(User.field_a.astext.in_(list_a), User.field_b.astext.in_(list_b)) - ) - .filter(User.xyz.is_(None)) - ) - - -def foo3(list_a, list_b): - return ( - # Standalone comment but weirdly placed. - User.query.filter(User.foo == "bar") - .filter( - db.or_(User.field_a.astext.in_(list_a), User.field_b.astext.in_(list_b)) - ) - .filter(User.xyz.is_(None)) - ) -``` - - 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 ad9938cb83183..86a5b37df9883 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 @@ -300,40 +300,7 @@ last_call() ) # note: no trailing comma pre-3.6 call(*gidgets[:2]) call(a, *gidgets[:2]) -@@ -208,24 +209,14 @@ - what_is_up_with_those_new_coord_names = (coord_names | set(vars_to_create)) - set( - vars_to_remove - ) --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() --) --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() --) -+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() -+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() - Ø = set() - authors.łukasz.say_thanks() - mapping = { -@@ -328,13 +319,18 @@ +@@ -328,13 +329,18 @@ ): return True if ( @@ -355,7 +322,7 @@ last_call() ^ aaaaaaaa.i << aaaaaaaa.k >> aaaaaaaa.l**aaaaaaaa.m // aaaaaaaa.n ): return True -@@ -342,7 +338,8 @@ +@@ -342,7 +348,8 @@ ~aaaaaaaaaaaaaaaa.a + aaaaaaaaaaaaaaaa.b - aaaaaaaaaaaaaaaa.c * aaaaaaaaaaaaaaaa.d @ aaaaaaaaaaaaaaaa.e @@ -581,14 +548,24 @@ what_is_up_with_those_new_coord_names = (coord_names + set(vars_to_create)) + se what_is_up_with_those_new_coord_names = (coord_names | set(vars_to_create)) - set( vars_to_remove ) -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() -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() +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() +) +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() +) Ø = set() authors.łukasz.say_thanks() mapping = { diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__fmtonoff.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__fmtonoff.py.snap index 88e2db7a36ea8..16f03bc4250a2 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__fmtonoff.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__fmtonoff.py.snap @@ -287,7 +287,7 @@ d={'a':1, # fmt: on goes + here, andhere, -@@ -80,38 +94,36 @@ +@@ -80,38 +94,42 @@ def import_as_names(): # fmt: off @@ -330,14 +330,19 @@ d={'a':1, - .filter(models.Customer.account_id == account_id, - models.Customer.email == email_address)\ - .order_by(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() ++ 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() ++ ) # fmt: on -@@ -132,10 +144,10 @@ +@@ -132,10 +150,10 @@ """Another known limitation.""" # fmt: on # fmt: off @@ -352,7 +357,7 @@ d={'a':1, # fmt: on # fmt: off # ...but comments still get reformatted even though they should not be -@@ -153,9 +165,7 @@ +@@ -153,9 +171,7 @@ ) ) # fmt: off @@ -363,7 +368,7 @@ d={'a':1, # fmt: on _type_comment_re = re.compile( r""" -@@ -178,7 +188,7 @@ +@@ -178,7 +194,7 @@ $ """, # fmt: off @@ -372,7 +377,7 @@ d={'a':1, # fmt: on ) -@@ -216,8 +226,7 @@ +@@ -216,8 +232,7 @@ xxxxxxxxxx_xxxxxxxxxxx_xxxxxxx_xxxxxxxxx=5, ) # fmt: off @@ -512,9 +517,15 @@ def yield_expr(): def example(session): # fmt: off - 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() + 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() + ) # fmt: on diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__function.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__function.py.snap index 0e3bfa106cfe5..dacb0b6cac47d 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__function.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__function.py.snap @@ -107,7 +107,7 @@ def __await__(): return (yield) ```diff --- Black +++ Ruff -@@ -65,18 +65,14 @@ +@@ -65,6 +65,7 @@ def spaces2(result=_core.Value(None)): assert fut is self._read_fut, (fut, self._read_fut) @@ -115,22 +115,6 @@ def __await__(): return (yield) def example(session): -- 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() -- ) -+ 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() - - - def long_lines(): ``` ## Ruff Output @@ -207,10 +191,15 @@ def spaces2(result=_core.Value(None)): def example(session): - 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() + 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() + ) def long_lines(): diff --git a/crates/ruff_python_formatter/tests/snapshots/format@call_chains.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@call_chains.py.snap new file mode 100644 index 0000000000000..6e8aed6dfcb53 --- /dev/null +++ b/crates/ruff_python_formatter/tests/snapshots/format@call_chains.py.snap @@ -0,0 +1,269 @@ +--- +source: crates/ruff_python_formatter/tests/fixtures.rs +input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/call_chains.py +--- +## Input +```py +# Test cases for call chains and optional parentheses, with and without fluent style + +raise OsError("") from a.aaaaa( + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa +).a(aaaa) + +raise OsError( + "sökdjffffsldkfjlhsakfjhalsökafhsöfdahsödfjösaaksjdllllllllllllll" +) from a.aaaaa( + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa +).a( + aaaa +) + +a1 = Blog.objects.filter(entry__headline__contains="Lennon").filter( + entry__pub_date__year=2008 +) + +a2 = Blog.objects.filter( + entry__headline__contains="Lennon", +).filter( + entry__pub_date__year=2008, +) + +raise OsError("") from ( + Blog.objects.filter( + entry__headline__contains="Lennon", + ) + .filter( + entry__pub_date__year=2008, + ) + .filter( + entry__pub_date__year=2008, + ) +) + +raise OsError("sökdjffffsldkfjlhsakfjhalsökafhsöfdahsödfjösaaksjdllllllllllllll") from ( + Blog.objects.filter( + entry__headline__contains="Lennon", + ) + .filter( + entry__pub_date__year=2008, + ) + .filter( + entry__pub_date__year=2008, + ) +) + +# Break only after calls and indexing +b1 = ( + session.query(models.Customer.id) + .filter( + models.Customer.account_id == account_id, models.Customer.email == email_address + ) + .count() +) + +b2 = ( + Blog.objects.filter( + entry__headline__contains="Lennon", + ) + .limit_results[:10] + .filter( + entry__pub_date__month=10, + ) +) + +# Nested call chains +c1 = ( + Blog.objects.filter( + entry__headline__contains="Lennon", + ).filter( + entry__pub_date__year=2008, + ) + + Blog.objects.filter( + entry__headline__contains="McCartney", + ) + .limit_results[:10] + .filter( + entry__pub_date__year=2010, + ) +).all() + +# Test different cases with trailing end of line comments: +# * fluent style, fits: no parentheses -> ignore the expand_parent +# * fluent style, doesn't fit: break all soft line breaks +# * default, fits: no parentheses +# * default, doesn't fit: parentheses but no soft line breaks + +# Fits, either style +d11 = x.e().e().e() # +d12 = (x.e().e().e()) # +d13 = ( + x.e() # + .e() + .e() +) + +# Doesn't fit, default +d2 = ( + x.e().esadjkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkfsdddd() # +) + +# Doesn't fit, fluent style +d3 = ( + x.e() # + .esadjkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkk() + .esadjkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkk() +) + +# Don't drop the bin op parentheses +e1 = (1 + 2).w().t() +e2 = (1 + 2)().w().t() +e3 = (1 + 2)[1].w().t() + +# Treat preserved parentheses correctly +# TODO: The implementation assumes `Parentheses::Preserve`, what's with other +# parentheses? +f1 = a.w().t(1,) +f2 = (a).w().t(1,) + +# Indent in the parentheses without breaking +g1 = ( + queryset.distinct().order_by(field.name).values_list(field_name_flat_long_long=True) +) + +``` + +## Output +```py +# Test cases for call chains and optional parentheses, with and without fluent style + +raise OsError("") from a.aaaaa( + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa +).a(aaaa) + +raise OsError( + "sökdjffffsldkfjlhsakfjhalsökafhsöfdahsödfjösaaksjdllllllllllllll" +) from a.aaaaa( + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa +).a(aaaa) + +a1 = Blog.objects.filter(entry__headline__contains="Lennon").filter( + entry__pub_date__year=2008 +) + +a2 = Blog.objects.filter( + entry__headline__contains="Lennon", +).filter( + entry__pub_date__year=2008, +) + +raise OsError("") from ( + Blog.objects.filter( + entry__headline__contains="Lennon", + ) + .filter( + entry__pub_date__year=2008, + ) + .filter( + entry__pub_date__year=2008, + ) +) + +raise OsError("sökdjffffsldkfjlhsakfjhalsökafhsöfdahsödfjösaaksjdllllllllllllll") from ( + Blog.objects.filter( + entry__headline__contains="Lennon", + ) + .filter( + entry__pub_date__year=2008, + ) + .filter( + entry__pub_date__year=2008, + ) +) + +# Break only after calls and indexing +b1 = ( + session.query(models.Customer.id) + .filter( + models.Customer.account_id == account_id, models.Customer.email == email_address + ) + .count() +) + +b2 = ( + Blog.objects.filter( + entry__headline__contains="Lennon", + ) + .limit_results[:10] + .filter( + entry__pub_date__month=10, + ) +) + +# Nested call chains +c1 = ( + Blog.objects.filter( + entry__headline__contains="Lennon", + ).filter( + entry__pub_date__year=2008, + ) + + Blog.objects.filter( + entry__headline__contains="McCartney", + ) + .limit_results[:10] + .filter( + entry__pub_date__year=2010, + ) +).all() + +# Test different cases with trailing end of line comments: +# * fluent style, fits: no parentheses -> ignore the expand_parent +# * fluent style, doesn't fit: break all soft line breaks +# * default, fits: no parentheses +# * default, doesn't fit: parentheses but no soft line breaks + +# Fits, either style +d11 = x.e().e().e() # +d12 = x.e().e().e() # +d13 = ( + x.e() # + .e() + .e() +) + +# Doesn't fit, default +d2 = x.e().esadjkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkfsdddd() # + +# Doesn't fit, fluent style +d3 = ( + x.e() # + .esadjkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkk() + .esadjkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkk() +) + +# Don't drop the bin op parentheses +e1 = (1 + 2).w().t() +e2 = (1 + 2)().w().t() +e3 = (1 + 2)[1].w().t() + +# Treat preserved parentheses correctly +# TODO: The implementation assumes `Parentheses::Preserve`, what's with other +# parentheses? +f1 = a.w().t( + 1, +) +f2 = ( + (a) + .w() + .t( + 1, + ) +) + +# Indent in the parentheses without breaking +g1 = ( + queryset.distinct().order_by(field.name).values_list(field_name_flat_long_long=True) +) +``` + + + diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__attribute.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__attribute.py.snap index f1a5dbb035c50..a51328e6e7a71 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__attribute.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__attribute.py.snap @@ -141,10 +141,10 @@ a = Namespace() ( - a. + a. # trailing dot comment # comment # in between - b # trailing dot comment # trailing identifier comment + b # trailing identifier comment ) @@ -155,10 +155,10 @@ a.aaaaaaaaaaaaaaaaaaaaa.lllllllllllllllllllllllllllloooooooooong.chaaaaaaaaaaaaa # chain if and only if we need them, that is if there are own line comments inside # the chain. x1 = ( - a.b. + a.b. # comment 2 # comment 1 # comment 3 - c.d # comment 2 + c.d ) x20 = a.b diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__call.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__call.py.snap index 0960faa65260f..8120fdd4d8bb9 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__call.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__call.py.snap @@ -58,7 +58,7 @@ f( hey_this_is_a_very_long_call=1, it_has_funny_attributes_asdf_asdf=1, too_long_for_the_line=1, really=True ) -# TODO(konstin): Call chains/fluent interface (https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#call-chains) +# Call chains/fluent interface (https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#call-chains) result = ( session.query(models.Customer.id) .filter( @@ -178,11 +178,16 @@ f( really=True, ) -# TODO(konstin): Call chains/fluent interface (https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#call-chains) -result = session.query(models.Customer.id).filter( - models.Customer.account_id == 10000, - models.Customer.email == "user@example.org", -).order_by(models.Customer.id.asc()).all() +# Call chains/fluent interface (https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#call-chains) +result = ( + session.query(models.Customer.id) + .filter( + models.Customer.account_id == 10000, + models.Customer.email == "user@example.org", + ) + .order_by(models.Customer.id.asc()) + .all() +) # TODO(konstin): Black has this special case for comment placement where everything stays in one line f("aaaaaaaa", "aaaaaaaa", "aaaaaaaa", "aaaaaaaa", "aaaaaaaa", "aaaaaaaa", "aaaaaaaa") From 42ad8944d87022bdb83b7cb3451d75bde28a7dcb Mon Sep 17 00:00:00 2001 From: konstin Date: Thu, 3 Aug 2023 14:10:57 +0200 Subject: [PATCH 02/15] Call chain formatting in fluent style --- .../src/expression/expr_attribute.rs | 17 +++-------------- .../tests/snapshots/format@call_chains.py.snap | 3 +-- 2 files changed, 4 insertions(+), 16 deletions(-) diff --git a/crates/ruff_python_formatter/src/expression/expr_attribute.rs b/crates/ruff_python_formatter/src/expression/expr_attribute.rs index 31237597340a6..12fc5ea28ec1c 100644 --- a/crates/ruff_python_formatter/src/expression/expr_attribute.rs +++ b/crates/ruff_python_formatter/src/expression/expr_attribute.rs @@ -4,7 +4,7 @@ use ruff_python_ast::{Constant, Expr, ExprAttribute, ExprConstant}; use crate::comments::{leading_comments, trailing_comments}; use crate::expression::parentheses::{ - is_expression_parenthesized, NeedsParentheses, OptionalParentheses, Parentheses, + NeedsParentheses, OptionalParentheses, Parentheses, }; use crate::prelude::*; use crate::FormatNodeRule; @@ -50,11 +50,8 @@ impl FormatNodeRule for FormatExprAttribute { if needs_parentheses { value.format().with_options(Parentheses::Always).fmt(f)?; } else if self.fluent_style { - // Fluent style: We need to pass the parentheses on to inner attributes or call chains match value.as_ref() { - Expr::Attribute(expr) => { - expr.format().with_options(self.fluent_style).fmt(f)?; - } + Expr::Attribute(expr) => expr.format().with_options(self.fluent_style).fmt(f)?, Expr::Call(expr) => { expr.format().with_options(self.fluent_style).fmt(f)?; // Format the dot on its own line @@ -65,15 +62,7 @@ impl FormatNodeRule for FormatExprAttribute { // Format the dot on its own line soft_line_break().fmt(f)?; } - _ => { - value.format().fmt(f)?; - // TODO: This is inefficient, we already check this in `FormatExpr`. - // TODO 2: This assumes `Parentheses::Preserve`, what's with other parentheses - // kinds - if is_expression_parenthesized(value.as_ref().into(), f.context().source()) { - soft_line_break().fmt(f)?; - } - } + _ => value.format().fmt(f)?, } } else { value.format().fmt(f)?; diff --git a/crates/ruff_python_formatter/tests/snapshots/format@call_chains.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@call_chains.py.snap index 6e8aed6dfcb53..55766ad452c1b 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@call_chains.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@call_chains.py.snap @@ -252,8 +252,7 @@ f1 = a.w().t( 1, ) f2 = ( - (a) - .w() + (a).w() .t( 1, ) From f9c2f63ab835aeedb3e706c14c28863de33c7c53 Mon Sep 17 00:00:00 2001 From: konstin Date: Thu, 3 Aug 2023 14:12:32 +0200 Subject: [PATCH 03/15] Call chain formatting in fluent style --- .../src/expression/expr_attribute.rs | 20 +++++++++---------- .../src/expression/expr_call.rs | 14 +++++-------- .../src/expression/expr_subscript.rs | 16 +++++---------- 3 files changed, 20 insertions(+), 30 deletions(-) diff --git a/crates/ruff_python_formatter/src/expression/expr_attribute.rs b/crates/ruff_python_formatter/src/expression/expr_attribute.rs index 12fc5ea28ec1c..6c635531f978d 100644 --- a/crates/ruff_python_formatter/src/expression/expr_attribute.rs +++ b/crates/ruff_python_formatter/src/expression/expr_attribute.rs @@ -3,9 +3,7 @@ use ruff_python_ast::node::AnyNodeRef; use ruff_python_ast::{Constant, Expr, ExprAttribute, ExprConstant}; use crate::comments::{leading_comments, trailing_comments}; -use crate::expression::parentheses::{ - NeedsParentheses, OptionalParentheses, Parentheses, -}; +use crate::expression::parentheses::{NeedsParentheses, OptionalParentheses, Parentheses}; use crate::prelude::*; use crate::FormatNodeRule; @@ -49,23 +47,25 @@ impl FormatNodeRule for FormatExprAttribute { if needs_parentheses { value.format().with_options(Parentheses::Always).fmt(f)?; - } else if self.fluent_style { + } else { match value.as_ref() { Expr::Attribute(expr) => expr.format().with_options(self.fluent_style).fmt(f)?, Expr::Call(expr) => { expr.format().with_options(self.fluent_style).fmt(f)?; - // Format the dot on its own line - soft_line_break().fmt(f)?; + if self.fluent_style { + // Format the dot on its own line + soft_line_break().fmt(f)?; + } } Expr::Subscript(expr) => { expr.format().with_options(self.fluent_style).fmt(f)?; - // Format the dot on its own line - soft_line_break().fmt(f)?; + if self.fluent_style { + // Format the dot on its own line + soft_line_break().fmt(f)?; + } } _ => value.format().fmt(f)?, } - } else { - value.format().fmt(f)?; } if comments.has_trailing_own_line_comments(value.as_ref()) { diff --git a/crates/ruff_python_formatter/src/expression/expr_call.rs b/crates/ruff_python_formatter/src/expression/expr_call.rs index 3c266c3391f47..c2596577ed629 100644 --- a/crates/ruff_python_formatter/src/expression/expr_call.rs +++ b/crates/ruff_python_formatter/src/expression/expr_call.rs @@ -28,15 +28,11 @@ impl FormatNodeRule for FormatExprCall { arguments, } = item; - if self.fluent_style { - match func.as_ref() { - Expr::Attribute(expr) => expr.format().with_options(self.fluent_style).fmt(f)?, - Expr::Call(expr) => expr.format().with_options(self.fluent_style).fmt(f)?, - Expr::Subscript(expr) => expr.format().with_options(self.fluent_style).fmt(f)?, - _ => func.format().fmt(f)?, - } - } else { - func.format().fmt(f)?; + match func.as_ref() { + Expr::Attribute(expr) => expr.format().with_options(self.fluent_style).fmt(f)?, + Expr::Call(expr) => expr.format().with_options(self.fluent_style).fmt(f)?, + Expr::Subscript(expr) => expr.format().with_options(self.fluent_style).fmt(f)?, + _ => func.format().fmt(f)?, } write!(f, [arguments.format()]) diff --git a/crates/ruff_python_formatter/src/expression/expr_subscript.rs b/crates/ruff_python_formatter/src/expression/expr_subscript.rs index 4eafb9ed06aee..4373920991256 100644 --- a/crates/ruff_python_formatter/src/expression/expr_subscript.rs +++ b/crates/ruff_python_formatter/src/expression/expr_subscript.rs @@ -40,17 +40,11 @@ impl FormatNodeRule for FormatExprSubscript { "A subscript expression can only have a single dangling comment, the one after the bracket" ); - let format_value = format_with(|f| { - if self.fluent_style { - match value.as_ref() { - Expr::Attribute(expr) => expr.format().with_options(self.fluent_style).fmt(f), - Expr::Call(expr) => expr.format().with_options(self.fluent_style).fmt(f), - Expr::Subscript(expr) => expr.format().with_options(self.fluent_style).fmt(f), - _ => value.format().fmt(f), - } - } else { - value.format().fmt(f) - } + let format_value = format_with(|f| match value.as_ref() { + Expr::Attribute(expr) => expr.format().with_options(self.fluent_style).fmt(f), + Expr::Call(expr) => expr.format().with_options(self.fluent_style).fmt(f), + Expr::Subscript(expr) => expr.format().with_options(self.fluent_style).fmt(f), + _ => value.format().fmt(f), }); if let NodeLevel::Expression(Some(_)) = f.context().node_level() { From 0a7e1a5a5ea2544d9bab7f8c0d70ead008feeed7 Mon Sep 17 00:00:00 2001 From: konstin Date: Thu, 3 Aug 2023 14:57:30 +0200 Subject: [PATCH 04/15] Fix instability --- .../test/fixtures/ruff/call_chains.py | 7 +++++ .../src/expression/mod.rs | 30 +++++++++++++------ .../snapshots/format@call_chains.py.snap | 13 ++++++++ 3 files changed, 41 insertions(+), 9 deletions(-) diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/call_chains.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/call_chains.py index 31d946357825b..ec8dc9bd5e057 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/call_chains.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/call_chains.py @@ -124,3 +124,10 @@ queryset.distinct().order_by(field.name).values_list(field_name_flat_long_long=True) ) +# TODO(konstin): We currently can't do call chains that is not the outermost expression +if ( + not a() + .b() + .cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc() +): + pass diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index 326a2b7ddfc33..903747315633a 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -68,7 +68,7 @@ impl FormatRule> for FormatExpr { // Nested expressions that are in some outer expression's parentheses may be formatted in // fluent style let fluent_style = f.context().node_level() == NodeLevel::ParenthesizedExpression - && is_fluent_style_call_chain(expression); + && is_fluent_style_call_chain(expression, f.context().source()); let format_expr = format_with(|f| match expression { Expr::BoolOp(expr) => expr.format().with_options(Some(parentheses)).fmt(f), @@ -173,18 +173,24 @@ impl Format> for MaybeParenthesizeExpression<'_> { // parentheses, comments or `needs_parentheses` let omit_optional_parentheses_result = can_omit_optional_parentheses(expression, f.context()); - if omit_optional_parentheses_result == CanOmitOptionalParentheses::FluentStyle { - return match expression { + let is_fluent_style = is_fluent_style_call_chain(expression, f.context().source()); + if is_fluent_style { + match expression { Expr::Attribute(expr) => { - parenthesize_if_expands(&group(&expr.format().with_options(true))).fmt(f) + return parenthesize_if_expands(&group(&expr.format().with_options(true))) + .fmt(f) } Expr::Call(expr) => { - parenthesize_if_expands(&group(&expr.format().with_options(true))).fmt(f) + return parenthesize_if_expands(&group(&expr.format().with_options(true))) + .fmt(f) } Expr::Subscript(expr) => { - parenthesize_if_expands(&group(&expr.format().with_options(true))).fmt(f) + return parenthesize_if_expands(&group(&expr.format().with_options(true))) + .fmt(f) + } + _ => { + // TODO(konstin): We currently can't do call chains that is not the outermost expression } - _ => unreachable!("Fluent style can only exist with attribute, call and subscript"), }; } @@ -534,7 +540,7 @@ impl<'input> PreorderVisitor<'input> for CanOmitOptionalParenthesesVisitor<'inpu /// ) /// ).all() /// ``` -fn is_fluent_style_call_chain(mut expr: &Expr) -> bool { +fn is_fluent_style_call_chain(mut expr: &Expr, source: &str) -> bool { let mut attributes_after_parentheses = 0; loop { match expr { @@ -549,7 +555,13 @@ fn is_fluent_style_call_chain(mut expr: &Expr) -> bool { | Expr::Subscript(ast::ExprSubscript { value: inner, .. }) => { expr = inner; } - _ => break, + _ => { + // We to format the following in fluent style: `f2 = (a).w().t(1,)` + if is_expression_parenthesized(AnyNodeRef::from(expr), source) { + attributes_after_parentheses += 1; + } + break; + } } } attributes_after_parentheses >= 2 diff --git a/crates/ruff_python_formatter/tests/snapshots/format@call_chains.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@call_chains.py.snap index 55766ad452c1b..b33054444d038 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@call_chains.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@call_chains.py.snap @@ -130,6 +130,13 @@ g1 = ( queryset.distinct().order_by(field.name).values_list(field_name_flat_long_long=True) ) +# TODO(konstin): We currently can't do call chains that is not the outermost expression +if ( + not a() + .b() + .cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc() +): + pass ``` ## Output @@ -262,6 +269,12 @@ f2 = ( g1 = ( queryset.distinct().order_by(field.name).values_list(field_name_flat_long_long=True) ) + +# TODO(konstin): We currently can't do call chains that is not the outermost expression +if ( + not a().b().cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc() +): + pass ``` From d157ac6d040ce6ff5505958668b52938042d9769 Mon Sep 17 00:00:00 2001 From: konstin Date: Thu, 3 Aug 2023 14:59:41 +0200 Subject: [PATCH 05/15] Refactor --- .../src/expression/mod.rs | 46 +++++-------------- 1 file changed, 12 insertions(+), 34 deletions(-) diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index 903747315633a..99ef53c3a53c3 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -171,8 +171,6 @@ impl Format> for MaybeParenthesizeExpression<'_> { // Fluent style means we always add parentheses, so we don't need to check for existing // parentheses, comments or `needs_parentheses` - let omit_optional_parentheses_result = - can_omit_optional_parentheses(expression, f.context()); let is_fluent_style = is_fluent_style_call_chain(expression, f.context().source()); if is_fluent_style { match expression { @@ -217,17 +215,15 @@ impl Format> for MaybeParenthesizeExpression<'_> { Parenthesize::Optional | Parenthesize::IfBreaks => needs_parentheses, }; + let can_omit_optional_parentheses = can_omit_optional_parentheses(expression, f.context()); match needs_parentheses { OptionalParentheses::Multiline if *parenthesize != Parenthesize::IfRequired => { - match omit_optional_parentheses_result { - CanOmitOptionalParentheses::Yes | CanOmitOptionalParentheses::FluentStyle => { - optional_parentheses(&expression.format().with_options(Parentheses::Never)) - .fmt(f) - } - CanOmitOptionalParentheses::No => parenthesize_if_expands( - &expression.format().with_options(Parentheses::Never), - ) - .fmt(f), + if can_omit_optional_parentheses { + optional_parentheses(&expression.format().with_options(Parentheses::Never)) + .fmt(f) + } else { + parenthesize_if_expands(&expression.format().with_options(Parentheses::Never)) + .fmt(f) } } OptionalParentheses::Always => { @@ -304,24 +300,17 @@ impl<'ast> IntoFormat> for Expr { /// * 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, -) -> CanOmitOptionalParentheses { +fn can_omit_optional_parentheses(expr: &Expr, context: &PyFormatContext) -> bool { let mut visitor = CanOmitOptionalParenthesesVisitor::new(context.source()); visitor.visit_subexpression(expr); if visitor.max_priority_count > 1 { - if visitor.max_priority == OperatorPriority::Attribute { - CanOmitOptionalParentheses::FluentStyle - } else { - CanOmitOptionalParentheses::No - } + false } else if visitor.max_priority == OperatorPriority::Attribute { - CanOmitOptionalParentheses::Yes + true } else if !visitor.any_parenthesized_expressions { // Only use the more complex IR when there is any expression that we can possibly split by - CanOmitOptionalParentheses::No + false } else { // Only use the layout if the first or last expression has parentheses of some sort. let first_parenthesized = visitor @@ -330,11 +319,7 @@ fn can_omit_optional_parentheses( let last_parenthesized = visitor .last .is_some_and(|last| has_parentheses(last, visitor.source)); - if first_parenthesized || last_parenthesized { - CanOmitOptionalParentheses::Yes - } else { - CanOmitOptionalParentheses::No - } + first_parenthesized || last_parenthesized } } @@ -348,13 +333,6 @@ struct CanOmitOptionalParenthesesVisitor<'input> { source: &'input str, } -#[derive(Copy, Clone, Debug, Eq, PartialEq)] -pub(crate) enum CanOmitOptionalParentheses { - Yes, - No, - FluentStyle, -} - impl<'input> CanOmitOptionalParenthesesVisitor<'input> { fn new(source: &'input str) -> Self { Self { From 31c62ced9fb3a364b471fb2caf222c86d1141892 Mon Sep 17 00:00:00 2001 From: konstin Date: Thu, 3 Aug 2023 15:24:53 +0200 Subject: [PATCH 06/15] Fluent style that is not the outermost expression --- .../tests/snapshots/format@call_chains.py.snap | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/ruff_python_formatter/tests/snapshots/format@call_chains.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@call_chains.py.snap index b33054444d038..19f3407776336 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@call_chains.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@call_chains.py.snap @@ -270,9 +270,11 @@ g1 = ( queryset.distinct().order_by(field.name).values_list(field_name_flat_long_long=True) ) -# TODO(konstin): We currently can't do call chains that is not the outermost expression +# Fluent style that is not the outermost expression if ( - not a().b().cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc() + not a() + .b() + .cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc() ): pass ``` From 1cb43d5b76d526e195b6ba766ee9148e263142db Mon Sep 17 00:00:00 2001 From: konstin Date: Thu, 3 Aug 2023 21:09:26 +0200 Subject: [PATCH 07/15] All fluent style in more places --- .../test/fixtures/ruff/call_chains.py | 23 +++- .../src/expression/expr_attribute.rs | 21 ++-- .../src/expression/expr_call.rs | 13 +- .../src/expression/expr_subscript.rs | 13 +- .../src/expression/mod.rs | 116 ++++++++++-------- .../snapshots/format@call_chains.py.snap | 46 ++++++- 6 files changed, 160 insertions(+), 72 deletions(-) diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/call_chains.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/call_chains.py index ec8dc9bd5e057..89ffeca6b9ac5 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/call_chains.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/call_chains.py @@ -124,10 +124,31 @@ queryset.distinct().order_by(field.name).values_list(field_name_flat_long_long=True) ) -# TODO(konstin): We currently can't do call chains that is not the outermost expression +# Fluent style in subexpressions if ( not a() .b() .cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc() ): pass +h2 = ( + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb + + ccccccccccccccccccccccccc() + .dddddddddddddddddddddd() + .eeeeeeeeee() + .ffffffffffffffffffffff() +) + +# Parentheses aren't allowed on statement level, don't use fluent style here +if True: + (alias).filter(content_typeold_content_type).update( + content_typenew_contesadfasfdant_type + ) + +zero( + one, +).two( + three, +).four( + five, +) diff --git a/crates/ruff_python_formatter/src/expression/expr_attribute.rs b/crates/ruff_python_formatter/src/expression/expr_attribute.rs index 6c635531f978d..36dcd52cde590 100644 --- a/crates/ruff_python_formatter/src/expression/expr_attribute.rs +++ b/crates/ruff_python_formatter/src/expression/expr_attribute.rs @@ -4,19 +4,20 @@ use ruff_python_ast::{Constant, Expr, ExprAttribute, ExprConstant}; use crate::comments::{leading_comments, trailing_comments}; use crate::expression::parentheses::{NeedsParentheses, OptionalParentheses, Parentheses}; +use crate::expression::CallChainLayout; use crate::prelude::*; use crate::FormatNodeRule; #[derive(Default)] pub struct FormatExprAttribute { - fluent_style: bool, + call_chain_layout: CallChainLayout, } impl FormatRuleWithOptions> for FormatExprAttribute { - type Options = bool; + type Options = CallChainLayout; fn with_options(mut self, options: Self::Options) -> Self { - self.fluent_style = options; + self.call_chain_layout = options; self } } @@ -49,17 +50,19 @@ impl FormatNodeRule for FormatExprAttribute { value.format().with_options(Parentheses::Always).fmt(f)?; } else { match value.as_ref() { - Expr::Attribute(expr) => expr.format().with_options(self.fluent_style).fmt(f)?, + Expr::Attribute(expr) => { + expr.format().with_options(self.call_chain_layout).fmt(f)? + } Expr::Call(expr) => { - expr.format().with_options(self.fluent_style).fmt(f)?; - if self.fluent_style { + expr.format().with_options(self.call_chain_layout).fmt(f)?; + if self.call_chain_layout == CallChainLayout::Fluent { // Format the dot on its own line soft_line_break().fmt(f)?; } } Expr::Subscript(expr) => { - expr.format().with_options(self.fluent_style).fmt(f)?; - if self.fluent_style { + expr.format().with_options(self.call_chain_layout).fmt(f)?; + if self.call_chain_layout == CallChainLayout::Fluent { // Format the dot on its own line soft_line_break().fmt(f)?; } @@ -72,7 +75,7 @@ impl FormatNodeRule for FormatExprAttribute { hard_line_break().fmt(f)?; } - if self.fluent_style { + if self.call_chain_layout == CallChainLayout::Fluent { // Fluent style has line breaks before the dot // ```python // blogs3 = ( diff --git a/crates/ruff_python_formatter/src/expression/expr_call.rs b/crates/ruff_python_formatter/src/expression/expr_call.rs index c2596577ed629..db4597eaf1bc9 100644 --- a/crates/ruff_python_formatter/src/expression/expr_call.rs +++ b/crates/ruff_python_formatter/src/expression/expr_call.rs @@ -1,3 +1,4 @@ +use crate::expression::CallChainLayout; use ruff_formatter::{write, FormatRuleWithOptions}; use ruff_python_ast::node::AnyNodeRef; use ruff_python_ast::{Expr, ExprCall}; @@ -8,14 +9,14 @@ use crate::FormatNodeRule; #[derive(Default)] pub struct FormatExprCall { - fluent_style: bool, + call_chain_layout: CallChainLayout, } impl FormatRuleWithOptions> for FormatExprCall { - type Options = bool; + type Options = CallChainLayout; fn with_options(mut self, options: Self::Options) -> Self { - self.fluent_style = options; + self.call_chain_layout = options; self } } @@ -29,9 +30,9 @@ impl FormatNodeRule for FormatExprCall { } = item; match func.as_ref() { - Expr::Attribute(expr) => expr.format().with_options(self.fluent_style).fmt(f)?, - Expr::Call(expr) => expr.format().with_options(self.fluent_style).fmt(f)?, - Expr::Subscript(expr) => expr.format().with_options(self.fluent_style).fmt(f)?, + Expr::Attribute(expr) => expr.format().with_options(self.call_chain_layout).fmt(f)?, + Expr::Call(expr) => expr.format().with_options(self.call_chain_layout).fmt(f)?, + Expr::Subscript(expr) => expr.format().with_options(self.call_chain_layout).fmt(f)?, _ => func.format().fmt(f)?, } diff --git a/crates/ruff_python_formatter/src/expression/expr_subscript.rs b/crates/ruff_python_formatter/src/expression/expr_subscript.rs index 4373920991256..656d2d282042f 100644 --- a/crates/ruff_python_formatter/src/expression/expr_subscript.rs +++ b/crates/ruff_python_formatter/src/expression/expr_subscript.rs @@ -7,19 +7,20 @@ use crate::context::PyFormatContext; use crate::context::{NodeLevel, WithNodeLevel}; use crate::expression::expr_tuple::TupleParentheses; use crate::expression::parentheses::{NeedsParentheses, OptionalParentheses}; +use crate::expression::CallChainLayout; use crate::prelude::*; use crate::FormatNodeRule; #[derive(Default)] pub struct FormatExprSubscript { - fluent_style: bool, + call_chain_layout: CallChainLayout, } impl FormatRuleWithOptions> for FormatExprSubscript { - type Options = bool; + type Options = CallChainLayout; fn with_options(mut self, options: Self::Options) -> Self { - self.fluent_style = options; + self.call_chain_layout = options; self } } @@ -41,9 +42,9 @@ impl FormatNodeRule for FormatExprSubscript { ); let format_value = format_with(|f| match value.as_ref() { - Expr::Attribute(expr) => expr.format().with_options(self.fluent_style).fmt(f), - Expr::Call(expr) => expr.format().with_options(self.fluent_style).fmt(f), - Expr::Subscript(expr) => expr.format().with_options(self.fluent_style).fmt(f), + Expr::Attribute(expr) => expr.format().with_options(self.call_chain_layout).fmt(f), + Expr::Call(expr) => expr.format().with_options(self.call_chain_layout).fmt(f), + Expr::Subscript(expr) => expr.format().with_options(self.call_chain_layout).fmt(f), _ => value.format().fmt(f), }); diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index 99ef53c3a53c3..c64c929d559e9 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -65,10 +65,16 @@ impl FormatRule> for FormatExpr { fn fmt(&self, expression: &Expr, f: &mut PyFormatter) -> FormatResult<()> { let parentheses = self.parentheses; - // Nested expressions that are in some outer expression's parentheses may be formatted in - // fluent style - let fluent_style = f.context().node_level() == NodeLevel::ParenthesizedExpression - && is_fluent_style_call_chain(expression, f.context().source()); + // All expressions that can have parentheses can be in fluent style. We need to exclude top + // expressions since they don't get parenthesized + let call_chain_layout = if !matches!( + f.context().node_level(), + NodeLevel::CompoundStatement | NodeLevel::TopLevel + ) { + CallChainLayout::from_expression(expression, f.context().source()) + } else { + CallChainLayout::Default + }; let format_expr = format_with(|f| match expression { Expr::BoolOp(expr) => expr.format().with_options(Some(parentheses)).fmt(f), @@ -87,12 +93,12 @@ impl FormatRule> for FormatExpr { Expr::Yield(expr) => expr.format().fmt(f), Expr::YieldFrom(expr) => expr.format().fmt(f), Expr::Compare(expr) => expr.format().with_options(Some(parentheses)).fmt(f), - Expr::Call(expr) => expr.format().with_options(fluent_style).fmt(f), + Expr::Call(expr) => expr.format().with_options(call_chain_layout).fmt(f), Expr::FormattedValue(expr) => expr.format().fmt(f), Expr::JoinedStr(expr) => expr.format().fmt(f), Expr::Constant(expr) => expr.format().fmt(f), - Expr::Attribute(expr) => expr.format().with_options(fluent_style).fmt(f), - Expr::Subscript(expr) => expr.format().with_options(fluent_style).fmt(f), + Expr::Attribute(expr) => expr.format().with_options(call_chain_layout).fmt(f), + Expr::Subscript(expr) => expr.format().with_options(call_chain_layout).fmt(f), Expr::Starred(expr) => expr.format().fmt(f), Expr::Name(expr) => expr.format().fmt(f), Expr::List(expr) => expr.format().fmt(f), @@ -128,7 +134,7 @@ impl FormatRule> for FormatExpr { // queryset.distinct().order_by(field.name).values_list(field_name_flat_long_long=True) // ) // ``` - if fluent_style { + if call_chain_layout == CallChainLayout::Fluent { write!(f, [group(&format_expr)]) } else { write!(f, [format_expr]) @@ -169,32 +175,38 @@ impl Format> for MaybeParenthesizeExpression<'_> { parenthesize, } = self; - // Fluent style means we always add parentheses, so we don't need to check for existing - // parentheses, comments or `needs_parentheses` - let is_fluent_style = is_fluent_style_call_chain(expression, f.context().source()); - if is_fluent_style { + // Fluent style means that when we break we always add parentheses, so we don't need the + // checks below for existing parentheses, comments or `needs_parentheses` + if CallChainLayout::from_expression(expression, f.context().source()) + == CallChainLayout::Fluent + { match expression { Expr::Attribute(expr) => { - return parenthesize_if_expands(&group(&expr.format().with_options(true))) - .fmt(f) + return parenthesize_if_expands(&group( + &expr.format().with_options(CallChainLayout::Fluent), + )) + .fmt(f) } Expr::Call(expr) => { - return parenthesize_if_expands(&group(&expr.format().with_options(true))) - .fmt(f) + return parenthesize_if_expands(&group( + &expr.format().with_options(CallChainLayout::Fluent), + )) + .fmt(f) } Expr::Subscript(expr) => { - return parenthesize_if_expands(&group(&expr.format().with_options(true))) - .fmt(f) - } - _ => { - // TODO(konstin): We currently can't do call chains that is not the outermost expression + return parenthesize_if_expands(&group( + &expr.format().with_options(CallChainLayout::Fluent), + )) + .fmt(f) } + // `call_chain_layout` check we're in one of the above + _ => unreachable!(), }; } let comments = f.context().comments(); let preserve_parentheses = parenthesize.is_optional() - && is_expression_parenthesized(AnyNodeRef::from(*expression), f.context().source()); + && is_expression_parenthesized((*expression).into(), f.context().source()); let has_comments = comments.has_leading_comments(*expression) || comments.has_trailing_own_line_comments(*expression); @@ -485,18 +497,13 @@ impl<'input> PreorderVisitor<'input> for CanOmitOptionalParenthesesVisitor<'inpu } } -/// Returns `true` if this expression is a call chain that should be formatted in fluent style. -/// /// A call chain consists only of attribute access (`.` operator), function/method calls and /// subscripts. We use fluent style for the call chain if there are at least two attribute dots /// after call parentheses or subscript brackets. In case of fluent style the parentheses/bracket /// will close on the previous line and the dot gets its own line, otherwise the line will start /// with the closing parentheses/bracket and the dot follows immediately after. /// -/// We only use this check if we're already in a parenthesized context, otherwise refer to -/// [`CanOmitOptionalParentheses`]. -/// -/// Below, the left hand side of addition has only a single attribute access after a call, the +/// Below, the left hand side of the addition has only a single attribute access after a call, the /// second `.filter`. The first `.filter` is a call, but it doesn't follow a call. The right hand /// side has two, the `.limit_results` after the call and the `.filter` after the subscript, so it /// gets formatted in fluent style. The outer expression we assign to `blogs` has zero since the @@ -518,31 +525,44 @@ impl<'input> PreorderVisitor<'input> for CanOmitOptionalParenthesesVisitor<'inpu /// ) /// ).all() /// ``` -fn is_fluent_style_call_chain(mut expr: &Expr, source: &str) -> bool { - let mut attributes_after_parentheses = 0; - loop { - match expr { - Expr::Attribute(ast::ExprAttribute { value, .. }) => { - // `f().x` | `data[:100].T` - if matches!(value.as_ref(), Expr::Call(_) | Expr::Subscript(_)) { - attributes_after_parentheses += 1; +#[derive(Copy, Clone, Debug, Default, PartialEq, Eq)] +pub enum CallChainLayout { + #[default] + Default, + Fluent, +} + +impl CallChainLayout { + pub(crate) fn from_expression(mut expr: &Expr, source: &str) -> Self { + let mut attributes_after_parentheses = 0; + loop { + match expr { + Expr::Attribute(ast::ExprAttribute { value, .. }) => { + // `f().x` | `data[:100].T` + if matches!(value.as_ref(), Expr::Call(_) | Expr::Subscript(_)) { + attributes_after_parentheses += 1; + } + expr = value; } - expr = value; - } - Expr::Call(ast::ExprCall { func: inner, .. }) - | Expr::Subscript(ast::ExprSubscript { value: inner, .. }) => { - expr = inner; - } - _ => { - // We to format the following in fluent style: `f2 = (a).w().t(1,)` - if is_expression_parenthesized(AnyNodeRef::from(expr), source) { - attributes_after_parentheses += 1; + Expr::Call(ast::ExprCall { func: inner, .. }) + | Expr::Subscript(ast::ExprSubscript { value: inner, .. }) => { + expr = inner; + } + _ => { + // We to format the following in fluent style: `f2 = (a).w().t(1,)` + if is_expression_parenthesized(AnyNodeRef::from(expr), source) { + attributes_after_parentheses += 1; + } + break; } - break; } } + if attributes_after_parentheses < 2 { + CallChainLayout::Default + } else { + CallChainLayout::Fluent + } } - attributes_after_parentheses >= 2 } fn has_parentheses(expr: &Expr, source: &str) -> bool { diff --git a/crates/ruff_python_formatter/tests/snapshots/format@call_chains.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@call_chains.py.snap index 19f3407776336..707669c7eabb4 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@call_chains.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@call_chains.py.snap @@ -130,13 +130,34 @@ g1 = ( queryset.distinct().order_by(field.name).values_list(field_name_flat_long_long=True) ) -# TODO(konstin): We currently can't do call chains that is not the outermost expression +# Fluent style in subexpressions if ( not a() .b() .cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc() ): pass +h2 = ( + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb + + ccccccccccccccccccccccccc() + .dddddddddddddddddddddd() + .eeeeeeeeee() + .ffffffffffffffffffffff() +) + +# Parentheses aren't allowed on statement level, don't use fluent style here +if True: + (alias).filter(content_typeold_content_type).update( + content_typenew_contesadfasfdant_type + ) + +zero( + one, +).two( + three, +).four( + five, +) ``` ## Output @@ -270,13 +291,34 @@ g1 = ( queryset.distinct().order_by(field.name).values_list(field_name_flat_long_long=True) ) -# Fluent style that is not the outermost expression +# Fluent style in subexpressions if ( not a() .b() .cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc() ): pass +h2 = ( + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb + + ccccccccccccccccccccccccc() + .dddddddddddddddddddddd() + .eeeeeeeeee() + .ffffffffffffffffffffff() +) + +# Parentheses aren't allowed on statement level, don't use fluent style here +if True: + (alias).filter(content_typeold_content_type).update( + content_typenew_contesadfasfdant_type + ) + +zero( + one, +).two( + three, +).four( + five, +) ``` From d72efa36dc5973c360a7a53962b1480df4cc2054 Mon Sep 17 00:00:00 2001 From: konstin Date: Thu, 3 Aug 2023 21:18:34 +0200 Subject: [PATCH 08/15] Clippy --- .../ruff_python_formatter/src/expression/expr_attribute.rs | 2 +- crates/ruff_python_formatter/src/expression/mod.rs | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/ruff_python_formatter/src/expression/expr_attribute.rs b/crates/ruff_python_formatter/src/expression/expr_attribute.rs index 36dcd52cde590..f4f3a0281ab2f 100644 --- a/crates/ruff_python_formatter/src/expression/expr_attribute.rs +++ b/crates/ruff_python_formatter/src/expression/expr_attribute.rs @@ -51,7 +51,7 @@ impl FormatNodeRule for FormatExprAttribute { } else { match value.as_ref() { Expr::Attribute(expr) => { - expr.format().with_options(self.call_chain_layout).fmt(f)? + expr.format().with_options(self.call_chain_layout).fmt(f)?; } Expr::Call(expr) => { expr.format().with_options(self.call_chain_layout).fmt(f)?; diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index c64c929d559e9..1e0f4c1ca5ef5 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -67,13 +67,13 @@ impl FormatRule> for FormatExpr { // All expressions that can have parentheses can be in fluent style. We need to exclude top // expressions since they don't get parenthesized - let call_chain_layout = if !matches!( + let call_chain_layout = if matches!( f.context().node_level(), NodeLevel::CompoundStatement | NodeLevel::TopLevel ) { - CallChainLayout::from_expression(expression, f.context().source()) - } else { CallChainLayout::Default + } else { + CallChainLayout::from_expression(expression, f.context().source()) }; let format_expr = format_with(|f| match expression { From 54b78ac7bdf0728ea0f0d91a426e460046d827e6 Mon Sep 17 00:00:00 2001 From: konstin Date: Thu, 3 Aug 2023 22:06:46 +0200 Subject: [PATCH 09/15] Remove a TODO --- .../resources/test/fixtures/ruff/call_chains.py | 2 -- crates/ruff_python_formatter/src/expression/mod.rs | 2 +- .../tests/snapshots/format@call_chains.py.snap | 4 ---- 3 files changed, 1 insertion(+), 7 deletions(-) diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/call_chains.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/call_chains.py index 89ffeca6b9ac5..90e297663a3ed 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/call_chains.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/call_chains.py @@ -114,8 +114,6 @@ e3 = (1 + 2)[1].w().t() # Treat preserved parentheses correctly -# TODO: The implementation assumes `Parentheses::Preserve`, what's with other -# parentheses? f1 = a.w().t(1,) f2 = (a).w().t(1,) diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index 1e0f4c1ca5ef5..69708f65f2330 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -199,7 +199,7 @@ impl Format> for MaybeParenthesizeExpression<'_> { )) .fmt(f) } - // `call_chain_layout` check we're in one of the above + // `call_chain_layout` checks we're in one of the above _ => unreachable!(), }; } diff --git a/crates/ruff_python_formatter/tests/snapshots/format@call_chains.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@call_chains.py.snap index 707669c7eabb4..6fcb420b783da 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@call_chains.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@call_chains.py.snap @@ -120,8 +120,6 @@ e2 = (1 + 2)().w().t() e3 = (1 + 2)[1].w().t() # Treat preserved parentheses correctly -# TODO: The implementation assumes `Parentheses::Preserve`, what's with other -# parentheses? f1 = a.w().t(1,) f2 = (a).w().t(1,) @@ -274,8 +272,6 @@ e2 = (1 + 2)().w().t() e3 = (1 + 2)[1].w().t() # Treat preserved parentheses correctly -# TODO: The implementation assumes `Parentheses::Preserve`, what's with other -# parentheses? f1 = a.w().t( 1, ) From 9a2ef452d74607e1e4c007b4c40af5a757abcf00 Mon Sep 17 00:00:00 2001 From: konstin Date: Fri, 4 Aug 2023 13:03:09 +0200 Subject: [PATCH 10/15] Move Fluent style out of `Expr` formatting Co-authored-by: Micha Reiser --- .../src/expression/expr_attribute.rs | 29 +++++-- .../src/expression/expr_call.rs | 50 ++++++++++-- .../src/expression/expr_subscript.rs | 27 +++++-- .../src/expression/mod.rs | 80 +++++-------------- 4 files changed, 104 insertions(+), 82 deletions(-) diff --git a/crates/ruff_python_formatter/src/expression/expr_attribute.rs b/crates/ruff_python_formatter/src/expression/expr_attribute.rs index f4f3a0281ab2f..e70316a73b58d 100644 --- a/crates/ruff_python_formatter/src/expression/expr_attribute.rs +++ b/crates/ruff_python_formatter/src/expression/expr_attribute.rs @@ -31,6 +31,17 @@ impl FormatNodeRule for FormatExprAttribute { ctx: _, } = item; + let call_chain_layout = match self.call_chain_layout { + CallChainLayout::Default => { + if f.context().node_level().is_parenthesized() { + CallChainLayout::from_expression(AnyNodeRef::from(item), f.context().source()) + } else { + CallChainLayout::NonFluent + } + } + layout @ (CallChainLayout::Fluent | CallChainLayout::NonFluent) => layout, + }; + let needs_parentheses = matches!( value.as_ref(), Expr::Constant(ExprConstant { @@ -51,18 +62,18 @@ impl FormatNodeRule for FormatExprAttribute { } else { match value.as_ref() { Expr::Attribute(expr) => { - expr.format().with_options(self.call_chain_layout).fmt(f)?; + expr.format().with_options(call_chain_layout).fmt(f)?; } Expr::Call(expr) => { - expr.format().with_options(self.call_chain_layout).fmt(f)?; - if self.call_chain_layout == CallChainLayout::Fluent { + expr.format().with_options(call_chain_layout).fmt(f)?; + if call_chain_layout == CallChainLayout::Fluent { // Format the dot on its own line soft_line_break().fmt(f)?; } } Expr::Subscript(expr) => { - expr.format().with_options(self.call_chain_layout).fmt(f)?; - if self.call_chain_layout == CallChainLayout::Fluent { + expr.format().with_options(call_chain_layout).fmt(f)?; + if call_chain_layout == CallChainLayout::Fluent { // Format the dot on its own line soft_line_break().fmt(f)?; } @@ -75,7 +86,7 @@ impl FormatNodeRule for FormatExprAttribute { hard_line_break().fmt(f)?; } - if self.call_chain_layout == CallChainLayout::Fluent { + if call_chain_layout == CallChainLayout::Fluent { // Fluent style has line breaks before the dot // ```python // blogs3 = ( @@ -139,7 +150,11 @@ impl NeedsParentheses for ExprAttribute { context: &PyFormatContext, ) -> OptionalParentheses { // Checks if there are any own line comments in an attribute chain (a.b.c). - if context + if CallChainLayout::from_expression(self.into(), context.source()) + == CallChainLayout::Fluent + { + OptionalParentheses::Multiline + } else if context .comments() .dangling_comments(self) .iter() diff --git a/crates/ruff_python_formatter/src/expression/expr_call.rs b/crates/ruff_python_formatter/src/expression/expr_call.rs index db4597eaf1bc9..822d0cdc1be06 100644 --- a/crates/ruff_python_formatter/src/expression/expr_call.rs +++ b/crates/ruff_python_formatter/src/expression/expr_call.rs @@ -1,6 +1,7 @@ use crate::expression::CallChainLayout; use ruff_formatter::{write, FormatRuleWithOptions}; use ruff_python_ast::node::AnyNodeRef; +use ruff_python_ast::Expr::Call; use ruff_python_ast::{Expr, ExprCall}; use crate::expression::parentheses::{NeedsParentheses, OptionalParentheses}; @@ -29,14 +30,41 @@ impl FormatNodeRule for FormatExprCall { arguments, } = item; - match func.as_ref() { - Expr::Attribute(expr) => expr.format().with_options(self.call_chain_layout).fmt(f)?, - Expr::Call(expr) => expr.format().with_options(self.call_chain_layout).fmt(f)?, - Expr::Subscript(expr) => expr.format().with_options(self.call_chain_layout).fmt(f)?, - _ => func.format().fmt(f)?, - } + let call_chain_layout = match self.call_chain_layout { + CallChainLayout::Default => { + if f.context().node_level().is_parenthesized() { + CallChainLayout::from_expression(AnyNodeRef::from(item), f.context().source()) + } else { + CallChainLayout::NonFluent + } + } + layout @ (CallChainLayout::Fluent | CallChainLayout::NonFluent) => layout, + }; + + let fmt_inner = format_with(|f| { + match func.as_ref() { + Expr::Attribute(expr) => expr.format().with_options(call_chain_layout).fmt(f)?, + Expr::Call(expr) => expr.format().with_options(call_chain_layout).fmt(f)?, + Expr::Subscript(expr) => expr.format().with_options(call_chain_layout).fmt(f)?, + _ => func.format().fmt(f)?, + } + + arguments.format().fmt(f) + }); - write!(f, [arguments.format()]) + // Allow to indent the parentheses while + // ```python + // g1 = ( + // queryset.distinct().order_by(field.name).values_list(field_name_flat_long_long=True) + // ) + // ``` + if call_chain_layout == CallChainLayout::Fluent + && self.call_chain_layout == CallChainLayout::Default + { + group(&fmt_inner).fmt(f) + } else { + fmt_inner.fmt(f) + } } } @@ -46,6 +74,12 @@ impl NeedsParentheses for ExprCall { _parent: AnyNodeRef, context: &PyFormatContext, ) -> OptionalParentheses { - self.func.needs_parentheses(self.into(), context) + if CallChainLayout::from_expression(self.into(), context.source()) + == CallChainLayout::Fluent + { + OptionalParentheses::Multiline + } else { + self.func.needs_parentheses(self.into(), context) + } } } diff --git a/crates/ruff_python_formatter/src/expression/expr_subscript.rs b/crates/ruff_python_formatter/src/expression/expr_subscript.rs index 656d2d282042f..38b52b839995a 100644 --- a/crates/ruff_python_formatter/src/expression/expr_subscript.rs +++ b/crates/ruff_python_formatter/src/expression/expr_subscript.rs @@ -34,6 +34,17 @@ impl FormatNodeRule for FormatExprSubscript { ctx: _, } = item; + let call_chain_layout = match self.call_chain_layout { + CallChainLayout::Default => { + if f.context().node_level().is_parenthesized() { + CallChainLayout::from_expression(AnyNodeRef::from(item), f.context().source()) + } else { + CallChainLayout::NonFluent + } + } + layout @ (CallChainLayout::Fluent | CallChainLayout::NonFluent) => layout, + }; + let comments = f.context().comments().clone(); let dangling_comments = comments.dangling_comments(item.as_any_node_ref()); debug_assert!( @@ -42,9 +53,9 @@ impl FormatNodeRule for FormatExprSubscript { ); let format_value = format_with(|f| match value.as_ref() { - Expr::Attribute(expr) => expr.format().with_options(self.call_chain_layout).fmt(f), - Expr::Call(expr) => expr.format().with_options(self.call_chain_layout).fmt(f), - Expr::Subscript(expr) => expr.format().with_options(self.call_chain_layout).fmt(f), + Expr::Attribute(expr) => expr.format().with_options(call_chain_layout).fmt(f), + Expr::Call(expr) => expr.format().with_options(call_chain_layout).fmt(f), + Expr::Subscript(expr) => expr.format().with_options(call_chain_layout).fmt(f), _ => value.format().fmt(f), }); @@ -91,10 +102,16 @@ impl NeedsParentheses for ExprSubscript { fn needs_parentheses( &self, _parent: AnyNodeRef, - _context: &PyFormatContext, + context: &PyFormatContext, ) -> OptionalParentheses { { - OptionalParentheses::Never + if CallChainLayout::from_expression(self.into(), context.source()) + == CallChainLayout::Fluent + { + OptionalParentheses::Multiline + } else { + self.value.needs_parentheses(self.into(), context) + } } } } diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index 69708f65f2330..63803a7482332 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -65,17 +65,6 @@ impl FormatRule> for FormatExpr { fn fmt(&self, expression: &Expr, f: &mut PyFormatter) -> FormatResult<()> { let parentheses = self.parentheses; - // All expressions that can have parentheses can be in fluent style. We need to exclude top - // expressions since they don't get parenthesized - let call_chain_layout = if matches!( - f.context().node_level(), - NodeLevel::CompoundStatement | NodeLevel::TopLevel - ) { - CallChainLayout::Default - } else { - CallChainLayout::from_expression(expression, f.context().source()) - }; - let format_expr = format_with(|f| match expression { Expr::BoolOp(expr) => expr.format().with_options(Some(parentheses)).fmt(f), Expr::NamedExpr(expr) => expr.format().fmt(f), @@ -93,12 +82,12 @@ impl FormatRule> for FormatExpr { Expr::Yield(expr) => expr.format().fmt(f), Expr::YieldFrom(expr) => expr.format().fmt(f), Expr::Compare(expr) => expr.format().with_options(Some(parentheses)).fmt(f), - Expr::Call(expr) => expr.format().with_options(call_chain_layout).fmt(f), + Expr::Call(expr) => expr.format().fmt(f), Expr::FormattedValue(expr) => expr.format().fmt(f), Expr::JoinedStr(expr) => expr.format().fmt(f), Expr::Constant(expr) => expr.format().fmt(f), - Expr::Attribute(expr) => expr.format().with_options(call_chain_layout).fmt(f), - Expr::Subscript(expr) => expr.format().with_options(call_chain_layout).fmt(f), + Expr::Attribute(expr) => expr.format().fmt(f), + Expr::Subscript(expr) => expr.format().fmt(f), Expr::Starred(expr) => expr.format().fmt(f), Expr::Name(expr) => expr.format().fmt(f), Expr::List(expr) => expr.format().fmt(f), @@ -128,17 +117,7 @@ impl FormatRule> for FormatExpr { let mut f = WithNodeLevel::new(level, f); - // Allow to indent the parentheses while - // ```python - // g1 = ( - // queryset.distinct().order_by(field.name).values_list(field_name_flat_long_long=True) - // ) - // ``` - if call_chain_layout == CallChainLayout::Fluent { - write!(f, [group(&format_expr)]) - } else { - write!(f, [format_expr]) - } + write!(f, [format_expr]) } } } @@ -175,35 +154,6 @@ impl Format> for MaybeParenthesizeExpression<'_> { parenthesize, } = self; - // Fluent style means that when we break we always add parentheses, so we don't need the - // checks below for existing parentheses, comments or `needs_parentheses` - if CallChainLayout::from_expression(expression, f.context().source()) - == CallChainLayout::Fluent - { - match expression { - Expr::Attribute(expr) => { - return parenthesize_if_expands(&group( - &expr.format().with_options(CallChainLayout::Fluent), - )) - .fmt(f) - } - Expr::Call(expr) => { - return parenthesize_if_expands(&group( - &expr.format().with_options(CallChainLayout::Fluent), - )) - .fmt(f) - } - Expr::Subscript(expr) => { - return parenthesize_if_expands(&group( - &expr.format().with_options(CallChainLayout::Fluent), - )) - .fmt(f) - } - // `call_chain_layout` checks we're in one of the above - _ => unreachable!(), - }; - } - let comments = f.context().comments(); let preserve_parentheses = parenthesize.is_optional() && is_expression_parenthesized((*expression).into(), f.context().source()); @@ -527,30 +477,36 @@ impl<'input> PreorderVisitor<'input> for CanOmitOptionalParenthesesVisitor<'inpu /// ``` #[derive(Copy, Clone, Debug, Default, PartialEq, Eq)] pub enum CallChainLayout { + /// The root of a call chain #[default] Default, + + /// A nested call chain element that uses fluent style. Fluent, + + /// A nested call chain element not using fluent style. + NonFluent, } impl CallChainLayout { - pub(crate) fn from_expression(mut expr: &Expr, source: &str) -> Self { + pub(crate) fn from_expression(mut expr: AnyNodeRef, source: &str) -> Self { let mut attributes_after_parentheses = 0; loop { match expr { - Expr::Attribute(ast::ExprAttribute { value, .. }) => { + AnyNodeRef::ExprAttribute(ast::ExprAttribute { value, .. }) => { // `f().x` | `data[:100].T` if matches!(value.as_ref(), Expr::Call(_) | Expr::Subscript(_)) { attributes_after_parentheses += 1; } - expr = value; + expr = AnyNodeRef::from(value.as_ref()); } - Expr::Call(ast::ExprCall { func: inner, .. }) - | Expr::Subscript(ast::ExprSubscript { value: inner, .. }) => { - expr = inner; + AnyNodeRef::ExprCall(ast::ExprCall { func: inner, .. }) + | AnyNodeRef::ExprSubscript(ast::ExprSubscript { value: inner, .. }) => { + expr = AnyNodeRef::from(inner.as_ref()); } _ => { // We to format the following in fluent style: `f2 = (a).w().t(1,)` - if is_expression_parenthesized(AnyNodeRef::from(expr), source) { + if is_expression_parenthesized(expr, source) { attributes_after_parentheses += 1; } break; @@ -558,7 +514,7 @@ impl CallChainLayout { } } if attributes_after_parentheses < 2 { - CallChainLayout::Default + CallChainLayout::NonFluent } else { CallChainLayout::Fluent } From 2699439635b87e727f9f00144f4b35375b3fffdb Mon Sep 17 00:00:00 2001 From: konstin Date: Fri, 4 Aug 2023 13:29:38 +0200 Subject: [PATCH 11/15] Extract `format_call_chain_layout` function --- .../src/expression/expr_attribute.rs | 13 ++----------- .../src/expression/expr_call.rs | 16 +++------------- .../src/expression/expr_subscript.rs | 13 ++----------- .../src/expression/mod.rs | 18 ++++++++++++++++++ 4 files changed, 25 insertions(+), 35 deletions(-) diff --git a/crates/ruff_python_formatter/src/expression/expr_attribute.rs b/crates/ruff_python_formatter/src/expression/expr_attribute.rs index e70316a73b58d..8270c71847a0e 100644 --- a/crates/ruff_python_formatter/src/expression/expr_attribute.rs +++ b/crates/ruff_python_formatter/src/expression/expr_attribute.rs @@ -4,7 +4,7 @@ use ruff_python_ast::{Constant, Expr, ExprAttribute, ExprConstant}; use crate::comments::{leading_comments, trailing_comments}; use crate::expression::parentheses::{NeedsParentheses, OptionalParentheses, Parentheses}; -use crate::expression::CallChainLayout; +use crate::expression::{format_call_chain_layout, CallChainLayout}; use crate::prelude::*; use crate::FormatNodeRule; @@ -31,16 +31,7 @@ impl FormatNodeRule for FormatExprAttribute { ctx: _, } = item; - let call_chain_layout = match self.call_chain_layout { - CallChainLayout::Default => { - if f.context().node_level().is_parenthesized() { - CallChainLayout::from_expression(AnyNodeRef::from(item), f.context().source()) - } else { - CallChainLayout::NonFluent - } - } - layout @ (CallChainLayout::Fluent | CallChainLayout::NonFluent) => layout, - }; + let call_chain_layout = format_call_chain_layout(f, self.call_chain_layout, item); let needs_parentheses = matches!( value.as_ref(), diff --git a/crates/ruff_python_formatter/src/expression/expr_call.rs b/crates/ruff_python_formatter/src/expression/expr_call.rs index 822d0cdc1be06..f00b94d52d06a 100644 --- a/crates/ruff_python_formatter/src/expression/expr_call.rs +++ b/crates/ruff_python_formatter/src/expression/expr_call.rs @@ -1,7 +1,6 @@ -use crate::expression::CallChainLayout; -use ruff_formatter::{write, FormatRuleWithOptions}; +use crate::expression::{format_call_chain_layout, CallChainLayout}; +use ruff_formatter::FormatRuleWithOptions; use ruff_python_ast::node::AnyNodeRef; -use ruff_python_ast::Expr::Call; use ruff_python_ast::{Expr, ExprCall}; use crate::expression::parentheses::{NeedsParentheses, OptionalParentheses}; @@ -30,16 +29,7 @@ impl FormatNodeRule for FormatExprCall { arguments, } = item; - let call_chain_layout = match self.call_chain_layout { - CallChainLayout::Default => { - if f.context().node_level().is_parenthesized() { - CallChainLayout::from_expression(AnyNodeRef::from(item), f.context().source()) - } else { - CallChainLayout::NonFluent - } - } - layout @ (CallChainLayout::Fluent | CallChainLayout::NonFluent) => layout, - }; + let call_chain_layout = format_call_chain_layout(f, self.call_chain_layout, item); let fmt_inner = format_with(|f| { match func.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 38b52b839995a..901112102b821 100644 --- a/crates/ruff_python_formatter/src/expression/expr_subscript.rs +++ b/crates/ruff_python_formatter/src/expression/expr_subscript.rs @@ -7,7 +7,7 @@ use crate::context::PyFormatContext; use crate::context::{NodeLevel, WithNodeLevel}; use crate::expression::expr_tuple::TupleParentheses; use crate::expression::parentheses::{NeedsParentheses, OptionalParentheses}; -use crate::expression::CallChainLayout; +use crate::expression::{format_call_chain_layout, CallChainLayout}; use crate::prelude::*; use crate::FormatNodeRule; @@ -34,16 +34,7 @@ impl FormatNodeRule for FormatExprSubscript { ctx: _, } = item; - let call_chain_layout = match self.call_chain_layout { - CallChainLayout::Default => { - if f.context().node_level().is_parenthesized() { - CallChainLayout::from_expression(AnyNodeRef::from(item), f.context().source()) - } else { - CallChainLayout::NonFluent - } - } - layout @ (CallChainLayout::Fluent | CallChainLayout::NonFluent) => layout, - }; + let call_chain_layout = format_call_chain_layout(f, self.call_chain_layout, item); let comments = f.context().comments().clone(); let dangling_comments = comments.dangling_comments(item.as_any_node_ref()); diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index 63803a7482332..f9468b22f516f 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -578,3 +578,21 @@ impl From for OperatorPriority { } } } + +/// Determine whether to actually apply fluent layout in attribute, call and subscript formatting +pub(crate) fn format_call_chain_layout<'a>( + f: &mut PyFormatter, + layout: CallChainLayout, + item: impl Into>, +) -> CallChainLayout { + match layout { + CallChainLayout::Default => { + if f.context().node_level().is_parenthesized() { + CallChainLayout::from_expression(item.into(), f.context().source()) + } else { + CallChainLayout::NonFluent + } + } + layout @ (CallChainLayout::Fluent | CallChainLayout::NonFluent) => layout, + } +} From 59e040c8d895add3b8cb3eb7fafd26901402cb80 Mon Sep 17 00:00:00 2001 From: konstin Date: Fri, 4 Aug 2023 13:31:07 +0200 Subject: [PATCH 12/15] Move function --- .../src/expression/mod.rs | 36 +++++++++---------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index f9468b22f516f..6da8b595724b1 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -521,6 +521,24 @@ impl CallChainLayout { } } +/// Determine whether to actually apply fluent layout in attribute, call and subscript formatting +pub(crate) fn format_call_chain_layout<'a>( + f: &mut PyFormatter, + layout: CallChainLayout, + item: impl Into>, +) -> CallChainLayout { + match layout { + CallChainLayout::Default => { + if f.context().node_level().is_parenthesized() { + CallChainLayout::from_expression(item.into(), f.context().source()) + } else { + CallChainLayout::NonFluent + } + } + layout @ (CallChainLayout::Fluent | CallChainLayout::NonFluent) => layout, + } +} + fn has_parentheses(expr: &Expr, source: &str) -> bool { has_own_parentheses(expr) || is_expression_parenthesized(AnyNodeRef::from(expr), source) } @@ -578,21 +596,3 @@ impl From for OperatorPriority { } } } - -/// Determine whether to actually apply fluent layout in attribute, call and subscript formatting -pub(crate) fn format_call_chain_layout<'a>( - f: &mut PyFormatter, - layout: CallChainLayout, - item: impl Into>, -) -> CallChainLayout { - match layout { - CallChainLayout::Default => { - if f.context().node_level().is_parenthesized() { - CallChainLayout::from_expression(item.into(), f.context().source()) - } else { - CallChainLayout::NonFluent - } - } - layout @ (CallChainLayout::Fluent | CallChainLayout::NonFluent) => layout, - } -} From 619562176234b24a2217ade78fcbf0ec8a6750f7 Mon Sep 17 00:00:00 2001 From: konstin Date: Fri, 4 Aug 2023 13:33:28 +0200 Subject: [PATCH 13/15] Refactoring --- .../src/expression/expr_attribute.rs | 4 +-- .../src/expression/expr_call.rs | 4 +-- .../src/expression/expr_subscript.rs | 4 +-- .../src/expression/mod.rs | 29 ++++++++++--------- 4 files changed, 21 insertions(+), 20 deletions(-) diff --git a/crates/ruff_python_formatter/src/expression/expr_attribute.rs b/crates/ruff_python_formatter/src/expression/expr_attribute.rs index 8270c71847a0e..bce209252abfe 100644 --- a/crates/ruff_python_formatter/src/expression/expr_attribute.rs +++ b/crates/ruff_python_formatter/src/expression/expr_attribute.rs @@ -4,7 +4,7 @@ use ruff_python_ast::{Constant, Expr, ExprAttribute, ExprConstant}; use crate::comments::{leading_comments, trailing_comments}; use crate::expression::parentheses::{NeedsParentheses, OptionalParentheses, Parentheses}; -use crate::expression::{format_call_chain_layout, CallChainLayout}; +use crate::expression::CallChainLayout; use crate::prelude::*; use crate::FormatNodeRule; @@ -31,7 +31,7 @@ impl FormatNodeRule for FormatExprAttribute { ctx: _, } = item; - let call_chain_layout = format_call_chain_layout(f, self.call_chain_layout, item); + let call_chain_layout = self.call_chain_layout.apply_in_node(item, f); let needs_parentheses = matches!( value.as_ref(), diff --git a/crates/ruff_python_formatter/src/expression/expr_call.rs b/crates/ruff_python_formatter/src/expression/expr_call.rs index f00b94d52d06a..5483aabdcaee8 100644 --- a/crates/ruff_python_formatter/src/expression/expr_call.rs +++ b/crates/ruff_python_formatter/src/expression/expr_call.rs @@ -1,4 +1,4 @@ -use crate::expression::{format_call_chain_layout, CallChainLayout}; +use crate::expression::CallChainLayout; use ruff_formatter::FormatRuleWithOptions; use ruff_python_ast::node::AnyNodeRef; use ruff_python_ast::{Expr, ExprCall}; @@ -29,7 +29,7 @@ impl FormatNodeRule for FormatExprCall { arguments, } = item; - let call_chain_layout = format_call_chain_layout(f, self.call_chain_layout, item); + let call_chain_layout = self.call_chain_layout.apply_in_node(item, f); let fmt_inner = format_with(|f| { match func.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 901112102b821..1a2304801d990 100644 --- a/crates/ruff_python_formatter/src/expression/expr_subscript.rs +++ b/crates/ruff_python_formatter/src/expression/expr_subscript.rs @@ -7,7 +7,7 @@ use crate::context::PyFormatContext; use crate::context::{NodeLevel, WithNodeLevel}; use crate::expression::expr_tuple::TupleParentheses; use crate::expression::parentheses::{NeedsParentheses, OptionalParentheses}; -use crate::expression::{format_call_chain_layout, CallChainLayout}; +use crate::expression::CallChainLayout; use crate::prelude::*; use crate::FormatNodeRule; @@ -34,7 +34,7 @@ impl FormatNodeRule for FormatExprSubscript { ctx: _, } = item; - let call_chain_layout = format_call_chain_layout(f, self.call_chain_layout, item); + let call_chain_layout = self.call_chain_layout.apply_in_node(item, f); let comments = f.context().comments().clone(); let dangling_comments = comments.dangling_comments(item.as_any_node_ref()); diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index 6da8b595724b1..a2516fe03bb42 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -519,23 +519,24 @@ impl CallChainLayout { CallChainLayout::Fluent } } -} -/// Determine whether to actually apply fluent layout in attribute, call and subscript formatting -pub(crate) fn format_call_chain_layout<'a>( - f: &mut PyFormatter, - layout: CallChainLayout, - item: impl Into>, -) -> CallChainLayout { - match layout { - CallChainLayout::Default => { - if f.context().node_level().is_parenthesized() { - CallChainLayout::from_expression(item.into(), f.context().source()) - } else { - CallChainLayout::NonFluent + /// Determine whether to actually apply fluent layout in attribute, call and subscript + /// formatting + pub(crate) fn apply_in_node<'a>( + self, + item: impl Into>, + f: &mut PyFormatter, + ) -> CallChainLayout { + match self { + CallChainLayout::Default => { + if f.context().node_level().is_parenthesized() { + CallChainLayout::from_expression(item.into(), f.context().source()) + } else { + CallChainLayout::NonFluent + } } + layout @ (CallChainLayout::Fluent | CallChainLayout::NonFluent) => layout, } - layout @ (CallChainLayout::Fluent | CallChainLayout::NonFluent) => layout, } } From c308eba3405e2948d72a47076dacdcffcfbda474 Mon Sep 17 00:00:00 2001 From: konstin Date: Fri, 4 Aug 2023 14:16:16 +0200 Subject: [PATCH 14/15] Add failing test case --- .../resources/test/fixtures/ruff/call_chains.py | 3 +++ .../src/expression/parentheses.rs | 17 +++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/call_chains.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/call_chains.py index 90e297663a3ed..6a1e0903931d3 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/call_chains.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/call_chains.py @@ -150,3 +150,6 @@ ).four( five, ) + +a = (b().c("asdfasfaefinitive Guidddddde to Django: Web Developfdddddddment Done Rdddight")).d() +a = b().c("asdfasfaefinitive Guidddddde to Django: Web Developfdddddddment Done Rdddight").d() diff --git a/crates/ruff_python_formatter/src/expression/parentheses.rs b/crates/ruff_python_formatter/src/expression/parentheses.rs index abe7cc9d623c2..5e49aa1a7c090 100644 --- a/crates/ruff_python_formatter/src/expression/parentheses.rs +++ b/crates/ruff_python_formatter/src/expression/parentheses.rs @@ -285,3 +285,20 @@ impl<'ast> Format> for FormatInParenthesesOnlyGroup<'_, 'a } } } + +#[cfg(test)] +mod tests { + use crate::expression::parentheses::is_expression_parenthesized; + use ruff_python_ast::node::AnyNodeRef; + use ruff_python_parser::parse_expression; + + #[test] + fn test_has_parentheses() { + let expression = r#"(b().c("")).d()"#; + let expr = parse_expression(expression, "").unwrap(); + assert!(!is_expression_parenthesized( + AnyNodeRef::from(&expr), + expression + )); + } +} From 0274a0b6bc102bcd7ff664e196858bc516c4f1e1 Mon Sep 17 00:00:00 2001 From: konstin Date: Fri, 4 Aug 2023 15:37:59 +0200 Subject: [PATCH 15/15] Correctly preserve parentheses --- .../test/fixtures/ruff/call_chains.py | 10 +++--- .../src/expression/expr_attribute.rs | 19 ++++++++++-- .../src/expression/mod.rs | 31 +++++++++++++++++-- .../snapshots/format@call_chains.py.snap | 29 ++++++++++++++--- 4 files changed, 74 insertions(+), 15 deletions(-) diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/call_chains.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/call_chains.py index 6a1e0903931d3..67b702c7055ac 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/call_chains.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/call_chains.py @@ -114,8 +114,11 @@ e3 = (1 + 2)[1].w().t() # Treat preserved parentheses correctly -f1 = a.w().t(1,) -f2 = (a).w().t(1,) +f1 = (b().c()).d(1,) +f2 = b().c().d(1,) +f3 = (b).c().d(1,) +f4 = (a)(b).c(1,) +f5 = (a.b()).c(1,) # Indent in the parentheses without breaking g1 = ( @@ -151,5 +154,4 @@ five, ) -a = (b().c("asdfasfaefinitive Guidddddde to Django: Web Developfdddddddment Done Rdddight")).d() -a = b().c("asdfasfaefinitive Guidddddde to Django: Web Developfdddddddment Done Rdddight").d() + diff --git a/crates/ruff_python_formatter/src/expression/expr_attribute.rs b/crates/ruff_python_formatter/src/expression/expr_attribute.rs index bce209252abfe..f734a5fc5bef6 100644 --- a/crates/ruff_python_formatter/src/expression/expr_attribute.rs +++ b/crates/ruff_python_formatter/src/expression/expr_attribute.rs @@ -3,7 +3,9 @@ use ruff_python_ast::node::AnyNodeRef; use ruff_python_ast::{Constant, Expr, ExprAttribute, ExprConstant}; use crate::comments::{leading_comments, trailing_comments}; -use crate::expression::parentheses::{NeedsParentheses, OptionalParentheses, Parentheses}; +use crate::expression::parentheses::{ + is_expression_parenthesized, NeedsParentheses, OptionalParentheses, Parentheses, +}; use crate::expression::CallChainLayout; use crate::prelude::*; use crate::FormatNodeRule; @@ -50,7 +52,7 @@ impl FormatNodeRule for FormatExprAttribute { if needs_parentheses { value.format().with_options(Parentheses::Always).fmt(f)?; - } else { + } else if call_chain_layout == CallChainLayout::Fluent { match value.as_ref() { Expr::Attribute(expr) => { expr.format().with_options(call_chain_layout).fmt(f)?; @@ -69,8 +71,19 @@ impl FormatNodeRule for FormatExprAttribute { soft_line_break().fmt(f)?; } } - _ => value.format().fmt(f)?, + _ => { + // This matches [`CallChainLayout::from_expression`] + if is_expression_parenthesized(value.as_ref().into(), f.context().source()) { + value.format().with_options(Parentheses::Always).fmt(f)?; + // Format the dot on its own line + soft_line_break().fmt(f)?; + } else { + value.format().fmt(f)?; + } + } } + } else { + value.format().fmt(f)?; } if comments.has_trailing_own_line_comments(value.as_ref()) { diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index a2516fe03bb42..48a946345909c 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -494,24 +494,49 @@ impl CallChainLayout { loop { match expr { AnyNodeRef::ExprAttribute(ast::ExprAttribute { value, .. }) => { - // `f().x` | `data[:100].T` + expr = AnyNodeRef::from(value.as_ref()); + // ``` + // f().g + // ^^^ value + // data[:100].T` + // ^^^^^^^^^^ value + // ``` if matches!(value.as_ref(), Expr::Call(_) | Expr::Subscript(_)) { attributes_after_parentheses += 1; + } else if is_expression_parenthesized(expr, source) { + // `(a).b`. We preserve these parentheses so don't recurse + attributes_after_parentheses += 1; + break; } - expr = AnyNodeRef::from(value.as_ref()); } + // ``` + // f() + // ^^^ expr + // ^ func + // data[:100] + // ^^^^^^^^^^ expr + // ^^^^ value + // ``` AnyNodeRef::ExprCall(ast::ExprCall { func: inner, .. }) | AnyNodeRef::ExprSubscript(ast::ExprSubscript { value: inner, .. }) => { expr = AnyNodeRef::from(inner.as_ref()); } _ => { - // We to format the following in fluent style: `f2 = (a).w().t(1,)` + // We to format the following in fluent style: + // ``` + // f2 = (a).w().t(1,) + // ^ expr + // ``` if is_expression_parenthesized(expr, source) { attributes_after_parentheses += 1; } break; } } + // We preserve these parentheses so don't recurse + if is_expression_parenthesized(expr, source) { + break; + } } if attributes_after_parentheses < 2 { CallChainLayout::NonFluent diff --git a/crates/ruff_python_formatter/tests/snapshots/format@call_chains.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@call_chains.py.snap index 6fcb420b783da..cca9b84a2f2df 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@call_chains.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@call_chains.py.snap @@ -120,8 +120,11 @@ e2 = (1 + 2)().w().t() e3 = (1 + 2)[1].w().t() # Treat preserved parentheses correctly -f1 = a.w().t(1,) -f2 = (a).w().t(1,) +f1 = (b().c()).d(1,) +f2 = b().c().d(1,) +f3 = (b).c().d(1,) +f4 = (a)(b).c(1,) +f5 = (a.b()).c(1,) # Indent in the parentheses without breaking g1 = ( @@ -156,6 +159,8 @@ zero( ).four( five, ) + + ``` ## Output @@ -272,15 +277,29 @@ e2 = (1 + 2)().w().t() e3 = (1 + 2)[1].w().t() # Treat preserved parentheses correctly -f1 = a.w().t( +f1 = (b().c()).d( 1, ) f2 = ( - (a).w() - .t( + b() + .c() + .d( + 1, + ) +) +f3 = ( + (b) + .c() + .d( 1, ) ) +f4 = (a)(b).c( + 1, +) +f5 = (a.b()).c( + 1, +) # Indent in the parentheses without breaking g1 = (