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

Duplicate comma with trailing_comma = "Always" in struct pattern #5066

Closed
dnbln opened this issue Nov 5, 2021 · 3 comments · Fixed by #5090
Closed

Duplicate comma with trailing_comma = "Always" in struct pattern #5066

dnbln opened this issue Nov 5, 2021 · 3 comments · Fixed by #5090
Labels
bug Panic, non-idempotency, invalid code, etc. only-with-option requires a non-default option value to reproduce

Comments

@dnbln
Copy link
Contributor

dnbln commented Nov 5, 2021

// src/main.rs
fn main() {
    let Foo { a, .. } = b;
}
# rustfmt.toml
edition = "2021"

unstable_features = true
trailing_comma = "Always"

Reformat with rustfmt --config-path rustfmt.toml src/main.rs

// src/main.rs
fn main() {
    let Foo { a,, .. } = b;
}

rustfmt --version: rustfmt 1.4.38-nightly (4961b10 2021-11-04)

@calebcartwright
Copy link
Member

Thanks for the report.

@ytmimi could you take a look at this one? Issue should be in/around some of the same struct-related code you were looking at not too long ago

@calebcartwright calebcartwright added only-with-option requires a non-default option value to reproduce bug Panic, non-idempotency, invalid code, etc. labels Nov 5, 2021
@ytmimi
Copy link
Contributor

ytmimi commented Nov 8, 2021

I'd be happy to take a look! I'll let you know what I can find.

@ytmimi
Copy link
Contributor

ytmimi commented Nov 17, 2021

@calebcartwright I took a look at this, and the reason that we're adding the comma twice is because when we try to rewrite the struct pattern in patterns::rewrite_struct_pat, there's an assumption that a trailing comma is always needed when the tactic is not DefinitiveListTactic::Vertical.

rustfmt/src/patterns.rs

Lines 330 to 339 in eee8f04

if !fields_str.is_empty() {
// there are preceding struct fields being matched on
if tactic == DefinitiveListTactic::Vertical {
// if the tactic is Vertical, write_list already added a trailing ,
fields_str.push(' ');
} else {
fields_str.push_str(", ");
}
}
}

In this particular case the tactic is actually DefinitiveListTactic::Horizontal so we end up appending ", " to the end of the fields_str.

would it make more sense to check if !fmt.needs_trailing_separator() instead of !tactic == DefinitiveListTactic::Horizontal to decide whether or not we append
" " or ", "?

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

Successfully merging a pull request may close this issue.

3 participants