Formatting struct with trailing comment is not idempotent

[gkanwar]

I am working with with trailing_comma = "Never" set in a rustfmt.toml config, running the nightly rustfmt, and find the following idempotency issue. My understanding is that instead of the following behavior, rustfmt should fully format the code once and not change on future executions.

Starting with the code:

struct Foo {
    field1: usize,
    field2: usize,
    // TODO: maybe we add more fields below
}

Calling rustfmt the first time gives

struct Foo {
    field1: usize,
    field2: usize
    // TODO: maybe we add more fields below
}

Calling rustfmt the second time gives

struct Foo {
    field1: usize,
    field2: usize // TODO: maybe we add more fields below
}

All subsequent calls leave it in this final state.

My rustfmt version is rustfmt 1.4.37-nightly (8cf990c 2021-05-15)

[gkanwar]

I'm also not sure whether moving the comment to the line above is the intended formatting behavior. I would have expected standalone comments to be kept on their own line, but perhaps this is intentionally not the case for comments within structs?

[calebcartwright]

My understanding is that instead of the following behavior, rustfmt should fully format the code once and not change on future executions

You are correct. Idempotency is a core tenant of rustfmt and non-idempotent formatting behavior is indeed a bug.

For some reason I feel like this is a duplicate but will keep it open unless I find a previous report.

I would have expected standalone comments to be kept on their own line, but perhaps this is intentionally not the case for comments within structs?

tl;dr comments are hard 😄

rustfmt is an AST-driven/pretty-printer style formatter, where comments are not explicitly represented within the tree so rustfmt has to peek back inside the source to recover comments and determine where to place them within the reformatted version of code. However, it can often be ambiguous which node the developer intended the comment to be associated to.

In the case of a comment that follows an element in a list like this (fields within a struct) the absence of a comma between the end of the element and start of a comma has always been interpreted as a trailing comment which works well except in some cases where the trailing comment is after the last element, as in your examples.

That behavior has been reported previously (refs #4654) and probably isn't too terribly far away from resolution, although the associated PR stalled out.

[ytmimi]

Hey @calebcartwright I've been looking into this and found that the issue sort of has to do with whether you're using trailing_comment=Never vs trailing_comment=Always, but I think it mostly has to do with rustfmt preserving newlines whether the original source code already has a trailing comma or not.

Maybe that's obvious based on the example, but I think the point helps us better understand the issue so I'm mentioning it. From my understanding, the source of the bug can be tracked to line 615 in lists::extract_post_comment.

rustfmt/src/lists.rs

Lines 607 to 628 in 365a2f8

	pub(crate) fn extract_post_comment(
	post_snippet: &str,
	comment_end: usize,
	separator: &str,
	) -> Option<String> {
	let white_space: &[_] = &[' ', '\t'];
	// Cleanup post-comment: strip separators and whitespace.
	let post_snippet = post_snippet[..comment_end].trim();
	let post_snippet_trimmed = if post_snippet.starts_with(|c| c == ',' || c == ':') {
	post_snippet[1..].trim_matches(white_space)
	} else if let Some(stripped) = post_snippet.strip_prefix(separator) {
	stripped.trim_matches(white_space)
	}
	// not comment or over two lines
	else if post_snippet.ends_with(',')
	&& (!post_snippet.trim().starts_with("//") || post_snippet.trim().contains('\n'))
	{
	post_snippet[..(post_snippet.len() - 1)].trim_matches(white_space)
	} else {
	post_snippet
	};

Given the first struct:

struct Foo {
    field1: usize,
    field2: usize,
    // TODO: maybe we add more fields below
}

the post_snippet passed to lists::extract_post_comment is ,\n // TODO: maybe we add more fields below } and code execution hits line 617. rustfmt preserves the newline because the white_space array doesn't contain newlines and outputs:

struct Foo {
    field1: usize,
    field2: usize
    // TODO: maybe we add more fields below
}

Running rustfmt again with trailing_comma=Never results in a post_snippet of \n // TODO: maybe we add more fields below } (Note: no leading ,). When we trim the post_snippet on line 615 this time around we lose the newline and rustfmt ultimately outputs:

struct Foo {
    field1: usize,
    field2: usize // TODO: maybe we add more fields below
}

Right now I'd like some guidance on what the right behavior should be. Do we preserve newlines or not? I quickly tested a solution where newlines were preserved and one where newlines were always removed and it looks like either way tests are going to break. looks like #4655 also ran into issues of tests breaking when trying to fix this.

[ytmimi]

Going to close this in favor of #4654