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

Formatting does not work with match arm having mixed OR clauses and comments #6491

Open
snprajwal opened this issue Mar 4, 2025 · 2 comments
Assignees
Labels
a-comments a-matches match arms, patterns, blocks, etc p-low

Comments

@snprajwal
Copy link

snprajwal commented Mar 4, 2025

Related to #6060 and #6044. For the below snippet, rustfmt does nothing at all:

enum Foo {
    A,
    B,
    C,
    D,
}

fn main() {
    let foo = Foo::A;
    match foo {
        // Here is a comment
Foo::A |
    // And another one
        Foo::B |
        // Hey look! There's another one
    Foo::C |
// That's a lot of comments
        Foo::D => {}
    }
}

If someone more knowledgeable about rustfmt internals can please confirm that this is a solvable bug, I'd be happy to contribute the fix.

@ytmimi
Copy link
Contributor

ytmimi commented Mar 4, 2025

When you try formatting this with error_on_unformatted=true configured, you'll get the following error message:

error[internal]: not formatted because a comment would be lost
  --> <stdin>:10
   |
10 |     match foo {
   |
   = note: set `error_on_unformatted = false` to suppress the warning against comments or string literals

rustfmt isn't expecting to find comments within the OR pattern and would remove them if the code was formatted.

I'm not sure how simple this would be to solve. We usually use itemize_list to handle formatting lists of things internally, which takes care of rewriting comments as well.

It might be as simple as defining items (line 100) using itemize_list instead of mapping over the pat_strs, but I haven't looked into this.

rustfmt/src/patterns.rs

Lines 90 to 117 in c6c8159

PatKind::Or(ref pats) => {
let pat_strs = pats
.iter()
.map(|p| p.rewrite_result(context, shape))
.collect::<Result<Vec<_>, RewriteError>>()?;
let use_mixed_layout = pats
.iter()
.zip(pat_strs.iter())
.all(|(pat, pat_str)| is_short_pattern(pat, pat_str));
let items: Vec<_> = pat_strs.into_iter().map(ListItem::from_str).collect();
let tactic = if use_mixed_layout {
DefinitiveListTactic::Mixed
} else {
definitive_tactic(
&items,
ListTactic::HorizontalVertical,
Separator::VerticalBar,
shape.width,
)
};
let fmt = ListFormatting::new(shape, context.config)
.tactic(tactic)
.separator(" |")
.separator_place(context.config.binop_separator())
.ends_with_newline(false);
write_list(&items, &fmt)
}

It's also likely that any change here would need to be gated.

If you're interested in working on this one you can comment @rustbot claim to assign yourself.

@ytmimi ytmimi added a-comments a-matches match arms, patterns, blocks, etc p-low labels Mar 4, 2025
@snprajwal
Copy link
Author

@rustbot claim

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-comments a-matches match arms, patterns, blocks, etc p-low
Projects
None yet
Development

No branches or pull requests

2 participants