Add `fn_call_layout` configuration option

[ytmimi]

trafficstars

Closes #5218

The fn_call_layout was 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.

[calebcartwright]

Haven't looked at the diff yet but quite excited about the possibilities from the title alone :smile:

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.

[ytmimi]

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 Compressed case can lead to some weird formatting when the arguments aren't "simple", but I've opted to not force that restriction in the event that the user has explicitly requested for the mixed layout since it feels like we should honor what the user is requesting. Something also seems to be a little off with the complex chain formatting in the Tall case.

[calebcartwright]

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!

[ytmimi]

To be honest, I was just as surprised when I started working on this. Fingers crossed the Tall case isn't too tricky to get right, and doesn't add too much to the size of the diff 🤞🏼.

[ytmimi]

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 Tall case before had to do with rewrite_method_call's call to rewrite_call. Since we don't want these changes to affect chain's I updated rewrite_call's signature to take force_list_tactic: Option<ListTactic>, and always pass None when rewriting methods.

[calebcartwright]

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 indent_style, hard_tabs, etc. along with the new variants for this option

[calebcartwright]

Few follow up questions:

• why are all the tests set with version to Two? is that just how you opted to structure the tests or is there some call-related formatting with the default that looks particularly odd with this option?
• is the min of 2 args required to trigger vertical aligned with what we do for params in signatures as well?

I like the consistency with the variant names aligning with those for fn_args_layout although at the same time given the higher degree of complexity with calls/args I won't be surprised if we need additional/different variants down the road.

Tangential, but we should really name fn_args_layout to something like fn_params_layout. Since this option has already been stabilized we'd have to be able to support a soft deprecate an automapping to the renamed/new option though. This doesn't need to be done in this PR of course, but would be good if we could get that change bundled with this in the same release

[ytmimi]

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 indent_style, hard_tabs, etc. along with the new variants for this option

Totally agree. I'll make sure to add some additional tests cases!

why are all the tests set with version to Two? is that just how you opted to structure the tests or is there some call-related formatting with the default that looks particularly odd with this option?

version=Two was used to format if a == 998765390 { -b * 3 } else { c } on a single line. It doesn't have any direct impact on fn_call_layout (that I can tell). I can see why the inclusion of version=Two would be confusing so I'm going to rebase and remove it.

is the min of 2 args required to trigger vertical aligned with what we do for params in signatures as well?

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!

[ytmimi]

Tangential, but we should really name fn_args_layout to something like fn_params_layout. Since this option has already been stabilized we'd have to be able to support a soft deprecate an automapping to the renamed/new option though. This doesn't need to be done in this PR of course, but would be good if we could get that change bundled with this in the same release

I'll open up a PR a little later to handle this soft deprecation.

[ytmimi]

@calebcartwright #5387 is the follow up PR that renames fn_args_layout.

[calebcartwright]

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 :sweat_smile:

[ytmimi]

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!

[calebcartwright]

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

[ytmimi]

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?

[calebcartwright]

Would testing be as simple as cloning each repo we want to test and then run this version of rustfmt on it?

Precisely :+1:

[ytmimi]

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 r-l repo, and other large rust codebases I can find on GitHub! I imagine we 100% need to test this on the compiler.

[calebcartwright]

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

[ytmimi]

Thanks for pointing me in the right direction. I'll report back once I've done some testing!👨‍🔬

[ytmimi]

Planning to run through some other repos, but just want to walk through the steps I took:

1. compiled a release binary for the fn_call_args version of rustfmt and copied it for later use
2. compiled a release binary of the latest master rustfmt and copied it for later use
3. cloned the rust repo
4. updated the submodules
5. ran cargo fmt in the root of the rust repo RUSTFMT=/path/to/fn_call_rustfmt cargo fmt > fn_call_diff.txt
6. ran cargo fmt in the root of the rust repo RUSTFMT=/path/to/rustfmt cargo fmt > rustfmt_diff.txt
7. compared the diffs git diff --no-index fn_call_diff.txt rustfmt_diff.txt

This was the only code change that the fn_call_args version produced that the current master did not.

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 Tall case.

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 Tall means, format horizontally, and as soon as you need to wrap go vertical instead of going mixed. When playing around with it, that interpretation of Tall lead to some tests breaking, so I amended it to only force a vertical layout if any list item except the last had a line comment. Trying to force a vertical layout if any list item including the last had aline comment also lead to existing tests failing. @calebcartwright I hope that additional context is helpful. Let me know if you'd prefer I just remove the Tall arm or if this is an acceptable behavior change.

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.

[calebcartwright]

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

[ytmimi]

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 Inferred or Computed to say that "rustfmt, you figure out what the best argument layout is", but maybe calling it Legacy or even Default would also work.

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 Version=Two, and this would change their formatting unexpectedly.

