Fix handling of attribute in enum #6286
Conversation
This fix was made thanks to the hint at [1]. This was reported in issue rust-lang#5662 [2]. Previously, a enum item containing an attribute (rustdoc or macro) would be considered multi-line, thus forcing the formatting strategy of all the items in the enum to be Vertical (i.e. multi-line formatting). When determining the formatting strategy for enum items, we should ignore the attributes. This is what we do in the `is_multi_line_variant` function. Or else, simply adding a rustdoc comment or a macro attribute would cause the formatting of the whole enum to change, which is not a desirable behavior. We will be adding tests in the following commits. - [1] rust-lang#5662 (comment) - [2] rust-lang#5662
Test case as reported in rust-lang#6280. This test case used to fail, but the code in the previous commit fixes it. We followed instructions on Contributing.md [1] to create a test case. `tests/target/rust-doc-in-enum/vertical-no-doc.rs` is being left unformatted (expected behavior), while `tests/target/rust-doc-in-enum/vertical-with-doc.rs` is formatted to `C { a: usize }` (unexpected behavior). The only different between the two samples is the `/// C` rustdoc added in `with-doc.rs`. This reproducing example is minimal: for example, after removing `d: usize` we do not reproduce the reported behavior. We found this issue while adding rustdocs to an existing project. We would expect that adding a line of documentation should not cause the formatting of the code to change. [1] https://github.com/rust-lang/rustfmt/blob/40f507526993651ad3b92eda89d5b1cebd0ed374/Contributing.md#L33
The enums modified here were not properly formatted. The fix in the previous commit now formats them properly. -> Regardless of the presence of comments or macro attributes, if any of the enum items is multi-line, all enum items should be formatted as multi-line.
…stivity Current naive implementation fails with multi-line macro attributes.
It's been a while since I made that comment but if I'm remembering correctly the idea with the new configuration was to allow mult-line and single-line variants to co-exist in an enum definition. Currently we'll try to rewrite all variants as multi-line if any of them are multi-line. I don't think it's necessary to add that option, but hopefully that gives you some additional context. |
There was a problem hiding this comment.
@malikolivier I appreciate you looking into this one. I know you mentioned that multi-line attributes are a problem for the current approach. I'd prefer if we tried to resolve that issue before moving forward.
|
Thank you for the review!
Thank you! I too believe the current approach of not having multi-line and single-line variants co-exist in an enum definition is all right. So I won't add any option.
Got it! Will attempt to have a complete fix with no edge-case left ;) |
Any formatting change we make must be gated [1, 2]. We wish for this change to be included in the Rust 2024 style edition, which is currently nightly only [3]. [1] https://github.com/rust-lang/rustfmt/blob/40f507526993651ad3b92eda89d5b1cebd0ed374/Contributing.md#gate-formatting-changes [2] https://rust-lang.github.io/rfcs/3338-style-evolution.html [3] https://doc.rust-lang.org/nightly/style-guide/editions.html#rust-2024-style-edition
…t fix" This reverts some part of commit 757e73d. The `enum.rs` should still be formatted with the default style edition. So no change should happen for those files.
The project has been using `>= StyleEdition::Edition2024` to indicate new formatting, and it's easier to grep for `Edition2021` to find all of the older formatting points. [1] [1] rust-lang#6286 (comment)
We properly handle multi-line attributes in front of an enum variant. There are several types of attributes [1], but the only attributes that can occur in this context are outer attributes like `#[repr(transparent)]`, `/// Example` or `/** Example */`. This commit deals with macro attributes like `#[repr(transparent)]`. We implement our own trivial macro attribute parser to exclude them from the variant definition, as we could not find a easy way of re-using existing parsing code (with the `syn` crate or `rustc_parse`). We will deal with outer documentation blocks in the next commit. [1] https://docs.rs/syn/2.0.75/syn/struct.Attribute.html
We properly handle outer documentation blocks in front of an enum variant. With this commit, we believe we have handled all edge-cases regarding attributes in front of enum variants.
@ytmimi I pushed code to the PR that resolves the issue (multi-line macro attributes: 277cb90, documentation block: 3d1b831). Can you review again when you are available? (at your request, I can rebase. This time, for easier review, I've added a few commits to fix the mentioned issues) |
Instead of returning `Option<String>`, `format_variant` now returns `(Option<String>, bool). Maybe this could be `Option<(String, bool)>` instead though?
src/items.rs
Outdated
| // Skip macro attributes in variant_str | ||
| // We skip one macro attribute per loop iteration | ||
| loop { | ||
| let mut macro_attribute_found = false; | ||
| let mut macro_attribute_start_i = 0; | ||
| let mut bracket_count = 0; | ||
| let mut chars = variant_str.chars().enumerate(); | ||
| while let Some((i, c)) = chars.next() { | ||
| match c { | ||
| '#' => { | ||
| if let Some((_, '[')) = chars.next() { | ||
| macro_attribute_start_i = i; | ||
| bracket_count += 1; | ||
| } | ||
| } | ||
| '[' => bracket_count += 1, | ||
| ']' => { | ||
| bracket_count -= 1; | ||
| if bracket_count == 0 { | ||
| // Macro attribute was found and ends at the i-th position | ||
| // We remove it from variant_str | ||
| let mut s = | ||
| variant_str[..macro_attribute_start_i].trim().to_owned(); | ||
| s.push_str(variant_str[(i + 1)..].trim()); | ||
| variant_str = s; | ||
| macro_attribute_found = true; | ||
| break; | ||
| } | ||
| } | ||
| '\'' => { | ||
| // Handle char in attribute | ||
| chars.next(); | ||
| chars.next(); | ||
| } | ||
| '"' => { | ||
| // Handle quoted strings within attribute | ||
| while let Some((_, c)) = chars.next() { | ||
| if c == '\\' { | ||
| chars.next(); // Skip escaped character | ||
| } else if c == '"' { | ||
| break; // end of string | ||
| } | ||
| } | ||
| } | ||
| _ => {} | ||
| } | ||
| } |
There was a problem hiding this comment.
This seemed pretty complicated and it was hard to tell how accurate this was. I thought it might be simpler if we just got the multi-lined property when rewriting the variant body so we wouldn't need to parse it out after the fact. I've pushed one commit as a proof of concept / reference for you to take a look at. Hopefully it'll serve as a good jumping off point for you.
There was a problem hiding this comment.
Thank you. Will look at it when I have time (probably next week).
Fix #5662.
Previously, an
enumitem containing an attribute (rustdoc or macro for example) would be considered multi-line, thus forcing the formatting strategy of all the items in the enum to beVertical(i.e. multi-line formatting).This PR does the following:
TODO
Multi-line macro attributes
#[...]are not handled properly (done in 277cb90)https://www.github.com/rust-lang/rustfmt/issues/5662#issuecomment-1386231624 mentions that a new config option would be needed to allow both single line and multi-line enum variants. I am not sure to understand why. Given some guidance from the maintainer, I can certainly add a new config option in this PR.