From d87c77bc5869aefaaf8e4faae4942f5f7a57872d Mon Sep 17 00:00:00 2001 From: Ayaz Hafiz Date: Sat, 9 May 2020 15:19:48 -0700 Subject: [PATCH 1/4] Preserve and format type aliases in extern blocks Previously, non-trivial type aliases in extern blocks were dropped by rustfmt because only the type alias name would be passed to a rewritter. This commit fixes that by passing all type information (generics, bounds, and assignments) to a type alias rewritter, and consolidates `rewrite_type_alias` and `rewrite_associated_type` as one function. Closes #4159 --- rustfmt-core/rustfmt-lib/src/items.rs | 95 +++++++++++-------- rustfmt-core/rustfmt-lib/src/visitor.rs | 16 ++-- .../rustfmt-lib/tests/target/issue-4159.rs | 18 ++++ 3 files changed, 81 insertions(+), 48 deletions(-) create mode 100644 rustfmt-core/rustfmt-lib/tests/target/issue-4159.rs diff --git a/rustfmt-core/rustfmt-lib/src/items.rs b/rustfmt-core/rustfmt-lib/src/items.rs index 3b71f1bb19d..45c225fb028 100644 --- a/rustfmt-core/rustfmt-lib/src/items.rs +++ b/rustfmt-core/rustfmt-lib/src/items.rs @@ -1562,6 +1562,7 @@ fn rewrite_type_prefix( prefix: &str, ident: ast::Ident, generics: &ast::Generics, + generic_bounds_opt: Option<&ast::GenericBounds>, ) -> Option { let mut result = String::with_capacity(128); result.push_str(prefix); @@ -1578,6 +1579,19 @@ fn rewrite_type_prefix( result.push_str(&generics_str); } + let type_bounds_str = if let Some(bounds) = generic_bounds_opt { + if bounds.is_empty() { + String::new() + } else { + // 2 = `: ` + let shape = Shape::indented(indent, context.config).offset_left(result.len() + 2)?; + bounds.rewrite(context, shape).map(|s| format!(": {}", s))? + } + } else { + String::new() + }; + result.push_str(&type_bounds_str); + let where_budget = context.budget(last_line_width(&result)); let option = WhereClauseOption::snuggled(&result); let where_clause_str = rewrite_where_clause( @@ -1604,6 +1618,7 @@ fn rewrite_type_item( ident: ast::Ident, rhs: &R, generics: &ast::Generics, + generic_bounds_opt: Option<&ast::GenericBounds>, vis: &ast::Visibility, ) -> Option { let mut result = String::with_capacity(128); @@ -1613,6 +1628,7 @@ fn rewrite_type_item( &format!("{}{} ", format_visibility(context, vis), prefix), ident, generics, + generic_bounds_opt, )?); if generics.where_clause.predicates.is_empty() { @@ -1627,17 +1643,6 @@ fn rewrite_type_item( rewrite_assign_rhs(context, result, rhs, rhs_shape).map(|s| s + ";") } -pub(crate) fn rewrite_type_alias( - context: &RewriteContext<'_>, - indent: Indent, - ident: ast::Ident, - ty: &ast::Ty, - generics: &ast::Generics, - vis: &ast::Visibility, -) -> Option { - rewrite_type_item(context, indent, "type", " =", ident, ty, generics, vis) -} - pub(crate) fn rewrite_opaque_type( context: &RewriteContext<'_>, indent: Indent, @@ -1655,6 +1660,7 @@ pub(crate) fn rewrite_opaque_type( ident, &opaque_type_bounds, generics, + Some(generic_bounds), vis, ) } @@ -1897,39 +1903,39 @@ fn rewrite_static( } } -pub(crate) fn rewrite_associated_type( +pub(crate) fn rewrite_type_alias( ident: ast::Ident, ty_opt: Option<&ptr::P>, generics: &ast::Generics, generic_bounds_opt: Option<&ast::GenericBounds>, context: &RewriteContext<'_>, indent: Indent, + vis: &ast::Visibility, ) -> Option { - let ident_str = rewrite_ident(context, ident); - // 5 = "type " - let generics_shape = Shape::indented(indent, context.config).offset_left(5)?; - let generics_str = rewrite_generics(context, ident_str, generics, generics_shape)?; - let prefix = format!("type {}", generics_str); - - let type_bounds_str = if let Some(bounds) = generic_bounds_opt { - if bounds.is_empty() { - String::new() - } else { - // 2 = ": ".len() - let shape = Shape::indented(indent, context.config).offset_left(prefix.len() + 2)?; - bounds.rewrite(context, shape).map(|s| format!(": {}", s))? - } - } else { - String::new() - }; + let mut prefix = rewrite_type_prefix( + context, + indent, + &format!("{}type ", format_visibility(context, vis)), + ident, + generics, + generic_bounds_opt, + )?; if let Some(ty) = ty_opt { // 1 = `;` let shape = Shape::indented(indent, context.config).sub_width(1)?; - let lhs = format!("{}{} =", prefix, type_bounds_str); + + // If there's a where clause, add a newline before the assignment. Otherwise just add a + // space. + if !generics.where_clause.predicates.is_empty() { + prefix.push_str(&indent.to_string_with_newline(context.config)); + } else { + prefix.push(' '); + } + let lhs = format!("{}=", prefix); rewrite_assign_rhs(context, lhs, &**ty, shape).map(|s| s + ";") } else { - Some(format!("{}{};", prefix, type_bounds_str)) + Some(format!("{};", prefix)) } } @@ -1973,13 +1979,14 @@ pub(crate) fn rewrite_opaque_impl_type( pub(crate) fn rewrite_associated_impl_type( ident: ast::Ident, + vis: &ast::Visibility, defaultness: ast::Defaultness, ty_opt: Option<&ptr::P>, generics: &ast::Generics, context: &RewriteContext<'_>, indent: Indent, ) -> Option { - let result = rewrite_associated_type(ident, ty_opt, generics, None, context, indent)?; + let result = rewrite_type_alias(ident, ty_opt, generics, None, context, indent, vis)?; match defaultness { ast::Defaultness::Default(..) => Some(format!("default {}", result)), @@ -3132,7 +3139,7 @@ impl Rewrite for ast::ForeignItem { // FIXME: this may be a faulty span from libsyntax. let span = mk_sp(self.span.lo(), self.span.hi() - BytePos(1)); - let item_str = match self.kind { + let item_str: String = match self.kind { ast::ForeignItemKind::Fn(_, ref fn_sig, ref generics, _) => rewrite_fn_base( context, shape.indent, @@ -3156,14 +3163,20 @@ impl Rewrite for ast::ForeignItem { // 1 = ; rewrite_assign_rhs(context, prefix, &**ty, shape.sub_width(1)?).map(|s| s + ";") } - ast::ForeignItemKind::TyAlias(..) => { - let vis = format_visibility(context, &self.vis); - Some(format!( - "{}type {};", - vis, - rewrite_ident(context, self.ident) - )) - } + ast::ForeignItemKind::TyAlias( + _, + ref generics, + ref generic_bounds, + ref type_default, + ) => rewrite_type_alias( + self.ident, + type_default.as_ref(), + generics, + Some(generic_bounds), + &context, + shape.indent, + &self.vis, + ), ast::ForeignItemKind::MacCall(ref mac) => { rewrite_macro(mac, None, context, shape, MacroPosition::Item) } diff --git a/rustfmt-core/rustfmt-lib/src/visitor.rs b/rustfmt-core/rustfmt-lib/src/visitor.rs index 1a1a5368d9f..02c6cffe4a9 100644 --- a/rustfmt-core/rustfmt-lib/src/visitor.rs +++ b/rustfmt-core/rustfmt-lib/src/visitor.rs @@ -9,9 +9,8 @@ use crate::comment::{rewrite_comment, CodeCharKind, CommentCodeSlices}; use crate::config::{BraceStyle, Config}; use crate::items::{ format_impl, format_trait, format_trait_alias, is_mod_decl, is_use_item, - rewrite_associated_impl_type, rewrite_associated_type, rewrite_extern_crate, - rewrite_opaque_impl_type, rewrite_opaque_type, rewrite_type_alias, FnBraceStyle, FnSig, - StaticParts, StructParts, + rewrite_associated_impl_type, rewrite_extern_crate, rewrite_opaque_impl_type, + rewrite_opaque_type, rewrite_type_alias, FnBraceStyle, FnSig, StaticParts, StructParts, }; use crate::macros::{macro_style, rewrite_macro, rewrite_macro_def, MacroPosition}; use crate::rewrite::{Rewrite, RewriteContext}; @@ -538,11 +537,12 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { ast::ItemKind::TyAlias(_, ref generics, ref generic_bounds, ref ty) => match ty { Some(ty) => { let rewrite = rewrite_type_alias( - &self.get_context(), - self.block_indent, item.ident, - &*ty, + Some(&*ty), generics, + Some(generic_bounds), + &self.get_context(), + self.block_indent, &item.vis, ); self.push_rewrite(item.span, rewrite); @@ -609,13 +609,14 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { ); } ast::AssocItemKind::TyAlias(_, ref generics, ref generic_bounds, ref type_default) => { - let rewrite = rewrite_associated_type( + let rewrite = rewrite_type_alias( ti.ident, type_default.as_ref(), generics, Some(generic_bounds), &self.get_context(), self.block_indent, + &ti.vis, ); self.push_rewrite(ti.span, rewrite); } @@ -656,6 +657,7 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { let rewrite_associated = || { rewrite_associated_impl_type( ii.ident, + &ii.vis, defaultness, ty.as_ref(), generics, diff --git a/rustfmt-core/rustfmt-lib/tests/target/issue-4159.rs b/rustfmt-core/rustfmt-lib/tests/target/issue-4159.rs new file mode 100644 index 00000000000..52abb132cae --- /dev/null +++ b/rustfmt-core/rustfmt-lib/tests/target/issue-4159.rs @@ -0,0 +1,18 @@ +extern "C" { + type A: Ord; + + type A<'a> + where + 'a: 'static,; + + type A + where + T: 'static,; + + type A = u8; + + type A<'a: 'static, T: Ord + 'static>: Eq + PartialEq + where + T: 'static + Copy, + = Vec; +} From 596c9512add234589699dc46b4e272d14ef597ad Mon Sep 17 00:00:00 2001 From: Ayaz Hafiz Date: Sat, 9 May 2020 16:01:42 -0700 Subject: [PATCH 2/4] fixup! Preserve and format type aliases in extern blocks --- rustfmt-core/rustfmt-lib/src/items.rs | 3 +++ rustfmt-core/rustfmt-lib/tests/target/issue-4159.rs | 6 ++++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/rustfmt-core/rustfmt-lib/src/items.rs b/rustfmt-core/rustfmt-lib/src/items.rs index 45c225fb028..130f2bce3d7 100644 --- a/rustfmt-core/rustfmt-lib/src/items.rs +++ b/rustfmt-core/rustfmt-lib/src/items.rs @@ -1935,6 +1935,9 @@ pub(crate) fn rewrite_type_alias( let lhs = format!("{}=", prefix); rewrite_assign_rhs(context, lhs, &**ty, shape).map(|s| s + ";") } else { + if !generics.where_clause.predicates.is_empty() { + prefix.push_str(&indent.to_string_with_newline(context.config)); + } Some(format!("{};", prefix)) } } diff --git a/rustfmt-core/rustfmt-lib/tests/target/issue-4159.rs b/rustfmt-core/rustfmt-lib/tests/target/issue-4159.rs index 52abb132cae..fed6095354a 100644 --- a/rustfmt-core/rustfmt-lib/tests/target/issue-4159.rs +++ b/rustfmt-core/rustfmt-lib/tests/target/issue-4159.rs @@ -3,11 +3,13 @@ extern "C" { type A<'a> where - 'a: 'static,; + 'a: 'static, + ; type A where - T: 'static,; + T: 'static, + ; type A = u8; From 43b5d586e1dc8490acc2794a0bb8b12bb57ea819 Mon Sep 17 00:00:00 2001 From: Ayaz Hafiz Date: Sun, 10 May 2020 10:06:23 -0700 Subject: [PATCH 3/4] fixup! Preserve and format type aliases in extern blocks --- rustfmt-core/rustfmt-lib/src/items.rs | 107 ++++++------------ .../rustfmt-lib/tests/target/issue-4159.rs | 6 +- 2 files changed, 36 insertions(+), 77 deletions(-) diff --git a/rustfmt-core/rustfmt-lib/src/items.rs b/rustfmt-core/rustfmt-lib/src/items.rs index 130f2bce3d7..dc3ba3dcd47 100644 --- a/rustfmt-core/rustfmt-lib/src/items.rs +++ b/rustfmt-core/rustfmt-lib/src/items.rs @@ -1556,22 +1556,23 @@ fn format_tuple_struct( Some(result) } -fn rewrite_type_prefix( +fn rewrite_type( context: &RewriteContext<'_>, indent: Indent, - prefix: &str, ident: ast::Ident, + vis: &ast::Visibility, generics: &ast::Generics, generic_bounds_opt: Option<&ast::GenericBounds>, + rhs: Option<&R>, ) -> Option { let mut result = String::with_capacity(128); - result.push_str(prefix); + result.push_str(&format!("{}type ", format_visibility(context, vis))); let ident_str = rewrite_ident(context, ident); - // 2 = `= ` if generics.params.is_empty() { result.push_str(ident_str) } else { + // 2 = `= ` let g_shape = Shape::indented(indent, context.config) .offset_left(result.len())? .sub_width(2)?; @@ -1579,21 +1580,20 @@ fn rewrite_type_prefix( result.push_str(&generics_str); } - let type_bounds_str = if let Some(bounds) = generic_bounds_opt { - if bounds.is_empty() { - String::new() - } else { + if let Some(bounds) = generic_bounds_opt { + if !bounds.is_empty() { // 2 = `: ` let shape = Shape::indented(indent, context.config).offset_left(result.len() + 2)?; - bounds.rewrite(context, shape).map(|s| format!(": {}", s))? + let type_bounds = bounds.rewrite(context, shape).map(|s| format!(": {}", s))?; + result.push_str(&type_bounds); } - } else { - String::new() - }; - result.push_str(&type_bounds_str); + } let where_budget = context.budget(last_line_width(&result)); - let option = WhereClauseOption::snuggled(&result); + let mut option = WhereClauseOption::snuggled(&result); + if rhs.is_none() { + option.suppress_comma(); + } let where_clause_str = rewrite_where_clause( context, &generics.where_clause, @@ -1607,40 +1607,22 @@ fn rewrite_type_prefix( )?; result.push_str(&where_clause_str); - Some(result) -} - -fn rewrite_type_item( - context: &RewriteContext<'_>, - indent: Indent, - prefix: &str, - suffix: &str, - ident: ast::Ident, - rhs: &R, - generics: &ast::Generics, - generic_bounds_opt: Option<&ast::GenericBounds>, - vis: &ast::Visibility, -) -> Option { - let mut result = String::with_capacity(128); - result.push_str(&rewrite_type_prefix( - context, - indent, - &format!("{}{} ", format_visibility(context, vis), prefix), - ident, - generics, - generic_bounds_opt, - )?); + if let Some(ty) = rhs { + // If there's a where clause, add a newline before the assignment. Otherwise just add a + // space. + if !generics.where_clause.predicates.is_empty() { + result.push_str(&indent.to_string_with_newline(context.config)); + } else { + result.push(' '); + } + let lhs = format!("{}=", result); - if generics.where_clause.predicates.is_empty() { - result.push_str(suffix); + // 1 = `;` + let shape = Shape::indented(indent, context.config).sub_width(1)?; + rewrite_assign_rhs(context, lhs, &*ty, shape).map(|s| s + ";") } else { - result.push_str(&indent.to_string_with_newline(context.config)); - result.push_str(suffix.trim_start()); + Some(format!("{};", result)) } - - // 1 = ";" - let rhs_shape = Shape::indented(indent, context.config).sub_width(1)?; - rewrite_assign_rhs(context, result, rhs, rhs_shape).map(|s| s + ";") } pub(crate) fn rewrite_opaque_type( @@ -1652,16 +1634,14 @@ pub(crate) fn rewrite_opaque_type( vis: &ast::Visibility, ) -> Option { let opaque_type_bounds = OpaqueTypeBounds { generic_bounds }; - rewrite_type_item( + rewrite_type( context, indent, - "type", - " =", ident, - &opaque_type_bounds, + vis, generics, Some(generic_bounds), - vis, + Some(&opaque_type_bounds), ) } @@ -1912,34 +1892,15 @@ pub(crate) fn rewrite_type_alias( indent: Indent, vis: &ast::Visibility, ) -> Option { - let mut prefix = rewrite_type_prefix( + rewrite_type( context, indent, - &format!("{}type ", format_visibility(context, vis)), ident, + vis, generics, generic_bounds_opt, - )?; - - if let Some(ty) = ty_opt { - // 1 = `;` - let shape = Shape::indented(indent, context.config).sub_width(1)?; - - // If there's a where clause, add a newline before the assignment. Otherwise just add a - // space. - if !generics.where_clause.predicates.is_empty() { - prefix.push_str(&indent.to_string_with_newline(context.config)); - } else { - prefix.push(' '); - } - let lhs = format!("{}=", prefix); - rewrite_assign_rhs(context, lhs, &**ty, shape).map(|s| s + ";") - } else { - if !generics.where_clause.predicates.is_empty() { - prefix.push_str(&indent.to_string_with_newline(context.config)); - } - Some(format!("{};", prefix)) - } + ty_opt, + ) } struct OpaqueType<'a> { diff --git a/rustfmt-core/rustfmt-lib/tests/target/issue-4159.rs b/rustfmt-core/rustfmt-lib/tests/target/issue-4159.rs index fed6095354a..2f8cf20da2c 100644 --- a/rustfmt-core/rustfmt-lib/tests/target/issue-4159.rs +++ b/rustfmt-core/rustfmt-lib/tests/target/issue-4159.rs @@ -3,13 +3,11 @@ extern "C" { type A<'a> where - 'a: 'static, - ; + 'a: 'static; type A where - T: 'static, - ; + T: 'static; type A = u8; From a427a146e75d64928c38a075726dbd78b3731e77 Mon Sep 17 00:00:00 2001 From: Ayaz Hafiz Date: Sun, 10 May 2020 10:11:36 -0700 Subject: [PATCH 4/4] fixup! Preserve and format type aliases in extern blocks --- rustfmt-core/rustfmt-lib/src/items.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rustfmt-core/rustfmt-lib/src/items.rs b/rustfmt-core/rustfmt-lib/src/items.rs index dc3ba3dcd47..27d442dab88 100644 --- a/rustfmt-core/rustfmt-lib/src/items.rs +++ b/rustfmt-core/rustfmt-lib/src/items.rs @@ -3103,7 +3103,7 @@ impl Rewrite for ast::ForeignItem { // FIXME: this may be a faulty span from libsyntax. let span = mk_sp(self.span.lo(), self.span.hi() - BytePos(1)); - let item_str: String = match self.kind { + let item_str = match self.kind { ast::ForeignItemKind::Fn(_, ref fn_sig, ref generics, _) => rewrite_fn_base( context, shape.indent,