fix: better formatting of leading/trailing line/block comments in exp…

…ression lists (#6338)

# Description

## Problem

While fixing the bug [this
PR](#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

@@ -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

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);

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 ;";

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);
     }
 

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);
     }
 

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*/
     );
 }