Include closures in allow_multi_line to prevent exponential time formatting of nested closures - #7077
Include closures in allow_multi_line to prevent exponential time formatting of nested closures#7077Manishearth wants to merge 1 commit into
Conversation
…atting of nested closures Fixes rust-lang#5626 This code does some backtracking: rustfmt tries to format a closure, and if it ends up needing multiple lines, for expressions that don't come with builtin braces it adds braces and tries to format it again. "for expressions that don't come with builtin braces" is checked by allow_multiline, and contains things like `match {}` etc. It does not contain closures themselves. It should: a closure already follows the rule of "has braces if multiline", so in *this* case (code is multiline) formatted closures can be counted as having builtin braces. Without this, the behavior is O(2^N): for N nested closures, each closure attempts to format the child closure as a brace-less expression-closure until it bottoms out. The innermost closure will format with a newline, which then causes each parent formatted closure to discard since the newline is treated as "format as a braced block-closure instead" signal. And then it tries to format the outermost closure as a block, but then you have the same problem with the N - 1 subclosures. Allowing closures to skip An LLM was used to investigate this issue. Issue body and code is mine (the test is a straight copy from the issue).
|
With this change will rustfmt try to format let f = |x| {
|y| {
let z = x + y;
z * 2
}
};back into this? let f = |x| |y| {
let z = x + y;
z * 2
};Also, this example reminded me of #5685 |
|
@ytmimi yes |
| pub fn g() { | ||
| || || || || || || || { | ||
| H || || { | ||
| isize | ||
| || || || || || || || { | ||
| (|| || || { | ||
| isize | ||
| || || || || || || || || { | ||
| isize || || || || || || || || || || || || || isize!(f(), 2) | ||
| } | ||
| })() | ||
| } | ||
| } | ||
| }; | ||
| isize!(::f(), 1); | ||
| } | ||
| } |
There was a problem hiding this comment.
It's tough to tell, but what is H in this context? It just looks like it's floating there.
There was a problem hiding this comment.
Yeah I need to fix the test, I copied it but I might have made a mistake when playing around with it.
|
As was brought up on #5685 we should probably consult @rust-lang/style if this change is going to impact closure formatting moving forward. Alternatively, we might want to figure out how we can specifically target this pathological case without impacting the "regular case". |
|
Yeah, if this is a thing where people have actual preferences then we should not fix it this way. I may or may not poke around at doing this in a way that preserves the actual formatting. |
Fixes #5626
This code does some backtracking: rustfmt tries to format a closure, and if it ends up needing multiple lines, for expressions that don't come with builtin braces it adds braces and tries to format it again.
"for expressions that don't come with builtin braces" is checked by allow_multiline, and contains things like
match {}etc.It does not contain closures themselves. It should: a closure already follows the rule of "has braces if multiline", so in this case (code is multiline) formatted closures can be counted as having builtin braces.
Without this, the behavior is O(2^N): for N nested closures, each closure attempts to format the child closure as a brace-less expression-closure until it bottoms out. The innermost closure will format with a newline, which then causes each parent formatted closure to discard since the newline is treated as "format as a braced block-closure instead" signal. And then it tries to format the outermost closure as a block, but then you have the same problem with the N - 1 subclosures. Allowing closures to skip
An LLM was used to investigate this issue. Issue body and code is mine (the test is a straight copy from the issue).
Breakages
The current implementation does introduce a formatting change, unfortunately.
would previously format as
but now it stays unchanged. If we want to retain the original behavior, then I think we just need to skip the expression code when we have nested closures. That would be more complicated.
We could also use a style edition gate here. I wanted to open a PR first before trying to discuss how to handle the breakage.