Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add configuration option for wrapping of assignment right-hand side #4886

Open
wants to merge 1 commit into
base: stbu
Choose a base branch
from

Conversation

molenzwiebel
Copy link

@molenzwiebel molenzwiebel commented Jul 2, 2021

This PR adds a new configuration option called righthand_indentation_strategy (WIP name) that allows configuring of how rustfmt will emit assignment statements if the value does not fit on a single line.

There"s three options. Heuristic will use the same heuristics as the current rustfmt, and is the default. SameLineAsLHS will always attempt to place the first part of the expression on the same line as the left-hand, as long as that does not exceed the maximum width. NewlineIndentRHS does the opposite, instead preferring to always newline-indent the expression if it does not fit entirely on the same line as the lhs.

A short example (where | represents the max width):

// Heuristic		|
let _ = foo().bar;  |
let _ =             |
    foo().bar().a;  |
let _ = foo()       | 
    .bar()          |
    .baz();         |
                    |
// SameLineAsLHS    |
let _ = foo().bar;  |
let _ = foo()       |
    .bar()          |
    .a;             |
let _ = foo()       | 
    .bar()          |
    .baz();         |
                    |
// NewlineIndentRHS |
let _ = foo().bar;  |
let _ =             |
    foo().bar().a;  |
let _ =             |
    foo()           |
        .bar()      |
        .baz();     |

There are better examples showing the exact differences in Configurations.md.


This PR addresses #3514, both the original request (NewlineIndentRHS) as well as the comment by @seanpianka (SameLineAsLHS, which was also the configuration option I wanted myself). As far as I can see the changes should be backwards-compatible and always produce legal formatting.


There are some things I didn"t fully know what to do with, so I"m counting on some guidance from you here. In particular:

Configuration name. I don"t like the current name, since it talks about indentation (we"re only doing different indentation in NewlineIndentRHS). Possibly something like assignment_wrapping_behavior?

TODO Items. There"s several todo items in the code that relate to situations in which we"re only able to format the code in one way even though the user specifically asks for a different way. Consider the following example, with a tab width of 12 (extreme example):

                       |
let a = some_long_name(|1).foo.bar;
                       |

This is how it"d look formatted with both NewlineIndentRHS and SameLineAsLHS:

// NewlineIndentRHS
let a =                |
            some_long_n|ame(1)
                       |.foo
                       |.bar;
                       |
// SameLineAsLHS       |
let a = some_long_name(|
            1          |
)                      |
.foo                   |
.bar;                  |

If the user has NewlineIndentRHS, then we have two options. Either we can fail entirely (since we weren"t able to format within the width they wanted), or we can fall back to the same-line layout that does work. Right now I"ve opted to do the second (and the documentation is written with this approach in mind), but I can also see the argument for failing entirely. The current TODOs in the code refer to this situation (and the inverse, where we"re able to do things on the newline but not on the same line).

Let me know your thoughts! As far as I can see, the changes should be isolated to just assigns and backwards compatible.

@calebcartwright
Copy link
Member

Know it"s been a long time since this was first opened so just wanted to acknowledge having seen it. I"m open to supporting something like this at some point down the road, but also want to be transparent that it"s not something I"ll be able to devote the requisite amount of time/attention to for a while.

This is on one of the hottest paths through the code base and has a large surface through which bugs, breaking formatting changes, etc. could be introduced so it"ll need thorough testing and review which we just won"t have the bandwidth for due to some higher priority items.

I"m happy to leave this open for now though, and can follow up here when we"re in a better position to work on landing something like this.

chachako added a commit to chachako/rustfmt that referenced this pull request Mar 25, 2022
chachako added a commit to chachako/rustfmt that referenced this pull request Mar 25, 2022
chachako added a commit to chachako/rustfmt that referenced this pull request Mar 25, 2022
@ytmimi ytmimi added pr-merge-conflict This PR has merge conflicts that must be resolved p-low labels Jul 27, 2022
@tklajnscek
Copy link

@calebcartwright has there been any movement on this at all since your last post in 2021?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p-low pr-merge-conflict This PR has merge conflicts that must be resolved pr-on-hold
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants