diff --git a/crates/ruff_python_ast/src/visitor.rs b/crates/ruff_python_ast/src/visitor.rs index 3c018c3b10..d0b62fa7af 100644 --- a/crates/ruff_python_ast/src/visitor.rs +++ b/crates/ruff_python_ast/src/visitor.rs @@ -575,7 +575,7 @@ pub fn walk_format_spec<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, format_spe pub fn walk_arguments<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, arguments: &'a Arguments) { // Note that the there might be keywords before the last arg, e.g. in // f(*args, a=2, *args2, **kwargs)`, but we follow Python in evaluating first `args` and then - // `keywords`. See also [Arguments::arguments_as_declared`]. + // `keywords`. See also [Arguments::arguments_source_order`]. for arg in &arguments.args { visitor.visit_expr(arg); } 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 bb4425afbb..7123bd85d4 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 @@ -242,3 +242,26 @@ f(x=( # comment 1 )) + +args = [2] +args2 = [3] +kwargs = {"4": 5} + +# https://github.com/astral-sh/ruff/issues/6498 +f(a=1, *args, **kwargs) +f(*args, a=1, **kwargs) +f(*args, a=1, *args2, **kwargs) +f( # a + * # b + args + # c + , # d + a=1, + # e + * # f + args2 + # g + ** # h + kwargs, +) + diff --git a/crates/ruff_python_formatter/src/comments/placement.rs b/crates/ruff_python_formatter/src/comments/placement.rs index f240cd0ffb..005cfb002d 100644 --- a/crates/ruff_python_formatter/src/comments/placement.rs +++ b/crates/ruff_python_formatter/src/comments/placement.rs @@ -38,7 +38,7 @@ pub(super) fn place_comment<'a>( /// ): /// ... /// ``` -/// The parentheses enclose `True`, but the range of `True`doesn't include the `# comment`. +/// The parentheses enclose `True`, but the range of `True` doesn't include the `# comment`. /// /// Default handling can get parenthesized comments wrong in a number of ways. For example, the /// comment here is marked (by default) as a trailing comment of `x`, when it should be a leading @@ -120,10 +120,8 @@ fn handle_parenthesized_comment<'a>( // For now, we _can_ assert, but to do so, we stop lexing when we hit a token that precedes an // identifier. if comment.line_position().is_end_of_line() { - let tokenizer = SimpleTokenizer::new( - locator.contents(), - TextRange::new(preceding.end(), comment.start()), - ); + let range = TextRange::new(preceding.end(), comment.start()); + let tokenizer = SimpleTokenizer::new(locator.contents(), range); if tokenizer .skip_trivia() .take_while(|token| { @@ -136,7 +134,7 @@ fn handle_parenthesized_comment<'a>( debug_assert!( !matches!(token.kind, SimpleTokenKind::Bogus), "Unexpected token between nodes: `{:?}`", - locator.slice(TextRange::new(preceding.end(), comment.start()),) + locator.slice(range) ); token.kind() == SimpleTokenKind::LParen @@ -145,10 +143,8 @@ fn handle_parenthesized_comment<'a>( return CommentPlacement::leading(following, comment); } } else { - let tokenizer = SimpleTokenizer::new( - locator.contents(), - TextRange::new(comment.end(), following.start()), - ); + let range = TextRange::new(comment.end(), following.start()); + let tokenizer = SimpleTokenizer::new(locator.contents(), range); if tokenizer .skip_trivia() .take_while(|token| { @@ -161,7 +157,7 @@ fn handle_parenthesized_comment<'a>( debug_assert!( !matches!(token.kind, SimpleTokenKind::Bogus), "Unexpected token between nodes: `{:?}`", - locator.slice(TextRange::new(comment.end(), following.start())) + locator.slice(range) ); token.kind() == SimpleTokenKind::RParen }) diff --git a/crates/ruff_python_formatter/src/other/arguments.rs b/crates/ruff_python_formatter/src/other/arguments.rs index 2f2335d3bc..5baa9fa741 100644 --- a/crates/ruff_python_formatter/src/other/arguments.rs +++ b/crates/ruff_python_formatter/src/other/arguments.rs @@ -1,6 +1,5 @@ use ruff_formatter::write; -use ruff_python_ast::node::AstNode; -use ruff_python_ast::{Arguments, Expr}; +use ruff_python_ast::{ArgOrKeyword, Arguments, Expr}; use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer}; use ruff_text_size::{Ranged, TextRange, TextSize}; @@ -14,6 +13,11 @@ pub struct FormatArguments; impl FormatNodeRule for FormatArguments { fn fmt_fields(&self, item: &Arguments, f: &mut PyFormatter) -> FormatResult<()> { + let Arguments { + range, + args, + keywords, + } = item; // We have a case with `f()` without any argument, which is a special case because we can // have a comment with no node attachment inside: // ```python @@ -21,7 +25,7 @@ impl FormatNodeRule for FormatArguments { // # This call has a dangling comment. // ) // ``` - if item.args.is_empty() && item.keywords.is_empty() { + if args.is_empty() && keywords.is_empty() { let comments = f.context().comments().clone(); let dangling = comments.dangling(item); return write!(f, [empty_parenthesized("(", dangling, ")")]); @@ -29,9 +33,9 @@ impl FormatNodeRule for FormatArguments { let all_arguments = format_with(|f: &mut PyFormatter| { let source = f.context().source(); - let mut joiner = f.join_comma_separated(item.end()); - match item.args.as_slice() { - [arg] if item.keywords.is_empty() => { + let mut joiner = f.join_comma_separated(range.end()); + match args.as_slice() { + [arg] if keywords.is_empty() => { match arg { Expr::GeneratorExp(generator_exp) => joiner.entry( generator_exp, @@ -41,7 +45,7 @@ impl FormatNodeRule for FormatArguments { ), other => { let parentheses = - if is_single_argument_parenthesized(arg, item.end(), source) { + if is_single_argument_parenthesized(arg, range.end(), source) { Parentheses::Always } else { // Note: no need to handle opening-parenthesis comments, since @@ -53,14 +57,17 @@ impl FormatNodeRule for FormatArguments { } }; } - args => { - joiner - .entries( - // We have the parentheses from the call so the item never need any - args.iter() - .map(|arg| (arg, arg.format().with_options(Parentheses::Preserve))), - ) - .nodes(item.keywords.iter()); + _ => { + for arg_or_keyword in item.arguments_source_order() { + match arg_or_keyword { + ArgOrKeyword::Arg(arg) => { + joiner.entry(arg, &arg.format()); + } + ArgOrKeyword::Keyword(keyword) => { + joiner.entry(keyword, &keyword.format()); + } + } + } } } @@ -76,7 +83,7 @@ impl FormatNodeRule for FormatArguments { // c, // ) let comments = f.context().comments().clone(); - let dangling_comments = comments.dangling(item.as_any_node_ref()); + let dangling_comments = comments.dangling(item); write!( f, diff --git a/crates/ruff_python_formatter/src/statement/stmt_function_def.rs b/crates/ruff_python_formatter/src/statement/stmt_function_def.rs index df4875cddd..9d1d3280a1 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_function_def.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_function_def.rs @@ -18,14 +18,9 @@ pub struct FormatStmtFunctionDef; impl FormatNodeRule for FormatStmtFunctionDef { fn fmt_fields(&self, item: &StmtFunctionDef, f: &mut PyFormatter) -> FormatResult<()> { let StmtFunctionDef { - range: _, - is_async, decorator_list, - name, - type_params, - parameters, - returns, body, + .. } = item; let comments = f.context().comments().clone(); @@ -47,101 +42,7 @@ impl FormatNodeRule for FormatStmtFunctionDef { clause_header( ClauseHeader::Function(item), trailing_definition_comments, - &format_with(|f| { - if *is_async { - write!(f, [token("async"), space()])?; - } - - write!(f, [token("def"), space(), name.format()])?; - - if let Some(type_params) = type_params.as_ref() { - write!(f, [type_params.format()])?; - } - - let format_inner = format_with(|f: &mut PyFormatter| { - write!(f, [parameters.format()])?; - - if let Some(return_annotation) = returns.as_ref() { - write!(f, [space(), token("->"), space()])?; - - if return_annotation.is_tuple_expr() { - let parentheses = - if comments.has_leading(return_annotation.as_ref()) { - Parentheses::Always - } else { - Parentheses::Never - }; - write!( - f, - [return_annotation.format().with_options(parentheses)] - )?; - } else if comments.has_trailing(return_annotation.as_ref()) { - // Intentionally parenthesize any return annotations with trailing comments. - // This avoids an instability in cases like: - // ```python - // def double( - // a: int - // ) -> ( - // int # Hello - // ): - // pass - // ``` - // If we allow this to break, it will be formatted as follows: - // ```python - // def double( - // a: int - // ) -> int: # Hello - // pass - // ``` - // On subsequent formats, the `# Hello` will be interpreted as a dangling - // comment on a function, yielding: - // ```python - // def double(a: int) -> int: # Hello - // pass - // ``` - // Ideally, we'd reach that final formatting in a single pass, but doing so - // requires that the parent be aware of how the child is formatted, which - // is challenging. As a compromise, we break those expressions to avoid an - // instability. - write!( - f, - [return_annotation - .format() - .with_options(Parentheses::Always)] - )?; - } else { - write!( - f, - [maybe_parenthesize_expression( - return_annotation, - item, - if empty_parameters(parameters, f.context().source()) { - // If the parameters are empty, add parentheses if the return annotation - // breaks at all. - Parenthesize::IfBreaksOrIfRequired - } else { - // Otherwise, use our normal rules for parentheses, which allows us to break - // like: - // ```python - // def f( - // x, - // ) -> Tuple[ - // int, - // int, - // ]: - // ... - // ``` - Parenthesize::IfBreaks - }, - )] - )?; - } - } - Ok(()) - }); - - group(&format_inner).fmt(f) - }), + &format_with(|f| format_function_header(f, item)), ), clause_body(body, trailing_definition_comments).with_kind(SuiteKind::Function), ] @@ -176,6 +77,109 @@ impl FormatNodeRule for FormatStmtFunctionDef { } } +fn format_function_header(f: &mut PyFormatter, item: &StmtFunctionDef) -> FormatResult<()> { + let StmtFunctionDef { + range: _, + is_async, + decorator_list: _, + name, + type_params, + parameters, + returns, + body: _, + } = item; + + let comments = f.context().comments().clone(); + + if *is_async { + write!(f, [token("async"), space()])?; + } + + write!(f, [token("def"), space(), name.format()])?; + + if let Some(type_params) = type_params.as_ref() { + write!(f, [type_params.format()])?; + } + + let format_inner = format_with(|f: &mut PyFormatter| { + write!(f, [parameters.format()])?; + + if let Some(return_annotation) = returns.as_ref() { + write!(f, [space(), token("->"), space()])?; + + if return_annotation.is_tuple_expr() { + let parentheses = if comments.has_leading(return_annotation.as_ref()) { + Parentheses::Always + } else { + Parentheses::Never + }; + write!(f, [return_annotation.format().with_options(parentheses)])?; + } else if comments.has_trailing(return_annotation.as_ref()) { + // Intentionally parenthesize any return annotations with trailing comments. + // This avoids an instability in cases like: + // ```python + // def double( + // a: int + // ) -> ( + // int # Hello + // ): + // pass + // ``` + // If we allow this to break, it will be formatted as follows: + // ```python + // def double( + // a: int + // ) -> int: # Hello + // pass + // ``` + // On subsequent formats, the `# Hello` will be interpreted as a dangling + // comment on a function, yielding: + // ```python + // def double(a: int) -> int: # Hello + // pass + // ``` + // Ideally, we'd reach that final formatting in a single pass, but doing so + // requires that the parent be aware of how the child is formatted, which + // is challenging. As a compromise, we break those expressions to avoid an + // instability. + write!( + f, + [return_annotation.format().with_options(Parentheses::Always)] + )?; + } else { + write!( + f, + [maybe_parenthesize_expression( + return_annotation, + item, + if empty_parameters(parameters, f.context().source()) { + // If the parameters are empty, add parentheses if the return annotation + // breaks at all. + Parenthesize::IfBreaksOrIfRequired + } else { + // Otherwise, use our normal rules for parentheses, which allows us to break + // like: + // ```python + // def f( + // x, + // ) -> Tuple[ + // int, + // int, + // ]: + // ... + // ``` + Parenthesize::IfBreaks + }, + )] + )?; + } + } + Ok(()) + }); + + group(&format_inner).fmt(f) +} + /// Returns `true` if [`Parameters`] is empty (no parameters, no comments, etc.). fn empty_parameters(parameters: &Parameters, source: &str) -> bool { let mut tokenizer = SimpleTokenizer::new(source, parameters.range()) 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 9c26b8a3d5..1e4b44427b 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 @@ -248,6 +248,29 @@ f(x=( # comment 1 )) + +args = [2] +args2 = [3] +kwargs = {"4": 5} + +# https://github.com/astral-sh/ruff/issues/6498 +f(a=1, *args, **kwargs) +f(*args, a=1, **kwargs) +f(*args, a=1, *args2, **kwargs) +f( # a + * # b + args + # c + , # d + a=1, + # e + * # f + args2 + # g + ** # h + kwargs, +) + ``` ## Output @@ -493,6 +516,27 @@ f( 1 ) ) + +args = [2] +args2 = [3] +kwargs = {"4": 5} + +# https://github.com/astral-sh/ruff/issues/6498 +f(a=1, *args, **kwargs) +f(*args, a=1, **kwargs) +f(*args, a=1, *args2, **kwargs) +f( # a + # b + *args, # d + # c + a=1, + # e + # f + *args2 + # g + ** # h + kwargs, +) ```