Support let guards and let chains

[camsteffen]

Currently blocked on formatting decision: rust-lang/style-team#169

Fixes #4955
Fixes #5177

[camsteffen]

I made an assumption that && let should always wrap. I suppose that can stir a debate and probably should be added to the style guide if we keep that?

[ytmimi]

Thanks for taking the time to implement Let formatting!

I think the implementation of rewrite_let looks good! I do want to make sure that we're not overlooking anything though. Would it be possible to include more tests with more complex patterns and expressions to fully test this out? Including some with Structs, Tuple Structs, fully qualified identifier paths, function calls, chains, etc. Including a test or two for patterns and expressions that are broken up over multiple lines would definitely be helpful, and even including tests where users leave pre and post inline or block comments inside the pattern and expression would be great. I think I'd also like to see similar tests with more complex patterns and expression for if let guards.

I know that's a lot so please let me know if you want help coming up with test cases.

Maybe you can also get some inspiration from tests in the compiler:

https://github.com/rust-lang/rust/tree/master/src/test/ui/rfc-2497-if-let-chains
https://github.com/rust-lang/rust/tree/master/src/test/ui/rfc-2294-if-let-guard

I made an assumption that && let should always wrap. I suppose that can stir a debate and probably should be added to the style guide if we keep that?

I had a similar thought about always forcing newlines, but I think it would probably be best to make an amendment to the style guide before codifying those rules into rustfmt. This is just my own personal preference, but I think I'd argue that your "must wrap" test case would be perfectly fine on a single line.

// must wrap
if xxx
    && let x = x

Lets see where the discussion goes around forcing newlines before removing those changes from the PR.

[camsteffen]

This is just my own personal preference, but I think I'd argue that your "must wrap" test case would be perfectly fine on a single line.

Yeah I agree for that case. But looking at cases where the let drifts farther right, wrapping seems more necessary. Of course rustfmt needs a codified rule to apply to every scenario. We could introduce a magic/configurable number: wrap && let if preceded by more than 12 (?) characters in the same line?

