From fb1a8ca67c58d87991358078e6c532b49824fdb8 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 5 Nov 2024 18:44:31 -0300 Subject: [PATCH] fix: let formatter respect newlines between comments (#6458) --- tooling/nargo_fmt/src/chunks.rs | 11 +++- tooling/nargo_fmt/src/formatter/buffer.rs | 4 +- tooling/nargo_fmt/src/formatter/expression.rs | 22 ++++++- tooling/nargo_fmt/src/formatter/function.rs | 58 +++++++++++++++---- tooling/nargo_fmt/src/formatter/statement.rs | 22 ++++++- tooling/nargo_fmt/tests/expected/parens.nr | 1 + 6 files changed, 102 insertions(+), 16 deletions(-) diff --git a/tooling/nargo_fmt/src/chunks.rs b/tooling/nargo_fmt/src/chunks.rs index 0e55dfad3b7..fcef261284d 100644 --- a/tooling/nargo_fmt/src/chunks.rs +++ b/tooling/nargo_fmt/src/chunks.rs @@ -915,11 +915,18 @@ impl<'a> Formatter<'a> { self.write_indentation(); } Chunk::LeadingComment(text_chunk) => { - let ends_with_newline = text_chunk.string.ends_with('\n'); + let ends_with_multiple_newlines = text_chunk.string.ends_with("\n\n"); + let ends_with_newline = + ends_with_multiple_newlines || text_chunk.string.ends_with('\n'); self.write_chunk_lines(text_chunk.string.trim()); // Respect whether the leading comment had a newline before what comes next or not - if ends_with_newline { + if ends_with_multiple_newlines { + // Remove any indentation that could exist (we'll add it later) + self.buffer.trim_spaces(); + self.write_multiple_lines_without_skipping_whitespace_and_comments(); + self.write_indentation(); + } else if ends_with_newline { self.write_line_without_skipping_whitespace_and_comments(); self.write_indentation(); } else { diff --git a/tooling/nargo_fmt/src/formatter/buffer.rs b/tooling/nargo_fmt/src/formatter/buffer.rs index e4740311bf6..3e4bebef608 100644 --- a/tooling/nargo_fmt/src/formatter/buffer.rs +++ b/tooling/nargo_fmt/src/formatter/buffer.rs @@ -38,9 +38,10 @@ impl Buffer { } /// Trim spaces from the end of the buffer. - pub(super) fn trim_spaces(&mut self) { + pub(crate) fn trim_spaces(&mut self) { while self.buffer.ends_with(' ') { self.buffer.truncate(self.buffer.len() - 1); + self.current_line_width -= 1; } } @@ -48,6 +49,7 @@ impl Buffer { pub(super) fn trim_comma(&mut self) -> bool { if self.buffer.ends_with(',') { self.buffer.truncate(self.buffer.len() - 1); + self.current_line_width -= 1; true } else { false diff --git a/tooling/nargo_fmt/src/formatter/expression.rs b/tooling/nargo_fmt/src/formatter/expression.rs index bff1799a298..0ac4c98bb95 100644 --- a/tooling/nargo_fmt/src/formatter/expression.rs +++ b/tooling/nargo_fmt/src/formatter/expression.rs @@ -1133,11 +1133,15 @@ impl<'a, 'b> ChunkFormatter<'a, 'b> { if count > 0 { // If newlines follow, we first add a line, then add the comment chunk group.lines(count > 1); - group.leading_comment(self.skip_comments_and_whitespace_chunk()); + group.leading_comment(self.chunk(|formatter| { + formatter.skip_comments_and_whitespace_writing_multiple_lines_if_found(); + })); ignore_next = self.ignore_next; } else { // Otherwise, add the comment first as it's a trailing comment - group.trailing_comment(self.skip_comments_and_whitespace_chunk()); + group.trailing_comment(self.chunk(|formatter| { + formatter.skip_comments_and_whitespace_writing_multiple_lines_if_found(); + })); ignore_next = self.ignore_next; group.line(); } @@ -1146,6 +1150,20 @@ impl<'a, 'b> ChunkFormatter<'a, 'b> { self.format_statement(statement, group, ignore_next); } + // See how many newlines follow the last statement + let count = self.following_newlines_count(); + + group.text(self.chunk(|formatter| { + formatter.skip_whitespace(); + })); + + // After skipping whitespace we check if there's a comment. If so, we respect + // how many lines were before that comment. + if count > 0 && matches!(self.token, Token::LineComment(..) | Token::BlockComment(..)) { + group.lines(count > 1); + } + + // Finally format the comment, if any group.text(self.chunk(|formatter| { formatter.skip_comments_and_whitespace(); })); diff --git a/tooling/nargo_fmt/src/formatter/function.rs b/tooling/nargo_fmt/src/formatter/function.rs index 154b81a2cb3..fd6977df613 100644 --- a/tooling/nargo_fmt/src/formatter/function.rs +++ b/tooling/nargo_fmt/src/formatter/function.rs @@ -280,16 +280,12 @@ impl<'a> Formatter<'a> { } pub(super) fn format_function_body(&mut self, body: BlockExpression) { - if body.is_empty() { - self.format_empty_block_contents(); - } else { - let mut group = ChunkGroup::new(); - self.chunk_formatter().format_non_empty_block_expression_contents( - body, true, // force multiple lines - &mut group, - ); - self.format_chunk_group(group); - } + let mut group = ChunkGroup::new(); + self.chunk_formatter().format_block_expression_contents( + body, true, // force multiple newlines + &mut group, + ); + self.format_chunk_group(group); } } @@ -537,4 +533,46 @@ fn baz() { let z = 3 ; "; assert_format(src, expected); } + + #[test] + fn comment_in_body_respects_newlines() { + let src = "fn foo() { + let x = 1; + + // comment + + let y = 2; +} +"; + let expected = src; + assert_format(src, expected); + } + + #[test] + fn final_comment_in_body_respects_newlines() { + let src = "fn foo() { + let x = 1; + + let y = 2; + + // comment +} +"; + let expected = src; + assert_format(src, expected); + } + + #[test] + fn initial_comment_in_body_respects_newlines() { + let src = "fn foo() { + // comment + + let x = 1; + + let y = 2; +} +"; + let expected = src; + assert_format(src, expected); + } } diff --git a/tooling/nargo_fmt/src/formatter/statement.rs b/tooling/nargo_fmt/src/formatter/statement.rs index 4d16c6819d0..50c286ff161 100644 --- a/tooling/nargo_fmt/src/formatter/statement.rs +++ b/tooling/nargo_fmt/src/formatter/statement.rs @@ -16,7 +16,15 @@ impl<'a, 'b> ChunkFormatter<'a, 'b> { group: &mut ChunkGroup, mut ignore_next: bool, ) { - group.leading_comment(self.skip_comments_and_whitespace_chunk()); + // First skip any whitespace to avoid writing multiple lines + group.text(self.chunk(|formatter| { + formatter.skip_whitespace(); + })); + + // Now write any leading comment respecting multiple newlines after them + group.leading_comment(self.chunk(|formatter| { + formatter.skip_comments_and_whitespace_writing_multiple_lines_if_found(); + })); ignore_next |= self.ignore_next; @@ -690,4 +698,16 @@ mod tests { "; assert_format_with_max_width(src, expected, " a_long_variable = foo(1, 2);".len() - 1); } + + #[test] + fn long_let_preceded_by_two_newlines() { + let src = "fn foo() { + let y = 0; + + let x = 123456; +} +"; + let expected = src; + assert_format_with_max_width(src, expected, " let x = 123456;".len()); + } } diff --git a/tooling/nargo_fmt/tests/expected/parens.nr b/tooling/nargo_fmt/tests/expected/parens.nr index 5ff0acf7330..2eaf3838ed6 100644 --- a/tooling/nargo_fmt/tests/expected/parens.nr +++ b/tooling/nargo_fmt/tests/expected/parens.nr @@ -25,6 +25,7 @@ fn main(x: u64, y: pub u64) { ( /*a*/ + ( // test 1