Changes for #4394 - Support preserving block wrapping of closure bodies #4519
Conversation
There was a problem hiding this comment.
Thank you for the PR @davidBar-On!
There's a key piece missing here with the absence of the config option, but once that's added/corrected (which should remove changes to current test cases) the only other piece would be to add new test cases which enable the new config option to verify behavior
src/formatting/closures.rs
Outdated
| // Try-without-block only if not a macro and original code is not a block (#4394) | ||
| ast::FnRetTy::Default(_) | ||
| if !context.inside_macro() | ||
| && !context.snippet(body.span).trim().starts_with('{') => |
There was a problem hiding this comment.
Note that the discussion in issue 4394 was about adding a new config option to allow users to opt into this, but we cannot change the default behavior to do this automatically (which would violate the Style Guide).
You'll want to add a net-new config option (e.g. PreserveClosureBlockWrapping though I'm open to bikeshedding on the name) and then include a check of that option this conditional
There was a problem hiding this comment.
Sorry I didn't notice the request to all new config issue. I now added the option with the name suggested. As this is the first time I am adding an option I hope it was done properly. (The automatic tests of the new changes failed on i686-pc-windows failed, but as far as I understand this is because of problems in the test environment.)
src/formatting/closures.rs
Outdated
| ast::FnRetTy::Default(_) if !context.inside_macro() => { | ||
| try_rewrite_without_block(body, &prefix, capture, context, shape, body_shape) | ||
| // Try-without-block only if configured and original code is not a block (#4394) | ||
| if !context.config.preserve_closure_block_wrapping() | ||
| || !context.snippet(body.span).trim().starts_with('{') | ||
| { |
There was a problem hiding this comment.
Thanks for the update with the config option! Bit of a nit, but I feel like we could write this in a way that's a bit more succinct and easy to read.
What would you think about moving these checks to the arm guard, or even creating an internal function (or closure 😄) just above this match expression (perhaps something like try_without_block or can_try_without_block), which would contain the logic checks, and then just using that fn/closure call as the guard on the match arm?
There was a problem hiding this comment.
I did the changes (hopefully) as requested. The function is called can_try_rewrite_without_block().
src/formatting/closures.rs
Outdated
| fn can_try_rewrite_without_block(body: &ast::Expr, context: &RewriteContext<'_>) -> bool { | ||
| if !context.inside_macro() | ||
| && (!context.config.preserve_closure_block_wrapping() | ||
| || !context.snippet(body.span).trim().starts_with('{')) | ||
| { | ||
| true | ||
| } else { | ||
| false | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't know that this necessarily needs to be a module-level function; an internal function or closure within rewrite_closure would probably have been better, though not necessarily a blocker.
Remember that the primary purpose of this is to improve readability and maintainability, so there's no requirement to do this check within a single control flow expression.:
I think this could be expressed in a more readable way such as something like:
if context.inside_macro() {
return false;
}
if context.config.preserve_closure_block_wrapping() && context.snippet(body.span).trim_start().starts_with('{') {
return false;
}
trueAlso, there's really no need to do anything with the end of the snippet, so prefer trim_start vs. trim. Another option (without utilizing a separate function like this), would be to create a local a variable immediately before the match fn_decl.output match expression and use that as the arm guard since the check is used as a guard on the very first arm anyway.
I really just want to try to avoid having a single guard be so complicated relative to the entire match expression itself
There was a problem hiding this comment.
I made the change using local variable for the condition. I hope it is good enough now.
(I see that several of the auto-tests failed, but as far as I understand this is because of test environment issues and not problem in the submitted code, so I am not re-submitting the changes.)
There was a problem hiding this comment.
(I see that several of the auto-tests failed, but as far as I understand this is because of test environment issues and not problem in the submitted code, so I am not re-submitting the changes.)
Yup, no worries. We have a broken toolstate issue at the moment that will prevent compilation with recent nightlies. Once I get that sorted will come back to this to re-start the CI checks
|
Thanks for the update! When you get a chance would you rebase on master to get the CI checks passing again? |
|
Resubmitted with cosmetic change to a comment (as I don't know how to rebase so CI checks will be done again ....). Now all checks passed. |
Just that a |
PR to resolve #4394 - Support preserving block wrapping of closure bodies.
No new test cases are added, but several modified test cases are included, as they include tests with closure bodies blocks that were removed before.