RFC: let-else statements by Fishrock123 · Pull Request #3137 · rust-lang/rfcs ·...

Copy link

burdges commented 28 days ago •

It's be more readable if else were replaced by otherwise, or really anything besides else since rust lacks then.

lebensterben commented 28 days ago •

The example

if let Some(a) = x {
    if let Some(b) = y {
        if let Some(c) = z {
            // ...
            do_something_with(a, b, c);
            // ...
        } else {
            return Err("bad z");
        }
    } else {
        return Err("bad y");
    }
} else {
    return Err("bad x");
}

can be written as

match (x, y, z) {
  ((Some(a), Some(b), Some(c)) => do_something_with(a, b, c),
  ((Some(a), Some(b), _)) => return Err("bad z"),
  ((Some(a), _ , _)) => return Err("bad y"),
  _ => return Err("bad x"),
}

which doesn't seem bad comparing to the proposed

let Some(a) = x else {
    return Err("bad x");
};
let Some(b) = y else {
    return Err("bad y");
};
let Some(c) = z else {
    return Err("bad z");
};
// ...
do_something_with(a, b, c);

The real-world example can just use if-let and I don't understand

consider how the following code would look without let-else

burdges commented 27 days ago

You meant

match (x, y, z) {
  ((Some(a), Some(b), Some(c)) => do_something_with(a, b, c),
  ((_, _, None)) => return Err("bad z"),
  ((_, None, _)) => return Err("bad y"),
  _ => return Err("bad x"),
}

but simpler competition exists, like

let a = if let Some(a) = x { a } else { return Err("bad x"); };
...

If you're doing anything with Result conversions like Option then idiomatic Rust code goes

let a = x.ok_or("bad x) ?;
let b = y.ok_or("bad y) ?;
let c = z.ok_or("bad z) ?;
do_something_with(a, b, c);

do_something_with(
    x.ok_or("bad x) ?,
    y.ok_or("bad y) ?,
    z.ok_or("bad z) ?
)

I'd say remove Option entirely from the RFC to avoid confusion. Another enum like Cow works better, maybe combined with Vec::reserve instead of returns because otherwise

let owned_or = |moo,err| if moo.is_owned() { Ok(moo.into_owned()) } else { Err(err) };
do_something_with(
    x.owned_or("bad x) ?,
    y.owned_or("bad y) ?,
    z.owned_or("bad z) ?
)

In reality, this shortcut would exist exclusively for custom enums and should probably never be used with anything from std.

@burdges
let a = if let Some(a) = x { a } else { return Err("bad x"); }; is not simpler.
When it's properly formatted it takes more lines than mine example, which is bad if concise source code is an objective.

Option doesn't cause any particular confusion to me. What I'm confused is why the proposed let-else is considered better than if-let and match.

If there's one refutable pattern to match, then if-let is good enough and let-else adds no utility to it.
If there are multiple refutable patterns to match, just use match and you will get concise and readable code.

Fishrock123 commented 27 days ago •

match (x, y, z) {
  ((Some(a), Some(b), Some(c)) => do_something_with(a, b, c),
  ((Some(a), Some(b), _)) => return Err("bad z"),
  ((Some(a), _ , _)) => return Err("bad y"),
  _ => return Err("bad x"),
}

That basic example may need to be improved a bit. Often times in practical use there is other code between the previous and next if statement, which cannot be represented by this match example. Keep in mind no example can perfectly represent all the possibilities in real-world code, which is why there are multiple examples.

If there's one refutable pattern to match, then if-let is good enough and let-else adds no utility to it.

It can in fact add practical utility. Please see the practical refactor section of the RFC.

If you're doing anything with Result conversions like Option then idiomatic Rust code goes

This is already covered in the RFC in more detail. See the alternatives section which notes that ok_or/ok_or_else + ? is great for Result/Option but untenable for other enums, particularly non-stdlib enums.

Also consider the following case

enum Fruit {
    Apple(u32),
    Banana(u32),
}

struct AppleJuice;

fn make_apple_juice(fruit: &Fruit) -> Result<AppleJuice, String> {
    match fruit {
        Fruit::Apple(v) => Ok(AppleJuice),
        Fruit::Banana(v) => Err(String::from("Require apple, found %s bananas", v)),
    }
}

This cannot be written with the proposed let-else construct or if-let construct.
So usage of let-else is really limited.

Fishrock123 commented 27 days ago •

enum Fruit {
    Apple(u32),
    Banana(u32),
}

Enums with multiple value-bearing variants do not necessarily need or want other variant values to be accessed in case of a failed match. The examples section does cover this. Also if it helps, the GeoJson enum used in the one example is this: https://docs.rs/geojson/0.22.2/geojson/enum.GeoJson.html

lebensterben commented 27 days ago •

@Fishrock123

It can in fact add practical utility. Please see the practical refactor section of the RFC.

This refactor is not a fair comparison.
How's

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

any better than

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

It merely removed two lines. Does this justify adding the new constuct let-else?

Fishrock123 commented 27 days ago •

Does this justify adding the new constuct let-else?

let x = if let Pattern(x) = y { x } else {
    // return or something
};

Any real-world code I've seen uses match over the if let such as the above. The if let like this reads especially poor.
It would be better if people were not likely to use the same variable name in the intermediate binding, but all examples that exist seem to do that.

That being said, yes I do think the few lines of elimination bring enhanced clarity and newcomer discoverability, especially for something which can be pretty common, and which desugars into something pretty simple without much complexity.

Quoting myself from the RFC:

While that refactor was positive, it should be noted that such alternatives were unclear the authors when they were less experienced rust programmers, and also that the resulting match code includes syntax boilerplate (e.g. the block) that could theoretically be reduced today but also interferes with rustfmt's rules:

joshtriplett commented 27 days ago

@lebensterben wrote:

can be written as

match (x, y, z) {
  ((Some(a), Some(b), Some(c)) => do_something_with(a, b, c),
  ((Some(a), Some(b), _)) => return Err("bad z"),
  ((Some(a), _ , _)) => return Err("bad y"),
  _ => return Err("bad x"),
}

This is assuming that the actual expressions involved in y and z (which are examples) don't involve any previous bindings. If y is actually f(x) this doesn't work. And that's a common pattern: refutably bind x or fail, then refutably bind y or fail...

nielsle commented 27 days ago

The best use case for let-else is probably something like the following (where foo and bar are functions with side effects)

let Some(a) = foo() else {
    log("Bad foo");
    return Err("foo failed");
};

let Some(b) = bar() else {
    log("Bad bar");
    return Err("bar failed");
};

This example can also be rewritten using if-else or map_err()? or two separate match statements, but admittedly the version with let-else is slightly prettier.

burdges commented 27 days ago

Does this justify adding the new constuct let-else?

I think only the word else adds confusion here. If another word gets used, ala let x = y otherwise { z };, then one conceptualizes this roughly like let x = y? ala let x = y otherwise { return None; }; or whatever.

Aaron1011 commented 4 days ago

Implementing things like this as library features or macros is never ideal, even assuming they look nice (which is rarely the case compared to a language feature).

Unless there's a compelling reason to bake this into the language itself, I think it's better to use a macro. Doing so avoids adding additional complexity to the core language, which already has a large (and still growing) number of features.

Enforcing that something is syntactic sugar is an implementation detail, so to speak. I don't think it is a good reason for/against implementing something like this as a macro or not.

Doing so makes it clear that this isn't adding anything new to the language - it's not a new concept to learn, it's just a more convenient packaging of existing features. The matches! macro is a good example of this - if you've understood match statements and patterns, then you have everything you need to understand matches!.

that are outside the scope of what I would consider syntactic sugar

Maybe I'm using the term 'syntactic sugar' in a different way. I meant that the feature is not adding anything new to the language - ignoring hygiene, you can manually 'expand' the macro yourself by writing a let (pat) = match expr { ... } yourself, which will have exactly the same effect.

kennytm commented 4 days ago •

@Aaron1011 Any syntactical solutions (macro_rules or proc macros) are never going to perfectly distinguish between fresh variables or constants. Your crate does not work on this, both None and n are considered Pat::Ident here.

let (None, n) = (None::<u32>, 2) else { panic!() };
assert_eq!(n, 2);

Aaron1011 commented 4 days ago •

@kennytm: That's true. However, in your example, it can be worked around by making None look like a struct pattern:

let_else!(let (None {}, n) = (None::<u32>, 2) else { panic!() });

This trick also works for matching on empty structs (e.g. the pattern Foo {} will match against an instance of struct Foo;).

This is certainly not ideal, but I don't think it will be a significant issue in practice. Adding a new language feature seems like a very heavy hammer to work around this kind of edge case in an otherwise equivalent proc-macro.

kevincox commented 4 days ago

That is a huge footgun that will be hard to debug. Rust it all about making it hard to do the wrong thing. This is a very usual thing that you will need to remember for this not-common case.

kennytm commented 4 days ago

@Aaron1011 The workaround doesn't work for an actual constant:

const MAGIC: u32 = 3;

// ...

let (MAGIC, n) = (3, 5) else { panic!() };
// // error[E0574]: expected struct, variant or union type, found constant `MAGIC`
// let_else!(let (MAGIC {}, n) = (3, 5) else { panic!() });
// //             ^^^^^ not a struct, variant or union type
assert_eq!(n, 5);
let (MAGIC, _m) = (4, 5) else { return };
panic!();

Yes together with #2920 we could use

let_else!(let (const { MAGIC }, n) = (3, 5) else { panic!() });

but this is a mismatch from other existing pattern usage:

if let (MAGIC, n) = (3, 5) {   // no `const {}` needed here
    assert_eq!(n, 5);
} else {
    panic!();
}

ojeda commented 4 days ago •

Unless there's a compelling reason to bake this into the language itself, I think it's better to use a macro. Doing so avoids adding additional complexity to the core language, which already has a large (and still growing) number of features.

Non-language features have worse syntax and may have longer compilation times and worse diagnostics.

C++ had a few of these mistakes too, e.g. std::variant and std::tuple.

In general, the fact that something can be implemented as a macro (or as a library in general) does not mean it should be.

Doing so makes it clear that this isn't adding anything new to the language - it's not a new concept to learn, it's just a more convenient packaging of existing features. The matches! macro is a good example of this - if you've understood match statements and patterns, then you have everything you need to understand matches!.

A proc macro is as hard to understand as the equivalent language feature -- in fact, they are harder, because you need at the very least the wrapping macro.

I am not sure why you say proc macros cannot introduce new concepts -- they definitely can, e.g. proc macros that parse entirely new syntax and DSLs, proc macros that use external files as input, proc macros that generate pinning code or vtables...

Maybe I'm using the term 'syntactic sugar' in a different way. I meant that the feature is not adding anything new to the language - ignoring hygiene, you can manually 'expand' the macro yourself by writing a let (pat) = match expr { ... } yourself, which will have exactly the same effect.

Yes, but even if we include complex transformations, something being a proc macro does not prove it is syntactic sugar (see previous point).

I'm about 75% sure this should be added. I don't think any more hypothetical examples or abstract arguments will change that at this point. I need to experience using the feature and reading real code before I can make a final judgement of whether the feature carries its weight. But I am 99% sure that this RFC is the best version of the feature we can come up with.

Fishrock123 commented 4 days ago •

in regards to comments saying it could just be a macro:

Yes, technically you are correct. In fact, the prior art section of the RFC mentions the ‘guard‘ crate which does exactly this. See https://github.com/rust-lang/rfcs/pull/3137/files#diff-66dec22a01118a5d83cc226af3c8c32342a0093d60502b30b9b8f2a263e319d5R492

This should probably also be mentioned in the alternatives section. The macro crate for this is not in widespread use, much less so than the early return pattern. I will not be using it in my projects because the gains here are more in language cohesiveness and newcomers anticipating a counterpart to the already in-language if-let so that they can apply good patterns out of the box with the language’s assistance.

jhpratt commented 3 days ago •

Postfix macros appear to be generally desired and @nikomatsakis has indicated he'd like to move forward with the proposal as recently as last month. As far as I can tell, this would wholly subsume the simple use cases while still leaving the use case requiring else if when let chains are stabilized (which imo would be clearer as match statements). While I think this proposal would be useful in its own right, I believe it's important to bring this possibility up.

From the example in the RFC:

let Some(x) = xyz else {
    info!("x was bad");
    return Err("bad x");
};
let Some(y) = x.thing() else {
    info!("y was bad");
    return Err("bad y");
};
let Some(z) = y.thing() else {
    info!("z was bad");
    return Err("bad z");
};
// ...
do_something_with(z);
// ...

could be written with a postfix macro as

let x = xyz.unwrap_or! {
    info!("x was bad");
    return Err("bad x");
};
let y = x.thing().unwrap_or! {
    info!("y was bad");
    return Err("bad y");
};
let z = y.thing().unwrap_or! {
    info!("z was bad");
    return Err("bad z");
};
// ...
do_something_with(z);
// ...

ijackson commented 3 days ago

@Fishrock123 I am keen on this feature, but I tend to agree with @nikomatsakis concern about the grammar. I have some experience of writing specs etc.; would you welcome a merge request to your let-else branch where I rewrite the spec part, preserving my understanding of your intent but writing it in a more formal style?

Fishrock123 commented 3 days ago

could be written with a postfix macro as

@jhpratt Please see the alternatives section of the RFC for what extensions to specifically Option and Result are not enough.

Fishrock123 commented 3 days ago

@ijackson I'd like to do another round of minor updates (may not get to it today) but I would certainly welcome that!

jhpratt commented 3 days ago

@jhpratt Please see the alternatives section of the RFC for what extensions to specifically Option and Result are not enough.

Sure, but a postfix macro would still be more than capable of handling it, no? Not necessarily that specific one, but you could conceivably create a .unwrap_foo_or_do_stuff_in_this_block! {} macro. Postfix macros wouldn't be limited to just Option/Result.

Fishrock123 commented 3 days ago •

By "postfix macros" I assume you mean #2442

My main argument against that is that it is still not obvious as a counter-part to if let, and I think it would still be harder for users to discover. Thought I think the last few comments about macro based approaches note it has some parsing shortcomings with constants?

Fishrock123 commented 3 days ago

On further thought I still have no idea how you'd get multiple bindings out of a postfix macro.

jhpratt commented 3 days ago

By "postfix macros" I assume you mean #2442

Correct.

Thought I think the last few comments about macro based approaches note it has some parsing shortcomings with constants?

Those comments were regarding a macro that would accept a pattern, which isn't the case here.

On further thought I still have no idea how you'd get multiple bindings out of a postfix macro.

In theory you could create a macro for it, though I admit that's not ideal. How common is the multiple binding use case though? I would imagine the most common by far is just the simple Ok(_)/Some(_) matching.

Fishrock123 commented 3 days ago

How common is the multiple binding use case though?

The RFC provides an example of that from real-world code my team wrote in the last couple of months. There are multiple variations of that and others in the codebase.

jhpratt commented 3 days ago

The RFC provides an example of that from real-world code my team wrote in the last couple of months. There are multiple variations of that and others in the codebase.

I presume you're referring to this single example?

let Some(x) | MyEnum::VariantA(_, _, x) | MyEnum::VariantB { x, .. } = a else { return; };

I certainly can't speak for everyone, but I'd find that clearer as a match statement. Regardless, that doesn't speak to the relative frequency.

Keep in mind that I am in favor of this RFC. I am just bringing up the fact that it may not be strictly necessary to obtain what I would consider idiomatic Rust. Basically, if both this RFC and postfix macros land, I would likely prefer .unwrap_or! over this when possible.

Fishrock123 commented 3 days ago •

No - that | chaining example is only a very hypothetical alternative / extension.

I mean the example in the examples section:

impl ActionView {
    pub(crate) fn new(history: &History<Action>) -> Result<Self, eyre::Report> {
        let mut iter = history.iter();
        let event = iter
            .next()
            .ok_or_else(|| eyre::eyre!("Entity has no history"))?;

        let Action::Register {
            actor: String,
            x: Vec<String>
            y: u32,
            z: String,
        } = event.action().clone() else {
            return Err(eyre::eyre!("must begin with a Register action"));
        };
        // ...
    }
}

Fishrock123 commented 3 days ago •

A lot of people keep quoting the example with just options so I am removing that example in favor of elaborating slightly more on the custom enum example, which is where let-else particularly shines.

Fishrock123 commented 3 days ago •

RFC updated ^. OP also updated with an amended summary.

@ijackson feel free to send more nuanced grammer amendments.

nikomatsakis commented 2 days ago

@rfcbot resolve clarify-grammar

rfcbot commented 2 days ago

This is now entering its final comment period, as per the review above.

scottmcm commented 2 days ago •

One thing that came to mind here:

There's a potential interpretation here as let x = (y else { z }); where else works like ?? in C# or ?: in Kotlin or // in Perl. Which could also be useful (in very different cases) but has a different type implication.

Ah the elvis operator - previous discussion.

ExpHP commented 2 days ago •

There's a potential interpretation here as let x = (y else { z }); where else works like ?? in C# or ?: in Kotlin or // in Perl. Which could also be useful, but has a different type implication.

Given that the body of the else block must diverge, I get the feeling that any confusion caused by this will be short-lived.

Edit: My use of the word "feeling" here is a bit weasel-y, but I did gave a look over the examples in the RFC and did not find this interpretation to be problematic for any of them.

Given

1. The inspiration from Swift's if-let construct
2. The requirement for divergence

Wouldn't it be clearer to call this guard, the name that Swift already uses for something similar*

guard let Some(x) = my_option else {
   return
}

guard 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"));
};

The guard name would make the requirement for divergence in the else block a little more intuitive IMHO. I see you had a similar thought since you mentioned the guard! macro. My intuition would be that a vanilla if-let-else syntax would more intuitively express the ?? behaviour in languages that other posters have mentioned, e.g.

let Some(x) = my_option else Some("foobar")

• Arguably in Swift guard is a sort of assert, but this feels similar in that you're implicitly asserting a certain branch of an ADT was selected