Delimited expressions should be allowed as last argument

[pitaj]

(this was originally a comment in #61)

I'm trying to dig up why the rustfmt overflow_delimited_expr shouldn't be enabled by default. And I found it derives from the following statement in the guide

Only where the multi-line sub-expression is a closure with an explicit block, this combining behaviour may be used where there are other arguments, as long as all the arguments and the first line of the closure fit on the first line, the closure is the last argument, and there is only one closure argument

History

I see in this comment @joshtriplett says

This should never apply if the outer construct contains more than just the single expression

// don't do this
let thing = Struct(first, OtherStruct(
    second,
    third,
));

// don't do this either
foo(thing, bar(
    param1,
    param2,
    param3,
));

In the next comment they suggest a possible exception for format macros

As a possible exception to the requirement that the outer construct must have only one argument, you can also apply this to an invocation of print!, println!, or format!, where the first argument consists of a format string and the second argument consists of a single expression:

println!("Thing: {}", generate_thing(
    long_argument_one,
    long_argument_two,
    long_argument_three,
));

AFAIK this did not make it into the guide

Then @nrc suggests an exception for closures as the last argument in this comment and @joshtriplett agrees

Re closures, I would like to restrict this to block closures, and in this case we allow any number of parameters as long as everything up to the { of the closure fits on one line (including inside the 'short' heuristic for function args), e.g.,:

foo(1 + 2, x, some_variable.bar(), |x| {
    closure_body();
    ret_value
})

This did make it into the guide

Then there's a lot of discussion about what control flow expressions should be allowed.

Then there's discussion about arrays where @kennytm brings up arrays and @joshtriplett agrees that it should be treated similarly.

However, there was never any discussion about allowing arrays, match, and other delimited expressions as the last argument.

Getting to the point

I don't see any reason why other multi-line delimited expressions shouldn't be allowed by default in the same context as closures. For instance, right now, rustfmt will format a match inside a closure and a match without a closure differently:

    // with a closure
    let f = fn_call(123, |cond| match cond {
        A => 1,
        B => 2,
    });
    // without a closure, not consistent
    let f = fn_call(
        123,
        match cond {
            A => 1,
            B => 2,
        },
    );
    // works fine with normal blocks though
    let f = fn_call(123, {
        let a = 1;
        let b = 2;
        a + b
    });
    // or without any other arguments
    let f = fn_call(match cond {
        A => 1,
        B => 2,
    });

That doesn't make any sense to me, and looks super ugly. Also, consider an append macro, used to push a list of elements to a Vec without copying or cloning:

     append!(v, [
        352732685652347985,
        348974392769832796759287,
        4738973482759382759832,
        178957349857932759834729857,
    ]);

Awesome for cleaning up dozens of lines of v.push(...) calls into an easily readable format. But rustfmt just kills me here (without enabling the overflow_delimited_expr option):

     append!(
        v,
        [
            352732685652347985,
            348974392769832796759287,
            4738973482759382759832,
            178957349857932759834729857,
        ]
    );

So my question is, why does this "last argument exception" not apply to match, arrays, vec![], etc? Seems inconsistent, yet alone ugly.

[calebcartwright]

Thanks for sharing your perspective but I think it probably makes sense to close given the age of the issue, existing contrary prescription of the style guide, and the presence of a configuration option that supports the desired style (though not by default).

The config option exists precisely to support those that shared your opinion, but I don't see how we could completely reverse course on the style guide and formatting behavior that would deliberately introduce breaking formatting changes.

Will leave open for a bit longer for any follow up dialog, but will likely close down the road

[pitaj]

Man I've taken a while to respond, sorry about that.
First, I'd like to put some numbers on it. Of the top 1000 rust repositories (when I took this poll):

• 614 had no rustfmt.toml
• 386 had a rustfmt.toml
• of those, 13 had overflow_delimited_expr: true

The 13 includes some quite popular projects, such as:

• Alacritty - 38.2k ⭐
• Quiche - 5.9k ⭐
• git-cliff - 3.8k ⭐
• Veloren - 2.1k ⭐
• and the rand crate

Now to address your points.

The config option exists precisely to support those that shared your opinion, but I don't see how we could completely reverse course on the style guide and formatting behavior that would deliberately introduce breaking formatting changes.

Breaking formatting happens pretty often in my experience. And this kind of formatting change is the least bad in my opinion: it only reduces whitespace.

I also don't think it's a reversal of course. I'd say it makes the styling more consistent, instead of having the single exception of closures. This is one of those cases that some types of projects hit way more often than others, but when you hit it, it's practically necessary to enable the option.

[joshtriplett]

This isn't a change we could make in the current rustfmt. It's potentially reasonable to consider for rustfmt in a new edition.

[calebcartwright]

Thanks for sharing those findings @pitaj and no worries on the timing. As Josh noted this isn't a change that could be readily made due to stability guarantees so we've got time to discuss and brainstorm.

I don't want detour the conversation too much as I think this thread will be best served by focusing on the merits for/against making the change, but will respond to a couple of your points to reinforce why we can't simply make the change (yet) even if you get everyone on board.

Breaking formatting happens pretty often in my experience
I also don't think it's a reversal of course

Think we may be using the terminology in different ways, or perhaps there's some important nuance that's missing. Due to the formatting stability guarantee, we can't decide to have rustfmt format something differently by default (even if it's better), and that in turn limits what changes we can (currently) make to the style guide which dictates rustfmt's default.

It's true that there have been cases where users would run a rustup update and a cargo fmt --check that was passing before would suddenly fail. However, that typically happens due to a bug fix, or rustfmt seeing and formatting some code for the first time. The latter happens because rustfmt currently will silently fail and leave code alone it can't process, because error_on_unformatted is false by default (something we hope to evolve to a silent/warn/error type of option). This sometimes creates scenarios where people think they have code that's been properly formatted by rustfmt, but which actually never has.

That's anticipated/allowed, whereas the type of change you're proposing explicitly modifies formatting behavior, and would be analogous to reformating those 987 other repositories with overflow_delimited_expr=true instead of their current overflow_delimited_expr=false; we couldn't do that today.

We do have some plans for providing a transition/evolution process to formatting (both rustfmt and the style guide) though so your proposal is certainly something that could be incorporated in the next evolution

[pitaj]

Thank you for your thoughtful response. I look forward to working together in the future to hopefully improve this default.

[joshtriplett]

FWIW, regarding my historical comments that were quoted in the opening post here: I no longer hold that position or have those concerns, and would be happy to see this enabled by default in a new style edition.