Add fn_call_layout configuration option
#5337
Conversation
|
Haven't looked at the diff yet but quite excited about the possibilities from the title alone 😄 Want to make sure you'd also seen the transitively linked issues in #2010 and #4146 too given some of the challenges and questions posed there. Let's also be sure we account for calls that include the various types of more complex args, like large chains, large closures, etc. |
| more_complex_args( | ||
| |a, b, c| { | ||
| if a == 998765390 { -b * 3 } else { c } | ||
| }, | ||
| std::ops::Range { start: 3, end: 5 }, | ||
| std::i8::MAX, | ||
| String::from(" hello world!!").as_bytes(), | ||
| thread::Builder::new().name("thread1".to_string()).spawn( | ||
| move || { | ||
| use std::sync::Arc; | ||
|
|
||
| let mut values = Arc::<[u32]>::new_uninit_slice(3); | ||
|
|
||
| // Deferred initialization: | ||
| let data = Arc::get_mut(&mut values).unwrap(); | ||
| data[0].write(1); | ||
| data[1].write(2); | ||
| data[2].write(3); | ||
|
|
||
| let values = unsafe { values.assume_init() }; | ||
| }, | ||
| ), | ||
| "another argument", | ||
| ) |
There was a problem hiding this comment.
The "Tall" case seems a little off here. Especially lines 95-96.
| more_complex_args( | ||
| |a, b, c| { | ||
| if a == 998765390 { -b * 3 } else { c } | ||
| }, | ||
| std::ops::Range { start: 3, end: 5 }, std::i8::MAX, | ||
| String::from(" hello world!!").as_bytes(), | ||
| thread::Builder::new() | ||
| .name("thread1".to_string()) | ||
| .spawn(move || { | ||
| use std::sync::Arc; | ||
|
|
||
| let mut values = Arc::<[u32]>::new_uninit_slice(3); | ||
|
|
||
| // Deferred initialization: | ||
| let data = Arc::get_mut(&mut values).unwrap(); | ||
| data[0].write(1); | ||
| data[1].write(2); | ||
| data[2].write(3); | ||
|
|
||
| let values = unsafe { values.assume_init() }; | ||
| }), | ||
| "another argument", | ||
| ) |
There was a problem hiding this comment.
Makes it a little hard to see all the arguments since line 32 has two arguments, while all the other lines contain a single argument, so maybe it makes sense to default to "Vertical" formatting if any of the items need to be formatted over multiple lines?
|
Thanks for linking those related issues. I went ahead and read the threads and added test cases for each linked issue. I also added a test case with some more complex arguments. Totally happy to add more. I think the |
|
Yeah that indentation is definitely off in the Tall case. I'll also add that I'm pleasantly surprised by the small size of the diff! |
|
To be honest, I was just as surprised when I started working on this. Fingers crossed the |
ytmimi
force-pushed
the
issue_5218
branch
2 times, most recently
from
05641ad to
6c96d16
Compare
|
Alright, I think i've got it working as intended. There might be some edge cases I'm not thinking about though. I believe the weird formatting we saw in the |
|
I'm content deferring this to a follow up, but want to make sure we've got test cases that include combinations with other (potentially) relevant config options, like |
|
Few follow up questions:
I like the consistency with the variant names aligning with those for Tangential, but we should really name |
Totally agree. I'll make sure to add some additional tests cases!
I believe so. testing locally seems to confirm that. I'll include a test case for this just to be sure the behavior doesn't change! |
I'll open up a PR a little later to handle this soft deprecation. |
|
@calebcartwright #5387 is the follow up PR that renames |
src/expr.rs
Outdated
| let force_list_tactic = if context.config.was_set().fn_call_layout() { | ||
| Some(context.config.fn_call_layout().to_list_tactic(args.len())) | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
Would you mind elaborating on your thinking with this behavior for me? Something that would be worrisome is if there's the potential for someone to run rustfmt foo.rs with no config file/no config overrides and get one result, and then run rustfmt foo.rs --config fn_call_layout=Tall and get a different result.
Are there scenarios where you could imagine that happening due to the keying on was_set?
There was a problem hiding this comment.
To be honest, I'm not sure why I wrote it that way. I don't think there would be any issues with removing the conditional and always setting force_list_tactic=Some(context.config.fn_call_layout().to_list_tactic(args.len()))
src/overflow.rs
Outdated
| match self.force_list_tactic { | ||
| Some(list_tactic) if list_tactic == ListTactic::Mixed => { | ||
| if tactic != DefinitiveListTactic::Horizontal { | ||
| tactic = DefinitiveListTactic::Mixed | ||
| } | ||
| } | ||
| // If we need to force a tactic (like Vertical), we should only do it if there are | ||
| // at least 2 items for us to format. Otherwise, use the tactic already determined. | ||
| Some(list_tactic) if self.items.len() > 1 => { | ||
| tactic = definitive_tactic( | ||
| &*list_items, | ||
| list_tactic, | ||
| Separator::Comma, | ||
| self.one_line_width, | ||
| ) | ||
| } | ||
| _ => {} | ||
| }; |
There was a problem hiding this comment.
Curious if we couldn't hoist this up to the top of the function with an early return as it seems like many paths for determining the definitive tactic would be known up front. I.e. if the user has set fn_call_style to Vertical is there really a need to perform all the above analysis and checks?
I know things start to get messy quickly with nesting though so lmk if I'm overlooking something
There was a problem hiding this comment.
I had a similar thought in mind when I first started working on this. I think all the code leading up to the match is important because there's the chance that the last item will be rewritten if it overflows so returning before the rewrite leads to weird formatting. Granted I might have also been getting weird formatting due to the rewrite_method issues I described in #5337 (comment). I'll play around with this and see if I can hoist this up earlier in the function
|
Oh, and not necessary but may want to consider adding an entry for this to the changelog to save ourselves some work down the road 😅 |
Great idea. I'll add the entry when I make the other changes! |
ytmimi
force-pushed
the
issue_5218
branch
2 times, most recently
from
d0b3bd1 to
ef9b83d
Compare
|
Thank you for pushing this one over the finish line! I'll do a final pass but I think we're good to go from a code perspective. Afterwards, if you're up for it, I would like to run some functional-type testing with this change against some larger codebase (we've got a couple in the r-l org obviously and can look at others like Tokio and friends) to make sure it's still idempotent as compared to current nightly. Just want to be cautious and not have another expression memoization scenario that causes unnecessary churn, even on nightly |
|
That's all totally reasonable! I'd definitely be up for helping out with the testing! Would testing be as simple as cloning each repo we want to test and then run this version of rustfmt on it? |
Precisely 👍 |
|
Great! If you have a set list of repo's we should include in testing please let me know, otherwise I'll just poke around the |
|
rust, clippy, and rls would be the first ones i'd look at under our org just because of their size and because they run nightly rustfmt as part of their ci checks (though worth noting that the rust repo is pinned to a rolling version that's a few weeks/months behind the latest, so minor diffs may be observed there). you may find some other decent candidates from the last we use for the "integration" tests in that one ci job |
|
Thanks for pointing me in the right direction. I'll report back once I've done some testing!👨🔬 |
|
Planning to run through some other repos, but just want to walk through the steps I took:
This was the only code change that the Here's the input if let Ok(check) = CastCheck::new(
&fn_ctxt, e, from_ty, to_ty,
// We won't show any error to the user, so we don't care what the span is here.
DUMMY_SP, DUMMY_SP,
) {Diff in rust/src/tools/clippy/clippy_lints/src/transmute/utils.rs at line 54:
);
if let Ok(check) = CastCheck::new(
- &fn_ctxt, e, from_ty, to_ty,
+ &fn_ctxt,
+ e,
+ from_ty,
+ to_ty,
// We won't show any error to the user, so we don't care what the span is here.
- DUMMY_SP, DUMMY_SP,
+ DUMMY_SP,
+ DUMMY_SP,The reason for the diff can be tracked down to the changes made for the In theory we could remove the match arm for the Tall case entirely, and things should basically continue to operate like the current version of rustfmt does. The reason why I added the check was that in my interpretation Again, I'm planning to run through the same process with other repos, but just thought I'd share my thoughts since I hit a formatting discrepancy right away. |
|
Thanks for testing and sharing the findings with such detail. Unfortunately we cannot make any changes to the default formatting behavior, even if they are objectively better, so we'll need to do something different. I'm not sure whether that's best accomplished via an additional variant that covers the current behavior and/or some combination with a version gate |
|
hmm I feel like going with a new option to handle the current behavior seems like the best approach to me. Maybe going with something like I think the reason why I wouldn't want to version gate the change is that there are probably plenty of projects that are already using |
Sounds good, especially with your reasoning around the version gate. I still want to keep this a priority, but given the findings so far I'm going to punt to the next release as it's more important we get it done correctly vs quickly. Happy to bikeshed on the name for a bit too. I'd like to avoid any additional cases of |
Here's what the style guide has to say about function calls, and function definitions. I'm struggling to come up with something derived from what's in the style guide, but I was thinking on it and maybe we can call the new option
|
|
Maybe we take inspiration from |
I'm not sure I follow, could you elaborate on what you mean? There's two things that should be our guiding principles, or really constraining bounds:
|
|
Sorry if that was unclear. Do let me know if I'm misunderstanding what you're saying. What I'm suggesting is that we keep Currently this is the code change that is causing the formatting discrepancy in the // we only care if the any element but the last has a sigle line comment
let any_but_last_contains_line_comment = list_items
.iter()
.rev()
.skip(1)
.any(|item| item.has_single_line_comment());
match self.force_list_tactic {
Some(Density::Tall)
if tactic == DefinitiveListTactic::Mixed && any_but_last_contains_line_comment =>
{
// If we determined a `Mixed` layout, but we configured tall then force
// the tactic to be vertical only if any of the items contain single line comments.
// Otherwise, the tacitc was properly set above.
tactic = DefinitiveListTactic::Vertical
}What I'm suggesting is we add a new variant (or a new option altogether) for // we only care if the any element but the last has a sigle line comment
let any_but_last_contains_line_comment = list_items
.iter()
.rev()
.skip(1)
.any(|item| item.has_single_line_comment());
match self.force_list_tactic {
- Some(Density::Tall)
+ Some(Density::HorizontalVertical)
if tactic == DefinitiveListTactic::Mixed && any_but_last_contains_line_comment =>
{
// If we determined a `Mixed` layout, but we configured tall then force
// the tactic to be vertical only if any of the items contain single line comments.
// Otherwise, the tacitc was properly set above.
tactic = DefinitiveListTactic::Vertical
}However if you don't want to introduce a new variant that's fine, and I'm happy to just remove the match arm for the |
|
Perhaps there's some nuance I'm missing as I've not been as deep into the weeds as this as you've been. I'm 💯 with you that we'll need a 4th variant, which for the sake of simplicity I'll just call
My impression is that your original definition and associated behavior for Thoughts? |
|
Okay, I can see where there might have been a disconnect. I've been pitching ideas to keep I'm going to go ahead and implement this 4th option for the default behavior, and we can brainstorm ideas while I continue to test! |
Closes 5218 The `fn_call_layout` was added to give users more control over how function calls are formatted. This change will only impact function calls, and will not have any affect on method calls. The values are identical to those for `fn_args_layout`, which is a stable option that controls how function declarations are laid out.
Through testing it was found that `Tall` as the default value for `fn_call_layout` was not backwards compatable and would lead to breaking formatting changes. Instead of version gating the new behavior, which would also lead to unexpected formatting changes for users who already set `version=Two`, it was decided that a new option should be add to encapsulate rustfmt's previous function argument behavior. In the context of `fn_call_layout` this new option has all the same variants as the `Density` option used for `fn_params_layout`.
|
Would |
|
Some notes on what's going on in the When we call Lines 496 to 501 in 23ef4b7 because there are line comments Lines 222 to 238 in 23ef4b7 jumping back to Lines 533 to 583 in 23ef4b7 And because we have more than one argument and the tactic is Lines 552 to 581 in 23ef4b7 So from what I can gather the behavior we're seeing in the default case comes down to whether the inputs are Regardless, here are some inputs I've been playing with that further illustrate what's going on: Input all elements are fn main() {
lorem(
ipsum,
dolor,
sit,
amet, // some inline comment
adipiscing,
elit,
);
}output fn main() {
lorem(
ipsum, dolor, sit, amet, // some inline comment
adipiscing, elit,
);
}Input Not all elements are fn main() {
lorem(
ipsum,
dolor,
sit(amet), // <--- function calls are not "simple"
adipiscing,
elit,
);
}output fn main() {
lorem(
ipsum,
dolor,
sit(amet), // <--- function calls are not "simple"
adipiscing,
elit,
);
}Input All elements are fn main() {
lorem(
ipsum,
dolor,
consectetur, // <--- this item is not "short"
adipiscing,
elit,
);
}output fn main() {
lorem(
ipsum,
dolor,
consectetur, // <--- this item is not "short"
adipiscing,
elit,
);
}Input The argument list does not fit within fn main() {
lorem(
ipsum,
dolor, // some inine comment
sit,
amet,
consectetur, // some inline comment
adipiscing,
elit,
praesent_turpis_diam,
aliquet_vel, // some inline comment
sagittis,
pretium,
cursus,
ut_augue, // some inline comment
Sed_id_mauris,
ligula,
Nulla, // some inline comment
facilisi,
Donec,
vehicula,
vel_libero, // some inline comment
in_finibus,
Sed_eleifend,
tellus, // some inline comment
hendrerit,
dapibus,
bibendum,
Quisque, // some inline comment
tincidunt,
orci_vitae,
semper,
lacinia,
In_posuere, // some inline comment
nisl_eget,
elementum,
sodales, // some inline comment
ante,
tortor_porta,
ante,
id_mollis_eros, // some inline comment
diam_sit_amet,
odio_Proin,
semper, // some inline comment
commodo,
);
}output fn main() {
lorem(
ipsum, dolor, // some inine comment
sit, amet, consectetur, // some inline comment
adipiscing, elit, praesent_turpis_diam, aliquet_vel, // some inline comment
sagittis, pretium, cursus, ut_augue, // some inline comment
Sed_id_mauris, ligula, Nulla, // some inline comment
facilisi, Donec, vehicula, vel_libero, // some inline comment
in_finibus, Sed_eleifend, tellus, // some inline comment
hendrerit, dapibus, bibendum, Quisque, // some inline comment
tincidunt, orci_vitae, semper, lacinia, In_posuere, // some inline comment
nisl_eget, elementum, sodales, // some inline comment
ante, tortor_porta, ante, id_mollis_eros, // some inline comment
diam_sit_amet, odio_Proin, semper, // some inline comment
commodo,
);
} |
hmmm I think the name I think we'd just need to make it extra clear from documentation that |
|
@ytmimi @calebcartwright I'm trying (motivated by #5578) to figure out the status of this PR. From your discussion it seems like the implementation is complete only remaining work was finding a suitable config name for it? |
Closes #5218
The
fn_call_layoutwas added to give users more control over how function calls are formatted.The values are identical to those for
fn_args_layout, which is a stable option that controls how function declarations are laid out.