Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rustfmt deletes comments in "or" patterns #6044

Open
WaffleLapkin opened this issue Jan 26, 2024 · 8 comments · May be fixed by #6125
Open

rustfmt deletes comments in "or" patterns #6044

WaffleLapkin opened this issue Jan 26, 2024 · 8 comments · May be fixed by #6125
Labels
a-comments bug Panic, non-idempotency, invalid code, etc.

Comments

@WaffleLapkin
Copy link
Member

This:

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

gets turned into this:

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

(play)

cc @fee1-dead

@ytmimi
Copy link
Contributor

ytmimi commented Jan 26, 2024

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.

rustfmt/src/patterns.rs

Lines 274 to 276 in cedb7b5

PatKind::Paren(ref pat) => pat
.rewrite(context, shape.offset_left(1)?.sub_width(1)?)
.map(|inner_pat| format!("({})", inner_pat)),

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.

rustfmt/src/patterns.rs

Lines 78 to 81 in cedb7b5

let pat_strs = pats
.iter()
.map(|p| p.rewrite(context, shape))
.collect::<Option<Vec<_>>>()?;

@ytmimi ytmimi added a-comments bug Panic, non-idempotency, invalid code, etc. labels Jan 26, 2024
@WaffleLapkin
Copy link
Member Author

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?)

@ytmimi
Copy link
Contributor

ytmimi commented Jan 27, 2024

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).

@fee1-dead
Copy link
Member

fee1-dead commented Jan 27, 2024

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 => { () }
}
}

@ytmimi
Copy link
Contributor

ytmimi commented Jan 27, 2024

@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.

@yuvraj-wale
Copy link

yuvraj-wale commented Mar 8, 2024

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!

@WaffleLapkin
Copy link
Member Author

WaffleLapkin commented Mar 8, 2024

[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...

@yuvraj-wale
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-comments bug Panic, non-idempotency, invalid code, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants