From ce990c807efe53f8879a8d89a9fd545799830abe Mon Sep 17 00:00:00 2001 From: Malik Olivier Boussejra Date: Sat, 17 Aug 2024 01:06:46 +0900 Subject: [PATCH 01/11] fix(items): Fix handling of rustdoc and macro attributes in enum This fix was made thanks to the hint at [1]. This was reported in issue #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] https://github.com/rust-lang/rustfmt/issues/5662#issuecomment-1386231624 - [2] https://github.com/rust-lang/rustfmt/issues/5662 --- src/items.rs | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/src/items.rs b/src/items.rs index 35591df0fd8..116905187e5 100644 --- a/src/items.rs +++ b/src/items.rs @@ -21,7 +21,9 @@ use crate::expr::{ rewrite_assign_rhs_with_comments, rewrite_else_kw_with_comments, rewrite_let_else_block, RhsAssignKind, RhsTactics, }; -use crate::lists::{definitive_tactic, itemize_list, write_list, ListFormatting, Separator}; +use crate::lists::{ + definitive_tactic, itemize_list, write_list, ListFormatting, ListItem, Separator, +}; use crate::macros::{rewrite_macro, MacroPosition}; use crate::overflow; use crate::rewrite::{Rewrite, RewriteContext, RewriteError, RewriteErrorExt, RewriteResult}; @@ -643,8 +645,27 @@ impl<'a> FmtVisitor<'a> { let mut items: Vec<_> = itemize_list_with(self.config.struct_variant_width()); // If one of the variants use multiple lines, use multi-lined formatting for all variants. - let has_multiline_variant = items.iter().any(|item| item.inner_as_ref().contains('\n')); - let has_single_line_variant = items.iter().any(|item| !item.inner_as_ref().contains('\n')); + fn is_multi_line_variant(item: &ListItem) -> bool { + let variant_str = item.inner_as_ref(); + let mut first_line_is_read = false; + for line in variant_str.split('\n') { + if first_line_is_read { + return false; + } + + // skip rustdoc comments and macro attributes + let line = line.trim_start(); + if line.starts_with("///") || line.starts_with("#") { + continue; + } else { + first_line_is_read = true; + } + } + + true + } + let has_multiline_variant = items.iter().any(is_multi_line_variant); + let has_single_line_variant = items.iter().any(|item| !is_multi_line_variant(item)); if has_multiline_variant && has_single_line_variant { items = itemize_list_with(0); } From 6f204cd4482f9e830d7e60b75a8b25e9b7fd9880 Mon Sep 17 00:00:00 2001 From: Malik Olivier Boussejra Date: Wed, 14 Aug 2024 08:14:53 +0900 Subject: [PATCH 02/11] tests/target: Add failing test for rustdoc in enum Test case as reported in https://github.com/rust-lang/rustfmt/issues/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 --- tests/target/rust-doc-in-enum/vertical-no-doc.rs | 12 ++++++++++++ tests/target/rust-doc-in-enum/vertical-with-doc.rs | 13 +++++++++++++ 2 files changed, 25 insertions(+) create mode 100644 tests/target/rust-doc-in-enum/vertical-no-doc.rs create mode 100644 tests/target/rust-doc-in-enum/vertical-with-doc.rs diff --git a/tests/target/rust-doc-in-enum/vertical-no-doc.rs b/tests/target/rust-doc-in-enum/vertical-no-doc.rs new file mode 100644 index 00000000000..1148c03a008 --- /dev/null +++ b/tests/target/rust-doc-in-enum/vertical-no-doc.rs @@ -0,0 +1,12 @@ +enum A { + B { + a: usize, + b: usize, + c: usize, + d: usize, + }, + + C { + a: usize, + }, +} diff --git a/tests/target/rust-doc-in-enum/vertical-with-doc.rs b/tests/target/rust-doc-in-enum/vertical-with-doc.rs new file mode 100644 index 00000000000..b61a018ac12 --- /dev/null +++ b/tests/target/rust-doc-in-enum/vertical-with-doc.rs @@ -0,0 +1,13 @@ +enum A { + B { + a: usize, + b: usize, + c: usize, + d: usize, + }, + + /// C + C { + a: usize, + }, +} From 757e73d770f8ce1b3937ae71c3244077654d5f9f Mon Sep 17 00:00:00 2001 From: Malik Olivier Boussejra Date: Sat, 17 Aug 2024 17:47:10 +0900 Subject: [PATCH 03/11] fix: Fix rustfmt formatting after previous commit fix 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. --- src/bin/main.rs | 12 +++++++++--- src/modules.rs | 8 ++++++-- tests/target/enum.rs | 5 +---- 3 files changed, 16 insertions(+), 9 deletions(-) diff --git a/src/bin/main.rs b/src/bin/main.rs index 1185454c8e7..59e0526b2ba 100644 --- a/src/bin/main.rs +++ b/src/bin/main.rs @@ -64,11 +64,17 @@ enum Operation { /// Print version information Version, /// Output default config to a file, or stdout if None - ConfigOutputDefault { path: Option }, + ConfigOutputDefault { + path: Option, + }, /// Output current config (as if formatting to a file) to stdout - ConfigOutputCurrent { path: Option }, + ConfigOutputCurrent { + path: Option, + }, /// No file specified, read from stdin - Stdin { input: String }, + Stdin { + input: String, + }, } /// Rustfmt operations errors. diff --git a/src/modules.rs b/src/modules.rs index 0590f28ee05..fb08c4d3d30 100644 --- a/src/modules.rs +++ b/src/modules.rs @@ -77,10 +77,14 @@ pub struct ModuleResolutionError { pub(crate) enum ModuleResolutionErrorKind { /// Find a file that cannot be parsed. #[error("cannot parse {file}")] - ParseError { file: PathBuf }, + ParseError { + file: PathBuf, + }, /// File cannot be found. #[error("{file} does not exist")] - NotFound { file: PathBuf }, + NotFound { + file: PathBuf, + }, /// File a.rs and a/mod.rs both exist #[error("file for module found at both {default_path:?} and {secondary_path:?}")] MultipleCandidates { diff --git a/tests/target/enum.rs b/tests/target/enum.rs index 70fc8ab376c..0c2c98d601a 100644 --- a/tests/target/enum.rs +++ b/tests/target/enum.rs @@ -68,10 +68,7 @@ pub enum EnumWithAttributes { SkippedItem(String,String,), // Post-comment #[another_attr] #[attr2] - ItemStruct { - x: usize, - y: usize, - }, /* Comment AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA */ + ItemStruct { x: usize, y: usize }, /* Comment AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA */ // And another ForcedPreflight, /* AAAAAAAAAAAAAAA AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA * AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA */ From df5b6d9e0a8d13366c4681257df727fbcf9a70a9 Mon Sep 17 00:00:00 2001 From: Malik Olivier Boussejra Date: Mon, 19 Aug 2024 08:16:34 +0900 Subject: [PATCH 04/11] test(rust-doc-in-enum): Add reproducing example for issue #5662 [1] https://github.com/rust-lang/rustfmt/issues/5662 --- tests/target/rust-doc-in-enum/horizontal-no-doc.rs | 6 ++++++ tests/target/rust-doc-in-enum/horizontal-with-doc.rs | 7 +++++++ 2 files changed, 13 insertions(+) create mode 100644 tests/target/rust-doc-in-enum/horizontal-no-doc.rs create mode 100644 tests/target/rust-doc-in-enum/horizontal-with-doc.rs diff --git a/tests/target/rust-doc-in-enum/horizontal-no-doc.rs b/tests/target/rust-doc-in-enum/horizontal-no-doc.rs new file mode 100644 index 00000000000..f869c902cc4 --- /dev/null +++ b/tests/target/rust-doc-in-enum/horizontal-no-doc.rs @@ -0,0 +1,6 @@ +enum MyType { + A { field1: bool, field2: bool }, + B { field1: bool, field2: bool }, + C { field1: bool, field2: bool }, + D { field1: bool, field2: bool }, +} diff --git a/tests/target/rust-doc-in-enum/horizontal-with-doc.rs b/tests/target/rust-doc-in-enum/horizontal-with-doc.rs new file mode 100644 index 00000000000..cbf1dc78e65 --- /dev/null +++ b/tests/target/rust-doc-in-enum/horizontal-with-doc.rs @@ -0,0 +1,7 @@ +enum MyType { + A { field1: bool, field2: bool }, + B { field1: bool, field2: bool }, + /// OMG a comment + C { field1: bool, field2: bool }, + D { field1: bool, field2: bool }, +} From 933e997a6c009992015a7e2ba9e9bc460cd062e9 Mon Sep 17 00:00:00 2001 From: Malik Olivier Boussejra Date: Mon, 19 Aug 2024 08:38:28 +0900 Subject: [PATCH 05/11] tests(attribute-in-enum): Add a test with a macro attribute for exhaustivity Current naive implementation fails with multi-line macro attributes. --- .../horizontal-no-doc.rs | 0 .../horizontal-with-doc.rs | 0 .../attribute-in-enum/vertical-macro-one-line.rs | 13 +++++++++++++ .../vertical-no-doc.rs | 0 .../vertical-with-doc.rs | 0 5 files changed, 13 insertions(+) rename tests/target/{rust-doc-in-enum => attribute-in-enum}/horizontal-no-doc.rs (100%) rename tests/target/{rust-doc-in-enum => attribute-in-enum}/horizontal-with-doc.rs (100%) create mode 100644 tests/target/attribute-in-enum/vertical-macro-one-line.rs rename tests/target/{rust-doc-in-enum => attribute-in-enum}/vertical-no-doc.rs (100%) rename tests/target/{rust-doc-in-enum => attribute-in-enum}/vertical-with-doc.rs (100%) diff --git a/tests/target/rust-doc-in-enum/horizontal-no-doc.rs b/tests/target/attribute-in-enum/horizontal-no-doc.rs similarity index 100% rename from tests/target/rust-doc-in-enum/horizontal-no-doc.rs rename to tests/target/attribute-in-enum/horizontal-no-doc.rs diff --git a/tests/target/rust-doc-in-enum/horizontal-with-doc.rs b/tests/target/attribute-in-enum/horizontal-with-doc.rs similarity index 100% rename from tests/target/rust-doc-in-enum/horizontal-with-doc.rs rename to tests/target/attribute-in-enum/horizontal-with-doc.rs diff --git a/tests/target/attribute-in-enum/vertical-macro-one-line.rs b/tests/target/attribute-in-enum/vertical-macro-one-line.rs new file mode 100644 index 00000000000..a6669b3003e --- /dev/null +++ b/tests/target/attribute-in-enum/vertical-macro-one-line.rs @@ -0,0 +1,13 @@ +enum A { + B { + a: usize, + b: usize, + c: usize, + d: usize, + }, + + #[attr] + C { + a: usize, + }, +} diff --git a/tests/target/rust-doc-in-enum/vertical-no-doc.rs b/tests/target/attribute-in-enum/vertical-no-doc.rs similarity index 100% rename from tests/target/rust-doc-in-enum/vertical-no-doc.rs rename to tests/target/attribute-in-enum/vertical-no-doc.rs diff --git a/tests/target/rust-doc-in-enum/vertical-with-doc.rs b/tests/target/attribute-in-enum/vertical-with-doc.rs similarity index 100% rename from tests/target/rust-doc-in-enum/vertical-with-doc.rs rename to tests/target/attribute-in-enum/vertical-with-doc.rs From 103d13e0590f2f03e76e60a1c7b8abf5b9959ce4 Mon Sep 17 00:00:00 2001 From: Malik Olivier Boussejra Date: Tue, 20 Aug 2024 08:29:24 +0900 Subject: [PATCH 06/11] fix(items): Gate formatting changes 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 --- src/items.rs | 10 ++++++++-- tests/target/attribute-in-enum/horizontal-no-doc.rs | 1 + tests/target/attribute-in-enum/horizontal-with-doc.rs | 1 + .../attribute-in-enum/vertical-macro-one-line.rs | 1 + tests/target/attribute-in-enum/vertical-no-doc.rs | 1 + tests/target/attribute-in-enum/vertical-with-doc.rs | 1 + 6 files changed, 13 insertions(+), 2 deletions(-) diff --git a/src/items.rs b/src/items.rs index 116905187e5..99efbf9e0ca 100644 --- a/src/items.rs +++ b/src/items.rs @@ -645,8 +645,14 @@ impl<'a> FmtVisitor<'a> { let mut items: Vec<_> = itemize_list_with(self.config.struct_variant_width()); // If one of the variants use multiple lines, use multi-lined formatting for all variants. - fn is_multi_line_variant(item: &ListItem) -> bool { + let is_multi_line_variant = |item: &ListItem| -> bool { let variant_str = item.inner_as_ref(); + if self.config.style_edition() < StyleEdition::Edition2024 { + // Fall back to previous naive implementation (#5662) because of + // rustfmt's stability guarantees + return variant_str.contains('\n'); + } + let mut first_line_is_read = false; for line in variant_str.split('\n') { if first_line_is_read { @@ -663,7 +669,7 @@ impl<'a> FmtVisitor<'a> { } true - } + }; let has_multiline_variant = items.iter().any(is_multi_line_variant); let has_single_line_variant = items.iter().any(|item| !is_multi_line_variant(item)); if has_multiline_variant && has_single_line_variant { diff --git a/tests/target/attribute-in-enum/horizontal-no-doc.rs b/tests/target/attribute-in-enum/horizontal-no-doc.rs index f869c902cc4..56c378cea97 100644 --- a/tests/target/attribute-in-enum/horizontal-no-doc.rs +++ b/tests/target/attribute-in-enum/horizontal-no-doc.rs @@ -1,3 +1,4 @@ +// rustfmt-style_edition: 2024 enum MyType { A { field1: bool, field2: bool }, B { field1: bool, field2: bool }, diff --git a/tests/target/attribute-in-enum/horizontal-with-doc.rs b/tests/target/attribute-in-enum/horizontal-with-doc.rs index cbf1dc78e65..0a0b374e76d 100644 --- a/tests/target/attribute-in-enum/horizontal-with-doc.rs +++ b/tests/target/attribute-in-enum/horizontal-with-doc.rs @@ -1,3 +1,4 @@ +// rustfmt-style_edition: 2024 enum MyType { A { field1: bool, field2: bool }, B { field1: bool, field2: bool }, diff --git a/tests/target/attribute-in-enum/vertical-macro-one-line.rs b/tests/target/attribute-in-enum/vertical-macro-one-line.rs index a6669b3003e..e7cfe64a1db 100644 --- a/tests/target/attribute-in-enum/vertical-macro-one-line.rs +++ b/tests/target/attribute-in-enum/vertical-macro-one-line.rs @@ -1,3 +1,4 @@ +// rustfmt-style_edition: 2024 enum A { B { a: usize, diff --git a/tests/target/attribute-in-enum/vertical-no-doc.rs b/tests/target/attribute-in-enum/vertical-no-doc.rs index 1148c03a008..2369bc368b7 100644 --- a/tests/target/attribute-in-enum/vertical-no-doc.rs +++ b/tests/target/attribute-in-enum/vertical-no-doc.rs @@ -1,3 +1,4 @@ +// rustfmt-style_edition: 2024 enum A { B { a: usize, diff --git a/tests/target/attribute-in-enum/vertical-with-doc.rs b/tests/target/attribute-in-enum/vertical-with-doc.rs index b61a018ac12..7b1d3315c2b 100644 --- a/tests/target/attribute-in-enum/vertical-with-doc.rs +++ b/tests/target/attribute-in-enum/vertical-with-doc.rs @@ -1,3 +1,4 @@ +// rustfmt-style_edition: 2024 enum A { B { a: usize, From 22b076c1bd78d1a88002563c6d2ee23176f833ba Mon Sep 17 00:00:00 2001 From: Malik Olivier Boussejra Date: Tue, 20 Aug 2024 08:36:41 +0900 Subject: [PATCH 07/11] Revert some part of "fix: Fix rustfmt formatting after previous commit fix" This reverts some part of commit 757e73d770f8ce1b3937ae71c3244077654d5f9f. The `enum.rs` should still be formatted with the default style edition. So no change should happen for those files. --- tests/target/enum.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/target/enum.rs b/tests/target/enum.rs index 0c2c98d601a..70fc8ab376c 100644 --- a/tests/target/enum.rs +++ b/tests/target/enum.rs @@ -68,7 +68,10 @@ pub enum EnumWithAttributes { SkippedItem(String,String,), // Post-comment #[another_attr] #[attr2] - ItemStruct { x: usize, y: usize }, /* Comment AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA */ + ItemStruct { + x: usize, + y: usize, + }, /* Comment AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA */ // And another ForcedPreflight, /* AAAAAAAAAAAAAAA AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA * AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA */ From 56014626737e25e7d9a3e2e12f656231d5dce761 Mon Sep 17 00:00:00 2001 From: Malik Olivier Boussejra Date: Tue, 20 Aug 2024 12:02:51 +0900 Subject: [PATCH 08/11] fix(items): Mention Edition 2021 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] https://github.com/rust-lang/rustfmt/pull/6286#discussion_r1722558105 --- src/items.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/items.rs b/src/items.rs index 99efbf9e0ca..f6d20b3bce3 100644 --- a/src/items.rs +++ b/src/items.rs @@ -647,7 +647,7 @@ impl<'a> FmtVisitor<'a> { // If one of the variants use multiple lines, use multi-lined formatting for all variants. let is_multi_line_variant = |item: &ListItem| -> bool { let variant_str = item.inner_as_ref(); - if self.config.style_edition() < StyleEdition::Edition2024 { + if self.config.style_edition() <= StyleEdition::Edition2021 { // Fall back to previous naive implementation (#5662) because of // rustfmt's stability guarantees return variant_str.contains('\n'); From 277cb90384b78b92b696b9fe84317829d950450b Mon Sep 17 00:00:00 2001 From: Malik Olivier Boussejra Date: Wed, 21 Aug 2024 10:30:06 +0900 Subject: [PATCH 09/11] fix(items): Take into account multi-line outer macro attributes 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 --- src/items.rs | 67 ++++++++++++++++--- .../vertical-macro-multi-line.rs | 44 ++++++++++++ 2 files changed, 100 insertions(+), 11 deletions(-) create mode 100644 tests/target/attribute-in-enum/vertical-macro-multi-line.rs diff --git a/src/items.rs b/src/items.rs index f6d20b3bce3..0b8bcb9a2cf 100644 --- a/src/items.rs +++ b/src/items.rs @@ -3,6 +3,7 @@ use std::borrow::Cow; use std::cmp::{max, min, Ordering}; +use itertools::Itertools; use regex::Regex; use rustc_ast::visit; use rustc_ast::{ast, ptr}; @@ -653,22 +654,66 @@ impl<'a> FmtVisitor<'a> { return variant_str.contains('\n'); } - let mut first_line_is_read = false; - for line in variant_str.split('\n') { - if first_line_is_read { - return false; + // First exclude all doc comments + let mut lines = variant_str + .split('\n') + .filter(|line| !line.trim().starts_with("///")); + + let mut variant_str = lines.join("\n"); + // 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 + } + } + } + _ => {} + } } - // skip rustdoc comments and macro attributes - let line = line.trim_start(); - if line.starts_with("///") || line.starts_with("#") { - continue; - } else { - first_line_is_read = true; + if !macro_attribute_found { + break; } } - true + variant_str.contains('\n') }; let has_multiline_variant = items.iter().any(is_multi_line_variant); let has_single_line_variant = items.iter().any(|item| !is_multi_line_variant(item)); diff --git a/tests/target/attribute-in-enum/vertical-macro-multi-line.rs b/tests/target/attribute-in-enum/vertical-macro-multi-line.rs new file mode 100644 index 00000000000..57b020f8371 --- /dev/null +++ b/tests/target/attribute-in-enum/vertical-macro-multi-line.rs @@ -0,0 +1,44 @@ +// rustfmt-style_edition: 2024 +enum A { + B { + a: usize, + b: usize, + c: usize, + d: usize, + }, + + #[multiline_macro_attribute( + very_very_long_option1, + very_very_long_option2, + very_very_long_option3 + )] + C { + a: usize, + }, + + #[attr_with_expression1(x = ']')] + D1 { + a: usize, + }, + + #[attr_with_expression2(x = vec![])] + D2 { + a: usize, + }, + + #[attr_with_expression3(x = "]")] + D3 { + a: usize, + }, + + #[attr_with_expression4(x = "\"]")] + D4 { + a: usize, + }, + + #[attr1] + #[attr2] + D5 { + a: usize, + }, +} From 3d1b831ad6e1db141cd51b0d1999781f284c2fd6 Mon Sep 17 00:00:00 2001 From: Malik Olivier Boussejra Date: Wed, 21 Aug 2024 12:25:02 +0900 Subject: [PATCH 10/11] fix(items): Take into account outer documentation blocks 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. --- src/items.rs | 37 ++++++++++++++++++- .../attribute-in-enum/horizontal-with-doc.rs | 3 +- 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/src/items.rs b/src/items.rs index 0b8bcb9a2cf..ee7e9a35363 100644 --- a/src/items.rs +++ b/src/items.rs @@ -654,12 +654,47 @@ impl<'a> FmtVisitor<'a> { return variant_str.contains('\n'); } - // First exclude all doc comments + // First exclude all outer one-line doc comments let mut lines = variant_str .split('\n') .filter(|line| !line.trim().starts_with("///")); let mut variant_str = lines.join("\n"); + + // Then exclude all outer documentation blocks + // Exclude one block per loop iteration + loop { + let mut block_found = false; + let mut chars = variant_str.chars().enumerate(); + 'block: while let Some((i, c)) = chars.next() { + if c != '/' { + continue; + } + let block_start = i; + if let Some((_, '*')) = chars.next() { + if let Some((_, '*')) = chars.next() { + while let Some((_, c)) = chars.next() { + if c == '*' { + if let Some((i, '/')) = chars.next() { + // block was found and ends at the i-th position + // We remove it from variant_str + let mut s = variant_str[..block_start].trim().to_owned(); + s.push_str(variant_str[(i + 1)..].trim()); + variant_str = s; + block_found = true; + break 'block; + } + } + } + } + } + } + + if !block_found { + break; + } + } + // Skip macro attributes in variant_str // We skip one macro attribute per loop iteration loop { diff --git a/tests/target/attribute-in-enum/horizontal-with-doc.rs b/tests/target/attribute-in-enum/horizontal-with-doc.rs index 0a0b374e76d..75531edcea0 100644 --- a/tests/target/attribute-in-enum/horizontal-with-doc.rs +++ b/tests/target/attribute-in-enum/horizontal-with-doc.rs @@ -2,7 +2,8 @@ enum MyType { A { field1: bool, field2: bool }, B { field1: bool, field2: bool }, - /// OMG a comment + /// One-line doc comment C { field1: bool, field2: bool }, + /** Documentation block */ D { field1: bool, field2: bool }, } From 7594e30b828e6acb85c678207b10f7114381fe28 Mon Sep 17 00:00:00 2001 From: Yacin Tmimi Date: Fri, 20 Sep 2024 13:04:40 -0400 Subject: [PATCH 11/11] let format_variant tell us if the variant is multi-line Instead of returning `Option`, `format_variant` now returns `(Option, bool). Maybe this could be `Option<(String, bool)>` instead though? --- src/items.rs | 182 ++++++++++++++++----------------------------------- src/lists.rs | 4 +- 2 files changed, 57 insertions(+), 129 deletions(-) diff --git a/src/items.rs b/src/items.rs index ee7e9a35363..dac90ac8152 100644 --- a/src/items.rs +++ b/src/items.rs @@ -620,7 +620,10 @@ impl<'a> FmtVisitor<'a> { .unwrap_or(&0); let itemize_list_with = |one_line_width: usize| { - itemize_list( + let mut has_multiline_variant = false; + let mut has_single_line_variant = false; + + let items: Vec<_> = itemize_list( self.snippet_provider, enum_def.variants.iter(), "}", @@ -634,126 +637,26 @@ impl<'a> FmtVisitor<'a> { }, |f| f.span.hi(), |f| { - self.format_variant(f, one_line_width, pad_discrim_ident_to) - .unknown_error() + let (result, is_multi_line) = + self.format_variant(f, one_line_width, pad_discrim_ident_to); + has_multiline_variant |= is_multi_line; + has_single_line_variant |= !is_multi_line; + result.unknown_error() }, body_lo, body_hi, false, ) - .collect() - }; - let mut items: Vec<_> = itemize_list_with(self.config.struct_variant_width()); - - // If one of the variants use multiple lines, use multi-lined formatting for all variants. - let is_multi_line_variant = |item: &ListItem| -> bool { - let variant_str = item.inner_as_ref(); - if self.config.style_edition() <= StyleEdition::Edition2021 { - // Fall back to previous naive implementation (#5662) because of - // rustfmt's stability guarantees - return variant_str.contains('\n'); - } - - // First exclude all outer one-line doc comments - let mut lines = variant_str - .split('\n') - .filter(|line| !line.trim().starts_with("///")); - - let mut variant_str = lines.join("\n"); - - // Then exclude all outer documentation blocks - // Exclude one block per loop iteration - loop { - let mut block_found = false; - let mut chars = variant_str.chars().enumerate(); - 'block: while let Some((i, c)) = chars.next() { - if c != '/' { - continue; - } - let block_start = i; - if let Some((_, '*')) = chars.next() { - if let Some((_, '*')) = chars.next() { - while let Some((_, c)) = chars.next() { - if c == '*' { - if let Some((i, '/')) = chars.next() { - // block was found and ends at the i-th position - // We remove it from variant_str - let mut s = variant_str[..block_start].trim().to_owned(); - s.push_str(variant_str[(i + 1)..].trim()); - variant_str = s; - block_found = true; - break 'block; - } - } - } - } - } - } - - if !block_found { - break; - } - } + .collect(); - // 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 - } - } - } - _ => {} - } - } + (items, has_multiline_variant, has_single_line_variant) + }; - if !macro_attribute_found { - break; - } - } + let (mut items, has_multiline_variant, has_single_line_variant) = + itemize_list_with(self.config.struct_variant_width()); - variant_str.contains('\n') - }; - let has_multiline_variant = items.iter().any(is_multi_line_variant); - let has_single_line_variant = items.iter().any(|item| !is_multi_line_variant(item)); if has_multiline_variant && has_single_line_variant { - items = itemize_list_with(0); + (items, _, _) = itemize_list_with(0); } let shape = self.shape().sub_width(2)?; @@ -774,23 +677,35 @@ impl<'a> FmtVisitor<'a> { field: &ast::Variant, one_line_width: usize, pad_discrim_ident_to: usize, - ) -> Option { + ) -> (Option, bool) { + // Makes the early return a little more ergonomic since we can't use `?` + macro_rules! unwrap_early_return { + ($option:expr, $is_multi_line:expr) => { + match $option { + Some(v) => v, + None => return (None, $is_multi_line), + } + }; + } + + let mut is_variant_multi_line = self.snippet(field.span).contains('\n'); if contains_skip(&field.attrs) { let lo = field.attrs[0].span.lo(); let span = mk_sp(lo, field.span.hi()); - return Some(self.snippet(span).to_owned()); + return (Some(self.snippet(span).to_owned()), is_variant_multi_line); } let context = self.get_context(); let shape = self.shape(); let attrs_str = if context.config.style_edition() >= StyleEdition::Edition2024 { - field.attrs.rewrite(&context, shape)? + unwrap_early_return!(field.attrs.rewrite(&context, shape), is_variant_multi_line) } else { // StyleEdition::Edition20{15|18|21} formatting that was off by 1. See issue #5801 - field.attrs.rewrite(&context, shape.sub_width(1)?)? + let shape = unwrap_early_return!(shape.sub_width(1), is_variant_multi_line); + unwrap_early_return!(field.attrs.rewrite(&context, shape), is_variant_multi_line) }; // sub_width(1) to take the trailing comma into account - let shape = shape.sub_width(1)?; + let shape = unwrap_early_return!(shape.sub_width(1), is_variant_multi_line); let lo = field .attrs @@ -799,32 +714,45 @@ impl<'a> FmtVisitor<'a> { let span = mk_sp(lo, field.span.lo()); let variant_body = match field.data { - ast::VariantData::Tuple(..) | ast::VariantData::Struct { .. } => format_struct( - &context, - &StructParts::from_variant(field, &context), - self.block_indent, - Some(one_line_width), - )?, + ast::VariantData::Tuple(..) | ast::VariantData::Struct { .. } => { + let rewrite = format_struct( + &context, + &StructParts::from_variant(field, &context), + self.block_indent, + Some(one_line_width), + ); + unwrap_early_return!(rewrite, is_variant_multi_line) + } ast::VariantData::Unit(..) => rewrite_ident(&context, field.ident).to_owned(), }; let variant_body = if let Some(ref expr) = field.disr_expr { let lhs = format!("{variant_body:pad_discrim_ident_to$} ="); let ex = &*expr.value; - rewrite_assign_rhs_with( + let rewrite = rewrite_assign_rhs_with( &context, lhs, ex, shape, &RhsAssignKind::Expr(&ex.kind, ex.span), RhsTactics::AllowOverflow, - )? + ); + unwrap_early_return!(rewrite, is_variant_multi_line) } else { variant_body }; - combine_strs_with_missing_comments(&context, &attrs_str, &variant_body, span, shape, false) - .ok() + is_variant_multi_line = variant_body.contains('\n'); + let rewirte = combine_strs_with_missing_comments( + &context, + &attrs_str, + &variant_body, + span, + shape, + false, + ) + .ok(); + (rewirte, is_variant_multi_line) } fn visit_impl_items(&mut self, items: &[ptr::P]) { diff --git a/src/lists.rs b/src/lists.rs index f9e722130cd..18c7b293d4f 100644 --- a/src/lists.rs +++ b/src/lists.rs @@ -745,7 +745,7 @@ where I: Iterator, F1: Fn(&T) -> BytePos, F2: Fn(&T) -> BytePos, - F3: Fn(&T) -> RewriteResult, + F3: FnMut(&T) -> RewriteResult, { type Item = ListItem; @@ -810,7 +810,7 @@ where I: Iterator, F1: Fn(&T) -> BytePos, F2: Fn(&T) -> BytePos, - F3: Fn(&T) -> RewriteResult, + F3: FnMut(&T) -> RewriteResult, { ListItems { snippet_provider,