From 3299c25cefb6e3eb4b55396b2f842138b658e42f Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 24 Oct 2024 15:16:31 -0300 Subject: [PATCH] fix: better formatting of leading/trailing line/block comments in expression lists (#6338) # Description ## Problem While fixing the bug [this PR](https://github.com/noir-lang/noir/pull/6337) was fixing I noticed that the formatter destroyed some array formatting, like this one: https://github.com/noir-lang/noir_json_parser/blob/bc7094394baeaa185c5bf56ae806e302c786bdd3/src/_string_tools/slice_packed_field.nr#L13-L19 ## Summary I decided to try to fix this because it's bad if the formatter doesn't respect this initial formatting. And this change if for any expression list, so it applies to arrays, tuples, call arguments, etc, so we only need to fix this once. ## Additional Context With this some leading spaces surrounding block comments are gone, but I think it's better because for example array literals don't have spaces after `[` and before `]`, and before this PR the formatter would generate `[ /* comment */ 1]` while now it generates `[/* comment */ 1]` which I think looks better (and is what rustfmt does too). I think I didn't want to spend too much time on these details on the initial formatter pass to avoid getting into an infinite improvement loop. ## Documentation Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- tooling/nargo_fmt/src/chunks.rs | 15 +++- .../src/formatter/comments_and_whitespace.rs | 4 +- tooling/nargo_fmt/src/formatter/expression.rs | 90 ++++++++++++++++--- tooling/nargo_fmt/src/formatter/use_tree.rs | 2 +- .../nargo_fmt/src/formatter/use_tree_merge.rs | 2 +- tooling/nargo_fmt/tests/expected/tuple.nr | 24 ++--- 6 files changed, 102 insertions(+), 35 deletions(-) diff --git a/tooling/nargo_fmt/src/chunks.rs b/tooling/nargo_fmt/src/chunks.rs index d4fb540a221..673cf020aed 100644 --- a/tooling/nargo_fmt/src/chunks.rs +++ b/tooling/nargo_fmt/src/chunks.rs @@ -849,10 +849,10 @@ impl<'a> Formatter<'a> { } Chunk::TrailingComment(text_chunk) | Chunk::LeadingComment(text_chunk) => { self.write(&text_chunk.string); - self.write(" "); + self.write_space_without_skipping_whitespace_and_comments(); } Chunk::Group(chunks) => self.format_chunk_group_impl(chunks), - Chunk::SpaceOrLine => self.write(" "), + Chunk::SpaceOrLine => self.write_space_without_skipping_whitespace_and_comments(), Chunk::IncreaseIndentation => self.increase_indentation(), Chunk::DecreaseIndentation => self.decrease_indentation(), Chunk::PushIndentation => self.push_indentation(), @@ -915,9 +915,16 @@ impl<'a> Formatter<'a> { self.write_indentation(); } Chunk::LeadingComment(text_chunk) => { + let ends_with_newline = text_chunk.string.ends_with('\n'); self.write_chunk_lines(text_chunk.string.trim()); - self.write_line_without_skipping_whitespace_and_comments(); - self.write_indentation(); + + // Respect whether the leading comment had a newline before what comes next or not + if ends_with_newline { + self.write_line_without_skipping_whitespace_and_comments(); + self.write_indentation(); + } else { + self.write_space_without_skipping_whitespace_and_comments(); + } } Chunk::Group(mut group) => { if chunks.force_multiline_on_children_with_same_tag_if_multiline diff --git a/tooling/nargo_fmt/src/formatter/comments_and_whitespace.rs b/tooling/nargo_fmt/src/formatter/comments_and_whitespace.rs index e5f15bc397e..4aba0481e24 100644 --- a/tooling/nargo_fmt/src/formatter/comments_and_whitespace.rs +++ b/tooling/nargo_fmt/src/formatter/comments_and_whitespace.rs @@ -301,8 +301,8 @@ mod tests { let src = "global x = [ /* hello */ 1 , 2 ] ;"; let expected = "global x = [ - /* hello */ - 1, 2, + /* hello */ 1, + 2, ]; "; assert_format_with_max_width(src, expected, 20); diff --git a/tooling/nargo_fmt/src/formatter/expression.rs b/tooling/nargo_fmt/src/formatter/expression.rs index 80b9886c047..ebfa3ae78fb 100644 --- a/tooling/nargo_fmt/src/formatter/expression.rs +++ b/tooling/nargo_fmt/src/formatter/expression.rs @@ -446,23 +446,22 @@ impl<'a, 'b> ChunkFormatter<'a, 'b> { { let mut comments_chunk = self.skip_comments_and_whitespace_chunk(); - // If the comment is not empty but doesn't have newlines, it's surely `/* comment */`. - // We format that with spaces surrounding it so it looks, for example, like `Foo { /* comment */ field ..`. - if !comments_chunk.string.trim().is_empty() && !comments_chunk.has_newlines { - // Note: there's no space after `{}` because space will be produced by format_items_separated_by_comma - comments_chunk.string = if surround_with_spaces { - format!(" {}", comments_chunk.string.trim()) - } else { - format!(" {} ", comments_chunk.string.trim()) - }; - group.text(comments_chunk); - + // Handle leading block vs. line comments a bit differently. + if comments_chunk.string.trim().starts_with("/*") { group.increase_indentation(); if surround_with_spaces { group.space_or_line(); } else { group.line(); } + + // Note: there's no space before `{}` because it was just produced + comments_chunk.string = if surround_with_spaces { + comments_chunk.string.trim().to_string() + } else { + format!("{} ", comments_chunk.string.trim()) + }; + group.leading_comment(comments_chunk); } else { group.increase_indentation(); if surround_with_spaces { @@ -479,7 +478,24 @@ impl<'a, 'b> ChunkFormatter<'a, 'b> { group.text_attached_to_last_group(self.chunk(|formatter| { formatter.write_comma(); })); - group.trailing_comment(self.skip_comments_and_whitespace_chunk()); + let newlines_count_before_comment = self.following_newlines_count(); + group.text(self.chunk(|formatter| { + formatter.skip_whitespace(); + })); + if let Token::BlockComment(..) = &self.token { + // We let block comments be part of the item that's going to be formatted + } else { + // Line comments can be trailing or leading, depending on whether there are newlines before them + let comments_and_whitespace_chunk = self.skip_comments_and_whitespace_chunk(); + if !comments_and_whitespace_chunk.string.trim().is_empty() { + if newlines_count_before_comment > 0 { + group.line(); + group.leading_comment(comments_and_whitespace_chunk); + } else { + group.trailing_comment(comments_and_whitespace_chunk); + } + } + } group.space_or_line(); } format_item(self, expr, group); @@ -1326,6 +1342,56 @@ global y = 1; assert_format_with_config(src, expected, config); } + #[test] + fn format_short_array_with_block_comment_before_elements() { + let src = "global x = [ /* one */ 1, /* two */ 2 ] ;"; + let expected = "global x = [/* one */ 1, /* two */ 2];\n"; + + assert_format(src, expected); + } + + #[test] + fn format_long_array_with_block_comment_before_elements() { + let src = "global x = [ /* one */ 1, /* two */ 123456789012345, 3, 4 ] ;"; + let expected = "global x = [ + /* one */ 1, + /* two */ 123456789012345, + 3, + 4, +]; +"; + + let config = + Config { short_array_element_width_threshold: 5, max_width: 30, ..Config::default() }; + assert_format_with_config(src, expected, config); + } + + #[test] + fn format_long_array_with_line_comment_before_elements() { + let src = "global x = [ + // one + 1, + // two + 123456789012345, + 3, + 4, +]; +"; + let expected = "global x = [ + // one + 1, + // two + 123456789012345, + 3, + 4, +]; +"; + + let config = + Config { short_array_element_width_threshold: 5, max_width: 30, ..Config::default() }; + assert_format_with_config(src, expected, config); + } + #[test] fn format_cast() { let src = "global x = 1 as u8 ;"; diff --git a/tooling/nargo_fmt/src/formatter/use_tree.rs b/tooling/nargo_fmt/src/formatter/use_tree.rs index bc8dc3fcabb..98d63ef6611 100644 --- a/tooling/nargo_fmt/src/formatter/use_tree.rs +++ b/tooling/nargo_fmt/src/formatter/use_tree.rs @@ -211,7 +211,7 @@ mod tests { #[test] fn format_use_list_one_item_with_comments() { let src = " use foo::{ /* do not remove me */ bar, };"; - let expected = "use foo::{ /* do not remove me */ bar};\n"; + let expected = "use foo::{/* do not remove me */ bar};\n"; assert_format(src, expected); } diff --git a/tooling/nargo_fmt/src/formatter/use_tree_merge.rs b/tooling/nargo_fmt/src/formatter/use_tree_merge.rs index e24b7b8cbf6..834280ddba3 100644 --- a/tooling/nargo_fmt/src/formatter/use_tree_merge.rs +++ b/tooling/nargo_fmt/src/formatter/use_tree_merge.rs @@ -397,7 +397,7 @@ mod tests { #[test] fn format_use_list_one_item_with_comments() { let src = " use foo::{ /* do not remove me */ bar, };"; - let expected = "use foo::{ /* do not remove me */ bar};\n"; + let expected = "use foo::{/* do not remove me */ bar};\n"; assert_format(src, expected); } diff --git a/tooling/nargo_fmt/tests/expected/tuple.nr b/tooling/nargo_fmt/tests/expected/tuple.nr index d4b8b239815..29f32f83e55 100644 --- a/tooling/nargo_fmt/tests/expected/tuple.nr +++ b/tooling/nargo_fmt/tests/expected/tuple.nr @@ -4,13 +4,13 @@ fn main() { // hello 1, ); - ( /*hello*/ 1,); + (/*hello*/ 1,); (1/*hello*/,); (1,); - ( /*test*/ 1,); - ( /*a*/ 1/*b*/,); - ( /*a*/ 1/*b*/, /*c*/ 2/*d*/, /*c*/ 2/*d*/); - ( /*a*/ 1/*b*/, /*c*/ 2/*d*/, /*c*/ 2/*d*/, /*e*/ 3/*f*/); + (/*test*/ 1,); + (/*a*/ 1/*b*/,); + (/*a*/ 1/*b*/, /*c*/ 2/*d*/, /*c*/ 2/*d*/); + (/*a*/ 1/*b*/, /*c*/ 2/*d*/, /*c*/ 2/*d*/, /*e*/ 3/*f*/); (1 /*1*/, 2 /* 2*/); @@ -28,7 +28,7 @@ fn main() { 2, ); - ( /*1*/ 1, /*2*/ 2); + (/*1*/ 1, /*2*/ 2); ( ( @@ -39,14 +39,8 @@ fn main() { ), ); ( - /*a*/ - 1 - /*b*/, - /*c*/ - 2/*d*/, - /*c*/ - 2/*d*/, - /*e*/ - 3,/*f*/ + /*a*/ 1 + /*b*/, /*c*/ + 2/*d*/, /*c*/ 2/*d*/, /*e*/ 3,/*f*/ ); }