Fix formatting of comments in empty structs

[Szymongib]

Currently, comments in empty structs are formatted weirdly. This PR attempts to fix it by indenting all one-line comments to the same one-depth indentation.

Examples can be found in the issue and tests.

Fixes: #4854

Reopen of #4875 due to branch changes (apologies for inconveniences).

[Szymongib]

@ytmimi I have made the changes you suggested in this PR, sorry for the inconvenience I did not realize that branches changed.

[ytmimi]

No inconvenience at all! Thank you very much for the work you put into your original solution, making these suggested changes, and for updating all those tests!

Everything looks good to me and I'm happy to move forward.

[calebcartwright]

Thanks for the quick turn around! Would you mind adding a few more tests?

• nested definitions (e.g. define the struct within another item like a mod)
• single line block-style comment (e.g. struct Foo { /* bar */ }) (and compare to current behavior
• coverage for some of the other snippets shared in associated/duplicate issues
  • Comments after the first one in empty struct are not formatted nicely #4854 (comment)
  • Multiline comment inside struct get unidented  #4976

[Szymongib]

Ah sorry @calebcartwright I thought I had the original commit with tests but probably lost it when rebasing and changing brnach. Added them now.

So formating of block comments changed from:

struct Foo { 
    /* bar */
}

to

struct Foo {/* bar */}

Let me know if that is ok.

[ytmimi]

@Szymongib I believe that's the correct behavior! However, I do think there's an issue with some of the examples that include multi line block comments. Here are the examples you have:

struct Struct5 {
    /*
    Comment block
    with many lines.
    */}

struct Struct6(
    /*
    Comment block
    with many lines.
    */);

struct Struct7 {
    /*
    Invalid
    format
    */}

struct Struct8(
    /*
    Invalid
    format
    */);

struct Struct10 {
    /* bar
    baz
    */}

And here's what I think the issue is; On line 1380 we check if we need to push an indented newline before adding the content of the string, however on line 1387 before deciding to add a newline we do a similar but slightly different check.

rustfmt/src/items.rs

Lines 1380 to 1390 in 5056f4c

	if !is_single_line(s) || first_line_contains_single_line_comment(s) {
	let nested_indent_str = offset
	.block_indent(context.config)
	.to_string_with_newline(context.config);
	result.push_str(&nested_indent_str);
	}
	result.push_str(s);
	if last_line_contains_single_line_comment(s) {
	result.push_str(&offset.to_string_with_newline(context.config));
	}
	}

I think if we updated Line 1387 to include the !is_single_line (multi-line) check it would fix the issue that we're seeing and move the closing bracket to the right spot. I had something like this in mind:

+      let is_multi_line = !is_single_line(s);
+      if is_multi_line || first_line_contains_single_line_comment(s) {
-      if !is_single_line(s) || first_line_contains_single_line_comment(s) 
          let nested_indent_str = offset 
              .block_indent(context.config) 
              .to_string_with_newline(context.config); 
          result.push_str(&nested_indent_str); 
      } 
      result.push_str(s);
+      if is_multi_line || last_line_contains_single_line_comment(s)
-      if last_line_contains_single_line_comment(s) { 
          result.push_str(&offset.to_string_with_newline(context.config)); 
      } 
  } 

[calebcartwright]

Let me know if that is ok.

We actually cannot make such a "breaking" formatting change, at least not without version gating it. We should try to find a way of preserving this type of formatting (comment on next line), but if not it will have to be gated. We've got some other code (or at least PRs) with examples of maintaining that type of comment behavior that I'm sure we could dig up as reference info

[ytmimi]

@calebcartwright correct me if my understanding is off, but your saying that we can't format single line block comments like this?

struct Foo {                            struct Foo {/* bar */}
    /* bar */        ---------->
}

I just checked and on the current master rustfmt 1.4.38-nightly (5056f4cf 2022-01-05) that's how things are formatted, so this PR doesn't introduce that change.

Is my comment about the closing brace also something that would be considered breaking?

[Szymongib]

Thanks @ytmimi.

Indeed master version does format single-line comments like that, so this behavior does not change.

struct Foo {/* bar */}

[calebcartwright]

My apologies as it seems I misunderstood, thank you both! So long as the bug fix doesn't introduce any idempotency issues for existing "valid" (see, non-buggy indentation) formatting it's fine

[calebcartwright]

One minor question inline re: existing tests but otherwise LGTM and ready to merge

Thanks so much for sticking with this one @Szymongib, it will be a solid first contribution!

[calebcartwright]

Was it necessary to modify these files? It's typically a flag when we have to change existing inputs to tests, but I'm guessing this isn't actually necessary, perhaps wasn't even explicitly intentional?

[Szymongib]

Ah sorry, I just thought we might want to remove the // This can be changed once https://github.com/rust-lang/rustfmt/issues/4854 is fixed part of the comment as the issue should be addressed with the PR. It makes sense to leave it as it is tho, so I will revert that change.

[ytmimi]

@calebcartwright are you trying to highlight that this file is the source file and that we don't need to update the formatting since it'll get checked against the corresponding target? Would we not want to remove the comment about issue 4854 to not confuse anyone down the line about whether or not the issue is fixed?

[calebcartwright]

I was really just speaking to changing of the indentation, I actually hadn't read the the comment text word for word. I'm okay with removing that first line though would prefer to defer that to a follow up PR if anyone is interested given how long this PR (and it's predecessor) have been basically ready, but would still be good in that potential follow up to leave the source contents with incorrect indentation

[calebcartwright]

Thanks again!