let-else formatting tracking issue

[calebcartwright]

Using this as our own formatting tracking issue given that the RFC for let-else statements has been merged. This issue will be used to track the overall implementation within rustfmt whereas discussion on the default style should occur in the Rust Style Guide repo that defines the formatting spec rustfmt applies (link to PR/issue in style guide to-be-added)

Broadly this work will entail:

• Default formatting/style rules codified in the Style Guide - https://github.com/rust-lang/rust/blob/master/src/doc/style-guide/src/statements.md#else-blocks-let-else-statements
• Barebones syntax handling (emit original span contents, this will be handled by the RFC implementer in rustc)
• Update rustfmt to apply Style Guide rules once finalized (Implement let-else formatting #5690)
• Figure out handling and impact of nested comments (Implement let-else formatting #5690)
• Review/apply any relevant existing config options, consider any potential new options (work is done, will shortly follow merge of Implement let-else formatting #5690 - "short" config option for let-else statements? #5684)

Edit - Updated to outline the high level steps and differentiate between style rules and rustfmt implementation

[joshtriplett]

As discussed in Zulip, we probably want to format this using the same rules for let, with the subsequent else { diverging block }; formatted in one of two ways, depending on the contents of the diverging block.

For short, one-statement diverging blocks (e.g. return x, yeet x, panic!("x")):

let pat = expr else { block };

For blocks that don't fit, or multi-statement blocks:

let pat = expr else {
    block
};

If the let pat = expr portion doesn't fit, we should wrap it just as we would for any other let.

If let pat = expr fits on one line, but let pat = expr else { does not, I don't think we should wrap else { alone to the start of the next line, because that formatting could make the let statement look like it doesn't have an else. We should instead wrap as we would if else { was part of expr and expr didn't fit on the line.

[digama0]

If let pat = expr fits on one line, but let pat = expr else { does not, I don't think we should wrap else { alone to the start of the next line, because that formatting could make the let statement look like it doesn't have an else. We should instead wrap as we would if else { was part of expr and expr didn't fit on the line.

Does that mean formatting like this?

let pat = foo
    .bar()
    .bar()
    .bar()
    .bar()
    .bar() else {
    block();
    return more
};

It works well when expr ends in a bunch of close parens and braces that are on their own line, but if the last line of expr is hanging because it contains other stuff you lose the dedented line effect which makes it hard to find the else { in the middle.

[joshtriplett]

Good point. In the circumstance, if we're wrapping the expression anyway, I wonder if we should wrap the else to the start of the following line (outdented to match the let).

[digama0]

If the expression needs more than one line, but the last line is not indented (in particular because it's ({ block }) and }) has been placed unindented on the last line), the else { should go on the same line, as in

let pat = ({
    block();
    bar
}) else {
    return more
};

[ayosec]

Please consider putting the else keyword in its own line. So, instead of:

let pat = expr else {
    block
};

It is:

let pat = expr
else {
    block
};

Motivation

In the current version of Rust there are some constructs that follow a structure similar to this:

<keyword> <bindings> {
    <block>
}

Here, the bindings are only visible in the block right after the keyword. Some examples:

if let Some(x) = map.get(key) {
    // x only lives here
}

for n in 0..10 {
    // n only lives here
}

while let Some(item) = iter.next() {
    // item only lives here
}

let-else statements don't follow this structure. For example, given this code:

let Some(data2) = data else {
    return Err(MissingData);
}

At first sight, it looks that data2 should be available inside the block in the next line of the let keyword. Of course, it is obvious that this is not the case when you see the else just before {. However, I think that the code is easier to understand if it is written as this:

let Some(data) = data
else {
    return Err(MissingData);
}

Since else is in its own line, it is easier to see that the block belongs to the else part of the let-else construct.

It is also closer to how if-else blocks are written (if and else keywords are written at the beginning of the line).

Similar to if-else, short expressions could be written in a single line.

let x = if y { 0 } else { 1 };

let Some(x) = get_x() else { return };

Examples from the RFC

1. Original:

   let Action::Register {
       actor: String,
       x: Vec<String>
       y: u32,
       z: String,
   } = event.action().clone() else {
       // RFC comment: Directly located next to the associated conditional.
       return Err(eyre::eyre!("must begin with a Register action"));
   };

   Proposed:

   let Action::Register {
       actor: String,
       x: Vec<String>
       y: u32,
       z: String,
   } = event.action().clone()
   else {
       // RFC comment: Directly located next to the associated conditional.
       return Err(eyre::eyre!("must begin with a Register action"));
   };

2. Original:

   let GeoJson::FeatureCollection(features) = geojson else {
       return Err(format_err_status!(
           422,
           "GeoJSON was not a Feature Collection",
       ));
   };

   Proposed:

   let GeoJson::FeatureCollection(features) = geojson
   else {
       return Err(format_err_status!(
           422,
           "GeoJSON was not a Feature Collection",
       ));
   };

3. Original:

   let Some(a) = an_option else {
       // Called if `an_option` is not `Option::Some(T)`.
       // This block must diverge (stop executing the existing context to the parent block or function).
       return;
   };

   Proposed:

   let Some(a) = an_option
   else {
       // Called if `an_option` is not `Option::Some(T)`.
       // This block must diverge (stop executing the existing context to the parent block or function).
       return;
   };

Examples from comments

From: #4914 (comment)

Original:

let pat = foo
    .bar()
    .bar()
    .bar()
    .bar()
    .bar() else {
    block();
    return more
};

Proposed:

let pat = foo
    .bar()
    .bar()
    .bar()
    .bar()
    .bar()
else {
    block();
    return more
};

[calebcartwright]

Appreciate all the input about the default formatting, but this really isn't the place to hammer that out. Ultimately the rules will need to be codified in the Rust Style Guide (and if someone wanted to open a PR over there for me that'd be great!) and that'll be a better place to discuss/debate the specifics.

rustfmt typically has to take an iterative approach to handling new syntax, which typically starts with just spitting back out whatever was there before (via the associated span). Once the rules have solidified in the style guide we then update rustfmt with the corresponding formatting implementation. However, we then also have to account for things like comments, interactions with configuration options, and other pieces that aren't really captured in the style guide and discussions about preferred default formatting.

I've opened this issue to track the bucket of work specific to implementation within rustfmt, but do not want this issue to be used for the style rules.

[camsteffen]

Opened a style guide PR at rust-lang/style-team#165

[est31]

I've found an example where rustfmt creates non-fixpoint output: #5213

[calebcartwright]

I've found an example where rustfmt creates non-fixpoint output: #5213

Thanks for the other report and cross posting, though worth noting it's not actually related. rustfmt isn't touching let-else's yet, and in a macro context that leave-alone behavior is hitting a separate, unrelated bug that's been around for quite a long a while.

[nikomatsakis]

My two cents from having used this for some time:

I'm generally in favor of making the else case easy to spot. It's very handy and I'm sure it will be widely used, but it's still a divergence from most expressions, which cannot break out. Moreover, the else keyword on its own requires context to disambiguate (is it part of an if or not?) and that context can be a bit hard to spot.

In my own code, I've always been happy to have the "body" of the else on a following line:

let Some(x) = foo else {
    return;
}

that makes it stand-out to me. On the other hand, I recently encountered a long expression

let Some((aaaaaaaaaaaaa, bbbbbbbbbbbbbbb, cccccccccccccc)) = 
    foo else {
        return;
    };

and that looked very bad to me, so I reformatted it to

let Some((aaaaaaaaaaaaa, bbbbbbbbbbbbbbb, cccccccccccccc)) = 
    foo
else {
    return
};

while the else on its own line is "surprising" to me, it's surprising in the same way that where being aligned with fn was surprising, and I've found that I quite like that convention over time. I suspect that just always putting the else on the next line will grow on me in the same way, as it makes the let-else combination very visually obvious. In that context, I think that a short block need not be further indented. For example:

let Some(x) = foo
else { return; }

I'm of mixed minds about very short cases like this:

let Some(x) = foo else { return; }

I can imagine scenarios where that compact formatting would be useful, but I think it'd be fragile, and I feel like I might come to value knowing that the else is always indented underneath the let (much as it lines up with an if for multi-line if statements).

UPDATE: Reading over my comment, I'm finding myself more and more of the opinion that the else should always be on its own line. It's still quite compact.

[nikomatsakis]

I'm generally in favor of making the else case easy to spot. It's very handy and I'm sure it will be widely used, but it's still a divergence from most expressions, which cannot break out.

To expand on this: I personally find that "scan for early return of some kind" is a fairly common thing that I do when debugging. I greatly appreciate ? for this reason (although I am forced to search for it sometimes, as it can be quite "quiet") and I feel like having else easily spotted will be helpful.

[scottmcm]

In that context, I think that a short block need not be further indented. For example:

let Some(x) = foo
else { return; }

One thing I'll add to this, @nikomatsakis, is that I find myself writing that pretty often, but it doesn't actually compile. It needs to be

let Some(x) = foo
else { return; };

So I keep wondering if we should make a language change to not require that final ; (like we usually don't require them on other block-terminated production).

Or at least ;}; looks kinda weird to me, so I get tempted to write

let Some(x) = foo
else { return };

But I think that's against the normal rustfmt rules for blocks.

[calebcartwright]

Friendly reminder that this is the implementation tracking issue, and discussion around style rules should be directed towards the fmt-rfcs/style guide repo (specifically in the open PR rust-lang/style-team#165)

[leana8959]

Hello,

Would it be possible for rustfmt to be not try to format let-else blocks for the moment?
I noticed that having trailing spaces anywhere in a if-else block would stop rustfmt from formatting anything else in the same file.

Thanks :)

[calebcartwright]

@leana8959 - rustfmt does not attempt to format let else statements whatsoever; it currently emits whatever the developer wrote, exactly as they wrote it. The only other alternative would be for rustfmt to delete those statements, which obviously isn't viable.

The erroring behavior is fairly core to rustfmt and isn't really something we can/will change, especially now that the Style Team exists, formatting rules have been codified, and we've nearly finished the formatting implementation.

If you/the developer have left trailing whitespace somewhere within a let else statement then your options are to either remove the whitespace manually/via editor features, or to instruct rustfmt to skip the statement altogether, e.g. https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=7398f01f51aa35f39baf8d49df07efdc

[OddCoincidence]

Just to add a data point here: I was just reviewing some code that used let-else, and because the else didn't stand out I misread it as an if let. So while it does seem somewhat ugly, I'm finding myself in favor of always putting the else on its own line as suggested above.

[plazmoid]

Any progress?

[Braymatter]

For reference, this is what rustfmt is doing to my bevy app's let-else statements; it's bad enough that we're considering just not using let-else statements at all.

Before:

After:

[calebcartwright]

I'm locking the issue because its only remaining utility is for the rustfmt team to inform subscribers when there's an update, and any additional comments from others are only going to add noise for those subscribers.

Again, I understand the frustration and appreciate the desire for formatting. It's unfortunate we found ourselves in a situation without a clearly designated team that owned the decision around what the style was supposed to be. In the absence of an operable time machine there's really nothing that can be done about the past. However, the root cause has been resolved, and we're working through the backlog of things that had built up in the absence of that resolution.

@Braymatter re:

For reference, this is what rustfmt is doing to my bevy app's let-else statements; it's bad enough that we're considering just not using let-else statements at all.

Before:

After:

This isn't really the right place for this, but it looks like your main issue is that you've got your editor configured to use a 2-space indent, while you're running rustfmt with the default 4-space indent. I'd suggest it's unwise to have different tools dueling on that front and encourage you to update one or the other to be consistent. Furthermore, it's that 2-space editor setting that's resulting in the offset for your let-else (rustfmt is maintaining your original 2 space indent there while it fixes the indenet elsewhere). let-else has to be manually formatted at the moment, so you can either leave your "after" as is, or manually apply the indent. Running rustfmt over the result will be idempotent either way.

[calebcartwright]

It was brought to my attention earlier that some of the latest status updates on this, while linked within the thread, weren't as clear/apparent as they could be.

As such I've updated the checkbox items with those links and commentary to make the current status easier to find.

I will also add that we are tantalizingly close, and ask that folks bear with us just a bit longer.