From 4c53bfe896e46ee8d1750be2472ec78955466469 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 2 Aug 2023 11:54:22 -0400 Subject: [PATCH] Add formatter support for call and class definition `Arguments` (#6274) ## Summary This PR leverages the `Arguments` AST node introduced in #6259 in the formatter, which ensures that we correctly handle trailing comments in calls, like: ```python f( 1, # comment ) pass ``` (Previously, this was treated as a leading comment on `pass`.) This also allows us to unify the argument handling across calls and class definitions. ## Test Plan A bunch of new fixture tests, plus improved Black compatibility. --- crates/ruff_python_ast/src/node.rs | 38 +++++ .../test/fixtures/ruff/expression/call.py | 25 +++ .../ruff/statement/class_definition.py | 52 ++++++ crates/ruff_python_formatter/src/builders.rs | 4 +- .../src/comments/placement.rs | 92 ++++++++++- .../src/comments/visitor.rs | 12 +- .../src/expression/expr_call.rs | 142 +--------------- crates/ruff_python_formatter/src/generated.rs | 32 ++++ .../src/other/arguments.rs | 152 +++++++++++++++++ crates/ruff_python_formatter/src/other/mod.rs | 1 + .../src/statement/stmt_class_def.rs | 154 ++++++++---------- .../src/statement/stmt_function_def.rs | 11 +- .../snapshots/format@expression__call.py.snap | 52 +++++- .../snapshots/format@expression__dict.py.snap | 3 +- .../format@expression__lambda.py.snap | 3 +- .../snapshots/format@expression__list.py.snap | 3 +- ...format@statement__class_definition.py.snap | 105 +++++++++++- .../format@statement__delete.py.snap | 3 +- .../snapshots/format@statement__raise.py.snap | 8 +- 19 files changed, 640 insertions(+), 252 deletions(-) create mode 100644 crates/ruff_python_formatter/src/other/arguments.rs diff --git a/crates/ruff_python_ast/src/node.rs b/crates/ruff_python_ast/src/node.rs index a3266e0e157d7..a9462cf48f36f 100644 --- a/crates/ruff_python_ast/src/node.rs +++ b/crates/ruff_python_ast/src/node.rs @@ -2627,6 +2627,34 @@ impl AstNode for Comprehension { AnyNode::from(self) } } +impl AstNode for Arguments { + fn cast(kind: AnyNode) -> Option + where + Self: Sized, + { + if let AnyNode::Arguments(node) = kind { + Some(node) + } else { + None + } + } + + fn cast_ref(kind: AnyNodeRef) -> Option<&Self> { + if let AnyNodeRef::Arguments(node) = kind { + Some(node) + } else { + None + } + } + + fn as_any_node_ref(&self) -> AnyNodeRef { + AnyNodeRef::from(self) + } + + fn into_any_node(self) -> AnyNode { + AnyNode::from(self) + } +} impl AstNode for Parameters { fn cast(kind: AnyNode) -> Option where @@ -3458,6 +3486,11 @@ impl From for AnyNode { AnyNode::Comprehension(node) } } +impl From for AnyNode { + fn from(node: Arguments) -> Self { + AnyNode::Arguments(node) + } +} impl From for AnyNode { fn from(node: Parameters) -> Self { AnyNode::Parameters(node) @@ -4909,6 +4942,11 @@ impl<'a> From<&'a Comprehension> for AnyNodeRef<'a> { AnyNodeRef::Comprehension(node) } } +impl<'a> From<&'a Arguments> for AnyNodeRef<'a> { + fn from(node: &'a Arguments) -> Self { + AnyNodeRef::Arguments(node) + } +} impl<'a> From<&'a Parameters> for AnyNodeRef<'a> { fn from(node: &'a Parameters) -> Self { AnyNodeRef::Parameters(node) 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 8c372180ce7f5..f5198c62ca652 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 @@ -86,3 +86,28 @@ def f(*args, **kwargs): f( a.very_long_function_function_that_is_so_long_that_it_expands_the_parent_but_its_only_a_single_argument() ) + +f( # abc +) + +f( # abc + # abc +) + +f( + # abc +) + +f ( # abc + 1 +) + +f ( + # abc + 1 +) + +f ( + 1 + # abc +) diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/class_definition.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/class_definition.py index 2a66123e13429..829457fdb19b3 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/class_definition.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/class_definition.py @@ -92,3 +92,55 @@ class Test: """Docstring""" # comment x = 1 + + +class C(): # comment + pass + + +class C( # comment +): + pass + + +class C( + # comment +): + pass + + +class C(): # comment + pass + + +class C( # comment + # comment + 1 +): + pass + + +class C( + 1 + # comment +): + pass + + +@dataclass +# Copied from transformers.models.clip.modeling_clip.CLIPOutput with CLIP->AltCLIP +class AltCLIPOutput(ModelOutput): + ... + + +@dataclass +class AltCLIPOutput( # Copied from transformers.models.clip.modeling_clip.CLIPOutput with CLIP->AltCLIP +): + ... + + +@dataclass +class AltCLIPOutput( + # Copied from transformers.models.clip.modeling_clip.CLIPOutput with CLIP->AltCLIP +): + ... diff --git a/crates/ruff_python_formatter/src/builders.rs b/crates/ruff_python_formatter/src/builders.rs index 9a0ebe93f06bb..8acf77004400b 100644 --- a/crates/ruff_python_formatter/src/builders.rs +++ b/crates/ruff_python_formatter/src/builders.rs @@ -3,7 +3,7 @@ use ruff_python_ast::Ranged; use ruff_python_trivia::{SimpleToken, SimpleTokenKind, SimpleTokenizer}; use ruff_text_size::{TextRange, TextSize}; -use crate::comments::{dangling_comments, SourceComment}; +use crate::comments::{dangling_comments, trailing_comments, SourceComment}; use crate::context::{NodeLevel, WithNodeLevel}; use crate::prelude::*; use crate::MagicTrailingComma; @@ -252,7 +252,7 @@ impl<'ast> Format> for EmptyWithDanglingComments<'_> { [group(&format_args![ self.opening, // end-of-line comments - dangling_comments(&self.comments[..end_of_line_split]), + trailing_comments(&self.comments[..end_of_line_split]), // own line comments, which need to be indented soft_block_indent(&dangling_comments(&self.comments[end_of_line_split..])), self.closing diff --git a/crates/ruff_python_formatter/src/comments/placement.rs b/crates/ruff_python_formatter/src/comments/placement.rs index 57272b8ddaa8e..09ea37a0ee5a1 100644 --- a/crates/ruff_python_formatter/src/comments/placement.rs +++ b/crates/ruff_python_formatter/src/comments/placement.rs @@ -3,8 +3,8 @@ use std::cmp::Ordering; use ruff_python_ast::node::AnyNodeRef; use ruff_python_ast::whitespace::indentation; use ruff_python_ast::{ - self as ast, Comprehension, Expr, ExprAttribute, ExprBinOp, ExprIfExp, ExprSlice, ExprStarred, - MatchCase, Parameters, Ranged, + self as ast, Arguments, Comprehension, Expr, ExprAttribute, ExprBinOp, ExprIfExp, ExprSlice, + ExprStarred, MatchCase, Parameters, Ranged, }; use ruff_python_trivia::{ indentation_at_offset, PythonWhitespace, SimpleToken, SimpleTokenKind, SimpleTokenizer, @@ -46,6 +46,7 @@ pub(super) fn place_comment<'a>( AnyNodeRef::Parameters(arguments) => { handle_parameters_separator_comment(comment, arguments, locator) } + AnyNodeRef::Arguments(arguments) => handle_arguments_comment(comment, arguments), AnyNodeRef::Comprehension(comprehension) => { handle_comprehension_comment(comment, comprehension, locator) } @@ -80,6 +81,9 @@ pub(super) fn place_comment<'a>( AnyNodeRef::StmtFunctionDef(_) | AnyNodeRef::StmtAsyncFunctionDef(_) => { handle_leading_function_with_decorators_comment(comment) } + AnyNodeRef::StmtClassDef(class_def) => { + handle_leading_class_with_decorators_comment(comment, class_def) + } AnyNodeRef::StmtImportFrom(import_from) => handle_import_from_comment(comment, import_from), _ => CommentPlacement::Default(comment), } @@ -843,6 +847,32 @@ fn handle_leading_function_with_decorators_comment(comment: DecoratedComment) -> } } +/// Handle comments between decorators and the decorated node. +/// +/// For example, given: +/// ```python +/// @dataclass +/// # comment +/// class Foo(Bar): +/// ... +/// ``` +/// +/// The comment should be attached to the enclosing [`ast::StmtClassDef`] as a dangling node, +/// as opposed to being treated as a leading comment on `Bar` or similar. +fn handle_leading_class_with_decorators_comment<'a>( + comment: DecoratedComment<'a>, + class_def: &'a ast::StmtClassDef, +) -> CommentPlacement<'a> { + if comment.start() < class_def.name.start() { + if let Some(decorator) = class_def.decorator_list.last() { + if decorator.end() < comment.start() { + return CommentPlacement::dangling(class_def, comment); + } + } + } + CommentPlacement::Default(comment) +} + /// Handles comments between `**` and the variable name in dict unpacking /// It attaches these to the appropriate value node /// @@ -1105,6 +1135,64 @@ fn find_only_token_in_range( token } +/// Attach an enclosed end-of-line comment to a set of [`Arguments`]. +/// +/// For example, given: +/// ```python +/// foo( # comment +/// bar, +/// ) +/// ``` +/// +/// The comment will be attached to the [`Arguments`] node as a dangling comment, to ensure +/// that it remains on the same line as open parenthesis. +fn handle_arguments_comment<'a>( + comment: DecoratedComment<'a>, + arguments: &'a Arguments, +) -> CommentPlacement<'a> { + // The comment needs to be on the same line, but before the first argument. For example, we want + // to treat this as a dangling comment: + // ```python + // foo( # comment + // bar, + // baz, + // qux, + // ) + // ``` + // However, this should _not_ be treated as a dangling comment: + // ```python + // foo(bar, # comment + // baz, + // qux, + // ) + // ``` + // Thus, we check whether the comment is an end-of-line comment _between_ the start of the + // statement and the first argument. If so, the only possible position is immediately following + // the open parenthesis. + if comment.line_position().is_end_of_line() { + let first_argument = match (arguments.args.as_slice(), arguments.keywords.as_slice()) { + ([first_arg, ..], [first_keyword, ..]) => { + if first_arg.start() < first_keyword.start() { + Some(first_arg.range()) + } else { + Some(first_keyword.range()) + } + } + ([first_arg, ..], []) => Some(first_arg.range()), + ([], [first_keyword, ..]) => Some(first_keyword.range()), + ([], []) => None, + }; + + if let Some(first_argument) = first_argument { + if arguments.start() < comment.start() && comment.end() < first_argument.start() { + return CommentPlacement::dangling(comment.enclosing_node(), comment); + } + } + } + + CommentPlacement::Default(comment) +} + /// Attach an enclosed end-of-line comment to a [`StmtImportFrom`]. /// /// For example, given: diff --git a/crates/ruff_python_formatter/src/comments/visitor.rs b/crates/ruff_python_formatter/src/comments/visitor.rs index 81202c854f1ed..ed664a95569d6 100644 --- a/crates/ruff_python_formatter/src/comments/visitor.rs +++ b/crates/ruff_python_formatter/src/comments/visitor.rs @@ -1,8 +1,9 @@ use std::iter::Peekable; use ruff_python_ast::{ - Alias, Comprehension, Decorator, ElifElseClause, ExceptHandler, Expr, Keyword, MatchCase, Mod, - Parameter, ParameterWithDefault, Parameters, Pattern, Ranged, Stmt, TypeParam, WithItem, + Alias, Arguments, Comprehension, Decorator, ElifElseClause, ExceptHandler, Expr, Keyword, + MatchCase, Mod, Parameter, ParameterWithDefault, Parameters, Pattern, Ranged, Stmt, TypeParam, + WithItem, }; use ruff_text_size::{TextRange, TextSize}; @@ -229,6 +230,13 @@ impl<'ast> PreorderVisitor<'ast> for CommentsVisitor<'ast> { self.finish_node(format_spec); } + fn visit_arguments(&mut self, arguments: &'ast Arguments) { + if self.start_node(arguments).is_traverse() { + walk_arguments(self, arguments); + } + self.finish_node(arguments); + } + fn visit_parameters(&mut self, parameters: &'ast Parameters) { if self.start_node(parameters).is_traverse() { walk_parameters(self, parameters); diff --git a/crates/ruff_python_formatter/src/expression/expr_call.rs b/crates/ruff_python_formatter/src/expression/expr_call.rs index 3801e7d8d84a2..af4fa6b21b66a 100644 --- a/crates/ruff_python_formatter/src/expression/expr_call.rs +++ b/crates/ruff_python_formatter/src/expression/expr_call.rs @@ -1,15 +1,6 @@ -use ruff_python_ast::{Arguments, Expr, ExprCall, Ranged}; -use ruff_text_size::{TextRange, TextSize}; - -use crate::builders::empty_parenthesized_with_dangling_comments; use ruff_formatter::write; -use ruff_python_ast::node::AnyNodeRef; -use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer}; +use ruff_python_ast::ExprCall; -use crate::expression::expr_generator_exp::GeneratorExpParentheses; -use crate::expression::parentheses::{ - parenthesized, NeedsParentheses, OptionalParentheses, Parentheses, -}; use crate::prelude::*; use crate::FormatNodeRule; @@ -21,136 +12,9 @@ impl FormatNodeRule for FormatExprCall { let ExprCall { range: _, func, - arguments: - Arguments { - args, - keywords, - range: _, - }, + arguments, } = 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 - // f( - // # This function has a dangling comment - // ) - // ``` - if args.is_empty() && keywords.is_empty() { - let comments = f.context().comments().clone(); - return write!( - f, - [ - func.format(), - empty_parenthesized_with_dangling_comments( - text("("), - comments.dangling_comments(item), - text(")"), - ) - ] - ); - } - - 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() => { - match argument { - Expr::GeneratorExp(generator_exp) => joiner.entry( - generator_exp, - &generator_exp - .format() - .with_options(GeneratorExpParentheses::StripIfOnlyFunctionArg), - ), - other => { - let parentheses = - if is_single_argument_parenthesized(argument, item.end(), source) { - Parentheses::Always - } else { - Parentheses::Never - }; - joiner.entry(other, &other.format().with_options(parentheses)) - } - }; - } - arguments => { - joiner - .entries( - // We have the parentheses from the call so the arguments never need any - arguments - .iter() - .map(|arg| (arg, arg.format().with_options(Parentheses::Preserve))), - ) - .nodes(keywords.iter()); - } - } - - joiner.finish() - }); - - write!( - f, - [ - func.format(), - // The outer group is for things like - // ```python - // get_collection( - // hey_this_is_a_very_long_call, - // it_has_funny_attributes_asdf_asdf, - // too_long_for_the_line, - // really=True, - // ) - // ``` - // The inner group is for things like: - // ```python - // get_collection( - // hey_this_is_a_very_long_call, it_has_funny_attributes_asdf_asdf, really=True - // ) - // ``` - // TODO(konstin): Doesn't work see wrongly formatted test - parenthesized("(", &group(&all_args), ")") - ] - ) - } - - fn fmt_dangling_comments(&self, _node: &ExprCall, _f: &mut PyFormatter) -> FormatResult<()> { - // Handled in `fmt_fields` - Ok(()) - } -} - -impl NeedsParentheses for ExprCall { - fn needs_parentheses( - &self, - _parent: AnyNodeRef, - context: &PyFormatContext, - ) -> OptionalParentheses { - self.func.needs_parentheses(self.into(), context) + write!(f, [func.format(), arguments.format()]) } } - -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() { - SimpleTokenKind::RParen => { - if has_seen_r_paren { - return true; - } - has_seen_r_paren = true; - } - // Skip over any trailing comma - SimpleTokenKind::Comma => continue, - _ => { - // Passed the arguments - break; - } - } - } - - false -} diff --git a/crates/ruff_python_formatter/src/generated.rs b/crates/ruff_python_formatter/src/generated.rs index 7c59811ea534b..e6346442f4a9a 100644 --- a/crates/ruff_python_formatter/src/generated.rs +++ b/crates/ruff_python_formatter/src/generated.rs @@ -2617,6 +2617,38 @@ impl<'ast> IntoFormat> for ast::Comprehension { } } +impl FormatRule> for crate::other::arguments::FormatArguments { + #[inline] + fn fmt( + &self, + node: &ast::Arguments, + f: &mut Formatter>, + ) -> FormatResult<()> { + FormatNodeRule::::fmt(self, node, f) + } +} +impl<'ast> AsFormat> for ast::Arguments { + type Format<'a> = FormatRefWithRule< + 'a, + ast::Arguments, + crate::other::arguments::FormatArguments, + PyFormatContext<'ast>, + >; + fn format(&self) -> Self::Format<'_> { + FormatRefWithRule::new(self, crate::other::arguments::FormatArguments::default()) + } +} +impl<'ast> IntoFormat> for ast::Arguments { + type Format = FormatOwnedWithRule< + ast::Arguments, + crate::other::arguments::FormatArguments, + PyFormatContext<'ast>, + >; + fn into_format(self) -> Self::Format { + FormatOwnedWithRule::new(self, crate::other::arguments::FormatArguments::default()) + } +} + impl FormatRule> for crate::other::parameters::FormatParameters { diff --git a/crates/ruff_python_formatter/src/other/arguments.rs b/crates/ruff_python_formatter/src/other/arguments.rs new file mode 100644 index 0000000000000..4aeefa5f0a444 --- /dev/null +++ b/crates/ruff_python_formatter/src/other/arguments.rs @@ -0,0 +1,152 @@ +use ruff_formatter::write; +use ruff_python_ast::node::{AnyNodeRef, AstNode}; +use ruff_python_ast::{Arguments, Expr, ExprCall, Ranged}; +use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer}; +use ruff_text_size::{TextRange, TextSize}; + +use crate::builders::empty_parenthesized_with_dangling_comments; +use crate::comments::trailing_comments; +use crate::expression::expr_generator_exp::GeneratorExpParentheses; +use crate::expression::parentheses::{ + parenthesized, NeedsParentheses, OptionalParentheses, Parentheses, +}; +use crate::prelude::*; +use crate::FormatNodeRule; + +#[derive(Default)] +pub struct FormatArguments; + +impl FormatNodeRule for FormatArguments { + fn fmt_fields(&self, item: &Arguments, f: &mut PyFormatter) -> FormatResult<()> { + // 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 + // f( + // # This call has a dangling comment. + // ) + // ``` + if item.args.is_empty() && item.keywords.is_empty() { + let comments = f.context().comments().clone(); + return write!( + f, + [empty_parenthesized_with_dangling_comments( + text("("), + comments.dangling_comments(item), + text(")"), + )] + ); + } + + // If the arguments are non-empty, then a dangling comment indicates a comment on the + // same line as the opening parenthesis, e.g.: + // ```python + // f( # This call has a dangling comment. + // a, + // b, + // c, + // ) + let comments = f.context().comments().clone(); + let dangling_comments = comments.dangling_comments(item.as_any_node_ref()); + write!(f, [trailing_comments(dangling_comments)])?; + + 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() => { + match arg { + Expr::GeneratorExp(generator_exp) => joiner.entry( + generator_exp, + &generator_exp + .format() + .with_options(GeneratorExpParentheses::StripIfOnlyFunctionArg), + ), + other => { + let parentheses = + if is_single_argument_parenthesized(arg, item.end(), source) { + Parentheses::Always + } else { + Parentheses::Never + }; + joiner.entry(other, &other.format().with_options(parentheses)) + } + }; + } + 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()); + } + } + + joiner.finish() + }); + + write!( + f, + [ + // The outer group is for things like + // ```python + // get_collection( + // hey_this_is_a_very_long_call, + // it_has_funny_attributes_asdf_asdf, + // too_long_for_the_line, + // really=True, + // ) + // ``` + // The inner group is for things like: + // ```python + // get_collection( + // hey_this_is_a_very_long_call, it_has_funny_attributes_asdf_asdf, really=True + // ) + // ``` + // TODO(konstin): Doesn't work see wrongly formatted test + parenthesized("(", &group(&all_arguments), ")") + ] + ) + } + + fn fmt_dangling_comments(&self, _node: &Arguments, _f: &mut PyFormatter) -> FormatResult<()> { + // Handled in `fmt_fields` + Ok(()) + } +} + +impl NeedsParentheses for ExprCall { + fn needs_parentheses( + &self, + _parent: AnyNodeRef, + context: &PyFormatContext, + ) -> OptionalParentheses { + self.func.needs_parentheses(self.into(), context) + } +} + +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() { + SimpleTokenKind::RParen => { + if has_seen_r_paren { + return true; + } + has_seen_r_paren = true; + } + // Skip over any trailing comma + SimpleTokenKind::Comma => continue, + _ => { + // Passed the arguments + break; + } + } + } + + false +} diff --git a/crates/ruff_python_formatter/src/other/mod.rs b/crates/ruff_python_formatter/src/other/mod.rs index e6d87fe6ccde7..4f9e3a52bba5c 100644 --- a/crates/ruff_python_formatter/src/other/mod.rs +++ b/crates/ruff_python_formatter/src/other/mod.rs @@ -1,4 +1,5 @@ pub(crate) mod alias; +pub(crate) mod arguments; pub(crate) mod comprehension; pub(crate) mod decorator; pub(crate) mod elif_else_clause; 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 8818f6a46a391..7596453172bb4 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_class_def.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_class_def.rs @@ -1,11 +1,8 @@ -use ruff_python_ast::{Arguments, Ranged, StmtClassDef}; -use ruff_text_size::TextRange; - use ruff_formatter::write; -use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer}; +use ruff_python_ast::{Ranged, StmtClassDef}; +use ruff_python_trivia::{lines_after, skip_trailing_trivia}; -use crate::comments::trailing_comments; -use crate::expression::parentheses::{parenthesized, Parentheses}; +use crate::comments::{leading_comments, trailing_comments}; use crate::prelude::*; use crate::statement::suite::SuiteKind; @@ -23,40 +20,84 @@ impl FormatNodeRule for FormatStmtClassDef { decorator_list, } = item; - f.join_with(hard_line_break()) - .entries(decorator_list.iter().formatted()) - .finish()?; + let comments = f.context().comments().clone(); + + let dangling_comments = comments.dangling_comments(item); + let trailing_definition_comments_start = + dangling_comments.partition_point(|comment| comment.line_position().is_own_line()); + + let (leading_definition_comments, trailing_definition_comments) = + dangling_comments.split_at(trailing_definition_comments_start); + + if let Some(last_decorator) = decorator_list.last() { + f.join_with(hard_line_break()) + .entries(decorator_list.iter().formatted()) + .finish()?; - if !decorator_list.is_empty() { - hard_line_break().fmt(f)?; + if leading_definition_comments.is_empty() { + write!(f, [hard_line_break()])?; + } else { + // Write any leading definition comments (between last decorator and the header) + // while maintaining the right amount of empty lines between the comment + // and the last decorator. + let decorator_end = + skip_trailing_trivia(last_decorator.end(), f.context().source()); + + let leading_line = if lines_after(decorator_end, f.context().source()) <= 1 { + hard_line_break() + } else { + empty_line() + }; + + write!( + f, + [leading_line, leading_comments(leading_definition_comments)] + )?; + } } write!(f, [text("class"), space(), name.format()])?; - if arguments - .as_ref() - .is_some_and(|Arguments { args, keywords, .. }| { - !(args.is_empty() && keywords.is_empty()) - }) - { - parenthesized( - "(", - &FormatInheritanceClause { - class_definition: item, - }, - ")", - ) - .fmt(f)?; + if let Some(arguments) = arguments { + // Drop empty parentheses, e.g., in: + // ```python + // class A(): + // ... + // ``` + // + // However, preserve any dangling end-of-line comments, e.g., in: + // ```python + // class A( # comment + // ): + // ... + // + // If the arguments contain any dangling own-line comments, we retain the parentheses, + // e.g., in: + // ```python + // class A( # comment + // # comment + // ): + // ... + // ``` + if arguments.args.is_empty() + && arguments.keywords.is_empty() + && comments + .dangling_comments(arguments) + .iter() + .all(|comment| comment.line_position().is_end_of_line()) + { + let dangling = comments.dangling_comments(arguments); + write!(f, [trailing_comments(dangling)])?; + } else { + write!(f, [arguments.format()])?; + } } - let comments = f.context().comments().clone(); - let trailing_head_comments = comments.dangling_comments(item); - write!( f, [ text(":"), - trailing_comments(trailing_head_comments), + trailing_comments(trailing_definition_comments), block_indent(&body.format().with_options(SuiteKind::Class)) ] ) @@ -71,58 +112,3 @@ impl FormatNodeRule for FormatStmtClassDef { Ok(()) } } - -struct FormatInheritanceClause<'a> { - class_definition: &'a StmtClassDef, -} - -impl Format> for FormatInheritanceClause<'_> { - fn fmt(&self, f: &mut Formatter>) -> FormatResult<()> { - let StmtClassDef { - arguments: - Some(Arguments { - args: bases, - keywords, - .. - }), - name, - body, - .. - } = self.class_definition - else { - return Ok(()); - }; - - let source = f.context().source(); - - 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` - // doesn't know that it should disregard the parentheses of the inheritance clause. - // ```python - // class Test(A) # A is not parenthesized, the parentheses belong to the inheritance clause - // class Test((A)) # A is parenthesized - // ``` - // parentheses from the inheritance clause belong to the expression. - let tokenizer = SimpleTokenizer::new(source, TextRange::new(name.end(), first.start())) - .skip_trivia(); - - let left_paren_count = tokenizer - .take_while(|token| token.kind() == SimpleTokenKind::LParen) - .count(); - - // Ignore the first parentheses count - let parentheses = if left_paren_count > 1 { - Parentheses::Always - } else { - Parentheses::Never - }; - - joiner.entry(first, &first.format().with_options(parentheses)); - joiner.nodes(rest.iter()); - } - - joiner.nodes(keywords.iter()).finish() - } -} 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 dc89011450695..7290a9ca853a0 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_function_def.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_function_def.rs @@ -44,7 +44,7 @@ impl FormatRule, PyFormatContext<'_>> for FormatAnyFun let trailing_definition_comments_start = dangling_comments.partition_point(|comment| comment.line_position().is_own_line()); - let (leading_function_definition_comments, trailing_definition_comments) = + let (leading_definition_comments, trailing_definition_comments) = dangling_comments.split_at(trailing_definition_comments_start); if let Some(last_decorator) = item.decorators().last() { @@ -52,10 +52,10 @@ impl FormatRule, PyFormatContext<'_>> for FormatAnyFun .entries(item.decorators().iter().formatted()) .finish()?; - if leading_function_definition_comments.is_empty() { + if leading_definition_comments.is_empty() { write!(f, [hard_line_break()])?; } else { - // Write any leading function comments (between last decorator and function header) + // Write any leading definition comments (between last decorator and the header) // while maintaining the right amount of empty lines between the comment // and the last decorator. let decorator_end = @@ -69,10 +69,7 @@ impl FormatRule, PyFormatContext<'_>> for FormatAnyFun write!( f, - [ - leading_line, - leading_comments(leading_function_definition_comments) - ] + [leading_line, leading_comments(leading_definition_comments)] )?; } } 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 bac556ce5f248..0960faa65260f 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 @@ -92,6 +92,31 @@ f( f( a.very_long_function_function_that_is_so_long_that_it_expands_the_parent_but_its_only_a_single_argument() ) + +f( # abc +) + +f( # abc + # abc +) + +f( + # abc +) + +f ( # abc + 1 +) + +f ( + # abc + 1 +) + +f ( + 1 + # abc +) ``` ## Output @@ -137,8 +162,9 @@ f( these_arguments_have_values_that_need_to_break_because_they_are_too_long3=session, ) -f() -# dangling comment +f( + # dangling comment +) f(only=1, short=1, arguments=1) @@ -177,6 +203,28 @@ f( f( a.very_long_function_function_that_is_so_long_that_it_expands_the_parent_but_its_only_a_single_argument() ) + +f() # abc + +f( # abc + # abc +) + +f( + # abc +) + +f(1) # abc + +f( + # abc + 1 +) + +f( + 1 + # abc +) ``` diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__dict.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__dict.py.snap index 300074af734b5..a799fe15a8564 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__dict.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__dict.py.snap @@ -130,8 +130,7 @@ a = { 3: True, } -x = { # dangling end of line comment -} +x = {} # dangling end of line comment ``` diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__lambda.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__lambda.py.snap index fbc6c3ac760cf..85d2b0e653491 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__lambda.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__lambda.py.snap @@ -149,8 +149,7 @@ a = ( # Regression test: lambda empty arguments ranges were too long, leading to unstable # formatting ( - lambda: ( # - ), + lambda: (), # ) diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__list.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__list.py.snap index e21436a052668..f84edaf59288b 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__list.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__list.py.snap @@ -33,8 +33,7 @@ b3 = [ ```py # Dangling comment placement in empty lists # Regression test for https://github.com/python/cpython/blob/03160630319ca26dcbbad65225da4248e54c45ec/Tools/c-analyzer/c_analyzer/datafiles.py#L14-L16 -a1 = [ # a -] +a1 = [] # a a2 = [ # a # b ] diff --git a/crates/ruff_python_formatter/tests/snapshots/format@statement__class_definition.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@statement__class_definition.py.snap index 8dadf4f8c2fa3..a5de9b0da9257 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@statement__class_definition.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@statement__class_definition.py.snap @@ -98,6 +98,58 @@ class Test: """Docstring""" # comment x = 1 + + +class C(): # comment + pass + + +class C( # comment +): + pass + + +class C( + # comment +): + pass + + +class C(): # comment + pass + + +class C( # comment + # comment + 1 +): + pass + + +class C( + 1 + # comment +): + pass + + +@dataclass +# Copied from transformers.models.clip.modeling_clip.CLIPOutput with CLIP->AltCLIP +class AltCLIPOutput(ModelOutput): + ... + + +@dataclass +class AltCLIPOutput( # Copied from transformers.models.clip.modeling_clip.CLIPOutput with CLIP->AltCLIP +): + ... + + +@dataclass +class AltCLIPOutput( + # Copied from transformers.models.clip.modeling_clip.CLIPOutput with CLIP->AltCLIP +): + ... ``` ## Output @@ -116,8 +168,7 @@ class Test((Aaaaaaaaaaaaaaaaa), Bbbbbbbbbbbbbbbb, metaclass=meta): pass -class Test( - # trailing class comment +class Test( # trailing class comment Aaaaaaaaaaaaaaaaa, # trailing comment # in between comment Bbbbbbbbbbbbbbbb, @@ -217,6 +268,56 @@ class Test: # comment x = 1 + + +class C: # comment + pass + + +class C: # comment + pass + + +class C( + # comment +): + pass + + +class C: # comment + pass + + +class C( # comment + # comment + 1 +): + pass + + +class C( + 1 + # comment +): + pass + + +@dataclass +# Copied from transformers.models.clip.modeling_clip.CLIPOutput with CLIP->AltCLIP +class AltCLIPOutput(ModelOutput): + ... + + +@dataclass +class AltCLIPOutput: # Copied from transformers.models.clip.modeling_clip.CLIPOutput with CLIP->AltCLIP + ... + + +@dataclass +class AltCLIPOutput( + # Copied from transformers.models.clip.modeling_clip.CLIPOutput with CLIP->AltCLIP +): + ... ``` diff --git a/crates/ruff_python_formatter/tests/snapshots/format@statement__delete.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@statement__delete.py.snap index b257c6486f864..39a4afbc07410 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@statement__delete.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@statement__delete.py.snap @@ -215,8 +215,7 @@ del ( ) # Completed # Done -del ( # dangling end of line comment -) +del () # dangling end of line comment ``` diff --git a/crates/ruff_python_formatter/tests/snapshots/format@statement__raise.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@statement__raise.py.snap index a90849f43ba08..1f50cb8320688 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@statement__raise.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@statement__raise.py.snap @@ -192,8 +192,7 @@ raise aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfk < ( ) # the other end # sneaky comment -raise ( # another comment -) +raise () # another comment raise () # what now @@ -201,8 +200,9 @@ raise ( # sould I stay here # just a comment here ) # trailing comment -raise hello() # sould I stay here -# just a comment here # trailing comment +raise hello( # sould I stay here + # just a comment here +) # trailing comment raise ( # sould I stay here