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

Reduce newline breaks for chains after closing block braces #5783

Closed
kurtbuilds opened this issue Jun 17, 2023 · 5 comments
Closed

Reduce newline breaks for chains after closing block braces #5783

kurtbuilds opened this issue Jun 17, 2023 · 5 comments

Comments

@kurtbuilds
Copy link

kurtbuilds commented Jun 17, 2023

This code is formatted pleasantly using rustfmt:

stream::iter(keys).for_each_concurrent(10, |key| async move {
    println!("{} -> new", key);
});

However, add a trailing .await, and it is gains a newline and extra level of indentation, which IMO reduces readability:

    stream::iter(keys)
        .for_each_concurrent(10, |key| async move {
            println!("{} -> new", key);
        })
        .await;

Same happens with .unwrap() and such. I'm not sure exactly what formatting decision is happening here, but I think it makes sense for rustfmt to allow chain_width for lines that are the closing braces of another block.

It's quite jarring to have it format differently from this similar code:

    let s = stream::iter(keys).for_each_concurrent(10, |key| async move {
        println!("{} -> new", key);
    });
    s.await;

It's also weird that the behavior seems inconsistent. The first line is nowhere near my max_width (I set it to 200), but it still gets wrapped. This similar code, does not get wrapped, although it puts the chained .await on its own line:

let s = stream::iter(keys);
s.for_each_concurrent(10, |key| async move {
    println!("{} -> new", key);
})
.await;

I looked at a few other types of blocks:

    async {
        println("hi");
    }
    .await;

I'd prefer if the line followed the chain_width directive though, so that lines containing only the closing parens/braces of previous expressions/blocks could be on the same line.

@ytmimi
Copy link
Contributor

ytmimi commented Jun 17, 2023

@kurtbuilds thanks for reaching out.

First I want to highlight that formatting chains is hard and there are a lot of different things rustfmt needs to take into consideration when doing so.

That being said, I believe the formatting you've described above s consistent with the style guide's prescription for formatting chains.

The formatting in your first example is a result of this rule.

If formatting on multiple lines, each field access or method call in the chain should be on its own line with the line-break before the . and after any ?. Each line should be block-indented. E.g.,

let foo = bar
    .baz?
    .qux();

In your second example, things don't get indented because of the small variable name. Since the name is so short you'd have a floating variable if rustfmt moved the chain onto the next line so instead rustfmt combines the root of the chain and the next element onto one line. (note this isn't a problem for the first example because the root of the chain is stream::iter(keys)).

This is a mouthful, but to quote the style guide

If the length of the last line of the first element plus its indentation is less than or equal to the indentation of the second line (and there is space), then combine the first and second lines.

In short that rule is trying to prevent this from happening:

let s = stream::iter(keys);
s
    .for_each_concurrent(10, |key| async move {
        println!("{} -> new", key);
    })
    .await;

Your final example is running into the rule for multi-line elements.

If any element in a chain is formatted across multiple lines, then that element and any later elements must be on their own line

Because the async block is multi-lined, the .await which is part of the chain needs to be placed on its own line.

I understand if you're still unhappy with the formatting, but I hope this explanation has cleared up why rustfmt is doing what it's doing.

@ytmimi ytmimi closed this as not planned Won't fix, can't repro, duplicate, stale Jun 17, 2023
@kurtbuilds
Copy link
Author

That's a disappointing response. If this is closed as won't fix, I'd rather hear an argument about why the style inconsistencies I brought up are warranted, rather than shifting responsibility to the current style guidelines.

What you're telling me (and other users) effectively is, you're okay with inconsistency in the style guide, which to me is bad stewardship.

@calebcartwright
Copy link
Member

I'm sorry to hear you're disappointed, but Yacin is correct, and I think your characterization of "bad stewardship" is unfair and certainly misplaced.

We, the rustfmt team, are responsible for the formatting tool, but we don't own/have authority over the style guide. That's owned by the style team and is a sub function of the lang team.

If you'd like to make a case for a change in the current style prescriptions, e.g. a change/addition/etc. to the relevant rules that could potentially get incorporated into the 2024 style edition, then I'd encourage you to do so in the correct place with the correct team (https://github.com/rust-lang/style-team/issues/new)

(disclaimer: i'm personally also a member of the style team, but that doesn't impact what I said above)

@kurtbuilds
Copy link
Author

I hear what you're saying that here in rustfmt, you're implementing the style guide. My bad stewardship comment was because neither of you have responded to the substance of the issue I filed.

I filed an issue because I found instances where nearly identical code was getting formatted quite differently, which to me is a strong indication of wrong behavior for a code formatter. It was immediately closed as "Not planned" with essentially a comment of "behaving as expected". Given that I, and I would guess also a casual observer, found the behavior emphatically not expected, I would find it more productive to hear something along the lines of "Yes, agreed that's weird, but rustfmt implements the style guide spec. You should file an issue there first to get this fixed." or "While this is weird, rustfmt has to behave this way because of ."

That said - do you agree that this behavior is strange, and should be corrected (which would necessitate filing an issue with the style guide), or is filing such a ticket likely a waste of my time? As a member of the style team, your opinion is more important than mine.

All that said, sorry that it sounds my comment came across as harsh. I appreciate the work that yourselves and all maintainers of the Rust ecosystem do to make Rust better.

@calebcartwright
Copy link
Member

Truth be told, we typically try to avoid getting into sharing personal opinions on what the style "should be" over here in rustfmt.

Part of that is because the rustfmt team has bandwidth struggles already around the things that are in scope for rustfmt, so opining on things that are outside our scope isn't something we really have time for.

Additionally, until fairly recently there was no mechanism in place for being able to evolve the default the style, so even if we did have our own subjective opinions, they were moot since there was nothing that could be done beyond yet-another custom config option to support something non-default.

The recent introduction of style_edition finally provides a mechanism for (functionally) evolving the default style, so in cases like this where someone would like to see a change in the default behavior then we're going to steer them towards the one and only mechanism through which that change could potentially be achieved.

As for my personal 0.02 on your proposed change, I understand your reasoning, but it's not something I care about personally nor that would bother me in my own code. More generally, increasingly over the years I find I've shifted more and more to letting the format tooling do its thing so I don't have to think about it. Sure there's still cases where I'd format something differently by hand, but I can't remember the last time a formatter did something I found too egregious to accept.

As for the likelihood of your proposal being accepted, I can't answer that. It's not something I'd personally support and promulgate, but I wouldn't oppose it either; very indifferent. There's cases that have been made previously which are very serious candidates being considered already for the 2024 style edition, and it's certainly within the realm of possibility that 2024 (or a future style edition) could have different default prescriptions for chains.

Whether or not officially proposing the change is worth your time is, of course, a determination you'll have to make for yourself

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants