rust-lang / style-team

Home of the Rust style team
Apache License 2.0
454 stars 56 forks source link

Add let else statements #165

Closed camsteffen closed 1 year ago

camsteffen commented 3 years ago

CC rust-lang/rustfmt#4914

calebcartwright commented 3 years ago

Thank you for writing this up! Just wanted to acknowledge that I've seen this. I have a couple minor thoughts and questions I'll try to write up soon, but largely just want to park it for a while so the community has ample opportunity to weigh in

scottmcm commented 2 years ago

Just to toss in my 2¢ here, I would like to see this usually formatted so that the else is physically proximate to the let to help make it easier to spot, and thus recognize that it's coming when one starts reading the whole construct. Perhaps that would be the else being usually at the start of a line, like let is.

joshtriplett commented 2 years ago

@scottmcm I would prefer to hang the else { anytime it fits, and only break it onto a new line if required.

I think the construct will stand out by having a braced block, and we could make it stand out more by never putting the else { diverge } all on the same line.

scottmcm commented 2 years ago

I think the construct will stand out by having a braced block

I'm not convinced by that. Something like

let // elided
    break;
};

already exists in various forms, like if that's a loop.

Though I guess that gets to the whole semicolon question again. I think it'd stand out more without the ;.

bstrie commented 2 years ago

Hm, I don't agree with the sentiment that we need to go to great lengths to maximize the visibility of the else or the braced block. This feels like the same argument that we fell into when we introduced ?, where much hand-wringing ensued as to it being "too implicit" or "too hard to see", which never became an issue in practice. Since let-else is already more visually distinct than ?, I think we will regret introducing verbose bespoke formatting here.

Thus I second scottmcm. There is nobody clamoring to format this:

