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

Add let else statements #165

Closed
wants to merge 2 commits into from
Closed

Conversation

camsteffen
Copy link

guide/statements.md Outdated Show resolved Hide resolved
guide/statements.md Outdated Show resolved Hide resolved
guide/statements.md Outdated Show resolved Hide resolved
```

If the `else` and opening brace do not fit on the same line as the initializer,
break before `else`. If a line begins with `else`, it should be indented at the same level as `let`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
break before `else`. If a line begins with `else`, it should be indented at the same level as `let`,
break before `else`. If a line begins with `else` (potentially preceded by closing braces or parentheses or similar), it should be indented at the same level as the matching `let`,

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "line begins with else" and "preceded by..." are mutually exclusive. Perhaps another paragraph is needed for that case.


If the `else` and opening brace do not fit on the same line as the initializer,
break before `else`. If a line begins with `else`, it should be indented at the same level as `let`,
and the block should be on the same line if it fits.
Copy link
Member

@joshtriplett joshtriplett Oct 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
and the block should be on the same line if it fits.
and there should always be a newline after the opening brace so that the block always appears on a subsequent line. This calls attention to the `else` block attached to the `let`, making it easy to visually distinguish a `let`-`else` from a plain `let`.

Additional justification: this only applies when the else { doesn't fit on the line with the initializer.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This calls attention to the else block

The style guide does not need to say "why" right?

I think what you are suggesting here should not be a dependent clause of "if a line begins with else". How about this?

If the else is not on the same line as let, the else block should not be written in one line.

I'm assuming that we can lean on the "blocks" section of the guide and so "not written in one line" is specific enough.

Regardless of how it's worded, I agree, this is more consistent with if/else.

let MyStruct { foo } = ({
statement;
fun()
}) else { return };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}) else { return };
}) else {
return
};

}) else { return };

let Some(1) = opt
else { return };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
else { return };
else {
return
};

guide/statements.md Outdated Show resolved Hide resolved
guide/statements.md Outdated Show resolved Hide resolved
Co-authored-by: Josh Triplett <josh@joshtriplett.org>
@calebcartwright
Copy link
Member

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
Copy link
Member

scottmcm commented Mar 2, 2022

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
Copy link
Member

@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
Copy link
Member

scottmcm commented Mar 5, 2022

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
Copy link

bstrie commented Mar 7, 2022

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
Copy link
Author

camsteffen commented Mar 7, 2022

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
Copy link
Member

@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
Copy link
Author

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
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."
    );
}
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
Copy link

seritools commented Oct 29, 2022

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
Copy link
Member

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

@est31
Copy link
Member

est31 commented Oct 29, 2022

Regarding semicolons I wanted to share an observation with you that I made while studying auto suggestions while working on rust-lang/rust-clippy#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
Copy link
Member

yaahc commented Nov 1, 2022

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: #165 (comment)

@digama0
Copy link

digama0 commented Nov 1, 2022

@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
Copy link
Member

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
Copy link

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
Copy link

andersk commented Nov 28, 2022

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
Copy link
Author

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

@andersk
Copy link

andersk commented Nov 28, 2022

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
Copy link
Member

After much discussion, the Style Team has decided to go with option one (as described in #165 (comment)), 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:

  • The Style Team will close this PR because the Style Guide text now lives in a different location
  • The Style Team will add detailed language to the Style Guide detailing the precise formatting rules
  • Those prescribed formatting rules will be implemented within Rustfmt
  • The Rustfmt Team and Style Team will collaborate and identify a communication plan to inform the community that rustfmt will start formatting let else statements starting in an upcoming version of rustfmt

Thanks to everyone that participated in the discussion!

@calebcartwright
Copy link
Member

For awareness the PR is up adding the rules to the style guide in rust-lang/rust#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

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

Successfully merging this pull request may close these issues.