fix: better formatting of leading/trailing line/block comments in expression lists

tooling/nargo_fmt/src/chunks.rs

@@ -827,10 +827,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(),
@@ -893,9 +893,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

@@ -433,23 +433,22 @@
     {
         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 {
@@ -466,7 +465,24 @@
                 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);
@@ -1313,6 +1329,56 @@
         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 ;";
@@ -1671,9 +1737,9 @@
 
     #[test]
     fn format_method_call_chain_3() {
         let src = "fn foo() {     assert(p4_affine.eq(Gaffine::new(6890855772600357754907169075114257697580319025794532037257385534741338397365, 4338620300185947561074059802482547481416142213883829469920100239455078257889)));  }";
         let expected = "fn foo() {
     assert(p4_affine.eq(Gaffine::new(
         6890855772600357754907169075114257697580319025794532037257385534741338397365,
         4338620300185947561074059802482547481416142213883829469920100239455078257889,
     )));
@@ -1709,7 +1775,7 @@
     fn format_nested_method_call_with_maximum_width_2() {
         let src = "fn foo() {
     assert(
         p4_affine.eq(Gaffine::new(
             6890855772600357754907169075114257697580319025794532037257385534741338397365,
             4338620300185947561074059802482547481416142213883829469920100239455078257889,
         )),
@@ -1717,7 +1783,7 @@
 }
 ";
         let expected = "fn foo() {
     assert(p4_affine.eq(Gaffine::new(
         6890855772600357754907169075114257697580319025794532037257385534741338397365,
         4338620300185947561074059802482547481416142213883829469920100239455078257889,
     )));

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