From a4d6f9d017294f347373bdf98bfbb6b868fde63b Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Wed, 12 Jul 2023 15:27:08 +0200 Subject: [PATCH] Handle right parens in join comma builder --- crates/ruff_python_formatter/src/builders.rs | 67 +++++++---- .../src/expression/expr_call.rs | 57 +++++++-- .../src/expression/expr_dict.rs | 2 +- .../src/expression/expr_list.rs | 8 +- .../src/expression/expr_tuple.rs | 20 ++-- .../src/statement/stmt_class_def.rs | 3 +- .../src/statement/stmt_delete.rs | 8 +- .../src/statement/stmt_import_from.rs | 4 +- .../src/statement/stmt_with.rs | 7 +- ...aneous__long_strings_flag_disabled.py.snap | 110 +++++------------- ...black_compatibility@py_38__pep_572.py.snap | 9 +- 11 files changed, 159 insertions(+), 136 deletions(-) diff --git a/crates/ruff_python_formatter/src/builders.rs b/crates/ruff_python_formatter/src/builders.rs index 368c651c6d5d3..5ff156917041e 100644 --- a/crates/ruff_python_formatter/src/builders.rs +++ b/crates/ruff_python_formatter/src/builders.rs @@ -1,9 +1,12 @@ +use ruff_text_size::{TextRange, TextSize}; +use rustpython_parser::ast::Ranged; + +use ruff_formatter::{format_args, write, Argument, Arguments}; + use crate::context::NodeLevel; use crate::prelude::*; -use crate::trivia::{first_non_trivia_token, lines_after, skip_trailing_trivia, Token, TokenKind}; -use ruff_formatter::{format_args, write, Argument, Arguments}; -use ruff_text_size::TextSize; -use rustpython_parser::ast::Ranged; +use crate::trivia::{lines_after, skip_trailing_trivia, SimpleTokenizer, Token, TokenKind}; +use crate::MagicTrailingComma; /// Adds parentheses and indents `content` if it doesn't fit on a line. pub(crate) fn parenthesize_if_expands<'ast, T>(content: &T) -> ParenthesizeIfExpands<'_, 'ast> @@ -53,7 +56,10 @@ pub(crate) trait PyFormatterExtensions<'ast, 'buf> { /// A builder that separates each element by a `,` and a [`soft_line_break_or_space`]. /// It emits a trailing `,` that is only shown if the enclosing group expands. It forces the enclosing /// group to expand if the last item has a trailing `comma` and the magical comma option is enabled. - fn join_comma_separated<'fmt>(&'fmt mut self) -> JoinCommaSeparatedBuilder<'fmt, 'ast, 'buf>; + fn join_comma_separated<'fmt>( + &'fmt mut self, + sequence_end: TextSize, + ) -> JoinCommaSeparatedBuilder<'fmt, 'ast, 'buf>; } impl<'buf, 'ast> PyFormatterExtensions<'ast, 'buf> for PyFormatter<'ast, 'buf> { @@ -61,8 +67,11 @@ impl<'buf, 'ast> PyFormatterExtensions<'ast, 'buf> for PyFormatter<'ast, 'buf> { JoinNodesBuilder::new(self, level) } - fn join_comma_separated<'fmt>(&'fmt mut self) -> JoinCommaSeparatedBuilder<'fmt, 'ast, 'buf> { - JoinCommaSeparatedBuilder::new(self) + fn join_comma_separated<'fmt>( + &'fmt mut self, + sequence_end: TextSize, + ) -> JoinCommaSeparatedBuilder<'fmt, 'ast, 'buf> { + JoinCommaSeparatedBuilder::new(self, sequence_end) } } @@ -194,18 +203,20 @@ pub(crate) struct JoinCommaSeparatedBuilder<'fmt, 'ast, 'buf> { result: FormatResult<()>, fmt: &'fmt mut PyFormatter<'ast, 'buf>, end_of_last_entry: Option, + sequence_end: TextSize, /// We need to track whether we have more than one entry since a sole entry doesn't get a /// magic trailing comma even when expanded len: usize, } impl<'fmt, 'ast, 'buf> JoinCommaSeparatedBuilder<'fmt, 'ast, 'buf> { - fn new(f: &'fmt mut PyFormatter<'ast, 'buf>) -> Self { + fn new(f: &'fmt mut PyFormatter<'ast, 'buf>, sequence_end: TextSize) -> Self { Self { fmt: f, result: Ok(()), end_of_last_entry: None, len: 0, + sequence_end, } } @@ -236,7 +247,7 @@ impl<'fmt, 'ast, 'buf> JoinCommaSeparatedBuilder<'fmt, 'ast, 'buf> { where T: Ranged, F: Format>, - I: Iterator, + I: IntoIterator, { for (node, content) in entries { self.entry(&node, &content); @@ -248,7 +259,7 @@ impl<'fmt, 'ast, 'buf> JoinCommaSeparatedBuilder<'fmt, 'ast, 'buf> { pub(crate) fn nodes<'a, T, I>(&mut self, entries: I) -> &mut Self where T: Ranged + AsFormat> + 'a, - I: Iterator, + I: IntoIterator, { for node in entries { self.entry(node, &node.format()); @@ -260,14 +271,26 @@ impl<'fmt, 'ast, 'buf> JoinCommaSeparatedBuilder<'fmt, 'ast, 'buf> { pub(crate) fn finish(&mut self) -> FormatResult<()> { self.result.and_then(|_| { if let Some(last_end) = self.end_of_last_entry.take() { - let magic_trailing_comma = self.fmt.options().magic_trailing_comma().is_respect() - && matches!( - first_non_trivia_token(last_end, self.fmt.context().source()), - Some(Token { - kind: TokenKind::Comma, - .. - }) - ); + let magic_trailing_comma = match self.fmt.options().magic_trailing_comma() { + MagicTrailingComma::Respect => { + let first_token = SimpleTokenizer::new( + self.fmt.context().source(), + TextRange::new(last_end, self.sequence_end), + ) + .skip_trivia() + // Skip over any closing parentheses belonging to the expression + .find(|token| token.kind() != TokenKind::RParen); + + matches!( + first_token, + Some(Token { + kind: TokenKind::Comma, + .. + }) + ) + } + MagicTrailingComma::Ignore => false, + }; // If there is a single entry, only keep the magic trailing comma, don't add it if // it wasn't there. If there is more than one entry, always add it. @@ -287,13 +310,15 @@ impl<'fmt, 'ast, 'buf> JoinCommaSeparatedBuilder<'fmt, 'ast, 'buf> { #[cfg(test)] mod tests { + use rustpython_parser::ast::ModModule; + use rustpython_parser::Parse; + + use ruff_formatter::format; + use crate::comments::Comments; use crate::context::{NodeLevel, PyFormatContext}; use crate::prelude::*; use crate::PyFormatOptions; - use ruff_formatter::format; - use rustpython_parser::ast::ModModule; - use rustpython_parser::Parse; fn format_ranged(level: NodeLevel) -> String { let source = r#" diff --git a/crates/ruff_python_formatter/src/expression/expr_call.rs b/crates/ruff_python_formatter/src/expression/expr_call.rs index 070027fc21f9f..b1dd972df267b 100644 --- a/crates/ruff_python_formatter/src/expression/expr_call.rs +++ b/crates/ruff_python_formatter/src/expression/expr_call.rs @@ -5,10 +5,12 @@ use crate::expression::parentheses::{ default_expression_needs_parentheses, parenthesized, NeedsParentheses, Parentheses, Parenthesize, }; +use crate::trivia::{SimpleTokenizer, TokenKind}; use crate::{AsFormat, FormatNodeRule, PyFormatter}; use ruff_formatter::prelude::{format_with, group, text}; use ruff_formatter::{write, Buffer, FormatResult}; -use rustpython_parser::ast::ExprCall; +use ruff_text_size::{TextRange, TextSize}; +use rustpython_parser::ast::{Expr, ExprCall, Ranged}; #[derive(Default)] pub struct FormatExprCall; @@ -43,15 +45,25 @@ impl FormatNodeRule for FormatExprCall { ); } - let all_args = format_with(|f| { - f.join_comma_separated() - .entries( - // We have the parentheses from the call so the arguments never need any - args.iter() - .map(|arg| (arg, arg.format().with_options(Parenthesize::Never))), - ) - .nodes(keywords.iter()) - .finish() + let all_args = format_with(|f: &mut PyFormatter| { + let source = f.context().source(); + let mut joiner = f.join_comma_separated(item.end()); + match args.as_slice() { + [argument] if keywords.is_empty() => { + let parentheses = + if is_single_argument_parenthesized(argument, item.end(), source) { + Parenthesize::Always + } else { + Parenthesize::Never + }; + joiner.entry(argument, &argument.format().with_options(parentheses)); + } + arguments => { + joiner.nodes(arguments).nodes(keywords.iter()); + } + } + + joiner.finish() }); write!( @@ -97,3 +109,28 @@ impl NeedsParentheses for ExprCall { } } } + +fn is_single_argument_parenthesized(argument: &Expr, call_end: TextSize, source: &str) -> bool { + let mut has_seen_r_paren = false; + + for token in + SimpleTokenizer::new(source, TextRange::new(argument.end(), call_end)).skip_trivia() + { + match token.kind() { + TokenKind::RParen => { + if has_seen_r_paren { + return true; + } + has_seen_r_paren = true; + } + // Skip over any trailing comma + TokenKind::Comma => continue, + _ => { + // Passed the arguments + break; + } + } + } + + false +} diff --git a/crates/ruff_python_formatter/src/expression/expr_dict.rs b/crates/ruff_python_formatter/src/expression/expr_dict.rs index 1f9fcca5681c9..5a00386779105 100644 --- a/crates/ruff_python_formatter/src/expression/expr_dict.rs +++ b/crates/ruff_python_formatter/src/expression/expr_dict.rs @@ -77,7 +77,7 @@ impl FormatNodeRule for FormatExprDict { } let format_pairs = format_with(|f| { - let mut joiner = f.join_comma_separated(); + let mut joiner = f.join_comma_separated(item.end()); for (key, value) in keys.iter().zip(values) { let key_value_pair = KeyValuePair { key, value }; diff --git a/crates/ruff_python_formatter/src/expression/expr_list.rs b/crates/ruff_python_formatter/src/expression/expr_list.rs index c090182762025..b35abdaa4339f 100644 --- a/crates/ruff_python_formatter/src/expression/expr_list.rs +++ b/crates/ruff_python_formatter/src/expression/expr_list.rs @@ -6,7 +6,7 @@ use crate::expression::parentheses::{ use crate::prelude::*; use crate::FormatNodeRule; use ruff_formatter::{format_args, write}; -use rustpython_parser::ast::ExprList; +use rustpython_parser::ast::{ExprList, Ranged}; #[derive(Default)] pub struct FormatExprList; @@ -53,7 +53,11 @@ impl FormatNodeRule for FormatExprList { "A non-empty expression list has dangling comments" ); - let items = format_with(|f| f.join_comma_separated().nodes(elts.iter()).finish()); + let items = format_with(|f| { + f.join_comma_separated(item.end()) + .nodes(elts.iter()) + .finish() + }); parenthesized("[", &items, "]").fmt(f) } diff --git a/crates/ruff_python_formatter/src/expression/expr_tuple.rs b/crates/ruff_python_formatter/src/expression/expr_tuple.rs index 78ad8279d633f..2fcad273871f0 100644 --- a/crates/ruff_python_formatter/src/expression/expr_tuple.rs +++ b/crates/ruff_python_formatter/src/expression/expr_tuple.rs @@ -108,14 +108,14 @@ impl FormatNodeRule for FormatExprTuple { // // Unlike other expression parentheses, tuple parentheses are part of the range of the // tuple itself. - elts if is_parenthesized(*range, elts, f.context().source()) + _ if is_parenthesized(*range, elts, f.context().source()) && self.parentheses != TupleParentheses::StripInsideForLoop => { - parenthesized("(", &ExprSequence::new(elts), ")").fmt(f) + parenthesized("(", &ExprSequence::new(item), ")").fmt(f) } - elts => match self.parentheses { - TupleParentheses::Subscript => group(&ExprSequence::new(elts)).fmt(f), - _ => parenthesize_if_expands(&ExprSequence::new(elts)).fmt(f), + _ => match self.parentheses { + TupleParentheses::Subscript => group(&ExprSequence::new(item)).fmt(f), + _ => parenthesize_if_expands(&ExprSequence::new(item)).fmt(f), }, } } @@ -128,18 +128,20 @@ impl FormatNodeRule for FormatExprTuple { #[derive(Debug)] struct ExprSequence<'a> { - elts: &'a [Expr], + tuple: &'a ExprTuple, } impl<'a> ExprSequence<'a> { - const fn new(elts: &'a [Expr]) -> Self { - Self { elts } + const fn new(expr: &'a ExprTuple) -> Self { + Self { tuple: expr } } } impl Format> for ExprSequence<'_> { fn fmt(&self, f: &mut Formatter>) -> FormatResult<()> { - f.join_comma_separated().nodes(self.elts.iter()).finish() + f.join_comma_separated(self.tuple.end()) + .nodes(&self.tuple.elts) + .finish() } } diff --git a/crates/ruff_python_formatter/src/statement/stmt_class_def.rs b/crates/ruff_python_formatter/src/statement/stmt_class_def.rs index 2388323ae1333..ab6d4be00e490 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_class_def.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_class_def.rs @@ -76,12 +76,13 @@ impl Format> for FormatInheritanceClause<'_> { bases, keywords, name, + body, .. } = self.class_definition; let source = f.context().source(); - let mut joiner = f.join_comma_separated(); + let mut joiner = f.join_comma_separated(body.first().unwrap().start()); if let Some((first, rest)) = bases.split_first() { // Manually handle parentheses for the first expression because the logic in `FormatExpr` diff --git a/crates/ruff_python_formatter/src/statement/stmt_delete.rs b/crates/ruff_python_formatter/src/statement/stmt_delete.rs index c3ba7814e8055..3b5d3225fcc3a 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_delete.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_delete.rs @@ -4,7 +4,7 @@ use crate::expression::parentheses::Parenthesize; use crate::{AsFormat, FormatNodeRule, PyFormatter}; use ruff_formatter::prelude::{block_indent, format_with, space, text}; use ruff_formatter::{write, Buffer, Format, FormatResult}; -use rustpython_parser::ast::StmtDelete; +use rustpython_parser::ast::{Ranged, StmtDelete}; #[derive(Default)] pub struct FormatStmtDelete; @@ -35,7 +35,11 @@ impl FormatNodeRule for FormatStmtDelete { write!(f, [single.format().with_options(Parenthesize::IfBreaks)]) } targets => { - let item = format_with(|f| f.join_comma_separated().nodes(targets.iter()).finish()); + let item = format_with(|f| { + f.join_comma_separated(item.end()) + .nodes(targets.iter()) + .finish() + }); parenthesize_if_expands(&item).fmt(f) } } diff --git a/crates/ruff_python_formatter/src/statement/stmt_import_from.rs b/crates/ruff_python_formatter/src/statement/stmt_import_from.rs index 353f920e2b496..66118999067d6 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_import_from.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_import_from.rs @@ -2,7 +2,7 @@ use crate::builders::{parenthesize_if_expands, PyFormatterExtensions}; use crate::{AsFormat, FormatNodeRule, PyFormatter}; use ruff_formatter::prelude::{dynamic_text, format_with, space, text}; use ruff_formatter::{write, Buffer, Format, FormatResult}; -use rustpython_parser::ast::StmtImportFrom; +use rustpython_parser::ast::{Ranged, StmtImportFrom}; #[derive(Default)] pub struct FormatStmtImportFrom; @@ -39,7 +39,7 @@ impl FormatNodeRule for FormatStmtImportFrom { } } let names = format_with(|f| { - f.join_comma_separated() + f.join_comma_separated(item.end()) .entries(names.iter().map(|name| (name, name.format()))) .finish() }); diff --git a/crates/ruff_python_formatter/src/statement/stmt_with.rs b/crates/ruff_python_formatter/src/statement/stmt_with.rs index 03932ff2c1e49..8474d702401b5 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_with.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_with.rs @@ -68,8 +68,11 @@ impl Format> for AnyStatementWith<'_> { let comments = f.context().comments().clone(); let dangling_comments = comments.dangling_comments(self); - let joined_items = - format_with(|f| f.join_comma_separated().nodes(self.items().iter()).finish()); + let joined_items = format_with(|f| { + f.join_comma_separated(self.body().first().unwrap().start()) + .nodes(self.items().iter()) + .finish() + }); if self.is_async() { write!(f, [text("async"), space()])?; diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@miscellaneous__long_strings_flag_disabled.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@miscellaneous__long_strings_flag_disabled.py.snap index 908e9c1cc9e36..6355a154f5480 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@miscellaneous__long_strings_flag_disabled.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@miscellaneous__long_strings_flag_disabled.py.snap @@ -315,30 +315,7 @@ long_unmergable_string_with_pragma = ( bad_split_func1( "But what should happen when code has already " -@@ -96,15 +96,13 @@ - ) - - bad_split_func3( -- ( -- "But what should happen when code has already " -- r"been formatted but in the wrong way? Like " -- "with a space at the end instead of the " -- r"beginning. Or what about when it is split too " -- r"soon? In the case of a split that is too " -- "short, black will try to honer the custom " -- "split." -- ), -+ "But what should happen when code has already " -+ r"been formatted but in the wrong way? Like " -+ "with a space at the end instead of the " -+ r"beginning. Or what about when it is split too " -+ r"soon? In the case of a split that is too " -+ "short, black will try to honer the custom " -+ "split.", - xxx, - yyy, - zzz, -@@ -143,9 +141,9 @@ +@@ -143,9 +143,9 @@ ) ) @@ -350,7 +327,7 @@ long_unmergable_string_with_pragma = ( comment_string = "Long lines with inline comments should have their comments appended to the reformatted string's enclosing right parentheses." # This comment gets thrown to the top. -@@ -165,25 +163,13 @@ +@@ -165,25 +165,13 @@ triple_quote_string = """This is a really really really long triple quote string assignment and it should not be touched.""" @@ -380,53 +357,18 @@ long_unmergable_string_with_pragma = ( some_function_call( "With a reallly generic name and with a really really long string that is, at some point down the line, " -@@ -212,29 +198,25 @@ - ) - - func_with_bad_comma( -- ( -- "This is a really long string argument to a function that has a trailing comma" -- " which should NOT be there." -- ), -+ "This is a really long string argument to a function that has a trailing comma" -+ " which should NOT be there." - ) - +@@ -221,8 +209,8 @@ func_with_bad_comma( -- ( -- "This is a really long string argument to a function that has a trailing comma" + ( + "This is a really long string argument to a function that has a trailing comma" - " which should NOT be there." - ), # comment after comma -+ "This is a really long string argument to a function that has a trailing comma" -+ " which should NOT be there." # comment after comma - ) - - func_with_bad_parens_that_wont_fit_in_one_line( -- ("short string that should have parens stripped"), x, y, z -+ "short string that should have parens stripped", x, y, z ++ " which should NOT be there." # comment after comma ++ ), ) func_with_bad_parens_that_wont_fit_in_one_line( -- x, y, ("short string that should have parens stripped"), z -+ x, y, "short string that should have parens stripped", z - ) - - func_with_bad_parens( -- ("short string that should have parens stripped"), -+ "short string that should have parens stripped", - x, - y, - z, -@@ -243,7 +225,7 @@ - func_with_bad_parens( - x, - y, -- ("short string that should have parens stripped"), -+ "short string that should have parens stripped", - z, - ) - -@@ -271,10 +253,10 @@ +@@ -271,10 +259,10 @@ def foo(): @@ -542,13 +484,15 @@ bad_split_func2( ) bad_split_func3( - "But what should happen when code has already " - r"been formatted but in the wrong way? Like " - "with a space at the end instead of the " - r"beginning. Or what about when it is split too " - r"soon? In the case of a split that is too " - "short, black will try to honer the custom " - "split.", + ( + "But what should happen when code has already " + r"been formatted but in the wrong way? Like " + "with a space at the end instead of the " + r"beginning. Or what about when it is split too " + r"soon? In the case of a split that is too " + "short, black will try to honer the custom " + "split." + ), xxx, yyy, zzz, @@ -644,25 +588,29 @@ func_with_bad_comma( ) func_with_bad_comma( - "This is a really long string argument to a function that has a trailing comma" - " which should NOT be there." + ( + "This is a really long string argument to a function that has a trailing comma" + " which should NOT be there." + ), ) func_with_bad_comma( - "This is a really long string argument to a function that has a trailing comma" - " which should NOT be there." # comment after comma + ( + "This is a really long string argument to a function that has a trailing comma" + " which should NOT be there." # comment after comma + ), ) func_with_bad_parens_that_wont_fit_in_one_line( - "short string that should have parens stripped", x, y, z + ("short string that should have parens stripped"), x, y, z ) func_with_bad_parens_that_wont_fit_in_one_line( - x, y, "short string that should have parens stripped", z + x, y, ("short string that should have parens stripped"), z ) func_with_bad_parens( - "short string that should have parens stripped", + ("short string that should have parens stripped"), x, y, z, @@ -671,7 +619,7 @@ func_with_bad_parens( func_with_bad_parens( x, y, - "short string that should have parens stripped", + ("short string that should have parens stripped"), z, ) diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@py_38__pep_572.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@py_38__pep_572.py.snap index cb2a031c4e29e..c411a16a6ec41 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@py_38__pep_572.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@py_38__pep_572.py.snap @@ -83,7 +83,7 @@ while x := f(x): x = (y := 0) (z := (y := (x := 0))) (info := (name, phone, *rest)) -@@ -31,17 +31,17 @@ +@@ -31,9 +31,9 @@ len(lines := f.readlines()) foo(x := 3, cat="vector") foo(cat=(category := "vector")) @@ -95,9 +95,8 @@ while x := f(x): return env_base if self._is_special and (ans := self._check_nans(context=context)): return ans - foo(b := 2, a=1) --foo((b := 2), a=1) -+foo(b := 2, a=1) +@@ -41,7 +41,7 @@ + foo((b := 2), a=1) foo(c=(b := 2), a=1) -while x := f(x): @@ -151,7 +150,7 @@ if (env_base := os.environ.get("PYTHONUSERBASE", None)): if self._is_special and (ans := self._check_nans(context=context)): return ans foo(b := 2, a=1) -foo(b := 2, a=1) +foo((b := 2), a=1) foo(c=(b := 2), a=1) while (x := f(x)):