Turn closure bodies from exprs into blocks if there are comments

[Solumin]

A closure like this:

let x = ||
    // a comment
    some_body_expr;

should be a block instead of a single expression, as per the fmt-rfcs.

Closes #4384

[Solumin]

My apologies for forgetting to squash commits before uploading to master, it's been a while.

[ayazhafiz]

I think this is really nice, the only thing is that it would not fix the case where the comment is between before a body that is already a block (e.g. || /* foo */ { 1 }). Maybe that can be considered a different issue.

[Solumin]

I thought of a really annoying edge case:

let one_liner = || /* ? */ 1;

This PR will rewrite it as:

let one_liner = || { /* ? */
    1
};

I'm pretty sure this is because rewrite_closure_with_block (which calls rewrite_block_with_visitor) turns it into:

let one_liner = || {
    1
};

and combine_strs_with_missing_comments will try to put all the strings on the same line due to there being no newlines in the original source:

rustfmt/src/formatting/comment.rs

Lines 204 to 212 in f7ba2db

	// Peek the the original source code and find out whether there is a newline between the first
	// expression and the second expression or the missing comment. We will preserve the original
	// layout whenever possible.
	let original_snippet = context.snippet(span);
	let prefer_same_line = if let Some(pos) = original_snippet.find('/') {
	!original_snippet[..pos].contains('\n')
	} else {
	!original_snippet.contains('\n')
	};

An edge case, to be sure, but this whole PR is addressing edge cases, so I'll see what I can figure out.

[Solumin]

@ayazhafiz Personally I'd consider that a separate issue, but I think the fix is quite similar to what I've added in this PR. So I'm going to pass on fixing that for now.

[Solumin]

I have encountered something quite fun.

I added this test case:

let one_line = || /* one line */ 1;

with expected output:

let one_line = || { /* one line */
    1
};

Because, as I mentioned above, combine_strs_with_missing_comments tries to put the comment on the same line as it is in the original source.

But this fails because rustfmt thinks that the output should be:

let one_line = || {
    /* one line */
    1
};

That's great, that's what I'd prefer to see anyway! So let's change the test case to use that output--

Mismatch at tests/source\issue-4384-2.rs:1:
 fn main() {
-    let one_line = || {
-        /* one line */
+    let one_line = || { /* one line */
         1
     };
 }

Um. rustfmt is somehow generating both versions?

[calebcartwright]

But this fails because rustfmt thinks that the output should be:

let one_line = || {
    /* one line */
    1
};

So let's change the test case to use that output--

Mismatch at tests/source\issue-4384-2.rs:1:
 fn main() {
-    let one_line = || {
-        /* one line */
+    let one_line = || { /* one line */
         1
     };
 }

Um. rustfmt is somehow generating both versions?

I'm not sure I follow what you're saying here, and I'm unable to reproduce this locally with the latest changes on your branch.

The preservation of original line placement with combine_strs_with_missing_comments is intentional, so that may not be a great approach in this case. As @ayazhafiz alluded to on the other thread, utilizing different comment util function like rewrite_missing_comment or recover_missing_comment_in_span may be a better fit for option 2.

I also wouldn't be too concerned about a minor refactor to add a new param to rewrite_closure_with_block to accept the target span as mentioned in your option 1. It's a private/internal function that's only called from a handful of places, and the param could even be Option wrapped with rewrite_closure_with_block using the body span in the none case.

[Solumin]

Thanks @ayazhafiz and @calebcartwright, the finer-grained control from using rewrite_missing_comment gives a better result.

I'm happy with the state of this now, it covers every case I could think of.

[calebcartwright]

Thanks @Solumin! Could you rebase/address the conflicts? Will take a final look over afterwards

[Solumin]

I don't see any conflicts now. Or at least GitHub says it can be merged.

[calebcartwright]

I can squash the commits but there's conflicts preventing a rebase. No worries, will look at it later

[Solumin]

I got it, just had to look up the right git incantation. Sorry!

[calebcartwright]

Thanks @Solumin! We should use the ? operator instead of unwrapping here to let rustfmt's failed formatting logic apply instead of panicking, but should be good to merge afterwards

[calebcartwright]

Perfect, thank you!