From 97c9aeff486156211cd045c27b21a98d3173b20f Mon Sep 17 00:00:00 2001 From: Yacin Tmimi Date: Mon, 6 Mar 2023 23:43:15 -0500 Subject: [PATCH 1/3] remove trailing whitespace from multi-line tuple struct field prefix Fixes 5703, Fixes 5525 visibility modifiers always contain a trailing space after them. If the formatted tuple field needs to be written over multiple lines then the extra space will cause issues. In the best case the space will offset the type name by an extra space and in the worst case it will lead to a "left behind trailing whitespace" error. --- src/items.rs | 7 +- tests/target/issue_5525.rs | 9 +++ tests/target/issue_5703.rs | 7 ++ tests/target/struct_field_doc_comment_v2.rs | 71 +++++++++++++++++++++ 4 files changed, 93 insertions(+), 1 deletion(-) create mode 100644 tests/target/issue_5525.rs create mode 100644 tests/target/issue_5703.rs create mode 100644 tests/target/struct_field_doc_comment_v2.rs diff --git a/src/items.rs b/src/items.rs index 5d01a2df068..59354ac78fa 100644 --- a/src/items.rs +++ b/src/items.rs @@ -1867,7 +1867,7 @@ pub(crate) fn rewrite_struct_field( } let type_annotation_spacing = type_annotation_spacing(context.config); - let prefix = rewrite_struct_field_prefix(context, field)?; + let mut prefix = rewrite_struct_field_prefix(context, field)?; let attrs_str = field.attrs.rewrite(context, shape)?; let attrs_extendable = field.ident.is_none() && is_attributes_extendable(&attrs_str); @@ -1910,6 +1910,11 @@ pub(crate) fn rewrite_struct_field( let is_prefix_empty = prefix.is_empty(); // We must use multiline. We are going to put attributes and a field on different lines. + if context.config.version() == Version::Two { + // Remove any additional whitespace at the end of the prefix. + // For example if there is a space after a visibility modifier. + prefix.truncate(prefix.trim_end().len()); + } let field_str = rewrite_assign_rhs(context, prefix, &*field.ty, &RhsAssignKind::Ty, shape)?; // Remove a leading white-space from `rewrite_assign_rhs()` when rewriting a tuple struct. let field_str = if is_prefix_empty { diff --git a/tests/target/issue_5525.rs b/tests/target/issue_5525.rs new file mode 100644 index 00000000000..e339b7e0526 --- /dev/null +++ b/tests/target/issue_5525.rs @@ -0,0 +1,9 @@ +// rustfmt-version: Two + +pub struct SomeCallback( + pub extern "C" fn( + long_argument_name_to_avoid_wrap: u32, + second_long_argument_name: u32, + third_long_argument_name: u32, + ), +); diff --git a/tests/target/issue_5703.rs b/tests/target/issue_5703.rs new file mode 100644 index 00000000000..358332f61eb --- /dev/null +++ b/tests/target/issue_5703.rs @@ -0,0 +1,7 @@ +// rustfmt-version: Two + +#[derive(Clone, Debug, Default)] +pub struct ReactionGroup( + pub(in crate::room::timeline) + IndexMap<(Option, Option), OwnedUserId>, +); diff --git a/tests/target/struct_field_doc_comment_v2.rs b/tests/target/struct_field_doc_comment_v2.rs new file mode 100644 index 00000000000..d5e342eccd8 --- /dev/null +++ b/tests/target/struct_field_doc_comment_v2.rs @@ -0,0 +1,71 @@ +// rustfmt-version: Two + +// #5215 +struct MyTuple( + /// Doc Comments + /* TODO note to add more to Doc Comments */ + u32, + /// Doc Comments + // TODO note + u64, +); + +struct MyTuple( + #[cfg(unix)] // some comment + u64, + #[cfg(not(unix))] /*block comment */ u32, +); + +struct MyTuple( + #[cfg(unix)] + // some comment + u64, + #[cfg(not(unix))] + /*block comment */ + u32, +); + +struct MyTuple( + #[cfg(unix)] // some comment + pub u64, + #[cfg(not(unix))] /*block comment */ pub(crate) u32, +); + +struct MyTuple( + /// Doc Comments + /* TODO note to add more to Doc Comments */ + pub u32, + /// Doc Comments + // TODO note + pub(crate) u64, +); + +struct MyStruct { + #[cfg(unix)] // some comment + a: u64, + #[cfg(not(unix))] /*block comment */ b: u32, +} + +struct MyStruct { + #[cfg(unix)] // some comment + pub a: u64, + #[cfg(not(unix))] /*block comment */ pub(crate) b: u32, +} + +struct MyStruct { + /// Doc Comments + /* TODO note to add more to Doc Comments */ + a: u32, + /// Doc Comments + // TODO note + b: u64, +} + +struct MyStruct { + /// Doc Comments + /* TODO note to add more to Doc Comments */ + pub a: u32, + /// Doc Comments + // TODO note + pub(crate) b: u64, +} From 09dbeda575897dd1c7d9b7aa3ca2828b3232fcbd Mon Sep 17 00:00:00 2001 From: Yacin Tmimi Date: Wed, 30 Aug 2023 23:26:12 -0400 Subject: [PATCH 2/3] Prevent format_visibility from adding trailing space with Version::Two --- src/imports.rs | 7 ++- src/items.rs | 137 +++++++++++++++++++++++++++++++++---------------- src/macros.rs | 10 +++- src/utils.rs | 7 ++- src/visitor.rs | 4 ++ 5 files changed, 117 insertions(+), 48 deletions(-) diff --git a/src/imports.rs b/src/imports.rs index 09f6e752338..d356d761762 100644 --- a/src/imports.rs +++ b/src/imports.rs @@ -332,9 +332,14 @@ impl UseTree { context: &RewriteContext<'_>, shape: Shape, ) -> Option { - let vis = self.visibility.as_ref().map_or(Cow::from(""), |vis| { + let mut vis = self.visibility.as_ref().map_or(Cow::from(""), |vis| { crate::utils::format_visibility(context, vis) }); + + if !vis.is_empty() && context.config.version() == Version::Two { + vis += " "; + } + let use_str = self .rewrite(context, shape.offset_left(vis.len())?) .map(|s| { diff --git a/src/items.rs b/src/items.rs index 59354ac78fa..ceaf6c6d355 100644 --- a/src/items.rs +++ b/src/items.rs @@ -340,7 +340,12 @@ impl<'a> FnSig<'a> { fn to_str(&self, context: &RewriteContext<'_>) -> String { let mut result = String::with_capacity(128); // Vis defaultness constness unsafety abi. - result.push_str(&*format_visibility(context, self.visibility)); + let vis = format_visibility(context, self.visibility); + result.push_str(&vis); + if !vis.is_empty() && context.config.version() == Version::Two { + // format_visibility doesn't have a trailing space in Version::Two + result.push(' '); + } result.push_str(format_defaultness(self.defaultness)); result.push_str(format_constness(self.constness)); self.coroutine_kind @@ -933,7 +938,12 @@ fn format_impl_ref_and_type( } = *iimpl; let mut result = String::with_capacity(128); - result.push_str(&format_visibility(context, &item.vis)); + let vis = format_visibility(context, &item.vis); + result.push_str(&vis); + if !vis.is_empty() && context.config.version() == Version::Two { + // format_visibility doesn't have a trailing space in Version::Two + result.push(' '); + } result.push_str(format_defaultness(defaultness)); result.push_str(format_unsafety(unsafety)); @@ -1142,12 +1152,16 @@ pub(crate) fn format_trait( } = **trait_kind; let mut result = String::with_capacity(128); - let header = format!( - "{}{}{}trait ", - format_visibility(context, &item.vis), - format_unsafety(unsafety), - format_auto(is_auto), - ); + + let mut vis = format_visibility(context, &item.vis); + if !vis.is_empty() && context.config.version() == Version::Two { + // format_visibility doesn't have a trailing space in Version::Two + vis += " "; + } + let unsafety = format_unsafety(unsafety); + let auto = format_auto(is_auto); + + let header = format!("{vis}{unsafety}{auto}trait "); result.push_str(&header); let body_lo = context.snippet_provider.span_after(item.span, "{"); @@ -1350,8 +1364,13 @@ pub(crate) fn format_trait_alias( // 6 = "trait ", 2 = " =" let g_shape = shape.offset_left(6)?.sub_width(2)?; let generics_str = rewrite_generics(context, alias, generics, g_shape)?; - let vis_str = format_visibility(context, vis); - let lhs = format!("{vis_str}trait {generics_str} ="); + let vis = format_visibility(context, vis); + let lhs = if !vis.is_empty() && context.config.version() == Version::Two { + // format_visibility doesn't have a trailing space in Version::Two + format!("{vis} trait {generics_str} =") + } else { + format!("{vis}trait {generics_str} =") + }; // 1 = ";" let trait_alias_bounds = TraitAliasBounds { generic_bounds, @@ -1738,7 +1757,13 @@ fn rewrite_ty( if !after_where_predicates.is_empty() { return None; } - result.push_str(&format!("{}type ", format_visibility(context, vis))); + let vis = format_visibility(context, vis); + result.push_str(&vis); + if !vis.is_empty() && context.config.version() == Version::Two { + // format_visibility doesn't have a trailing space in Version::Two + result.push(' '); + } + result.push_str("type "); let ident_str = rewrite_ident(context, ident); if generics.params.is_empty() { @@ -1836,18 +1861,22 @@ fn type_annotation_spacing(config: &Config) -> (&str, &str) { pub(crate) fn rewrite_struct_field_prefix( context: &RewriteContext<'_>, field: &ast::FieldDef, -) -> Option { +) -> Option> { let vis = format_visibility(context, &field.vis); - let type_annotation_spacing = type_annotation_spacing(context.config); - Some(match field.ident { - Some(name) => format!( - "{}{}{}:", - vis, - rewrite_ident(context, name), - type_annotation_spacing.0 - ), - None => vis.to_string(), - }) + + let Some(name) = field.ident else { + return Some(vis); + }; + + let (space_before_colon, _) = type_annotation_spacing(context.config); + let ident = rewrite_ident(context, name); + + let prefix = if !vis.is_empty() && context.config.version() == Version::Two { + format!("{vis} {ident}{space_before_colon}:") + } else { + format!("{vis}{ident}{space_before_colon}:") + }; + Some(Cow::from(prefix)) } impl Rewrite for ast::FieldDef { @@ -1867,7 +1896,7 @@ pub(crate) fn rewrite_struct_field( } let type_annotation_spacing = type_annotation_spacing(context.config); - let mut prefix = rewrite_struct_field_prefix(context, field)?; + let prefix = rewrite_struct_field_prefix(context, field)?; let attrs_str = field.attrs.rewrite(context, shape)?; let attrs_extendable = field.ident.is_none() && is_attributes_extendable(&attrs_str); @@ -1899,6 +1928,14 @@ pub(crate) fn rewrite_struct_field( if prefix.is_empty() && !attrs_str.is_empty() && attrs_extendable && spacing.is_empty() { spacing.push(' '); } + + if !prefix.is_empty() && field.ident.is_none() && context.config.version() == Version::Two { + // For tuple struct fields, which only have a visibility modifier a space is needed + // when using Version::Two since rewrite_struct_field_prefix won't add a trailing space + // after the visibilty modifier. + spacing.push(' '); + } + let orig_ty = shape .offset_left(overhead + spacing.len()) .and_then(|ty_shape| field.ty.rewrite(context, ty_shape)); @@ -1910,11 +1947,6 @@ pub(crate) fn rewrite_struct_field( let is_prefix_empty = prefix.is_empty(); // We must use multiline. We are going to put attributes and a field on different lines. - if context.config.version() == Version::Two { - // Remove any additional whitespace at the end of the prefix. - // For example if there is a space after a visibility modifier. - prefix.truncate(prefix.trim_end().len()); - } let field_str = rewrite_assign_rhs(context, prefix, &*field.ty, &RhsAssignKind::Ty, shape)?; // Remove a leading white-space from `rewrite_assign_rhs()` when rewriting a tuple struct. let field_str = if is_prefix_empty { @@ -2002,15 +2034,18 @@ fn rewrite_static( offset: Indent, ) -> Option { let colon = colon_spaces(context.config); - let mut prefix = format!( - "{}{}{} {}{}{}", - format_visibility(context, static_parts.vis), - static_parts.defaultness.map_or("", format_defaultness), - static_parts.prefix, - format_mutability(static_parts.mutability), - rewrite_ident(context, static_parts.ident), - colon, - ); + + let vis = format_visibility(context, static_parts.vis); + let defaultness = static_parts.defaultness.map_or("", format_defaultness); + let static_prefix = static_parts.prefix; + let mutability = format_mutability(static_parts.mutability); + let ident = rewrite_ident(context, static_parts.ident); + let mut prefix = if !vis.is_empty() && context.config.version() == Version::Two { + // format_visibility doesn't have a trailing space in Version::Two + format!("{vis} {defaultness}{static_prefix} {mutability}{ident}{colon}") + } else { + format!("{vis}{defaultness}{static_prefix} {mutability}{ident}{colon}") + }; // 2 = " =".len() let ty_shape = Shape::indented(offset.block_only(), context.config).offset_left(prefix.len() + 2)?; @@ -3165,7 +3200,13 @@ fn format_header( let mut result = String::with_capacity(128); let shape = Shape::indented(offset, context.config); - result.push_str(format_visibility(context, vis).trim()); + let visibility = format_visibility(context, vis); + if context.config.version() == Version::Two { + // format_visibility doesn't have a trailing space in Version::Two + result.push_str(&visibility); + } else { + result.push_str(visibility.trim()); + } // Check for a missing comment between the visibility and the item name. let after_vis = vis.span.hi(); @@ -3351,12 +3392,13 @@ impl Rewrite for ast::ForeignItem { // function kw here. let vis = format_visibility(context, &self.vis); let mut_str = format_mutability(mutability); - let prefix = format!( - "{}static {}{}:", - vis, - mut_str, - rewrite_ident(context, self.ident) - ); + let ident = rewrite_ident(context, self.ident); + let prefix = if !vis.is_empty() && context.config.version() == Version::Two { + // format_visibility doesn't have a trailing space in Version::Two + format!("{vis} static {mut_str}{ident}:") + } else { + format!("{vis}static {mut_str}{ident}:") + }; // 1 = ; rewrite_assign_rhs( context, @@ -3434,7 +3476,12 @@ pub(crate) fn rewrite_mod( attrs_shape: Shape, ) -> Option { let mut result = String::with_capacity(32); - result.push_str(&*format_visibility(context, &item.vis)); + let vis = format_visibility(context, &item.vis); + result.push_str(&vis); + if !vis.is_empty() && context.config.version() == Version::Two { + // format_visibility doesn't have a trailing space in Version::Two + result.push(' '); + } result.push_str("mod "); result.push_str(rewrite_ident(context, item.ident)); result.push(';'); diff --git a/src/macros.rs b/src/macros.rs index 86694089fd6..dcfac9867eb 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -25,6 +25,7 @@ use crate::comment::{ contains_comment, CharClasses, FindUncommented, FullCodeCharKind, LineClasses, }; use crate::config::lists::*; +use crate::config::Version; use crate::expr::{rewrite_array, rewrite_assign_rhs, RhsAssignKind}; use crate::lists::{itemize_list, write_list, ListFormatting}; use crate::overflow; @@ -419,6 +420,9 @@ pub(crate) fn rewrite_macro_def( let mut result = if def.macro_rules { String::from("macro_rules!") + } else if context.config.version() == Version::Two { + // format_visibility doesn't have a trailing space in Version::Two + format!("{} macro", format_visibility(context, vis)) } else { format!("{}macro", format_visibility(context, vis)) }; @@ -1369,7 +1373,11 @@ fn format_lazy_static( let last = parsed_elems.len() - 1; for (i, (vis, id, ty, expr)) in parsed_elems.iter().enumerate() { // Rewrite as a static item. - let vis = crate::utils::format_visibility(context, vis); + let mut vis = crate::utils::format_visibility(context, vis); + if !vis.is_empty() && context.config.version() == Version::Two { + // format_visibility doesn't have a trailing space in Version::Two + vis += " "; + }; let mut stmt = String::with_capacity(128); stmt.push_str(&format!( "{}static ref {}: {} =", diff --git a/src/utils.rs b/src/utils.rs index 7d7bbf11529..59dbb6481e1 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -55,6 +55,7 @@ pub(crate) fn format_visibility( vis: &Visibility, ) -> Cow<'static, str> { match vis.kind { + VisibilityKind::Public if context.config.version() == Version::Two => Cow::from("pub"), VisibilityKind::Public => Cow::from("pub "), VisibilityKind::Inherited => Cow::from(""), VisibilityKind::Restricted { ref path, .. } => { @@ -69,7 +70,11 @@ pub(crate) fn format_visibility( let path = segments_iter.collect::>().join("::"); let in_str = if is_keyword(&path) { "" } else { "in " }; - Cow::from(format!("pub({in_str}{path}) ")) + if context.config.version() == Version::Two { + Cow::from(format!("pub({in_str}{path})")) + } else { + Cow::from(format!("pub({in_str}{path}) ")) + } } } } diff --git a/src/visitor.rs b/src/visitor.rs index f4d84d1381f..3cd688dd0a9 100644 --- a/src/visitor.rs +++ b/src/visitor.rs @@ -922,6 +922,10 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { ) { let vis_str = utils::format_visibility(&self.get_context(), vis); self.push_str(&*vis_str); + if !vis_str.is_empty() && self.config.version() == Version::Two { + // format_visibility doesn't have a trailing space in Version::Two + self.push_str(" "); + } self.push_str(format_unsafety(unsafety)); self.push_str("mod "); // Calling `to_owned()` to work around borrow checker. From 7894f70d392c8b048adcc0e3c5fb2f0e5cc2eba3 Mon Sep 17 00:00:00 2001 From: Yacin Tmimi Date: Thu, 28 Dec 2023 21:17:11 -0500 Subject: [PATCH 3/3] Add test case for issue 5997 --- tests/source/issue_5997.rs | 15 +++++++++++++++ tests/target/issue_5997.rs | 15 +++++++++++++++ 2 files changed, 30 insertions(+) create mode 100644 tests/source/issue_5997.rs create mode 100644 tests/target/issue_5997.rs diff --git a/tests/source/issue_5997.rs b/tests/source/issue_5997.rs new file mode 100644 index 00000000000..d184c760f62 --- /dev/null +++ b/tests/source/issue_5997.rs @@ -0,0 +1,15 @@ +// rustfmt-version: Two + +// This formats with two spaces: +pub struct Newtype( + /// Doc + #[doc()] // + pub Vec, +); + +// This formats with one: +pub struct Newtype( + /// Doc + #[doc()] + pub Vec, +); diff --git a/tests/target/issue_5997.rs b/tests/target/issue_5997.rs new file mode 100644 index 00000000000..881bc0fcb98 --- /dev/null +++ b/tests/target/issue_5997.rs @@ -0,0 +1,15 @@ +// rustfmt-version: Two + +// This formats with two spaces: +pub struct Newtype( + /// Doc + #[doc()] // + pub Vec, +); + +// This formats with one: +pub struct Newtype( + /// Doc + #[doc()] + pub Vec, +);