handle field attributes when aligning a struct's fields

[scampi]

the items::rewrite_struct_field method was already handling multiline
field names via last_line_width, which was rendered moot by the
previous logic in
vertical::AlignedItem::struct_field_prefix_max_min_width.

Close #2869

[scampi]

@topecongiro Thanks for the comments ;o)
In my point of view I don't think we should try to make this option "smarter" by taking into consideration comments or other things. I believe we should keep the option simple and have its effect obvious: it aligns a struct's fields types, regardless or attributes/comment/... .

[scampi]

Following previous comment, SomeType at https://github.com/rust-lang/rustfmt/pull/3513/files#diff-0a38f958508924d579f0171392aa81bcR41 should be aligned with the rest of the struct's fields I think.

[dcarosone]

I agree about essentially ignoring attributes. It's not likely to be useful to consider them as "group separators" because (for example with serde or elastic or several other crates that implement custom derives) if you have attributes on struct fields you might well have them on all (or at least most) fields. Here's a real-world example:

pub struct Output {
    // things shared / derived
    time: DateTime<Utc>,

    cs_method: Intern<String>,
    cs_uri_stem: Intern<String>,
    #[serde(skip_serializing_if = "Option::is_none")]
    cs_uri_query: Option<Intern<String>>,
    #[serde(skip_serializing_if = "Option::is_none")]
    cs_user_agent: Option<Intern<String>>,
    #[serde(skip_serializing_if = "Option::is_none")]
    cs_referer: Option<Intern<String>>,
    #[serde(skip_serializing_if = "Option::is_none")]
    cs_host: Option<Intern<String>>,

    // things from IIS
    #[serde(skip_serializing_if = "Option::is_none")]
    iis_time: Option<DateTime<Utc>>,
    #[serde(skip_serializing_if = "Option::is_none")]
    iis_ip: Option<Ipv4Addr>,
    #[serde(skip_serializing_if = "Option::is_none")]
    iis_port: Option<u16>,
    #[serde(skip_serializing_if = "Option::is_none")]
    iis_username: Option<Intern<String>>,

    .... // many more
}

Comments, especially doc-comments, likewise - if you're documenting your fields, you should document them all.

Blank lines should separate alignment groups. Comments and attributes should be ignored.

[scampi]

@topecongiro Are you ok with the comments ? If so, I will fix the alignment when there is a comment #3513 (comment)

[topecongiro]

@scampi Sorry for the late review. IMHO I do not see the benefit of aligning fields when they are not consecutive. Also, it does not make sense to align fields when the comment or attributes between them have multiple lines (FYI gofmt doesn't align fields that are separated by comment or empty lines).

[scampi]

@topecongiro Thanks for sharing ;o) If we align fields only when they are together, then there is very little practical use to the config option I think: most of the time, I expect fields to have a doc comment.

The concept of grouping could be handled by a separate config option:

• group by blank lines
• group fields up to a maximum field length
• ...

BTW, this grouping option would fit nicely in the structured configuration you proposed once #3487:

[struct.field]
struct_field_align_threshold = 100

[struct.field.group]
# whether to group by blank lines
blank_lines = "yes"
# whether to group by comments
comment = "yes"
# whether to group by attributes
attribute = "no"

[scampi]

Taking a struct from rustfmt codebase:

struct ChainFormatterShared<'a> {
    // The current working set of child items.
    children:         &'a [ChainItem],
    // The current rewrites of items (includes trailing `?`s, but not any way to
    // connect the rewrites together).
    rewrites:         Vec<String>,
    // Whether the chain can fit on one line.
    fits_single_line: bool,
    // The number of children in the chain. This is not equal to `self.children.len()`
    // because `self.children` will change size as we process the chain.
    child_count:      usize,
}

struct ChainFormatterShared<'a> {
    // The current working set of child items.
    children: &'a [ChainItem],
    // The current rewrites of items (includes trailing `?`s, but not any way to
    // connect the rewrites together).
    rewrites: Vec<String>,
    // Whether the chain can fit on one line.
    fits_single_line: bool,
    // The number of children in the chain. This is not equal to `self.children.len()`
    // because `self.children` will change size as we process the chain.
    child_count: usize,
}

I thought as well aligned fields would look strange because of the multiline comments inbetween, but it is more readable when aligned as it looks less packed together. It's a matter of taste.

[topecongiro]

Hmm, looking at your example, I think it's fine to align fields even when there are a multi-line comment or attributes. I personally don't use this feature, so you should have a better idea of what is good or not.

Blank lines should separate alignment groups. Comments and attributes should be ignored.

Sorry that it took a long time to reach the design direction, but could you update this PR according to this comment?

The concept of grouping could be handled by a separate config option:

This is cool, but probably too nitch to provide as an option, I think.

[scampi]

@topecongiro sorry for the delay in coming back to this PR. What do you think of the changes?

[topecongiro]

@scampi Well, this is my turn to say sorry about the delay 🙉 The code looks good to me. Thanks!

[scampi]

@topecongiro I don't mind ;o) thanks for taking time reviewing this!