[calebcartwright]

hmm I feel like going with a new option to handle the current behavior seems like the best approach to me

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 Default named variants given their temporal nature and potential for a change to the default at some point down the road, and I'd also prefer to avoid Legacy because of the associated connotations. Perhaps there's a phrase we could derive from the behavior currently prescribed by the Style Guide?

[ytmimi]

Perhaps there's a phrase we could derive from the behavior currently prescribed by the Style Guide?

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 Wraps or Wrap.

Tall could then be used to transition between Horizontal and Vertical, while Wrap(s) is a more gradual transition from Horizontal -> Compressed -> Vertical (aka the current behavior).

[ytmimi]

Maybe we take inspiration from imports_layout, and keep Tall as the default to maintain the old behavior, but add a new HorizontalVertical option, which will always force a vertical layout if a horizontal layout isn't possible.

[calebcartwright]

Maybe we take inspiration from imports_layout, and keep Tall as the default to maintain the old behavior, but add a new HorizontalVertical option, which will always force a vertical layout if a horizontal layout isn't possible.

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:

• The default variant for this new option has to be identical to current behavior so that rustfmt defaults will remain idempotent after we release this
• The variants for this new option that have the same name as varants for fn_params_layout should have the same behavior. E.g. I think it would be too confusing if Tall did one thing for params, while Tall for args did something different

[ytmimi]

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 Tall as is, and we introduce a new HorizontalVertical option to handle the formatting that I mentioned.

Currently this is the code change that is causing the formatting discrepancy in the Tall case:

        // 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 Density::HorizontalVertical that will act as the Tall case in the current implementation above ☝🏼, so that Tall retains its current meaning, and we use this new option to introduce the "breaking change".

        // 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 Tall case, which would also solve the formatting issue that we're seeing here.

[calebcartwright]

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 :100: with you that we'll need a 4th variant, which for the sake of simplicity I'll just call Foo for now. However, I feel like there's some outstanding questions, or at least items where I'm not sure what you're proposing:

• What the associated formatting behavior would be for the Foo and Tall variants
• Whether the default variant would be Foo or Tall

My impression is that your original definition and associated behavior for Tall was logical, consistent, and thus "correct" in the sense of being synonymous with the formatting behavior of the Tall variant for fn_params_layout, but that we needed to implement the new Foo variant that covers whatever the current behavior is, and make Foo the default.

Thoughts?

[ytmimi]

Okay, I can see where there might have been a disconnect. I've been pitching ideas to keep Tall as the default and introduce something else as the "new" behavior that I described. I'm totally on board with making Foo (name pending) the default, and keeping Tall as I previously defined it. We're on the same page 👍🏼.

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!

[calebcartwright]

Would TallWithComments be an accurate (albeit poor) reflection of the behavior associated with our current Foo

[ytmimi]

Some notes on what's going on in the Foo case:

When we call try_overflow_last_item we calculate the initial tactic on line 496-501

https://github.com/rust-lang/rustfmt/blob/23ef4b7ac476714886582865b39d396d615a3901/src/overflow.rs#L496-L501

because there are line comments definitive_tactic returns DefinitiveListTactic::Vertical

https://github.com/rust-lang/rustfmt/blob/23ef4b7ac476714886582865b39d396d615a3901/src/lists.rs#L222-L238

jumping back to try_overflow_last_item we hit this last match arm:

https://github.com/rust-lang/rustfmt/blob/23ef4b7ac476714886582865b39d396d615a3901/src/overflow.rs#L533-L583

And because we have more than one argument and the tactic is Vertical we enter the following if block to see if another list tactic might work, and this is where we end up deciding we should assign a Mixed layout.

https://github.com/rust-lang/rustfmt/blob/23ef4b7ac476714886582865b39d396d615a3901/src/overflow.rs#L552-L581

So from what I can gather the behavior we're seeing in the default case comes down to whether the inputs are simple and fall under the short_array_element_width_threshold. I do find it odd that an option that's intended for use with array elements might change how my function arguments are laid out. Maybe this is worth discussing in a future issue, but part of me wonders if try_overflow_last_item is a little too generic, and maybe we should pass some additional information to it so that we know what node the list items came from. Maybe an enum like expr::RhsAssignKind but for list items?

Regardless, here are some inputs I've been playing with that further illustrate what's going on:

Input all elements are simple, and each element is short.

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 simple, but each element is short.

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 simple, but consectetur is not short because it exceed the default short_array_element_width_threshold=10.

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 max_width normally, but with the comments each line technically does. We also need to set short_array_element_width_threshold=20 since praesent_turpis_diam is 20 chars.

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,
    );
}