From a3716527a2517541e9161933e8597bdc7ec88005 Mon Sep 17 00:00:00 2001 From: Radim Date: Wed, 5 Feb 2025 18:19:41 +0100 Subject: [PATCH 1/5] struct_field_align with shorthand and rest/base This works with the new target files, but breaks in surprising previously working places. test with cargo +nightly-2025-01-02 test system_tests -- --nocapture --- src/expr.rs | 17 +++----- src/items.rs | 1 + src/test/mod.rs | 8 +++- src/vertical.rs | 79 ++++++++++++++++++++++++++++++++++---- tests/source/issue_6080.rs | 16 ++++++++ tests/source/issue_6096.rs | 17 ++++++++ tests/target/issue_6080.rs | 16 ++++++++ tests/target/issue_6096.rs | 17 ++++++++ 8 files changed, 152 insertions(+), 19 deletions(-) create mode 100644 tests/source/issue_6080.rs create mode 100644 tests/source/issue_6096.rs create mode 100644 tests/target/issue_6080.rs create mode 100644 tests/target/issue_6096.rs diff --git a/src/expr.rs b/src/expr.rs index 8031ab68290..f7d06a15198 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -1620,10 +1620,6 @@ fn rewrite_index( } } -fn struct_lit_can_be_aligned(fields: &[ast::ExprField], has_base: bool) -> bool { - !has_base && fields.iter().all(|field| !field.is_shorthand) -} - fn rewrite_struct_lit<'a>( context: &RewriteContext<'_>, path: &ast::Path, @@ -1660,15 +1656,14 @@ fn rewrite_struct_lit<'a>( let one_line_width = h_shape.map_or(0, |shape| shape.width); let body_lo = context.snippet_provider.span_after(span, "{"); - let fields_str = if struct_lit_can_be_aligned(fields, has_base_or_rest) - && context.config.struct_field_align_threshold() > 0 - { + let fields_str = if context.config.struct_field_align_threshold() > 0 { rewrite_with_alignment( fields, context, v_shape, mk_sp(body_lo, span.hi()), one_line_width, + Some(struct_rest), ) .unknown_error()? } else { @@ -1720,10 +1715,10 @@ fn rewrite_struct_lit<'a>( body_lo, span.hi(), false, - ); - let item_vec = items.collect::>(); + ) + .collect::>(); - let tactic = struct_lit_tactic(h_shape, context, &item_vec); + let tactic = struct_lit_tactic(h_shape, context, &items); let nested_shape = shape_for_tactic(tactic, h_shape, v_shape); let ends_with_comma = span_ends_with_comma(context, span); @@ -1736,7 +1731,7 @@ fn rewrite_struct_lit<'a>( force_no_trailing_comma || has_base_or_rest || !context.use_block_indent(), ); - write_list(&item_vec, &fmt)? + write_list(&items, &fmt)? }; let fields_str = diff --git a/src/items.rs b/src/items.rs index 901ad44edab..416d70e7638 100644 --- a/src/items.rs +++ b/src/items.rs @@ -1503,6 +1503,7 @@ pub(crate) fn format_struct_struct( Shape::indented(offset.block_indent(context.config), context.config).sub_width_opt(1)?, mk_sp(body_lo, span.hi()), one_line_budget, + None )?; if !items_str.contains('\n') diff --git a/src/test/mod.rs b/src/test/mod.rs index d62da08fff8..924dd93d72e 100644 --- a/src/test/mod.rs +++ b/src/test/mod.rs @@ -181,7 +181,13 @@ fn system_tests() { init_log(); run_test_with(&TestSetting::default(), || { // Get all files in the tests/source directory. - let files = get_test_files(Path::new("tests/source"), true); + // let files = get_test_files(Path::new("tests/source"), true); + let files = vec![ + // PathBuf::from("tests/source/issue_6096.rs"), + // PathBuf::from("tests/source/issue_6080.rs") + PathBuf::from("tests/source/configs/struct_field_align_threshold/20.rs") + ]; + // dbg!(&files); let (_reports, count, fails) = check_files(files, &None); // Display results. diff --git a/src/vertical.rs b/src/vertical.rs index 21e34d29710..9840a592106 100644 --- a/src/vertical.rs +++ b/src/vertical.rs @@ -6,7 +6,7 @@ use itertools::Itertools; use rustc_ast::ast; use rustc_span::{BytePos, Span}; -use crate::comment::combine_strs_with_missing_comments; +use crate::comment::{FindUncommented, combine_strs_with_missing_comments}; use crate::config::lists::*; use crate::expr::rewrite_field; use crate::items::{rewrite_struct_field, rewrite_struct_field_prefix}; @@ -108,12 +108,13 @@ impl AlignedItem for ast::ExprField { } } -pub(crate) fn rewrite_with_alignment( +pub(crate) fn rewrite_with_alignment( fields: &[T], context: &RewriteContext<'_>, shape: Shape, span: Span, one_line_width: usize, + struct_rest: Option<&ast::StructRest>, ) -> Option { let (spaces, group_index) = if context.config.struct_field_align_threshold() > 0 { group_aligned_items(context, fields) @@ -170,12 +171,15 @@ pub(crate) fn rewrite_with_alignment( shape.indent, one_line_width, force_separator, + struct_rest, + shape, )?; 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, struct_rest)?; Some(format!( "{}{}\n{}{}", result, @@ -211,6 +215,8 @@ fn rewrite_aligned_items_inner( offset: Indent, one_line_width: usize, force_trailing_separator: bool, + struct_rest: Option<&ast::StructRest>, + shape: Shape, ) -> Option { // 1 = "," let item_shape = Shape::indented(offset, context.config).sub_width_opt(1)?; @@ -221,14 +227,71 @@ fn rewrite_aligned_items_inner( field_prefix_max_width = 0; } + enum StructLitField<'a, T> { + Regular(&'a T), + Base(&'a ast::Expr), + Rest(Span), + } + + let has_base_or_rest = match struct_rest { + Some(rest) => match rest { + // ast::StructRest::None if fields.is_empty() => return format!("{path_str} {{}}"), + // ast::StructRest::Rest(_) if fields.is_empty() => { + // return Ok(format!("{path_str} {{ .. }}")); + // } + ast::StructRest::Rest(_) | ast::StructRest::Base(_) => true, + _ => false, + }, + None => false, + }; + + let field_iter = fields.iter().map(StructLitField::Regular).chain( + struct_rest + .and_then(|rest| match rest { + ast::StructRest::Base(expr) => Some(StructLitField::Base(&**expr)), + ast::StructRest::Rest(span) => Some(StructLitField::Rest(*span)), + ast::StructRest::None => None, + }) + .into_iter(), + ); + + let span_lo = |item: &StructLitField<'_, T>| match *item { + StructLitField::Regular(field) => field.get_span().lo(), + StructLitField::Base(expr) => { + let last_field_hi = fields + .last() + .map_or(span.lo(), |field| field.get_span().hi()); + let snippet = context.snippet(mk_sp(last_field_hi, expr.span.lo())); + let pos = snippet.find_uncommented("..").unwrap(); + last_field_hi + BytePos(pos as u32) + } + StructLitField::Rest(span) => span.lo(), + }; + let span_hi = |item: &StructLitField<'_, T>| match *item { + StructLitField::Regular(field) => field.get_span().hi(), + StructLitField::Base(expr) => expr.span.hi(), + StructLitField::Rest(span) => span.hi(), + }; + let rewrite = |item: &StructLitField<'_, T>| match *item { + StructLitField::Regular(field) => { + field.rewrite_aligned_item(context, item_shape, field_prefix_max_width) + } + StructLitField::Base(expr) => { + // 2 = .. + expr.rewrite_result(context, shape.offset_left(2, span)?) + .map(|s| format!("..{}", s)) + } + StructLitField::Rest(_) => Ok("..".to_owned()), + }; + let mut items = itemize_list( context.snippet_provider, - fields.iter(), + field_iter, "}", ",", - |field| field.get_span().lo(), - |field| field.get_span().hi(), - |field| field.rewrite_aligned_item(context, item_shape, field_prefix_max_width), + span_lo, + span_hi, + rewrite, span.lo(), span.hi(), false, @@ -258,6 +321,8 @@ fn rewrite_aligned_items_inner( let separator_tactic = if force_trailing_separator { SeparatorTactic::Always + } else if has_base_or_rest { + SeparatorTactic::Never } else { context.config.trailing_comma() }; diff --git a/tests/source/issue_6080.rs b/tests/source/issue_6080.rs new file mode 100644 index 00000000000..ae45ddecf71 --- /dev/null +++ b/tests/source/issue_6080.rs @@ -0,0 +1,16 @@ +// rustfmt-struct_field_align_threshold: 30 + +#[derive(Default)] +struct Foo { + id: u8, + age: u8, + name: String, +} + +fn main() { + foo = Foo { + id: 5, + name: "John".into(), + ..Default::default() + }; +} diff --git a/tests/source/issue_6096.rs b/tests/source/issue_6096.rs new file mode 100644 index 00000000000..ffdabb69128 --- /dev/null +++ b/tests/source/issue_6096.rs @@ -0,0 +1,17 @@ +// rustfmt-struct_field_align_threshold: 30 + +struct Foo { + id: u8, + name: String, + x: u8, +} + +fn main() { + let x = 5; + + foo = Foo { + id: 3, + x, + name: "John".into(), + }; +} diff --git a/tests/target/issue_6080.rs b/tests/target/issue_6080.rs new file mode 100644 index 00000000000..fd2d5ad2b6f --- /dev/null +++ b/tests/target/issue_6080.rs @@ -0,0 +1,16 @@ +// rustfmt-struct_field_align_threshold: 30 + +#[derive(Default)] +struct Foo { + id: u8, + age: u8, + name: String, +} + +fn main() { + foo = Foo { + id: 5, + name: "John".into(), + ..Default::default() + }; +} diff --git a/tests/target/issue_6096.rs b/tests/target/issue_6096.rs new file mode 100644 index 00000000000..34a775936c3 --- /dev/null +++ b/tests/target/issue_6096.rs @@ -0,0 +1,17 @@ +// rustfmt-struct_field_align_threshold: 30 + +struct Foo { + id: u8, + name: String, + x: u8, +} + +fn main() { + let x = 5; + + foo = Foo { + id: 3, + x, + name: "John".into(), + }; +} From e445aadd0d7cdfb9a3e9d8504c36f8e95f163dc8 Mon Sep 17 00:00:00 2001 From: Radim Date: Fri, 7 Feb 2025 00:05:34 +0100 Subject: [PATCH 2/5] Fix All system_tests pass now! --- src/expr.rs | 2 +- src/test/mod.rs | 12 ++++++------ src/vertical.rs | 4 ---- .../deeply_nested_struct_with_long_field_names.rs | 2 +- .../issue-4926/struct_with_long_field_names.rs | 2 +- .../deeply_nested_struct_with_long_field_names.rs | 2 +- .../issue-4926/struct_with_long_field_names.rs | 2 +- 7 files changed, 11 insertions(+), 15 deletions(-) diff --git a/src/expr.rs b/src/expr.rs index f7d06a15198..07336518ab1 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -1656,7 +1656,7 @@ fn rewrite_struct_lit<'a>( let one_line_width = h_shape.map_or(0, |shape| shape.width); let body_lo = context.snippet_provider.span_after(span, "{"); - let fields_str = if context.config.struct_field_align_threshold() > 0 { + let fields_str = if !fields.is_empty() && context.config.struct_field_align_threshold() > 0 { rewrite_with_alignment( fields, context, diff --git a/src/test/mod.rs b/src/test/mod.rs index 924dd93d72e..931b9d9c656 100644 --- a/src/test/mod.rs +++ b/src/test/mod.rs @@ -181,12 +181,12 @@ fn system_tests() { init_log(); run_test_with(&TestSetting::default(), || { // Get all files in the tests/source directory. - // let files = get_test_files(Path::new("tests/source"), true); - let files = vec![ - // PathBuf::from("tests/source/issue_6096.rs"), - // PathBuf::from("tests/source/issue_6080.rs") - PathBuf::from("tests/source/configs/struct_field_align_threshold/20.rs") - ]; + let files = get_test_files(Path::new("tests/source"), true); + // let files = vec![ + // PathBuf::from("tests/source/issue_6096.rs"), + // PathBuf::from("tests/source/issue_6080.rs"), + // PathBuf::from("tests/source/issue-4926/deeply_nested_struct_with_long_field_names.rs"), + // ]; // dbg!(&files); let (_reports, count, fails) = check_files(files, &None); diff --git a/src/vertical.rs b/src/vertical.rs index 9840a592106..a40298900b5 100644 --- a/src/vertical.rs +++ b/src/vertical.rs @@ -235,10 +235,6 @@ fn rewrite_aligned_items_inner( let has_base_or_rest = match struct_rest { Some(rest) => match rest { - // ast::StructRest::None if fields.is_empty() => return format!("{path_str} {{}}"), - // ast::StructRest::Rest(_) if fields.is_empty() => { - // return Ok(format!("{path_str} {{ .. }}")); - // } ast::StructRest::Rest(_) | ast::StructRest::Base(_) => true, _ => false, }, diff --git a/tests/source/issue-4926/deeply_nested_struct_with_long_field_names.rs b/tests/source/issue-4926/deeply_nested_struct_with_long_field_names.rs index 516699fa2b8..40b0e2ffc84 100644 --- a/tests/source/issue-4926/deeply_nested_struct_with_long_field_names.rs +++ b/tests/source/issue-4926/deeply_nested_struct_with_long_field_names.rs @@ -1,4 +1,4 @@ -// rustfmt-struct_field_align_threshold: 30 +// rustfmt-struct_field_align_threshold: 27 struct X { really_really_long_field_a: i32, diff --git a/tests/source/issue-4926/struct_with_long_field_names.rs b/tests/source/issue-4926/struct_with_long_field_names.rs index b8a37f0714e..27db3f62798 100644 --- a/tests/source/issue-4926/struct_with_long_field_names.rs +++ b/tests/source/issue-4926/struct_with_long_field_names.rs @@ -1,4 +1,4 @@ -// rustfmt-struct_field_align_threshold: 30 +// rustfmt-struct_field_align_threshold: 27 struct X { really_really_long_field_a: i32, diff --git a/tests/target/issue-4926/deeply_nested_struct_with_long_field_names.rs b/tests/target/issue-4926/deeply_nested_struct_with_long_field_names.rs index c7bc7f7296d..958e92bdc1a 100644 --- a/tests/target/issue-4926/deeply_nested_struct_with_long_field_names.rs +++ b/tests/target/issue-4926/deeply_nested_struct_with_long_field_names.rs @@ -1,4 +1,4 @@ -// rustfmt-struct_field_align_threshold: 30 +// rustfmt-struct_field_align_threshold: 27 struct X { really_really_long_field_a: i32, diff --git a/tests/target/issue-4926/struct_with_long_field_names.rs b/tests/target/issue-4926/struct_with_long_field_names.rs index ac4674ab5d5..eabb8ff0040 100644 --- a/tests/target/issue-4926/struct_with_long_field_names.rs +++ b/tests/target/issue-4926/struct_with_long_field_names.rs @@ -1,4 +1,4 @@ -// rustfmt-struct_field_align_threshold: 30 +// rustfmt-struct_field_align_threshold: 27 struct X { really_really_long_field_a: i32, From 3b86657f132a5b8d1ebf9e7f3b5eb8d9bd5807d5 Mon Sep 17 00:00:00 2001 From: Radim Date: Fri, 7 Feb 2025 00:10:04 +0100 Subject: [PATCH 3/5] Remove testing commenst --- src/test/mod.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/test/mod.rs b/src/test/mod.rs index 931b9d9c656..d62da08fff8 100644 --- a/src/test/mod.rs +++ b/src/test/mod.rs @@ -182,12 +182,6 @@ fn system_tests() { run_test_with(&TestSetting::default(), || { // Get all files in the tests/source directory. let files = get_test_files(Path::new("tests/source"), true); - // let files = vec![ - // PathBuf::from("tests/source/issue_6096.rs"), - // PathBuf::from("tests/source/issue_6080.rs"), - // PathBuf::from("tests/source/issue-4926/deeply_nested_struct_with_long_field_names.rs"), - // ]; - // dbg!(&files); let (_reports, count, fails) = check_files(files, &None); // Display results. From 387ae6cf512cbd3d7b94dae5cebd0c121decbabd Mon Sep 17 00:00:00 2001 From: Radim Date: Fri, 7 Feb 2025 00:16:21 +0100 Subject: [PATCH 4/5] fmt items --- src/items.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/items.rs b/src/items.rs index 416d70e7638..62f7706da36 100644 --- a/src/items.rs +++ b/src/items.rs @@ -1503,7 +1503,7 @@ pub(crate) fn format_struct_struct( Shape::indented(offset.block_indent(context.config), context.config).sub_width_opt(1)?, mk_sp(body_lo, span.hi()), one_line_budget, - None + None, )?; if !items_str.contains('\n') From 1e41b3b83d6d3f0a4713feadb8be5194453f1881 Mon Sep 17 00:00:00 2001 From: Radim Date: Fri, 7 Feb 2025 00:55:04 +0100 Subject: [PATCH 5/5] Leave one issue file with alignment --- .../deeply_nested_struct_with_long_field_names.rs | 2 +- .../deeply_nested_struct_with_long_field_names.rs | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/source/issue-4926/deeply_nested_struct_with_long_field_names.rs b/tests/source/issue-4926/deeply_nested_struct_with_long_field_names.rs index 40b0e2ffc84..516699fa2b8 100644 --- a/tests/source/issue-4926/deeply_nested_struct_with_long_field_names.rs +++ b/tests/source/issue-4926/deeply_nested_struct_with_long_field_names.rs @@ -1,4 +1,4 @@ -// rustfmt-struct_field_align_threshold: 27 +// rustfmt-struct_field_align_threshold: 30 struct X { really_really_long_field_a: i32, diff --git a/tests/target/issue-4926/deeply_nested_struct_with_long_field_names.rs b/tests/target/issue-4926/deeply_nested_struct_with_long_field_names.rs index 958e92bdc1a..f2d990c4803 100644 --- a/tests/target/issue-4926/deeply_nested_struct_with_long_field_names.rs +++ b/tests/target/issue-4926/deeply_nested_struct_with_long_field_names.rs @@ -1,4 +1,4 @@ -// rustfmt-struct_field_align_threshold: 27 +// rustfmt-struct_field_align_threshold: 30 struct X { really_really_long_field_a: i32, @@ -20,10 +20,10 @@ fn test(x: X) { matches!( x, X { - really_really_long_field_a: 10, - really_really_really_long_field_b: 10, - really_really_really_really_long_field_c: 10, - really_really_really_really_really_long_field_d: 10, + really_really_long_field_a: 10, + really_really_really_long_field_b: 10, + really_really_really_really_long_field_c: 10, + really_really_really_really_really_long_field_d: 10, really_really_really_really_really_really_long_field_e: 10, .. }