From b6ba3abbd6bee1d8279109cc2b85b76333556240 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Tue, 22 Aug 2023 10:54:45 +0200 Subject: [PATCH] Use BestFits for non-call chain call expressions --- Cargo.lock | 1 + .../src/format_element/document.rs | 7 ++- crates/ruff_python_formatter/Cargo.toml | 1 + .../fixtures/ruff/expression/unspittable.py | 5 ++ .../test/fixtures/ruff/expression/yield.py | 7 +++ .../src/expression/expr_call.rs | 23 ++++----- .../src/expression/expr_constant.rs | 14 +++--- .../src/expression/mod.rs | 50 ++++++++++++++----- .../src/expression/parentheses.rs | 1 - crates/ruff_python_formatter/src/lib.rs | 2 + ...__trailing_commas_in_leading_parts.py.snap | 20 ++++++-- .../format@expression__unspittable.py.snap | 8 +++ .../format@expression__yield.py.snap | 18 +++++++ 13 files changed, 113 insertions(+), 44 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 386f03e63088a3..a206ca6caa39fe 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2340,6 +2340,7 @@ dependencies = [ "insta", "is-macro", "itertools", + "memchr", "once_cell", "ruff_formatter", "ruff_python_ast", diff --git a/crates/ruff_formatter/src/format_element/document.rs b/crates/ruff_formatter/src/format_element/document.rs index add834202fbecc..42bc2b1f39961f 100644 --- a/crates/ruff_formatter/src/format_element/document.rs +++ b/crates/ruff_formatter/src/format_element/document.rs @@ -661,18 +661,17 @@ impl Format> 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() { diff --git a/crates/ruff_python_formatter/Cargo.toml b/crates/ruff_python_formatter/Cargo.toml index 637276bab11bd8..e63bbeb8720b75 100644 --- a/crates/ruff_python_formatter/Cargo.toml +++ b/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 } diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/unspittable.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/unspittable.py index 83a1158ec32aed..a81cb284b773bf 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/unspittable.py +++ b/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 +) diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/yield.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/yield.py index ff587bd8120cb7..0959855cf53bd7 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/yield.py +++ b/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, diff --git a/crates/ruff_python_formatter/src/expression/expr_call.rs b/crates/ruff_python_formatter/src/expression/expr_call.rs index fe2813626f61c5..0d2ee2e27581f3 100644 --- a/crates/ruff_python_formatter/src/expression/expr_call.rs +++ b/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 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 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::BestFit => OptionalParentheses::Never, + OptionalParentheses::BestFit => OptionalParentheses::IfFits, parentheses => parentheses, } } diff --git a/crates/ruff_python_formatter/src/expression/expr_constant.rs b/crates/ruff_python_formatter/src/expression/expr_constant.rs index 43a707a00083c2..365bf9a12de263 100644 --- a/crates/ruff_python_formatter/src/expression/expr_constant.rs +++ b/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 } diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index bf5331b1349346..6a2a9eb26d53e6 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -232,26 +232,50 @@ impl Format> 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 { diff --git a/crates/ruff_python_formatter/src/expression/parentheses.rs b/crates/ruff_python_formatter/src/expression/parentheses.rs index b1b6b8fc53f43e..499c285b9e1258 100644 --- a/crates/ruff_python_formatter/src/expression/parentheses.rs +++ b/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, } diff --git a/crates/ruff_python_formatter/src/lib.rs b/crates/ruff_python_formatter/src/lib.rs index 4303eac0d8f067..b40fba29670641 100644 --- a/crates/ruff_python_formatter/src/lib.rs +++ b/crates/ruff_python_formatter/src/lib.rs @@ -1,3 +1,5 @@ +extern crate core; + use thiserror::Error; use ruff_formatter::format_element::tag; diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__trailing_commas_in_leading_parts.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__trailing_commas_in_leading_parts.py.snap index 3abf27d802c1b1..514ea6258ad1e1 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__trailing_commas_in_leading_parts.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__trailing_commas_in_leading_parts.py.snap @@ -56,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 diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__unspittable.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__unspittable.py.snap index 8d9fb5b06e18aa..8c6a7536ada8b1 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__unspittable.py.snap +++ b/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 ``` diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__yield.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__yield.py.snap index f8090247fce2f8..7c78a361807a9a 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__yield.py.snap +++ b/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 "