rust-lang/style-team#189: rhs-should-use-indent-of-last-line-of-lhs #6305

johnhuichen · 2024-09-01T09:53:12Z

Pull Request Template

Checklist

Confirmed that cargo test passes

Related Issues/PRs

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

src/expr.rs

ytmimi · 2024-09-04T20:13:51Z

src/expr.rs

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


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

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

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.

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

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

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

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 · 2024-09-04T20:20:19Z

src/expr.rs

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


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

Suggested change

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 · 2024-09-04T20:26:49Z

src/expr.rs

+        }
+    };
+    let shape = if context.config.style_edition() >= StyleEdition::Edition2024 {
+        get_lhs_last_line_shape()


I'm wondering if get_lhs_last_line_shape is the right name. Technically what we're doing here is getting the shape used to format the rhs, but it's based on the lhs indentation. If you think you can come up with a succinct name that conveys that meaning that would be great!

in that case I will just call it get_rhs_shape, leave the implementation details out of the naming

ytmimi · 2024-09-04T20:27:50Z

src/shape.rs

@@ -278,6 +278,12 @@ impl Shape {
        offset_indent.to_string_inner(config, 0)
    }

+    pub(crate) fn to_string(&self, config: &Config) -> Cow<'static, str> {


Probably worth adding a doc comment so that we know that this function gives us a single line of indentation.

ytmimi · 2024-09-04T22:52:20Z

src/expr.rs

-use rustc_ast::{ForLoopKind, MatchKind, ast, ptr, token};
+use rustc_ast::{ast, ptr, token, ForLoopKind, MatchKind};


I don't think this was rebased correctly. The imports should not have changed.

I know, I am resolving some merge conflicts. Give me a few minutes

So the mismatch was due to my formatter (Neoformat using neovim) using a different version of rustfmt than ~~nightly~~ the current master

calebcartwright · 2024-09-23T16:15:11Z

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 · 2024-09-25T23:24:38Z

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 · 2024-09-28T19:08:16Z

tests/target/rust-lang/style-team#189.rs

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 · 2024-10-22T17:13:27Z

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

ytmimi · 2024-10-22T17:28:43Z

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 · 2024-10-23T15:32:48Z

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 · 2024-10-23T16:19:31Z

No outstanding questions besides my last two comments.

calebcartwright · 2024-10-24T16:34:02Z

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 · 2024-10-31T02:10:46Z

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

rustbot added the pr-not-reviewed label Sep 1, 2024

johnhuichen force-pushed the rust-lang/style-team#189-rhs-should-use-indent-of-last-line-of-lhs branch from e79db51 to fa8b854 Compare September 1, 2024 09:55

johnhuichen changed the title ~~Issue-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

johnhuichen force-pushed the rust-lang/style-team#189-rhs-should-use-indent-of-last-line-of-lhs branch from 8e01441 to 23bfe0e Compare September 1, 2024 10:34

johnhuichen marked this pull request as ready for review September 1, 2024 10:35

johnhuichen force-pushed the rust-lang/style-team#189-rhs-should-use-indent-of-last-line-of-lhs branch 2 times, most recently from fb84edc to 6da81be Compare September 1, 2024 11:00

ytmimi reviewed Sep 4, 2024

View reviewed changes

src/expr.rs Outdated Show resolved Hide resolved

ytmimi reviewed Sep 4, 2024

View reviewed changes

rust-lang/style-team#189: rhs-should-use-indent-of-last-line-of-lhs

89f1e2b

johnhuichen force-pushed the rust-lang/style-team#189-rhs-should-use-indent-of-last-line-of-lhs branch from 6da81be to 24509fe Compare September 4, 2024 22:47

ytmimi reviewed Sep 4, 2024

View reviewed changes

Update per comments

58ca556

johnhuichen force-pushed the rust-lang/style-team#189-rhs-should-use-indent-of-last-line-of-lhs branch from f389fa1 to 58ca556 Compare September 4, 2024 22:55

ytmimi added pr-follow-up-review-pending and removed pr-not-reviewed labels Sep 9, 2024

calebcartwright added p-high a-2024-edition Style Edition 2024 labels Sep 23, 2024

ytmimi reviewed Sep 28, 2024

View reviewed changes

calebcartwright mentioned this pull request Oct 31, 2024

Tracking Issue for Rust 2024: rustfmt assignment operators rhs indentation rust-lang/rust#132380

Open

4 tasks

calebcartwright removed the pr-follow-up-review-pending label Oct 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rust-lang/style-team#189: rhs-should-use-indent-of-last-line-of-lhs #6305

rust-lang/style-team#189: rhs-should-use-indent-of-last-line-of-lhs #6305

johnhuichen commented Sep 1, 2024 •

edited

Loading

ytmimi Sep 4, 2024

johnhuichen Sep 4, 2024 •

edited

Loading

ytmimi Sep 4, 2024

johnhuichen Sep 4, 2024 •

edited

Loading

johnhuichen Sep 4, 2024 •

edited

Loading

johnhuichen Sep 7, 2024

ytmimi Sep 9, 2024 •

edited

Loading

ytmimi Sep 4, 2024

ytmimi Sep 4, 2024

johnhuichen Sep 4, 2024

ytmimi Sep 4, 2024

ytmimi Sep 4, 2024 •

edited

Loading

johnhuichen Sep 4, 2024

johnhuichen Sep 4, 2024 •

edited

Loading

calebcartwright commented Sep 23, 2024

johnhuichen commented Sep 25, 2024

ytmimi Sep 28, 2024

calebcartwright commented Oct 22, 2024

ytmimi commented Oct 22, 2024

calebcartwright commented Oct 23, 2024

ytmimi commented Oct 23, 2024

calebcartwright commented Oct 24, 2024

calebcartwright commented Oct 31, 2024

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

		use rustc_ast::{ForLoopKind, MatchKind, ast, ptr, token};
		use rustc_ast::{ast, ptr, token, ForLoopKind, MatchKind};

rust-lang/style-team#189: rhs-should-use-indent-of-last-line-of-lhs #6305

Are you sure you want to change the base?

rust-lang/style-team#189: rhs-should-use-indent-of-last-line-of-lhs #6305

Conversation

johnhuichen commented Sep 1, 2024 • edited Loading

Pull Request Template

Checklist

Related Issues/PRs

Changes

Testing

Choose a reason for hiding this comment

johnhuichen Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnhuichen Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

johnhuichen Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ytmimi Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ytmimi Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnhuichen Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

calebcartwright commented Sep 23, 2024

johnhuichen commented Sep 25, 2024

Choose a reason for hiding this comment

calebcartwright commented Oct 22, 2024

ytmimi commented Oct 22, 2024

calebcartwright commented Oct 23, 2024

ytmimi commented Oct 23, 2024

calebcartwright commented Oct 24, 2024

calebcartwright commented Oct 31, 2024

johnhuichen commented Sep 1, 2024 •

edited

Loading

johnhuichen Sep 4, 2024 •

edited

Loading

johnhuichen Sep 4, 2024 •

edited

Loading

johnhuichen Sep 4, 2024 •

edited

Loading

ytmimi Sep 9, 2024 •

edited

Loading

ytmimi Sep 4, 2024 •

edited

Loading

johnhuichen Sep 4, 2024 •

edited

Loading