stabilise fn_args_density

[scampi]

I think fn_args_density passes the conditions to be made stable.

Is the default value correct ?

Tall is the best compromise for formatting arguments by default:

• horizontally when there is enough space
• vertically otherwise

The design and implementation of the option are sound and clean.

An enum Density is used so it is easy to add a new kind of density later if needed without breaking backward compatibility. That enum is mapped internally to another enum called ListTactic, the core logic is limited to the function lists::definitive_tactic which looks clear.

The enum Density is used to rewrite where clauses which should probably be decoupled. However, this doesn't prevent stabilisation: it looks like it can be replaced straightforwardly by a boolean though:

src/items.rs
703-            &generics.where_clause,
704-            context.config.brace_style(),
705-            Shape::legacy(where_budget, offset.block_only()),
706:            Density::Vertical,
707-            "{",
708-            where_span_end,
709-            self_ty.span.hi(),
--
1045-
1046-        // Rewrite where-clause.
1047-        if !generics.where_clause.predicates.is_empty() {
1048:            let where_density = if context.config.indent_style() == IndentStyle::Block {
1049:                Density::Compressed
1050-            } else {
1051:                Density::Tall
1052-            };
1053-
1054-            let where_budget = context.budget(last_line_width(&result));
--
1063-                &generics.where_clause,
1064-                context.config.brace_style(),
1065-                Shape::legacy(where_budget, offset.block_only()),
1066:                where_density,
1067-                "{",
1068-                None,
1069-                pos_before_where,
--
1382-                &generics.where_clause,
1383-                context.config.brace_style(),
1384-                Shape::legacy(where_budget, offset.block_only()),
1385:                Density::Compressed,
1386-                ";",
1387-                None,
1388-                body_hi,
--
1459-        &generics.where_clause,
1460-        context.config.brace_style(),
1461-        Shape::legacy(where_budget, indent),
1462:        Density::Vertical,
1463-        "=",
1464-        None,
1465-        generics.span.hi(),
--
2211-        where_clause,
2212-        context.config.brace_style(),
2213-        Shape::indented(indent, context.config),
2214:        Density::Tall,
2215-        "{",
2216-        Some(span.hi()),
2217-        pos_before_where,
--
2322-        &arg_items,
2323-        context
2324-            .config
2325:            .fn_args_density()
2326-            .to_list_tactic(arg_items.len()),
2327-        Separator::Comma,
2328-        one_line_budget,
--
2564-    where_clause: &ast::WhereClause,
2565-    brace_style: BraceStyle,
2566-    shape: Shape,
2567:    density: Density,
2568-    terminator: &str,
2569-    span_end: Option<BytePos>,
2570-    span_end_before_where: BytePos,
--
2646-    } else {
2647-        terminator.len()
2648-    };
2649:    if density == Density::Tall
2650-        || preds_str.contains('\n')
2651-        || shape.indent.width() + " where ".len() + preds_str.len() + end_length > shape.width
2652-    {
--
2737-            &generics.where_clause,
2738-            brace_style,
2739-            Shape::legacy(budget, offset.block_only()),
2740:            Density::Tall,
2741-            "{",
2742-            Some(span.hi()),
2743-            span_end_before_where,

The option is well tested, both in unit tests and, optimally, in real usage.

Plenty of time passed with rustfmt being released with that option, giving it plenty of real usage:

• Vertical variant got added 3 years ago db9d129
• Compressed and Tall were added 4 years ago 89cda8d

In that time frame, it got only 19 issues showing its implementation is quite stable.

Also, there are few tests about all 3 variants of Density:

tests/target/fn-custom.rs
1:// rustfmt-fn_args_density: Compressed

tests/target/fn_args_density-vertical.rs
1:// rustfmt-fn_args_density: Vertical

tests/target/configs/fn_args_density/tall.rs
1:// rustfmt-fn_args_density: Tall

tests/target/configs/fn_args_density/compressed.rs
1:// rustfmt-fn_args_density: Compressed

tests/target/configs/fn_args_density/vertical.rs
1:// rustfmt-fn_args_density: Vertical

tests/target/fn-custom-7.rs
2:// rustfmt-fn_args_density: Vertical

There is no open bug about the option that prevents its use.

None.

Close #3375

[topecongiro]

Seems reasonable to me. My concern is its name, density sounds odd. I prefer to rename it to fn_args_layout.

[scampi]

@topecongiro Actually sounds good to me! It reminds me of css flex so using a similar term gives a good idea of what the option does.
@softprops @aldanor you on board with this ?

[aldanor]

@scampi Looks good!

fn_args_layout seems reasonable, one question though - in case it's renamed, would fn_args_density become an invalid option?

[scampi]

in case it's renamed, would fn_args_density become an invalid option?

Yes, that's the price of an unstable option. If it were stable, that'd be a totally different story!

[scampi]

@topecongiro it has been renamed in the latest commit

[scampi]

It keeps on failing with the error below, but locally it builds fine:

error[E0433]: failed to resolve: use of undeclared type or module `Density`
    --> src/items.rs:1170:13
     |
1170 |             Density::Compressed,
     |             ^^^^^^^ use of undeclared type or module `Density`
error: aborting due to previous error
For more information about this error, try `rustc --explain E0433`.
error: Could not compile `rustfmt-nightly`.

[topecongiro]

Thank you for the updates!

[softprops]

@topecongiro How soon before we could see this in a release?