avoid inserting newline between opening curly brace and comment

[RalfJung]

This ports #4745 to the current master branch. That led to a bunch of test failures so I hacked around until they were all gone... but the codebase is entirely foreign to me so the result may not make any sense at all.^^

In particular, I have no idea what the contract of rewrite_comment is (and it doesn't have a doc comment that would clarify): is the result guaranteed to contain everything "relevant" from the input, so I can just advance last_pos to the end of the line, or do I have to worry about this only formatting a prefix of the input and last_pos needs to be advancd more carefully?

Fixes #3255

[RalfJung]

The original PR did self.last_pos + BytePos(first_line_snip.len() as u32) here. That led to a bunch of test failures where rustfmt inserts seemingly random characters into previously empty blocks. I think what happens is that if the trim() above removes characters from the start of the string, we'd advance last_pos by too little here, and so the cursor doesn't quite point to the end of the comment -- so the last characters of the comment get duplicated.

I'm not sure what the proper fix for that is. Skipping to the end of the line seemed to make most sense, since we should have processed the entire line.

[ytmimi]

@RalfJung Thanks for taking this one on. Overall the code changes seem straightforward, and I think your approach of advancing last_pos to the end of the line makes sense.

I left two minor comments about adding some extra test cases, and out of an abundance of caution I think we should gate this change on style_edition=2024.

[ytmimi]

Probably good to add a test case with a multi-line block comment that doesn't have bare lines. Something like this for example:

fn foo(){  
    if true {     /* Sample
                   * another line
                   * another line
                   * end
                   */
        1
    }
}

I noticed that the indentation of the first line was the only one that got updated. Maybe the other's should have as well since they're part of the same block comment? I imagine that might be tougher to get right though. This is the output that I'm currently getting when formatting with this branch:

fn foo() {
    if true { /* Sample
                   * another line
                   * another line
                   * end
                   */
        1
    }
}

[RalfJung]

Yeah, the entire approach is based on processing just the code from the { to the end of the line, so it doesn't take into account the other lines at all. I have no idea how to fix this.

The alternative originally implemented by #4745 was to not touch /*-style comments at all, but that does not seem better: the result looks nicer but is more likely to change the semantic meaning of the comment.

[ytmimi]

I spent some time looking at this one tonight. One issue I can see is that the shape used to format the first line comment is technically incorrect. It doesn't take into account the length of the conditional logic that already occupies the line, e.g if true {, so rewrite_comment thinks it has more room to work with than it actually does.

As a result, this interacts poorly with wrap_comments=true since it'll wraps at an incorrect location and reports a max_width error (assuming you've got error_on_unformatted=true` set). Also, after we wrap the comment, part of the comment will be aligned with the opening brace and the rest of it will be aligned with the statements (not necessarily a bad thing). here's an example input:

fn foo() {
    if true { // some long comment explaining what this condition is all about. when `wrap_comments=true` is used this will wrap.
        1
    }
}

This is the output I get when running this with wrap_comments=true:

fn foo() {
    if true { // some long comment explaining what this condition is all about. when `wrap_comments=true`
        // is used this will wrap.
        1
    }
}

and I'm also seeing this error:

error[internal]: line formatted, but exceeded maximum width (maximum: 100 (see `max_width` option), found: 105)
 --> <stdin>:2:2:101
  |
2 |     if true { // some long comment explaining what this condition is all about. when `wrap_comments=true`
  |                                                                                                     ^^^^^
  |
  = note: set `error_on_unformatted = false` to suppress the warning against comments or string literals

warning: rustfmt has failed to format. See previous 1 errors.

[ytmimi]

Here's one idea I had to try and handle block comments formatting. There are likely other issues with this code, but I also ran into the same issue that I described above. Since we don't know how much room if true { occupies on the first line we can't align things correctly, and the block comments end up getting aligned with the statements + 1 since it's trying to align the *. This might not be what we go with, but I thought I'd share what I'd looked into.

diff --git a/src/visitor.rs b/src/visitor.rs
index b57a49e5..62533d82 100644
--- a/src/visitor.rs
+++ b/src/visitor.rs
@@ -1,6 +1,7 @@
 use std::cell::{Cell, RefCell};
 use std::rc::Rc;
 
+use rustc_ast::HasAttrs;
 use rustc_ast::{ast, token::Delimiter, visit};
 use rustc_data_structures::sync::Lrc;
 use rustc_span::{symbol, BytePos, Pos, Span};
@@ -244,14 +245,42 @@ impl<'b, 'a: 'b> FmtVisitor<'a> {
                         .trim();
 
                     if contains_comment(first_line_snip) {
-                        if let Ok(comment) =
-                            rewrite_comment(first_line_snip, false, self.shape(), self.config)
-                        {
+                        // Now that we know the first line contains a comment, lets determine the
+                        // bounds of the comment(s).
+                        let comment_byte_pos_hi = match inner_attrs {
+                            Some(attrs) if !attrs.is_empty() => attrs.first().unwrap().span.hi(),
+                            _ => b
+                                .stmts
+                                .first()
+                                .map(|s| {
+                                    match s
+                                        .attrs()
+                                        .iter()
+                                        .filter(|a| a.style == ast::AttrStyle::Outer)
+                                        .next()
+                                    {
+                                        Some(attr) => attr.span.hi() - BytePos(1),
+                                        None => s.span.hi() - BytePos(1),
+                                    }
+                                })
+                                .unwrap_or_else(|| self.snippet_provider.span_before(b.span, "}")),
+                        };
+                        let comment_snippet = self.snippet(mk_sp(self.last_pos, comment_byte_pos_hi)).trim();
+
+                        // FIXME(ytmimi) it would be nice to update the Shape's width
+                        // when formatting these first line comments, especially block comments
+                        // since those comment lines are logically grouped, but the current
+                        // FmtVisitor doesn't have any context about how much room is already
+                        // occupied on the line we're adding the comment to
+                        if let Ok(comment) = rewrite_comment(
+                            comment_snippet,
+                            false,
+                            self.shape(),
+                            self.config,
+                        ) {
                             self.push_str(" ");
                             self.push_str(&comment);
-                            // This line has no statements, so we can jump to the end of it
-                            // (but *before* the newline).
-                            self.last_pos = first_line_bounds.end - BytePos::from_usize(1);
+                            self.last_pos = comment_byte_pos_hi;
                         }
                     }
                 }

[RalfJung]

SO there's no way to adjust the shape that we pass to rewrite_comment to tell it how much space it actually has available?

How does it usually know how much space it has (e.g. to account for indentation)? I would have thought it looks at the cursor position in the current line in the output, which should always be reliable, but that doesn't seem to be the case.