Thanks for the report.

I just took a quick look at this. rustfmt isn't expecting to find comments between the opening paren and the start of the pattern. Also, rustfmt isn't looking for comments between the end of the pattern and the closing paren.

I think we could recover the comments within the Or pattern by using itemize_list here, which is how we typically recover comments when rewriting lists of items.

With how much there are comment-deletion-related bugs I do wander, would it make sense to change the parser to output nodes for comments? (or am I misunderstanding the root cause?)

I don't know how much it would complicate the parser to have it also output comment spans. We've had some recent conversations about it in our team meetings, and I think where we left off was that comments aren't rustfmts biggest issue right now. If there's a separate effort to modify the parser in a way that would benefit rustfmt that would be great, but I don't think we'd make those changes ourselves (at least not in the immediate future).

A related issue occurs when a line comment is placed on an or pattern, which causes the entire match statement to be left unformatted. (play)

fn main() {
    match x {
  Foo::
  Bar => { () },
  Foo::
  A
  // FIXME lol
  | Foo::
  B
  | Foo::
  C => { () }
}
}

@fee1-dead Right! That's because we have code on the format_expr code path that checks to see if reformatting the AST node would result in removing comments, and if so rustfmt won't rewrite the expr. I'm pretty sure that same check doesn't apply when rewriting the pat in the ast::Local node that represents let <pat>:<ty> = <expr>;.

We could probably craft an example combining these two observation where rustfmt would fail to rewrite the expression because of potential comment loss, and therefore fail to rewrite the let binding, and not remove any comments. I think I agree with these being related they just result in different outcomes because of the additional logic in format_expr.

Hi @ytmimi,

I've been looking into this issue for a while now, and I'm trying to understand what exactly we're trying to fix. From my observations, it seems like everything is working as intended. For example, when we format this:

fn main() {
    let (/**/ () /**/ | /**/ () /**/) = ();
}

we get:

fn main() {
    let (() | ()) = ();
}

The comments are removed because they're empty, which I believe is a design choice. However, if we fill in any of these empty comments, like this:

fn main() {
    let (/**/ () /**/ | /**/ () /*non-empty*/) = ();
}

the comments are preserved, and we get a warning:

error[internal]: not formatted because a comment would be lost

Similarly, when we try to format this:

fn main() {
    match x {
        Foo::Bar => { () },
        Foo::A
        // FIXME lol
        | Foo::B
        | Foo::C => { () }
    }
}

the entire match statement is left unformatted with the same warning. However, if we use an empty comment, the match statements do get formatted:

fn main() {
    match x {
        Foo::Bar => { () },
        Foo::A
        /**/
        | Foo::B
        | Foo::C => { () }
    }
}

So, I'm not entirely sure what the issue is that we're trying to fix. Could someone help me understand the following questions:

1. Are we looking to preserve empty comments like /**/ or //, and it's not actually a design choice to delete empty comments?
2. Are we looking for an alternative to the warning check (error_on_unformatted, I believe) and want to format everything else except the specific comment line, possibly modifying format_expr to handle comments better? (I might be completely off here, just trying to understand the scope of the issue 😅)

Or is there something else we're aiming for in the bigger picture? Apologies if I'm missing any context, I'm relatively new here. Thanks for the help!

[not a rustfmt representative, but] I believe the answers are

Are we looking to preserve empty comments like /**/ or //, and it's not actually a design choice to delete empty comments?

Yes, rustfmt is not supposed to delete any comments. (rustfmt should only move them to be aligned with other formatted code, I think)

Here is an example of rustfmt handling comments correctly (play, you can use tools->rustfmt):

fn f(

      // jcn;ejh;he
x: u32

) {}

fn f(
    // jcn;ejh;he
    x: u32,
) {
}

Are we looking for an alternative to the warning check (error_on_unformatted, I believe) and want to format everything else except the specific comment line, possibly modifying format_expr to handle comments better? (I might be completely off here, just trying to understand the scope of the issue 😅)

I'm not sure why you are calling it a warning, when it's an error (are you perhaps looking for word "diagnostic"?).

Either way error[internal] from my understanding is basically a workaround to make rustfmt bugs not just delete stuff or silently ignore stuff, i.e. ideally it should never be seen, it's an indication of a bug.

What we want is to get rustfmt to format everything correctly. Correctly is hard to define, but generally: the same formatting as without comments, but with comments. For example (the first match is not formatted currently, this example shows an ideal situation):

fn main() {
    match x {
        Foo::Bar => (),
        MyAwesomeEnumAhahaha::Aaaaaaaaa
        // FIXME lol
        | MyAwesomeEnumAhahaha::Bbbbbbbbbbb

        | MyAwesomeEnumAhahaha::Cccccccccc => (),
    };
    
    match x {
        Foo::Bar => (),
        MyAwesomeEnumAhahaha::Aaaaaaaaa
        | MyAwesomeEnumAhahaha::Bbbbbbbbbbb

        | MyAwesomeEnumAhahaha::Cccccccccc => (),
    };
}

fn main() {
    match x {
        Foo::Bar => (),
        MyAwesomeEnumAhahaha::Aaaaaaaaa
        // FIXME lol
        | MyAwesomeEnumAhahaha::Bbbbbbbbbbb
        | MyAwesomeEnumAhahaha::Cccccccccc => (),
    };

    match x {
        Foo::Bar => (),
        MyAwesomeEnumAhahaha::Aaaaaaaaa
        | MyAwesomeEnumAhahaha::Bbbbbbbbbbb
        | MyAwesomeEnumAhahaha::Cccccccccc => (),
    };
}

Note that without the comment the or pattern gets squished to a single line (play):

fn main() {
    match x {
        Foo::Bar => { () },
        Foo::A
        | Foo::B
        | Foo::C => { () }
    }
}

fn main() {
    match x {
        Foo::Bar => (),
        Foo::A | Foo::B | Foo::C => (),
    }
}

You can't do the same with a // comment in between things, but can with a /**/ one. So it's a bit tricky...

Hey that makes a lot of sense. Thanks for the clarifications, really appreciate it!