Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: let formatter respect newlines between comments #6458

Merged
merged 1 commit into from
Nov 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions tooling/nargo_fmt/src/chunks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
4 changes: 3 additions & 1 deletion tooling/nargo_fmt/src/formatter/buffer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,16 +38,18 @@ 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;
}
}

/// Trim commas from the end of the buffer. Returns true if a comma was trimmed.
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
Expand Down
22 changes: 20 additions & 2 deletions tooling/nargo_fmt/src/formatter/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1133,11 +1133,15 @@
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();
}
Expand All @@ -1146,6 +1150,20 @@
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();
}));
Expand Down Expand Up @@ -1771,9 +1789,9 @@

#[test]
fn format_method_call_chain_3() {
let src = "fn foo() { assert(p4_affine.eq(Gaffine::new(6890855772600357754907169075114257697580319025794532037257385534741338397365, 4338620300185947561074059802482547481416142213883829469920100239455078257889))); }";

Check warning on line 1792 in tooling/nargo_fmt/src/formatter/expression.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (Gaffine)
let expected = "fn foo() {
assert(p4_affine.eq(Gaffine::new(

Check warning on line 1794 in tooling/nargo_fmt/src/formatter/expression.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (Gaffine)
6890855772600357754907169075114257697580319025794532037257385534741338397365,
4338620300185947561074059802482547481416142213883829469920100239455078257889,
)));
Expand Down Expand Up @@ -1809,7 +1827,7 @@
fn format_nested_method_call_with_maximum_width_2() {
let src = "fn foo() {
assert(
p4_affine.eq(Gaffine::new(

Check warning on line 1830 in tooling/nargo_fmt/src/formatter/expression.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (Gaffine)
6890855772600357754907169075114257697580319025794532037257385534741338397365,
4338620300185947561074059802482547481416142213883829469920100239455078257889,
)),
Expand All @@ -1817,7 +1835,7 @@
}
";
let expected = "fn foo() {
assert(p4_affine.eq(Gaffine::new(

Check warning on line 1838 in tooling/nargo_fmt/src/formatter/expression.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (Gaffine)
6890855772600357754907169075114257697580319025794532037257385534741338397365,
4338620300185947561074059802482547481416142213883829469920100239455078257889,
)));
Expand Down
58 changes: 48 additions & 10 deletions tooling/nargo_fmt/src/formatter/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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);
}
}
22 changes: 21 additions & 1 deletion tooling/nargo_fmt/src/formatter/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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());
}
}
1 change: 1 addition & 0 deletions tooling/nargo_fmt/tests/expected/parens.nr
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ fn main(x: u64, y: pub u64) {

(
/*a*/

(
// test
1
Expand Down
Loading