let x = match foo {

...like this:

let x =
match foo {

...so I don't see the argument for doing so here. Let's just do what is most consistent, whatever that turns out to be.

camsteffen commented 2 years ago

I believe our available options may be summed up as the following:

  1. It can all be on one line
    let Some(variable) = expression else { return };
  2. The else block must be multiple lines
    let Some(variable) = expression else {
       return
    };
  3. else must always wrap
    let Some(variable) = expression
    else { return };
  4. else must always wrap AND the else block must be multiple lines
    let Some(variable) = expression
    else {
      return
    };

(Note this is considering shorter cases first. Of course you need a fallback for longer lines.)

I would rule out option 4 where let-else is always 4+ lines.

I noticed that if/else statements (not necessarily if/else expressions) always have blocks in multiple lines. If we want to be consistent with if/else in that way, then rule out option 3.

If you agree that option 4 is bad and that the else block should be multiple lines, then you can't always have else be close to let.

joshtriplett commented 2 years ago

@camsteffen That's a good summary.

I would propose option 2: don't let the else hide on the same line, require a multi-line else so that the control flow stands out, but always hang the else.

(Unless the line is so long you have to break it in multiple places, in which case I think I'd prefer a break after the = before a break before the else.)

I'm weakly opposed to 1 (I think the control flow should stand out more than that), and I'm more strongly opposed to 3 and 4. (4 because it's too verbose, 3 because it has no precedent and I think it stands out less than 2.)

camsteffen commented 2 years ago

I also like options 1 and 2 but I lean a little more towards option 1. I think we need to be wary of "unfamiliarity bias" - wanting to add more line breaks to make the unfamiliar feature stand out. Eventually let-else will be just another tool in your bag and then you might be more comfortable with "just put it all one one line". The extra lines could get verbose if you have several let-else lines in a row, and I think that will be somewhat common.

Compare options 1 and 2 at the bottom of the sea ```rust fn hole_in_the_bottom_of_the_sea(hole: Hole) { let Some(log) = hole.find_log() else { return } let Some(branch) = log.find_branch() else { return } let Some(bump) = branch.find_bump() else { return } let Some(frog) = bump.take_frog() else { return } println!( "There's a frog on the bump on the branch on the log in the hole in the bottom of the sea." ); } ``` ```rust fn hole_in_the_bottom_of_the_sea(hole: Hole) { let Some(log) = hole.find_log() else { return }; let Some(branch) = log.find_branch() else { return }; let Some(bump) = branch.find_bump() else { return }; let Some(frog) = bump.take_frog() else { return }; println!( "There's a frog on the bump on the branch on the log in the hole in the bottom of the sea." ); } ```
seritools commented 2 years ago

Friendly reminder that let-else is stabilized in 1.65 (#93628), it would be nice to have support for it not too far from stabilization

compiler-errors commented 2 years ago

Thanks -- the style team is aware of this stabilization, and deciding on formatting syntax for let else is on our radar!

est31 commented 2 years ago

Regarding semicolons I wanted to share an observation with you that I made while studying auto suggestions while working on https://github.com/rust-lang/rust-clippy/pull/8437 : Semicolons are not always optional. For simple cases like return, panic!() or function calls with ! as return type they are, but in other instances they aren't due to how Rust's type checker works. E.g. this, while giving a very legitimate unreachable_code warning, doesn't compile without the semicolon:

fn bar() {
    let _ = () else { if panic!() {}; };
}

FTR, this isn't a new weirdness introduced by let-else though, but already a thing with functions that return !:

fn foo() -> ! {
    if panic!() {};
}

Similarly this also needs a ; for compilation (and also triggers unreachable_code):

fn foo() -> ! {
    if true {
        panic!()
    } else {
        panic!();
        42
    }
}

There are also more examples like [panic!()], (panic!(),). Thankfully this is all unreasonable code, and I couldn't find a case that doesn't trigger unreachable_code, but rustfmt of course has to work in such a setting as well.

yaahc commented 2 years ago

Semicolons are not always optional. For simple cases like return, panic!() or function calls with ! as return type they are, but in other instances they aren't due to how Rust's type checker works.

I just.... wow.


Regarding @camsteffen's summary, I think I'm more in line with cam on this one. I don't feel like the having the else { return }; on the same line is particularly hard to notice so I'm not super worried about making it stand out more. I definitely feel like the brevity concern wins out for me between the two priorities.

I do however seem to have a reaction that I haven't seen mentioned so far, which is that the bare else on a line of it's own looks very jarring to me. I tried looking at other formatting constructs in the style guide and couldn't find any other constructs that we format similarly. I'm not opposed to the idea that I might just get used to it with time but I do want to throw out an alternative suggestion for when the the initializer expression is too long to fit in one line. I think it might help to ensure that we're consistent with if { ... } else { ... } and always have a leading } before the else when it's broken out to it's own line, which in my mind clearly indicates that the expression is a piece of a compound expression from the previous lines.

I think this amounts to mostly liking the current wording of the RFC except wanting to unconditionally insert a block around the initializer expression whenever the else block needs to be broken out into it's own line, as well as potentially removing the last section (not sure if it would still be necessary at that point).

nvm: https://github.com/rust-dev-tools/fmt-rfcs/pull/165#issuecomment-1298010644

digama0 commented 2 years ago

@yaahc IIUC that syntax is specifically disallowed in order to prevent it from looking too much like the trailing half of an if-else block. It would have to be:

let Some(1) = ({
    some_particularly_long_initializer_expression_that_forces_it_to_wrap
}) else { return };
Alexendoo commented 2 years ago

To adapt an example from the clippy::needless_return tests, here's one that requires the semicolon but doesn't trigger unreachable_code:

use std::cell::RefCell;

pub fn temporary_outlives_local() -> String {
    let () = () else {
        let x = RefCell::<String>::default();
        return x.borrow().clone(); // without the semi: `x` does not live long enough
    };

    String::new()
}
traviscross commented 2 years ago

As one data point, I've been formatting my let-else blocks exactly as is described in @camsteffen's draft.

Significantly, I did it this way, in every detail, before reading his draft. This suggests that his draft may be the most consistent and expected way to format this construct in Rust. I'd be happy if rustfmt did it this way for me automatically.

Regarding suggestions that would have us format this construct more verbosely, perhaps we should remember that let-else's purpose is to be space-saving syntactic sugar. If

    let Some(thing) = x.get_thing_base()
    else {
        return;
    };

takes the same number of lines as:

    let thing = match x.get_thing_base() {
        Some(x) => x,
        _ => return,
    };

then we may wonder whether there was much point in adding let-else to the language.

andersk commented 1 year ago

Semicolons are not always optional.

Conversely, here’s an example where the type checker forbids a semicolon, that I expect will be pretty common (until yeet arrives to save the day?):

fn main() -> Result<(), Box<dyn std::error::Error>> {
    let _ = () else {
        Err("oops")? // here
    };
    Ok(())
}

With a semicolon at // here, the type checker infers Result<(), &str> rather than Result<!, &str>. (I think this is related to the fallback issues that have been preventing stabilization of !: rust-lang/rust#35121, rust-lang/rust#79366. This was later filed as rust-lang/rust#106357.)

camsteffen commented 1 year ago

Hmm how does that compile? I don't know what is the Ok variant type for Err("oops").

andersk commented 1 year ago

The else clause must have type !, so without the semicolon, we can infer Err("oops")?: ! and Err("oops"): Result<!, &str>.

(Perhaps it’s surprising to see ! inferred in stable code, but if you split it up like

    let _ = () else {
        let e = Err("oops");
        let q = e?;
        q
    };

you can see e: Result<!, &str> and q: ! confirmed by rust-analyzer.)

calebcartwright commented 1 year ago

After much discussion, the Style Team has decided to go with option one (as described in https://github.com/rust-lang/fmt-rfcs/pull/165#issuecomment-1060973041), though with the inclusion of some additional considerations in order to be consistent with the current Style Guide edition.

Specifically, the else branch will be formatted similarly to the else block of an if-else expression. This means that whether the entire the entire construct can be formatted across one line will be determined by a "short" heuristic (which rustfmt will make configurable) as well as the contents contained within the else branch, including the load-bearing behavior of the semicolon.

As for next steps:

Thanks to everyone that participated in the discussion!

calebcartwright commented 1 year ago

For awareness the PR is up adding the rules to the style guide in https://github.com/rust-lang/rust/pull/107312

The thrust of the text is largely the same as what was proposed and discussed here given the aforementioned decision by the style team, with some adjustments to reuse existing language in the style guide via links and/or inline reuse