// 7 characters - no wrap
if xxx && let Some(x) = y {

// 13 characters - wrap it
if cond(xxx)
    && let Some(x) = y {

We can also just not introduce new behavior with regard to wrapping in this PR since the change is easily separable. I'm not very opposed to just leaving the behavior as is. Maybe we should get more experience with the feature first?

[ytmimi]

We can also just not introduce new behavior with regard to wrapping in this PR since the change is easily separable.

I think in this case that's probably the best course of action. I think the main goal for this PR should be to introduce formatting for Let expressions, and then we can worry about wrapping rule changes in the future (most likely with an amendment to the style guide).

[camsteffen]

Opened rust-lang/style-team#169 to discuss the wrapping question.

[camsteffen]

Updated the implementation to use rewrite_assign_rhs_with_comments so that comments after = are handled. Also added more tests.

[calebcartwright]

Thanks for this and for the good discussion. Definitely agreed that forced wrapping would at a minimum need to reflect change Style Guide text as well, likely echoed on the rules for control flows and matches.

In the same vein is various discussions elsewhere, we typically only get one shot at introducing the "right" formatting because of the strong desire to avoid introducing breaking formatting changes multiple times. The chains are still too new in my opinion for enough folks to have had enough time to fully formulate how these should be formatted (especially since that formatting will likely remain the same indefinitely), so let's park this for the time being.

[petrochenkov]

Any plans to land this?
The feature is widely useful, and projects using it are accumulating a lot of "formatting debt" now due to rustfmt refusing to format it (see e.g. rust-lang/rust#95262).

[ytmimi]

we typically only get one shot at introducing the "right" formatting because of the strong desire to avoid introducing breaking formatting changes multiple times.

@petrochenkov I think that's the main concern here. We want to make sure we're not overlooking anything.

The chains are still too new in my opinion for enough folks to have had enough time to fully formulate how these should be formatted

The feature has been around for some time since that comment, so maybe we should get more eyes and community engagement on rust-lang/style-team#169 before moving forward with this? It would be great if the authors from those projects could give their input on the format RFC.

@calebcartwright, would another option be to move forward with the proposed changes, but lock them behind a new unstable configuration option, which defaults to false? Something like format_let? What's our stance on breaking formatting changes for unstable options?

[calebcartwright]

The context and constraints haven't changed, and this PR is intentionally still parked and will continue to be so for the foreseeable future.

@petrochenkov - I hope rust-lang/rust#95262 (comment) provides some more context. I appreciate the angle of accumulating unformatted code, but there's nothing particularly novel with these new let constructs in that respect. It's a side effect of the historic disjointed syntax/lang and formatting processes that's happened before and will continue to occur until we can bring those closer together.

However, I'm not going to violate our core operating process just because folks are excited and adopting these newer constructs and want to have them formatted

[calebcartwright]

@calebcartwright, would another option be to move forward with the proposed changes, but lock them behind a new unstable configuration option

Understand the line of thinking, but no I don't see that being viable.

There's a difference between knowing what should be done but having an untested implementation vs. not even knowing what should be done. We're unequivocally in that latter camp until we actually have rules defined. Conditionally rolling out behind a config option could be a consideration if we've got a partial implementation with known gaps (e.g. we know it'll blow up if there's comments), but not as a means of doing some arbitrary, undefined formatting. This follows the same rationale of why we wouldn't want to change the sequence for language features to first do the implementation behind a feature gate, and then follow up the implementation with an RFC

[thomcc]

Let chains are stabilizing soon (it's already stable on nightly, from what I can gather) and it seems very unfortunate if rustfmt doesn't support it when it hits stable. During RustConf, @compiler-errors indicated that they've been using this patch in a local build of rustfmt for their contributions to the compiler. That sounds like this patch works well enough for us to just go with it.

I gather there's some concerns about this not having a formatting RFC? If this is considered a blocker for rustfmt to support new rust-lang syntax, I think we should probably start requiring formatting guidance into RFCs that add new syntax into the language.

[calebcartwright]

I'm going to somewhat preemptively cut off any additional discussion about broader processes because we need to be able to keep this PR focused on the changes to the codebase proposed here.

However, I understand why questions and comments are surfacing so I'll share what is hopefully helpful context around why this is blocked.

First and foremost, we on the rustfmt team do not get to arbitrarily determine what the default style/format is for Rust code (nor do we want to). Those rules are defined in the Rust Style Guide, and rustfmt is the implementing tool. rustfmt cannot apply formatting rules that do not exist, so if the Style Guide lacks prescription for new syntax, then so too does rustfmt. The relevant rules have not been defined yet, and do not even have anything approaching a consensus

Having the Style Guide rules defined is not a nice to have nor optional component, but an unequivocal prerequisite. It's quite similar to making any non-trivial change to the Rust compiler, language, etc. where an RFC is a mandatory up front step.

Additionally, rustfmt output does not have a nightly vs. stable concept when it comes to default formatting. The formatting stability guarantee applies across all channels, so we can't just pick one style one day and then change it something different as more information comes in/more opinions are raised.

Part of the problem is that there is no longer a Style Team (owners of the Style Guide) as a subset of the lang team, and the other is that the rustfmt team typically isn't even aware of syntax changes until very, very late in the game. The relevant folks are aware of these problems and there have been/are continued discussions around how to resolve them (rust-lang/rust#93628, lang team meetings, etc.). However, such solutions are naturally going to be future looking, and things currently at hand (including let exprs and chains) will still have to deal in the realities of the present, and in this case that means "blocked"

[goffrie]

Can this be gated behind an unstable option in the interim? The fact that rustfmt completely skips constructs that use let_chains makes the feature rather unusable.

[ytmimi]

@goffrie I asked the same question #5203 (comment) here, and was already told that it's not a viable option #5203 (comment). The main issues is that we still don't know how to format let-chains. If you'd like to contribute to the formatting discussion you can do so at rust-lang/style-team#169

[ytmimi]

Did another review of the implementation and I think what we currently have is good, but still waiting to get updates from the style team on the formatting rules. However, I am wondering if we should extend what we currently have. For example, I wonder if we should also try to recover comments between the pattern and the = instead of only recovering them between the = and the expr. We recently had an issue filed where that was a problem (#5590).

Could we also add test cases with comments after the = to the match.rs file, and can we add some examples with line comments to both test files please.

After looking at this PR again, I found the following example interesting but not sure if we can format this any better.

    if let XXXXXXXXX {
        xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx,
        yyyyyyyyyyyyy,
        zzzzzzzzzzzzz,
    } = xxxxxxx()
        && let Foo = bar()
    {
        todo!()
    }

I don't want to get too in the weeds on styling, just wanted to call out examples that I found interesting.

I'm assuming this is how the following would be formatted if we had two patterns in a chain that needed to be broken up over multiple lines.

    if let XXXXXXXXX {
        xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx,
        yyyyyyyyyyyyy,
        zzzzzzzzzzzzz,
    } = xxxxxxx()
        && let XXXXXXXXX {
            xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx,
            yyyyyyyyyyyyy,
            zzzzzzzzzzzzz,
        } = xxxxxxx()
    {
        todo!()
    }

Also, not saying we need to refactor any of this just yet, but there might be some common logic that can be shared between ExprKind::Let (let pat = expr) and ast::Local (let pat:ty = expr;)

[ytmimi]

This can be closed now that #5910 was merged.