From e2ec42539bfedc13270a2164faebe61e3c2e51dd Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Fri, 29 Sep 2023 11:45:01 +0200 Subject: [PATCH] Attach dangling comments to the comprehension instead of the `if` or `iter` nodes (#7693) --- crates/ruff_python_formatter/Cargo.toml | 2 +- .../fixtures/ruff/expression/list_comp.py | 7 +++ .../src/comments/placement.rs | 10 ++-- .../src/expression/expr_compare.rs | 3 +- .../src/expression/expr_dict_comp.rs | 11 +++- .../src/expression/expr_f_string.rs | 10 ++++ .../src/expression/expr_name.rs | 3 +- .../src/expression/expr_slice.rs | 9 +++ crates/ruff_python_formatter/src/lib.rs | 5 ++ .../src/module/mod_module.rs | 5 +- .../src/other/comprehension.rs | 58 +++++++++++++++---- .../src/pattern/pattern_match_sequence.rs | 10 ++++ .../format@expression__list_comp.py.snap | 13 +++++ 13 files changed, 124 insertions(+), 22 deletions(-) diff --git a/crates/ruff_python_formatter/Cargo.toml b/crates/ruff_python_formatter/Cargo.toml index 75782a5d25..2e2a650197 100644 --- a/crates/ruff_python_formatter/Cargo.toml +++ b/crates/ruff_python_formatter/Cargo.toml @@ -38,7 +38,7 @@ tracing = { workspace = true } unicode-width = { workspace = true } [dev-dependencies] -ruff_formatter = { path = "../ruff_formatter", features = ["serde"] } +ruff_formatter = { path = "../ruff_formatter" } insta = { workspace = true, features = ["glob"] } serde = { workspace = true } diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/list_comp.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/list_comp.py index f2d8757c28..4e63cf11db 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/list_comp.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/list_comp.py @@ -95,3 +95,10 @@ aaaaaaaaaaaaaaaaaaaaa = [ x for (x, y,) in z if head_name ] + +[ + 1 for components in # pylint: disable=undefined-loop-variable + b + # integer 1 may only have decimal 01-09 + c # negative decimal +] + diff --git a/crates/ruff_python_formatter/src/comments/placement.rs b/crates/ruff_python_formatter/src/comments/placement.rs index bd67a0771a..88b72cdd89 100644 --- a/crates/ruff_python_formatter/src/comments/placement.rs +++ b/crates/ruff_python_formatter/src/comments/placement.rs @@ -2083,7 +2083,7 @@ fn handle_comprehension_comment<'a>( CommentPlacement::Default(comment) } else { // after the `for` - CommentPlacement::dangling(comment.enclosing_node(), comment) + CommentPlacement::dangling(comprehension, comment) }; } @@ -2106,7 +2106,7 @@ fn handle_comprehension_comment<'a>( // attach as dangling comments on the target // (to be rendered as leading on the "in") return if is_own_line { - CommentPlacement::dangling(comment.enclosing_node(), comment) + CommentPlacement::dangling(comprehension, comment) } else { // correctly trailing on the target CommentPlacement::Default(comment) @@ -2126,7 +2126,7 @@ fn handle_comprehension_comment<'a>( CommentPlacement::Default(comment) } else { // after the `in` but same line, turn into trailing on the `in` token - CommentPlacement::dangling(&comprehension.iter, comment) + CommentPlacement::dangling(comprehension, comment) }; } @@ -2157,10 +2157,10 @@ fn handle_comprehension_comment<'a>( ); if is_own_line { if last_end < comment.start() && comment.start() < if_token.start() { - return CommentPlacement::dangling(if_node, comment); + return CommentPlacement::dangling(comprehension, comment); } } else if if_token.start() < comment.start() && comment.start() < if_node.start() { - return CommentPlacement::dangling(if_node, comment); + return CommentPlacement::dangling(comprehension, comment); } last_end = if_node.end(); } diff --git a/crates/ruff_python_formatter/src/expression/expr_compare.rs b/crates/ruff_python_formatter/src/expression/expr_compare.rs index e3c75e8891..ae4e4d9b49 100644 --- a/crates/ruff_python_formatter/src/expression/expr_compare.rs +++ b/crates/ruff_python_formatter/src/expression/expr_compare.rs @@ -20,10 +20,11 @@ impl FormatNodeRule for FormatExprCompare { fn fmt_dangling_comments( &self, - _dangling_comments: &[SourceComment], + dangling_comments: &[SourceComment], _f: &mut PyFormatter, ) -> FormatResult<()> { // Node can not have dangling comments + debug_assert!(dangling_comments.is_empty()); Ok(()) } } diff --git a/crates/ruff_python_formatter/src/expression/expr_dict_comp.rs b/crates/ruff_python_formatter/src/expression/expr_dict_comp.rs index 33c3450af8..fa3566809a 100644 --- a/crates/ruff_python_formatter/src/expression/expr_dict_comp.rs +++ b/crates/ruff_python_formatter/src/expression/expr_dict_comp.rs @@ -3,7 +3,7 @@ use ruff_python_ast::node::AnyNodeRef; use ruff_python_ast::ExprDictComp; use ruff_text_size::Ranged; -use crate::comments::dangling_comments; +use crate::comments::{dangling_comments, SourceComment}; use crate::expression::parentheses::{parenthesized, NeedsParentheses, OptionalParentheses}; use crate::prelude::*; @@ -58,6 +58,15 @@ impl FormatNodeRule for FormatExprDictComp { .with_dangling_comments(open_parenthesis_comments)] ) } + + fn fmt_dangling_comments( + &self, + _dangling_node_comments: &[SourceComment], + _f: &mut PyFormatter, + ) -> FormatResult<()> { + // Handled as part of `fmt_fields` + Ok(()) + } } impl NeedsParentheses for ExprDictComp { diff --git a/crates/ruff_python_formatter/src/expression/expr_f_string.rs b/crates/ruff_python_formatter/src/expression/expr_f_string.rs index fc12c2c5c0..5a9612f39f 100644 --- a/crates/ruff_python_formatter/src/expression/expr_f_string.rs +++ b/crates/ruff_python_formatter/src/expression/expr_f_string.rs @@ -1,5 +1,6 @@ use memchr::memchr2; +use crate::comments::SourceComment; use ruff_formatter::FormatResult; use ruff_python_ast::node::AnyNodeRef; use ruff_python_ast::ExprFString; @@ -16,6 +17,15 @@ impl FormatNodeRule for FormatExprFString { fn fmt_fields(&self, item: &ExprFString, f: &mut PyFormatter) -> FormatResult<()> { FormatString::new(&AnyString::FString(item)).fmt(f) } + + fn fmt_dangling_comments( + &self, + _dangling_node_comments: &[SourceComment], + _f: &mut PyFormatter, + ) -> FormatResult<()> { + // Handled as part of `fmt_fields` + Ok(()) + } } impl NeedsParentheses for ExprFString { diff --git a/crates/ruff_python_formatter/src/expression/expr_name.rs b/crates/ruff_python_formatter/src/expression/expr_name.rs index 244943eacf..7a49de0894 100644 --- a/crates/ruff_python_formatter/src/expression/expr_name.rs +++ b/crates/ruff_python_formatter/src/expression/expr_name.rs @@ -26,10 +26,11 @@ impl FormatNodeRule for FormatExprName { fn fmt_dangling_comments( &self, - _dangling_comments: &[SourceComment], + dangling_comments: &[SourceComment], _f: &mut PyFormatter, ) -> FormatResult<()> { // Node cannot have dangling comments + debug_assert!(dangling_comments.is_empty()); Ok(()) } } diff --git a/crates/ruff_python_formatter/src/expression/expr_slice.rs b/crates/ruff_python_formatter/src/expression/expr_slice.rs index c8177e9f80..85d3822029 100644 --- a/crates/ruff_python_formatter/src/expression/expr_slice.rs +++ b/crates/ruff_python_formatter/src/expression/expr_slice.rs @@ -146,6 +146,15 @@ impl FormatNodeRule for FormatExprSlice { } Ok(()) } + + fn fmt_dangling_comments( + &self, + _dangling_node_comments: &[SourceComment], + _f: &mut PyFormatter, + ) -> FormatResult<()> { + // Handled as part of `fmt_fields` + Ok(()) + } } /// We're in a slice, so we know there's a first colon, but with have to look into the source diff --git a/crates/ruff_python_formatter/src/lib.rs b/crates/ruff_python_formatter/src/lib.rs index 4ebdfb7723..3bd8ae3f4d 100644 --- a/crates/ruff_python_formatter/src/lib.rs +++ b/crates/ruff_python_formatter/src/lib.rs @@ -85,6 +85,11 @@ where dangling_node_comments: &[SourceComment], f: &mut PyFormatter, ) -> FormatResult<()> { + debug_assert!( + dangling_node_comments.is_empty(), + "The node has dangling comments that need to be formatted manually. Add the special dangling comments handling to `fmt_fields` and override `fmt_dangling_comments` with an empty implementation that returns `Ok(())`." + ); + dangling_comments(dangling_node_comments).fmt(f) } diff --git a/crates/ruff_python_formatter/src/module/mod_module.rs b/crates/ruff_python_formatter/src/module/mod_module.rs index 304c885d41..c42f79a1c1 100644 --- a/crates/ruff_python_formatter/src/module/mod_module.rs +++ b/crates/ruff_python_formatter/src/module/mod_module.rs @@ -37,10 +37,11 @@ impl FormatNodeRule for FormatModModule { fn fmt_dangling_comments( &self, - _dangling_comments: &[SourceComment], + dangling_comments: &[SourceComment], _f: &mut PyFormatter, ) -> FormatResult<()> { - // Handled as part of `fmt_fields` + // Node can't have dangling comments. + debug_assert!(dangling_comments.is_empty()); Ok(()) } } diff --git a/crates/ruff_python_formatter/src/other/comprehension.rs b/crates/ruff_python_formatter/src/other/comprehension.rs index 788e782ab7..82e4b4e119 100644 --- a/crates/ruff_python_formatter/src/other/comprehension.rs +++ b/crates/ruff_python_formatter/src/other/comprehension.rs @@ -1,6 +1,7 @@ -use ruff_formatter::{format_args, write, Buffer, FormatResult}; +use ruff_formatter::{format_args, write, Buffer, FormatError, FormatResult}; use ruff_python_ast::{Comprehension, Expr}; -use ruff_text_size::Ranged; +use ruff_python_trivia::{SimpleToken, SimpleTokenKind, SimpleTokenizer}; +use ruff_text_size::{Ranged, TextRange}; use crate::comments::{leading_comments, trailing_comments, SourceComment}; use crate::expression::expr_tuple::TupleParentheses; @@ -37,11 +38,35 @@ impl FormatNodeRule for FormatComprehension { let comments = f.context().comments().clone(); let dangling_item_comments = comments.dangling(item); - let (before_target_comments, before_in_comments) = dangling_item_comments.split_at( + let (before_target_comments, dangling_comments) = dangling_item_comments.split_at( dangling_item_comments.partition_point(|comment| comment.end() < target.start()), ); - let trailing_in_comments = comments.dangling(iter); + let maybe_in_token = SimpleTokenizer::new( + f.context().source(), + TextRange::new(target.end(), iter.start()), + ) + .skip_trivia() + .next(); + + let Some( + in_keyword @ SimpleToken { + kind: SimpleTokenKind::In, + .. + }, + ) = maybe_in_token + else { + return Err(FormatError::syntax_error( + "Expected `in` keyword between the `target` and `iter`.", + )); + }; + + let (before_in_comments, dangling_comments) = dangling_comments.split_at( + dangling_comments.partition_point(|comment| comment.end() < in_keyword.start()), + ); + + let (trailing_in_comments, dangling_if_comments) = dangling_comments + .split_at(dangling_comments.partition_point(|comment| comment.start() < iter.start())); let in_spacer = format_with(|f| { if before_in_comments.is_empty() { @@ -66,17 +91,23 @@ impl FormatNodeRule for FormatComprehension { iter.format(), ] )?; + if !ifs.is_empty() { let joined = format_with(|f| { let mut joiner = f.join_with(soft_line_break_or_space()); - for if_case in ifs { - let dangling_if_comments = comments.dangling(if_case); + let mut dangling_if_comments = dangling_if_comments; + + for if_case in ifs { + let (if_comments, rest) = dangling_if_comments.split_at( + dangling_if_comments + .partition_point(|comment| comment.start() < if_case.start()), + ); + + let (own_line_if_comments, end_of_line_if_comments) = if_comments.split_at( + if_comments + .partition_point(|comment| comment.line_position().is_own_line()), + ); - let (own_line_if_comments, end_of_line_if_comments) = dangling_if_comments - .split_at( - dangling_if_comments - .partition_point(|comment| comment.line_position().is_own_line()), - ); joiner.entry(&format_args!( leading_comments(own_line_if_comments), token("if"), @@ -84,7 +115,12 @@ impl FormatNodeRule for FormatComprehension { Spacer(if_case), if_case.format(), )); + + dangling_if_comments = rest; } + + debug_assert!(dangling_if_comments.is_empty()); + joiner.finish() }); diff --git a/crates/ruff_python_formatter/src/pattern/pattern_match_sequence.rs b/crates/ruff_python_formatter/src/pattern/pattern_match_sequence.rs index 86f4d6f7f6..8f5f10ce52 100644 --- a/crates/ruff_python_formatter/src/pattern/pattern_match_sequence.rs +++ b/crates/ruff_python_formatter/src/pattern/pattern_match_sequence.rs @@ -1,3 +1,4 @@ +use crate::comments::SourceComment; use ruff_formatter::{format_args, Format, FormatResult}; use ruff_python_ast::node::AnyNodeRef; use ruff_python_ast::PatternMatchSequence; @@ -55,6 +56,15 @@ impl FormatNodeRule for FormatPatternMatchSequence { SequenceType::TupleNoParens => optional_parentheses(&items).fmt(f), } } + + fn fmt_dangling_comments( + &self, + _dangling_node_comments: &[SourceComment], + _f: &mut PyFormatter, + ) -> FormatResult<()> { + // Handled as part of `fmt_fields` + Ok(()) + } } impl NeedsParentheses for PatternMatchSequence { diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__list_comp.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__list_comp.py.snap index b3a09e301c..dcfd484622 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__list_comp.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__list_comp.py.snap @@ -101,6 +101,13 @@ aaaaaaaaaaaaaaaaaaaaa = [ x for (x, y,) in z if head_name ] + +[ + 1 for components in # pylint: disable=undefined-loop-variable + b + # integer 1 may only have decimal 01-09 + c # negative decimal +] + ``` ## Output @@ -234,6 +241,12 @@ aaaaaaaaaaaaaaaaaaaaa = [ ) in z if head_name ] + +[ + 1 + for components in b # pylint: disable=undefined-loop-variable # integer 1 may only have decimal 01-09 + + c # negative decimal +] ```