Use BestFits for non-call chain call expressions

crates/ruff_formatter/src/format_element/document.rs

@@ -661,18 +661,17 @@ impl Format<IrFormatContext<'_>> for ContentArrayEnd {
 
 impl FormatElements for [FormatElement] {
     fn will_break(&self) -> bool {
-        use Tag::{EndLineSuffix, StartLineSuffix};
         let mut ignore_depth = 0usize;
 
         for element in self {
             match element {
                 // Line suffix
                 // Ignore if any of its content breaks
-                FormatElement::Tag(StartLineSuffix) => {
+                FormatElement::Tag(Tag::StartLineSuffix | Tag::StartFitsExpanded(_)) => {
                     ignore_depth += 1;
                 }
-                FormatElement::Tag(EndLineSuffix) => {
-                    ignore_depth -= 1;
+                FormatElement::Tag(Tag::EndLineSuffix | Tag::EndFitsExpanded) => {
+                    ignore_depth = ignore_depth.saturating_sub(1);
                 }
                 FormatElement::Interned(interned) if ignore_depth == 0 => {
                     if interned.will_break() {

crates/ruff_python_formatter/Cargo.toml

@@ -25,6 +25,7 @@ clap = { workspace = true }
 countme = "3.0.1"
 is-macro = { workspace = true }
 itertools = { workspace = true }
+memchr = { workspace = true }
 once_cell = { workspace = true }
 rustc-hash = { workspace = true }
 serde = { workspace = true, optional = true }

crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/unspittable.py

@@ -52,3 +52,8 @@
     expression
 ) + expression.get_db_converters(connection):
     ...
+
+
+aaa = (
+    bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb # awkward comment
+)

crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/yield.py

@@ -66,6 +66,13 @@ def foo():
         1
     )
 
+    yield (
+        "#   * Make sure each ForeignKey and OneToOneField has `on_delete` set "
+        "to the desired behavior"
+    )
+
+    yield aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb + ccccccccccccccccccccccccccccccccccccccccccccccccccccccc
+
 yield ("Cache key will cause errors if used with memcached: %r " "(longer than %s)" % (
     key,
     MEMCACHE_MAX_KEY_LENGTH,

crates/ruff_python_formatter/src/expression/expr_call.rs

@@ -1,5 +1,6 @@
 use crate::expression::CallChainLayout;
-use ruff_formatter::FormatRuleWithOptions;
+
+use ruff_formatter::{format_args, write, FormatRuleWithOptions};
 use ruff_python_ast::node::AnyNodeRef;
 use ruff_python_ast::{Expr, ExprCall};
 
@@ -31,15 +32,11 @@ impl FormatNodeRule<ExprCall> for FormatExprCall {
 
         let call_chain_layout = self.call_chain_layout.apply_in_node(item, f);
 
-        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)
+        let fmt_func = 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),
         });
 
         // Allow to indent the parentheses while
@@ -51,9 +48,9 @@ impl FormatNodeRule<ExprCall> for FormatExprCall {
         if call_chain_layout == CallChainLayout::Fluent
             && self.call_chain_layout == CallChainLayout::Default
         {
-            group(&fmt_inner).fmt(f)
+            group(&format_args![fmt_func, arguments.format()]).fmt(f)
         } else {
-            fmt_inner.fmt(f)
+            write!(f, [fmt_func, arguments.format()])
         }
     }
 }
@@ -70,7 +67,7 @@ impl NeedsParentheses for ExprCall {
             OptionalParentheses::Multiline
         } else {
             match self.func.needs_parentheses(self.into(), context) {
-                OptionalParentheses::IfFits => OptionalParentheses::Never,
+                OptionalParentheses::IfFits => OptionalParentheses::IfFits,
                 parentheses => parentheses,
             }
         }

crates/ruff_python_formatter/src/expression/expr_constant.rs

@@ -79,13 +79,10 @@ impl NeedsParentheses for ExprConstant {
         _parent: AnyNodeRef,
         context: &PyFormatContext,
     ) -> OptionalParentheses {
-        if self.value.is_implicit_concatenated() {
-            // Don't wrap triple quoted strings
-            if is_multiline_string(self, context.source()) {
-                OptionalParentheses::Never
-            } else {
-                OptionalParentheses::Multiline
-            }
+        if is_multiline_string(self, context.source()) {
+            OptionalParentheses::Never
+        } else if self.value.is_implicit_concatenated() {
+            OptionalParentheses::Multiline
         } else if self.value.is_none() | self.value.is_bool() || self.value.is_ellipsis() {
             OptionalParentheses::Never
         } else {
@@ -107,7 +104,8 @@ pub(super) fn is_multiline_string(constant: &ExprConstant, source: &str) -> bool
         let quotes =
             StringQuotes::parse(&contents[TextRange::new(prefix.text_len(), contents.text_len())]);
 
-        quotes.is_some_and(StringQuotes::is_triple) && contents.contains(['\n', '\r'])
+        quotes.is_some_and(StringQuotes::is_triple)
+            && memchr::memchr2(b'\n', b'\r', contents.as_bytes()).is_some()
     } else {
         false
     }

crates/ruff_python_formatter/src/expression/mod.rs

@@ -232,26 +232,50 @@ impl Format<PyFormatContext<'_>> for MaybeParenthesizeExpression<'_> {
                 Parenthesize::IfBreaks => {
                     let group_id = f.group_id("optional_parentheses");
                     let f = &mut WithNodeLevel::new(NodeLevel::Expression(Some(group_id)), f);
-                    let format_expression = expression
+                    let mut format_expression = expression
                         .format()
                         .with_options(Parentheses::Never)
                         .memoized();
 
-                    best_fitting![
-                        group(&format_expression).with_group_id(Some(group_id)),
-                        group(&format_args![
-                            text("("),
-                            soft_block_indent(&format_expression),
-                            text(")")
-                        ])
-                        .with_group_id(Some(group_id))
-                        .should_expand(true),
+                    // Don't use best fitting if it is known that the expression can never fit
+                    if format_expression.inspect(f)?.will_break() {
+                        // The group here is necessary because `format_expression` may contain IR elements
+                        // that refer to the group id
                         group(&format_expression)
                             .with_group_id(Some(group_id))
                             .should_expand(true)
-                    ]
-                    .with_mode(BestFittingMode::AllLines)
-                    .fmt(f)
+                            .fmt(f)
+                    } else {
+                        // Only add parentheses if it makes the expression fit on the line.
+                        // Using the flat version as the most expanded version gives a left-to-right splitting behavior
+                        // which differs from when using regular groups, because they split right-to-left.
+                        best_fitting![
+                            // ---------------------------------------------------------------------
+                            // Variant 1:
+                            // Try to fit the expression without any parentheses
+                            group(&format_expression).with_group_id(Some(group_id)),
+                            // ---------------------------------------------------------------------
+                            // Variant 2:
+                            // Try to fit the expression by adding parentheses and indenting the expression.
+                            group(&format_args![
+                                text("("),
+                                soft_block_indent(&format_expression),
+                                text(")")
+                            ])
+                            .with_group_id(Some(group_id))
+                            .should_expand(true),
+                            // ---------------------------------------------------------------------
+                            // Variant 3: Fallback, no parentheses
+                            // Expression doesn't fit regardless of adding the parentheses. Remove the parentheses again.
+                            group(&format_expression)
+                                .with_group_id(Some(group_id))
+                                .should_expand(true)
+                        ]
+                        // Measure all lines, to avoid that the printer decides that this fits right after hitting
+                        // the `(`.
+                        .with_mode(BestFittingMode::AllLines)
+                        .fmt(f)
+                    }
                 }
             },
             OptionalParentheses::Never => match parenthesize {

crates/ruff_python_formatter/src/expression/parentheses.rs

@@ -53,7 +53,6 @@ pub(crate) enum Parenthesize {
     /// Parenthesizes the expression if the group doesn't fit on a line (e.g., even name expressions are parenthesized), or if
     /// the expression doesn't break, but _does_ reports that it always requires parentheses in this position (e.g., walrus
     /// operators in function return annotations).
-    // TODO can this be replaced by IfBreaks
     IfBreaksOrIfRequired,
 }
 

crates/ruff_python_formatter/src/lib.rs

@@ -1,3 +1,5 @@
+extern crate core;
+
 use thiserror::Error;
 
 use ruff_formatter::format_element::tag;

crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__trailing_commas_in_leading_parts.py.snap

@@ -56,6 +56,18 @@ assert xxxxxxxxx.xxxxxxxxx.xxxxxxxxx(
  
  
  # Example from https://github.com/psf/black/issues/3229
+@@ -43,8 +41,6 @@
+ )
+ 
+ # Regression test for https://github.com/psf/black/issues/3414.
+-assert xxxxxxxxx.xxxxxxxxx.xxxxxxxxx(
+-    xxxxxxxxx
+-).xxxxxxxxxxxxxxxxxx(), (
+-    "xxx {xxxxxxxxx} xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
+-)
++assert (
++    xxxxxxxxx.xxxxxxxxx.xxxxxxxxx(xxxxxxxxx).xxxxxxxxxxxxxxxxxx()
++), "xxx {xxxxxxxxx} xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
 ```
 
 ## Ruff Output
@@ -104,11 +116,9 @@ assert (
 )
 
 # Regression test for https://github.com/psf/black/issues/3414.
-assert xxxxxxxxx.xxxxxxxxx.xxxxxxxxx(
-    xxxxxxxxx
-).xxxxxxxxxxxxxxxxxx(), (
-    "xxx {xxxxxxxxx} xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
-)
+assert (
+    xxxxxxxxx.xxxxxxxxx.xxxxxxxxx(xxxxxxxxx).xxxxxxxxxxxxxxxxxx()
+), "xxx {xxxxxxxxx} xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
 ```
 
 ## Black Output

crates/ruff_python_formatter/tests/snapshots/format@expression__unspittable.py.snap

@@ -58,6 +58,11 @@ for converter in connection.ops.get_db_converters(
     expression
 ) + expression.get_db_converters(connection):
     ...
+
+
+aaa = (
+    bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb # awkward comment
+)
 ```
 
 ## Output
@@ -120,6 +125,9 @@ for converter in connection.ops.get_db_converters(
     expression
 ) + expression.get_db_converters(connection):
     ...
+
+
+aaa = bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb  # awkward comment
 ```
 
 

crates/ruff_python_formatter/tests/snapshots/format@expression__yield.py.snap

@@ -72,6 +72,13 @@ def foo():
         1
     )
 
+    yield (
+        "#   * Make sure each ForeignKey and OneToOneField has `on_delete` set "
+        "to the desired behavior"
+    )
+
+    yield aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb + ccccccccccccccccccccccccccccccccccccccccccccccccccccccc
+
 yield ("Cache key will cause errors if used with memcached: %r " "(longer than %s)" % (
     key,
     MEMCACHE_MAX_KEY_LENGTH,
@@ -168,6 +175,17 @@ def foo():
         )
     )
 
+    yield (
+        "#   * Make sure each ForeignKey and OneToOneField has `on_delete` set "
+        "to the desired behavior"
+    )
+
+    yield (
+        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+        + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
+        + ccccccccccccccccccccccccccccccccccccccccccccccccccccccc
+    )
+
 
 yield (
     "Cache key will cause errors if used with memcached: %r "