rust-lang/style-team#189: rhs-should-use-indent-of-last-line-of-lhs by johnhuichen · Pull Request #6305 · rust-lang/rustfmt (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation24 Commits2 Checks26 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

johnhuichen

Pull Request Template

Checklist

rust-lang/style-team#189

Changes

Make sure that the rhs is formatted using the indentation of the last line of lhs, as opposed to the first line

Testing

added a test case

@johnhuichen johnhuichen changed the titleIssue-5892: Type alias generic rewrite follows the same rule as trait rust-lang/style-team#189: rhs-should-use-indent-of-last-line-of-lhs

Sep 1, 2024

ytmimi

ytmimi

Comment on lines +2107 to +2132

let new_shape = shape
.block_indent(tab_spaces)
.saturating_sub_width(tab_spaces);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain the call to .saturating_sub_width(tab_spaces). I'm not sure if that's necessary.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without saturating_sub_width,

impl SomeType { fn method(&mut self) { self.array[array_index as usize] .as_mut() .expect("thing must exist") .extra_info = long_long_long_long_long_long_long_long_long_long_long_long_long_long_long; } }

is formatted to

impl SomeType { fn method(&mut self) { self.array[array_index as usize] .as_mut() .expect("thing must exist") .extra_info = long_long_long_long_long_long_long_long_long_long_long_long_long_long_long; } }

and throws

line formatted, but exceeded maximum width (maximum: 100 (see `max_width` option), found: 101)

rather than

impl SomeType { fn method(&mut self) { self.array[array_index as usize] .as_mut() .expect("thing must exist") .extra_info = long_long_long_long_long_long_long_long_long_long_long_long_long_long_long; } }

I think the remaining width is not accounted for properly if I only use block_indent

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain why we're using tab_spaces? What happens if we don't use .saturating_sub_width(tab_spaces), but the rhs was even longer? Does it wrap correctly then?

Part of me wonders if the shape is off to begin with.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the shape that was used before adding extra block_indent is

old shape = Shape { width: 91, indent: Indent { block_indent: 8, alignment: 0 }, offset: 0 }

and the new shape with block_indent but without saturating_sub_width is

new shape = Shape { width: 91, indent: Indent { block_indent: 12, alignment: 0 }, offset: 0

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the new shape is also the shape of the last line of lhs

            .extra_info = long_long_long_long_long_long_long_long_long_long_long_long_long_long_long;

has 12 spaces before .extra_info and .extra_info = long_long_long_long_long_long_long_long_long_long_long_long_long_long_long; is 89 chars. So the remaining width shouldn't be 91, but 87

i.e. when indent was 8 spaces, width = 91, max width = 100, so
width = max - indent - 1

when indent is 12, width = 100 - 12 - 1 =87

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ytmimi I think the above should answer your last question in this PR. Can you take a look?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed explanation. I didn't have time to revisit this PR over the weekend, but I plan on giving this another look later this week

ytmimi

Comment on lines 2110 to 2111

let extra_indent_string = new_shape.to_string(&context.config).to_string();
if last_line.starts_with(&extra_indent_string) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't need to allocate a string with to_string(). We should be able to call as_ref():

let extra_indent_string = new_shape.to_string(&context.config).to_string();
if last_line.starts_with(&extra_indent_string) {
let extra_indent_string = new_shape.to_string(&context.config);
if last_line.starts_with(extra_indent_string.as_ref()) {

ytmimi

ytmimi

@johnhuichen

ytmimi

@johnhuichen

@calebcartwright

Thank you both for the work on this, just noting I've added some labels to indicate the criticality of this one as it ties into 2024 deliverables.

I can help with the review if beneficial

@johnhuichen

Thank you both for the work on this, just noting I've added some labels to indicate the criticality of this one as it ties into 2024 deliverables.

I can help with the review if beneficial

Hey @calebcartwright now I am just waiting for a second look from @ytmimi. Do let me know if you see any changes I should make

ytmimi

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few other test cases that I think we should add:

impl SomeType { fn method(&mut self) { self.array[array_index as usize] .as_mut() .expect("thing must exist") .extra_info = Some(ExtraInfo { parent, count: count as u16, children: children.into_boxed_slice(), }) + Some(ExtraInfo { parent, count: count as u16, children: children.into_boxed_slice(), }); } }

impl SomeType { fn method(&mut self) { self.array[array_index as usize] .as_mut() .expect("thing must exist") .extra_info = long_long_long_long_long_long_long_long_long_long_long_long_long_long_long .as_mut() .another_call() .get_extra_info(); } }

impl SomeType { fn method(&mut self) { self.array[array_index as usize] .as_mut() .expect("thing must exist") .extra_info = long_long_long_long_long_long_long_long_long_long_long_long_long_long_long .as_mut() .another_call() .get_extra_info() + Some(ExtraInfo { parent, count: count as u16, children: children.into_boxed_slice(), }); } }

@calebcartwright

just checking for a status on this one, what's pending?

@ytmimi

just checking for a status on this one, what's pending?

For me, I'd like to confirm the formatting for the extra test cases mentioned in #6305 (comment).

Assuming those get formatted as expected I think we're good to go for assignments.

@calebcartwright we may also need to tweak how we format binary expressions, but I'm not 100% sure. Just going based off of rust-lang/style-team#189 (comment). For example:

impl SomeType { fn method(&mut self) { self.array[array_index as usize] .as_mut() .expect("thing must exist") .extra_info + long_long_long_long_long_long_long_long_long_long_long_long_long_long_long .as_mut() .another_call() .get_extra_info();

         // ^^^ is this the right level of indentation to use for the `rhs` of the binary expression?
}

}

If we need to make updates to binary expression formatting, that might be better to do in a follow up PR.

@calebcartwright

if there's a set of snippets where we'd like style to give a perspective then i'm happy to bring that before the team, and we do have a meeting later today.

Do the last two comments cover all the remaining questions or are there others?

@ytmimi

@calebcartwright

We reviewed this (the formatting improvements provided by this PR) in the t-style meeting yesterday and overall were pleased with the results. There were some thoughts on potential future improvements, and one potential issue/bug observed with the way this proposed implementation formats one of the snippets in

because this input 👇

impl SomeType { fn method(&mut self) { self.array[array_index as usize] .as_mut() .expect("thing must exist") .extra_info = long_long_long_long_long_long_long_long_long_long_long_long_long_long_long .as_mut() .another_call() .get_extra_info() + Some(ExtraInfo { parent, count: count as u16, children: children.into_boxed_slice(), }); } }

is getting formatted without indentation on the ExtraInfo body:

impl SomeType { fn method(&mut self) { self.array[array_index as usize] .as_mut() .expect("thing must exist") .extra_info = long_long_long_long_long_long_long_long_long_long_long_long_long_long_long .as_mut() .another_call() .get_extra_info() + Some(ExtraInfo { parent, count: count as u16, children: children.into_boxed_slice(), }); } }

that's something I'd like to see if we can get addressed ASAP so we can get this merged and into nightly

@calebcartwright

We've been discussing this item quite a bit more in the style team meetings, and reached a decision that we only want to move forward with the indent change around assignment operators (so for our purposes within rustfmt we'd only need to look at assignment and assignmentop expressions).

I had extended what was done here to also directly support binary expressions, and we found that the results were mixed in more complex binary expression trees and felt we'd need some more nuanced rules that better account for expression tree structure and/or accounted for operator precedence rules.

We are hitting crunch time for the edition items, so if we're a bit stalled here then I may grab some commits from this PR branch and/or add co-authors on commits on a PR of my own to ensure we get it through

This was referenced

Dec 5, 2024