fmt-rfcs
fmt-rfcs copied to clipboard
Delimited expressions should be allowed as last argument
(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!
, orformat!
, 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.
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
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 :star:
- Quiche - 5.9k :star:
- git-cliff - 3.8k :star:
- Veloren - 2.1k :star:
- 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.
This isn't a change we could make in the current rustfmt. It's potentially reasonable to consider for rustfmt in a new edition.
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
Thank you for your thoughtful response. I look forward to working together in the future to hopefully improve this default.
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.