reorder_impl_items adds empty line between single-line macros

[repi]

We have some macros that we use inside impl statements that look like this:

impl Value {
    impl_value!(as_i64, from_i64, data_i64, as_data_i64, i64, Int64);
    impl_value!(as_f32, from_f32, data_f32, as_data_f32, f32, Float32);
    impl_value!(as_vec2, from_vec2, data_vec2, as_data_vec2, [f32; 2], Vec2);
    impl_value!(as_vec3, from_vec3, data_vec3, as_data_vec3, [f32; 3], Vec3);
    impl_value!(as_vec4, from_vec4, data_vec4, as_data_vec4, [f32; 4], Vec4);
    impl_value!(as_quat, from_quat, data_quat, as_data_quat, [f32; 4], Quat);

that when formatting with reorder_impl_items=true gets rearranged to have a new empty line between them:

impl Value {
    impl_value!(as_i64, from_i64, data_i64, as_data_i64, i64, Int64);

    impl_value!(as_f32, from_f32, data_f32, as_data_f32, f32, Float32);

    impl_value!(as_vec2, from_vec2, data_vec2, as_data_vec2, [f32; 2], Vec2);

    impl_value!(as_vec3, from_vec3, data_vec3, as_data_vec3, [f32; 3], Vec3);

    impl_value!(as_vec4, from_vec4, data_vec4, as_data_vec4, [f32; 4], Vec4);

    impl_value!(as_quat, from_quat, data_quat, as_data_quat, [f32; 4], Quat);

Which is a bit more verbose. Should/could this option skip macros inside impl statements or maybe specifically skip single-line macros? That would be nice!

Not sure how how common this pattern is but we do use it for some types. We could potentially move the macro out of the impl statement, could be a bit more standard but do quite like that this location is clear that it just adds implementation.

[ytmimi]

Thanks for the report. Confirming I can reproduce this on rustfmt 1.4.38-nightly (8572b343 2022-04-24). Just a quick glance through the issue tracker, it seems #4882 has the opposite problem. Might have something to do with const items as apposed to macros, but I'm not totally sure.

[ytmimi]

Was able to track this down to needs_empty_line. This explains why consts don't keep their spacing and why extra spacing gets added in your case.

rustfmt/src/items.rs

Lines 597 to 605 in a37d3ab

	let need_empty_line = |a: &ast::AssocItemKind, b: &ast::AssocItemKind| match (a, b) {
	(TyAlias(lty), TyAlias(rty))
	if both_type(&lty.ty, &rty.ty) || both_opaque(&lty.ty, &rty.ty) =>
	{
	false
	}
	(Const(..), Const(..)) => false,
	_ => true,
	};

It feels to me like the best way to solve this would be to introduce a new option to control the line spacing of items in impl blocks.

[repi]

Nice. So maybe option then for setting if macros (or functions) and maybe all the other types should have empty line between them? Functions do get that with this also, which I think most do prefer but could be configurable as well.

[ytmimi]

I think it would be great if we had the flexibility to independently control the spacing for all ast::AssocItemKind variants. I have some ideas for what that option might look like. I'll try to set aside some time this weekend to work on it!

[calebcartwright]

I'd like to suggest some caution here. Trying to maintain/manipulate blank lines relative to items that are getting moved around isn't something I'd be terribly keen on trying to implement nor maintain.

We already have other options relative to minimum/maximum spacing that should be applicable in this context, but those have their own outstanding implementation issues and bugs. Before seriously considering another bevy of configuration options I'd really want to understand the use case(s) and associated prevalence, alongside respective gaps relative to the existing surface of configuration options.

[calebcartwright]

Also to be clear, I'm not saying "no"; just want to talk it out a bit before we invest in developing something

[ytmimi]

@calebcartwright I hear you! I think I quickly found where we'd need to make changes to try to implement custom spacing between items and at that point the gears started turning on how we might go about it. Admittedly, I started brainstorming ideas before exploring the other spacing options that we already have so I'll be sure to take a look at them before workin on any implementation of a new feature.

Taking a quick look now blank_lines_lower_bound and blank_lines_upper_bound could probably be used, but that would apply globally, and not just in the context of impl blocks.

I also think if we were to implement an option it would control the spacing between similar items like a const and const, but not between const and an fn

[calebcartwright]

I also think if we were to implement an option it would control the spacing between similar items like a const and const, but not between const and an fn

I worry about the potential to spiral, especially if we end up supporting both reordering/regrouping. We can always go into it thinking we'll just do one for the current ask, but it's practically inevitable that sooner or later someone will come asking for a slight variant and it gets increasingly difficult to justify one but not the other.