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

Fix failure with line comment following .. in a struct pattern #6117

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mousany
Copy link

@mousany mousany commented Mar 14, 2024

Fixes #6040

When a struct pattern that contained a .. was formatted, it failed to consider the case when there is line comment following it. Instead it produced a formatting error because it realizes that if it went ahead with the rewirte it would remove the line comment.

Now, the following line comment is taken into consideration by adopting the approach to rewrite_struct_lit, which is a similar function, for rewriting structs, but doesn't seem to suffer from this trailing comment problem because rewrite_struct_lit considers the .. as part of the struct fields that it needs to rewrite.

@mousany
Copy link
Author

mousany commented Mar 14, 2024

A little side note here. While making this patch, I noticed some counter-intuitive details in the old implementation. To be specific, the old one uses the fields of the struct without the .. to determine the tactic for formatting the struct pattern, and tries to merge the .. to the back. This somehow merges the vertical tactic into a mixed tactic, or the following if my understanding of the mixed tactic is incorrect.

Foo {
    a, ..
}

The following are two examples from previous issues. Currently the struct_lit_tactic will never produce a mixed tactic:

  1. In Formatting of match arms with long guards is a bit awkward.  #3005 , the following is produced because when ignoring the .., the fields is formatted in a horizontal tactic, then the .. is appended after. Eventually after some wrapping and other stuff, a mixed tactic is created. However, if taking the .. into account, the total length of the struct pattern will exceed the limit of 18 and the struct_lit_tactic will produce a vertical tactic.
match *token {
    Token::Dimension {
        value, ref unit, ..
    } if num_context.is_ok(context.parsing_mode, value) => {
        return NoCalcLength::parse_dimension(context, value, unit)
            .map(LengthOrPercentage::Length)
            .map_err(|()| location.new_unexpected_token_error(token.clone()));
    }
}
  1. In Duplicate comma with trailing_comma = "Always" in struct pattern #5066 , the following is produced though the tactic generated by only considering a is vertical, but a mixed tactic is created because .. is merged after.
let Foo {
    a, ..
} = b;

And if the merge fails due to constraints like struct_lit_width, it falls back to

let Foo {
    a, 
    ..
} = b;

I believe there should be a more specific way to determine when to use the mixed tactic, rather than the confusing way of determining the tactic without the .. and trying to merge it after. In my patch, I failed to find any specifications on this thus I just recreated the old behavior.

@ytmimi
Copy link
Contributor

ytmimi commented Mar 21, 2024

@mousany I haven't had a chance to review these changes yet, but I wanted to let you know that we strongly discourage merge commits. Please rebase on the latest master when you have a moment.

@mousany
Copy link
Author

mousany commented Mar 21, 2024

@ytmimi I've rebased to the latest master now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rustfmt stops formatting after line comment following .. in a struct pattern
3 participants