From 1695605a6d0e0b6175e43418307541677b83813b Mon Sep 17 00:00:00 2001 From: Oliver Bristow Date: Wed, 4 Sep 2024 00:48:38 +0100 Subject: [PATCH 1/2] Add #[rustfmt::sort] for enum variants --- src/items.rs | 26 ++++++++++++++++++-------- src/skip.rs | 11 +++++++++++ src/utils.rs | 34 ++++++++++++++++++++++++++++++++++ src/visitor.rs | 13 ++++++++++--- tests/source/enum.rs | 18 ++++++++++++++++++ tests/target/enum.rs | 16 ++++++++++++++++ 6 files changed, 107 insertions(+), 11 deletions(-) diff --git a/src/items.rs b/src/items.rs index ddcbfac1299..b455e2e3544 100644 --- a/src/items.rs +++ b/src/items.rs @@ -1,12 +1,12 @@ // Formatting top-level items - functions, structs, enums, traits, impls. -use std::borrow::Cow; -use std::cmp::{Ordering, max, min}; - +use itertools::Itertools; use regex::Regex; -use rustc_ast::visit; +use rustc_ast::{AttrVec, visit}; use rustc_ast::{ast, ptr}; use rustc_span::{BytePos, DUMMY_SP, Span, symbol}; +use std::borrow::Cow; +use std::cmp::{Ordering, max, min}; use crate::attr::filter_inline_attrs; use crate::comment::{ @@ -536,6 +536,7 @@ impl<'a> FmtVisitor<'a> { enum_def: &ast::EnumDef, generics: &ast::Generics, span: Span, + sort: bool, ) { let enum_header = format_header(&self.get_context(), "enum ", ident, vis, self.block_indent); @@ -563,7 +564,7 @@ impl<'a> FmtVisitor<'a> { self.last_pos = body_start; - match self.format_variant_list(enum_def, body_start, span.hi()) { + match self.format_variant_list(enum_def, body_start, span.hi(), sort) { Some(ref s) if enum_def.variants.is_empty() => self.push_str(s), rw => { self.push_rewrite(mk_sp(body_start, span.hi()), rw); @@ -578,6 +579,7 @@ impl<'a> FmtVisitor<'a> { enum_def: &ast::EnumDef, body_lo: BytePos, body_hi: BytePos, + sort: bool, ) -> Option { if enum_def.variants.is_empty() { let mut buffer = String::with_capacity(128); @@ -615,7 +617,7 @@ impl<'a> FmtVisitor<'a> { .unwrap_or(&0); let itemize_list_with = |one_line_width: usize| { - itemize_list( + let iter = itemize_list( self.snippet_provider, enum_def.variants.iter(), "}", @@ -635,8 +637,16 @@ impl<'a> FmtVisitor<'a> { body_lo, body_hi, false, - ) - .collect() + ); + if sort { + // sort the items by their name as this enum has the rustfmt::sort attr + iter.enumerate() + .sorted_by_key(|&(i, _)| enum_def.variants[i].ident.name.as_str()) + .map(|(_, item)| item) + .collect() + } else { + iter.collect() + } }; let mut items: Vec<_> = itemize_list_with(self.config.struct_variant_width()); diff --git a/src/skip.rs b/src/skip.rs index d733f7068fd..babd5887190 100644 --- a/src/skip.rs +++ b/src/skip.rs @@ -85,6 +85,7 @@ impl SkipNameContext { static RUSTFMT: &str = "rustfmt"; static SKIP: &str = "skip"; +static SORT: &str = "sort"; /// Say if you're playing with `rustfmt`'s skip attribute pub(crate) fn is_skip_attr(segments: &[ast::PathSegment]) -> bool { @@ -103,6 +104,16 @@ pub(crate) fn is_skip_attr(segments: &[ast::PathSegment]) -> bool { } } +pub(crate) fn is_sort_attr(segments: &[ast::PathSegment]) -> bool { + if segments.len() < 2 || segments[0].ident.to_string() != RUSTFMT { + return false; + } + match segments.len() { + 2 => segments[1].ident.to_string() == SORT, + _ => false, + } +} + fn get_skip_names(kind: &str, attrs: &[ast::Attribute]) -> Vec { let mut skip_names = vec![]; let path = format!("{RUSTFMT}::{SKIP}::{kind}"); diff --git a/src/utils.rs b/src/utils.rs index be21e89f760..4aa96ffcd90 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -24,6 +24,11 @@ pub(crate) fn skip_annotation() -> Symbol { Symbol::intern("rustfmt::skip") } +#[inline] +pub(crate) fn sort_annotation() -> Symbol { + Symbol::intern("rustfmt::sort") +} + pub(crate) fn rewrite_ident<'a>(context: &'a RewriteContext<'_>, ident: symbol::Ident) -> &'a str { context.snippet(ident.span) } @@ -271,6 +276,35 @@ pub(crate) fn contains_skip(attrs: &[Attribute]) -> bool { .any(|a| a.meta().map_or(false, |a| is_skip(&a))) } +#[inline] +pub(crate) fn contains_sort(attrs: &[Attribute]) -> bool { + attrs + .iter() + .any(|a| a.meta().map_or(false, |a| is_sort(&a))) +} + +#[inline] +fn is_sort(meta_item: &MetaItem) -> bool { + match meta_item.kind { + MetaItemKind::Word => { + let path_str = pprust::path_to_string(&meta_item.path); + path_str == sort_annotation().as_str() + } + MetaItemKind::List(ref l) => { + meta_item.has_name(sym::cfg_attr) && l.len() == 2 && crate::utils::is_sort_nested(&l[1]) + } + _ => false, + } +} + +#[inline] +fn is_sort_nested(meta_item: &NestedMetaItem) -> bool { + match meta_item { + NestedMetaItem::MetaItem(ref mi) => crate::utils::is_sort(mi), + NestedMetaItem::Lit(_) => false, + } +} + #[inline] pub(crate) fn semicolon_for_expr(context: &RewriteContext<'_>, expr: &ast::Expr) -> bool { // Never try to insert semicolons on expressions when we're inside diff --git a/src/visitor.rs b/src/visitor.rs index afb54c8e2bc..a0384e37bc9 100644 --- a/src/visitor.rs +++ b/src/visitor.rs @@ -18,7 +18,7 @@ use crate::modules::Module; use crate::parse::session::ParseSess; use crate::rewrite::{Rewrite, RewriteContext}; use crate::shape::{Indent, Shape}; -use crate::skip::{SkipContext, is_skip_attr}; +use crate::skip::{SkipContext, is_skip_attr, is_sort_attr}; use crate::source_map::{LineRangeUtils, SpanUtils}; use crate::spanned::Spanned; use crate::stmt::Stmt; @@ -515,7 +515,14 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { } ast::ItemKind::Enum(ref def, ref generics) => { self.format_missing_with_indent(source!(self, item.span).lo()); - self.visit_enum(item.ident, &item.vis, def, generics, item.span); + self.visit_enum( + item.ident, + &item.vis, + def, + generics, + item.span, + contains_sort(&item.attrs), + ); self.last_pos = source!(self, item.span).hi(); } ast::ItemKind::Mod(safety, ref mod_kind) => { @@ -858,7 +865,7 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { if segments[0].ident.to_string() != "rustfmt" { return false; } - !is_skip_attr(segments) + !(is_skip_attr(segments) | is_sort_attr(segments)) } fn walk_mod_items(&mut self, items: &[rustc_ast::ptr::P]) { diff --git a/tests/source/enum.rs b/tests/source/enum.rs index a7b9616929c..ba355a35fe6 100644 --- a/tests/source/enum.rs +++ b/tests/source/enum.rs @@ -210,3 +210,21 @@ pub enum E { A { a: u32 } = 0x100, B { field1: u32, field2: u8, field3: m::M } = 0x300 // comment } + +// #3422 +#[rustfmt::sort] +enum SortE { + + Y, // whitespace from above dropped + + X, // whitespace from above dropped + E, + // something + D(), + C(), + /// Comment for B + B, + /// Comment for A + #[rustfmt::skip] + A, +} \ No newline at end of file diff --git a/tests/target/enum.rs b/tests/target/enum.rs index 70fc8ab376c..307cf3a7097 100644 --- a/tests/target/enum.rs +++ b/tests/target/enum.rs @@ -287,3 +287,19 @@ pub enum E { field3: m::M, } = 0x300, // comment } + +// #3422 +#[rustfmt::sort] +enum SortE { + /// Comment for A + #[rustfmt::skip] + A, + /// Comment for B + B, + C(), + // something + D(), + E, + X, // whitespace from above dropped + Y, // whitespace from above dropped +} From 6af9fb6b243f26a55a4c5fcb0ddc9e948d22dced Mon Sep 17 00:00:00 2001 From: Oliver Bristow Date: Wed, 4 Sep 2024 18:59:04 +0100 Subject: [PATCH 2/2] Add #[rustfmt::sort] for struct structs --- src/expr.rs | 1 + src/items.rs | 43 ++++++++++++++++++++++++++++++++++++----- src/vertical.rs | 20 ++++++++++++++++++- src/visitor.rs | 7 ++++--- tests/source/structs.rs | 21 ++++++++++++++++++++ tests/target/structs.rs | 20 +++++++++++++++++++ 6 files changed, 103 insertions(+), 9 deletions(-) diff --git a/src/expr.rs b/src/expr.rs index 5bd87d00b1d..91c49c0b1d2 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -1684,6 +1684,7 @@ fn rewrite_struct_lit<'a>( v_shape, mk_sp(body_lo, span.hi()), one_line_width, + None, ) .unknown_error()? } else { diff --git a/src/items.rs b/src/items.rs index b455e2e3544..f49e6eb8604 100644 --- a/src/items.rs +++ b/src/items.rs @@ -2,7 +2,7 @@ use itertools::Itertools; use regex::Regex; -use rustc_ast::{AttrVec, visit}; +use rustc_ast::visit; use rustc_ast::{ast, ptr}; use rustc_span::{BytePos, DUMMY_SP, Span, symbol}; use std::borrow::Cow; @@ -519,13 +519,19 @@ impl<'a> FmtVisitor<'a> { self.push_rewrite(static_parts.span, rewrite); } - pub(crate) fn visit_struct(&mut self, struct_parts: &StructParts<'_>) { + pub(crate) fn visit_struct(&mut self, struct_parts: &StructParts<'_>, sort: bool) { let is_tuple = match struct_parts.def { ast::VariantData::Tuple(..) => true, _ => false, }; - let rewrite = format_struct(&self.get_context(), struct_parts, self.block_indent, None) - .map(|s| if is_tuple { s + ";" } else { s }); + let rewrite = format_struct( + &self.get_context(), + struct_parts, + self.block_indent, + None, + sort, + ) + .map(|s| if is_tuple { s + ";" } else { s }); self.push_rewrite(struct_parts.span, rewrite); } @@ -705,6 +711,7 @@ impl<'a> FmtVisitor<'a> { &StructParts::from_variant(field, &context), self.block_indent, Some(one_line_width), + false, )?, ast::VariantData::Unit(..) => rewrite_ident(&context, field.ident).to_owned(), }; @@ -1153,6 +1160,7 @@ fn format_struct( struct_parts: &StructParts<'_>, offset: Indent, one_line_width: Option, + sort: bool, ) -> Option { match struct_parts.def { ast::VariantData::Unit(..) => format_unit_struct(context, struct_parts, offset), @@ -1160,7 +1168,7 @@ fn format_struct( format_tuple_struct(context, struct_parts, fields, offset) } ast::VariantData::Struct { fields, .. } => { - format_struct_struct(context, struct_parts, fields, offset, one_line_width) + format_struct_struct(context, struct_parts, fields, offset, one_line_width, sort) } } } @@ -1439,6 +1447,7 @@ pub(crate) fn format_struct_struct( fields: &[ast::FieldDef], offset: Indent, one_line_width: Option, + sort: bool, ) -> Option { let mut result = String::with_capacity(1024); let span = struct_parts.span; @@ -1507,12 +1516,36 @@ pub(crate) fn format_struct_struct( let one_line_budget = one_line_width.map_or(0, |one_line_width| min(one_line_width, one_line_budget)); + let ranks: Option> = if sort { + // get the sequence of indices that would sort the vec + let indices: Vec = fields + .iter() + .enumerate() + .sorted_by(|(_, field_a), (_, field_b)| { + field_a + .ident + .zip(field_b.ident) + .map(|(a, b)| a.name.as_str().cmp(b.name.as_str())) + .unwrap_or(Ordering::Equal) + }) + .map(|(i, _)| i) + .collect(); + // create a vec with ranks for the fields, allowing for use in Itertools.sorted_by_key + let mut ranks = vec![0; indices.len()]; + for (rank, original_index) in indices.into_iter().enumerate() { + ranks[original_index] = rank; + } + Some(ranks) + } else { + None + }; let items_str = rewrite_with_alignment( fields, context, Shape::indented(offset.block_indent(context.config), context.config).sub_width(1)?, mk_sp(body_lo, span.hi()), one_line_budget, + ranks.as_ref().map(|v| v.as_slice()), )?; if !items_str.contains('\n') diff --git a/src/vertical.rs b/src/vertical.rs index 1ec239c3821..e2d83b9edcb 100644 --- a/src/vertical.rs +++ b/src/vertical.rs @@ -114,6 +114,7 @@ pub(crate) fn rewrite_with_alignment( shape: Shape, span: Span, one_line_width: usize, + ranks: Option<&[usize]>, ) -> Option { let (spaces, group_index) = if context.config.struct_field_align_threshold() > 0 { group_aligned_items(context, fields) @@ -170,12 +171,20 @@ pub(crate) fn rewrite_with_alignment( shape.indent, one_line_width, force_separator, + ranks.map(|v| &v[0..=group_index]), )?; if rest.is_empty() { Some(result + spaces) } else { let rest_span = mk_sp(init_last_pos, span.hi()); - let rest_str = rewrite_with_alignment(rest, context, shape, rest_span, one_line_width)?; + let rest_str = rewrite_with_alignment( + rest, + context, + shape, + rest_span, + one_line_width, + ranks.map(|v| &v[group_index + 1..]), + )?; Some(format!( "{}{}\n{}{}", result, @@ -211,6 +220,7 @@ fn rewrite_aligned_items_inner( offset: Indent, one_line_width: usize, force_trailing_separator: bool, + ranks: Option<&[usize]>, ) -> Option { // 1 = "," let item_shape = Shape::indented(offset, context.config).sub_width(1)?; @@ -266,6 +276,14 @@ fn rewrite_aligned_items_inner( .tactic(tactic) .trailing_separator(separator_tactic) .preserve_newline(true); + if let Some(ranks) = ranks { + items = ranks + .iter() + .zip(items.into_iter()) + .sorted_by_key(|&(index, _)| *index) + .map(|(_, item)| item) + .collect(); + } write_list(&items, &fmt).ok() } diff --git a/src/visitor.rs b/src/visitor.rs index a0384e37bc9..08e9ab91741 100644 --- a/src/visitor.rs +++ b/src/visitor.rs @@ -23,8 +23,9 @@ use crate::source_map::{LineRangeUtils, SpanUtils}; use crate::spanned::Spanned; use crate::stmt::Stmt; use crate::utils::{ - self, contains_skip, count_newlines, depr_skip_annotation, format_safety, inner_attributes, - last_line_width, mk_sp, ptr_vec_to_ref_vec, rewrite_ident, starts_with_newline, stmt_expr, + self, contains_skip, contains_sort, count_newlines, depr_skip_annotation, format_safety, + inner_attributes, last_line_width, mk_sp, ptr_vec_to_ref_vec, rewrite_ident, + starts_with_newline, stmt_expr, }; use crate::{ErrorKind, FormatReport, FormattingError}; @@ -511,7 +512,7 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { self.push_rewrite(span, rw); } ast::ItemKind::Struct(..) | ast::ItemKind::Union(..) => { - self.visit_struct(&StructParts::from_item(item)); + self.visit_struct(&StructParts::from_item(item), contains_sort(&item.attrs)); } ast::ItemKind::Enum(ref def, ref generics) => { self.format_missing_with_indent(source!(self, item.span).lo()); diff --git a/tests/source/structs.rs b/tests/source/structs.rs index 537151b276a..7cb6bd19f6e 100644 --- a/tests/source/structs.rs +++ b/tests/source/structs.rs @@ -296,3 +296,24 @@ struct Test { // #2818 struct Paren((i32)) where i32: Trait; struct Parens((i32, i32)) where i32: Trait; + +// #3422 +#[rustfmt::sort] +struct Foo { + + #[skip] + b: u32, + a: u32, // a + + bb: u32, + /// A + aa: u32, +} + +#[rustfmt::sort] +struct Fooy { + a: u32, // a + b: u32, + /// C + c: u32, +} diff --git a/tests/target/structs.rs b/tests/target/structs.rs index 4948e37a5a3..7bb2c9db894 100644 --- a/tests/target/structs.rs +++ b/tests/target/structs.rs @@ -356,3 +356,23 @@ where struct Parens((i32, i32)) where i32: Trait; + +// #3422 +#[rustfmt::sort] +struct Foo { + a: u32, // a + + /// A + aa: u32, + #[skip] + b: u32, + bb: u32, +} + +#[rustfmt::sort] +struct Fooy { + a: u32, // a + b: u32, + /// C + c: u32, +}