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

let-else does not support else if #111910

Open
lcnr opened this issue May 24, 2023 · 8 comments
Open

let-else does not support else if #111910

lcnr opened this issue May 24, 2023 · 8 comments
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. F-let_else Issues related to let-else statements (RFC 3137)

Comments

@lcnr
Copy link
Contributor

lcnr commented May 24, 2023

fn main() {
    let Some(x) = Some(1)
    else if true {
        return;
    } else {
        return;
    }
}

results in the following error but should compile imo:

error: conditional `else if` is not supported for `let...else`
 --> src/main.rs:3:10
  |
3 |     else if true {
  |          ^^ expected `{`
  |
help: try placing this code inside a block
  |
3 ~     else { if true {
4 |         return;
5 |     } else {
6 |         return;
7 ~     } }
  |

the improved diagnostic has been added in #89974 which refers to #87335 (comment) by @joshtriplett:

let ... else if definitely isn't something the RFC specifies, and I don't think we should. A better error message might make sense, though ("cannot use else if with let ... else").

I personally disagree with "and I don't think we should". For me this not working was fairly surprising and conflicts with my intuition of let else as a shorthand for

let Some(var) = expr else { return; }

let var = if let Some(var) = expr {
    var
} else {
    return
}

or as stated in the RFC

This is a counterpart to if let expressions, and the pattern matching works identically, except that the value from the pattern match is assigned to the surrounding scope rather than the block's scope.

I would like to add support for this myself or mentor someone adding this. I am not sure what process is required here. If not desired, please FCP close this.

@lcnr lcnr added I-lang-nominated Nominated for discussion during a lang team meeting. F-let_else Issues related to let-else statements (RFC 3137) labels May 24, 2023
@fee1-dead
Copy link
Member

I'd be willing to work on this if this is desired.

@est31
Copy link
Member

est31 commented May 24, 2023

FTR this is the real world code that my comment in the let else tracking issue was referring to (the link bitrotted):

let path = if let Some((path, _)) = &used_crate_source.rlib {
path
} else if used_crate_source.rmeta.is_some() {
return Err(format!(
"could not find rlib for: `{}`, found rmeta (metadata) file",
name
));
} else {
return Err(format!("could not find rlib for: `{}`", name));
};

Nowadays it's an if let but one could imagine code afterwards that one doesn't want to be affected by rightward drift.

IMO it's a good idea to have it, but back when let else was still unstable it was definitely better to have shipped without it to keep the feature minimal.

It also has low conflict potential with other extension proposals for let else. The worst I could think of is confusion potential with extensions that look (but not are) very close:

let Foo::BarA(foo) = hi
else if let Foo::BarB(foo) { return; } // We could theoretically allow dropping the expr here and just match on the `hi` expr
else if let Foo::BarC(foo, foo2) { return; }
else if true { // Is this tail if confusing? It does *not* try to do something with the `hi` expr
    panic!()
} else {
    return;
};

or the match equivalent:

let Foo::BarA(foo) = hi
else match {
    Foo::BarC(foo) => panic!(),
    Foo::BarD(foo, foo2) => panic!(),
    _ => panic!(),
};

@scottmcm
Copy link
Member

(Not speaking for the team)

Personally, I'm not inclined to do this. We generally don't have "remove one level of extra indentation" features, and (more importantly) I think that if we're going to extend this, it would be something that more directly integrates with the pattern behaviour. Like there was the previous discussion of something like let Ok(x) = ... else match { Err(e) => ... };, for example, which is intimately connected with the core let-else feature, and not something that would work already by just putting it inside the else braces.

(I consider normal else if to be linearly less nesting because of chaining, but a let-else-if isn't chainable, so doesn't get that extra mountain-range-avoidance bonus.)

@lcnr
Copy link
Contributor Author

lcnr commented Jun 20, 2023

to me this is not about "remove one level of extra indentation" but much rather that else if and else are part of the same language construct. else without else if fundamentally feels inconsistent to me. Other features, like else match however introduce a new construct, as currently else match is not supported anywhere.

@scottmcm
Copy link
Member

I guess I don't see it that way as there's no if in the first place. To me, else if is going with if, not because of the else, I guess.

(For example, for-else in Python doesn't support elif, as far as I know, since elif is part of if, not part of else.)

@joshtriplett
Copy link
Member

I'm similarly not inclined to do this; else if doesn't seem like an inherent part of let-else, and it doesn't feel like it adds new functionality (since you must diverge). If this added or unlocked functionality you couldn't get otherwise, that'd make it more appealing. But as it stands, it seems like a minor syntax tweak.

@lcnr
Copy link
Contributor Author

lcnr commented Aug 2, 2023

this is a discussion about syntactic sugar, it can never unlock functionality you couldn't get otherwise. let-else itself is a minor syntax tweak.

Looking at #111910 (comment) as an example where this might be useful:

        let Some((path, _)) = &used_crate_source.rlib
        else if used_crate_source.rmeta.is_some() {
            return Err(format!(
                "could not find rlib for: `{}`, found rmeta (metadata) file",
                name
            ));
        } else {
            return Err(format!("could not find rlib for: `{}`", name));
        };

vs

        let Some((path, _)) = &used_crate_source.rlib else {
            if used_crate_source.rmeta.is_some() {
                return Err(format!(
                    "could not find rlib for: `{}`, found rmeta (metadata) file",
                    name
                ));
            } else {
                return Err(format!("could not find rlib for: `{}`", name));
            }
        };

I think the formatting required for else if is worse than the formatting with else { ... } here. having let PAT = EXPR else if COND { on one line feels fairly hard to read, even if it could fit on one line.

I still think having the option to use else if if the else is forced to be on a separate line anyways is nice, but 🤷 experimenting with the formatting of it does make the argument less convincing.

Going to drop the nomination because this issue doesn't feel important enough to stay nominated for months.

@lcnr lcnr added C-feature-request Category: A feature request, i.e: not implemented / a PR. F-let_else Issues related to let-else statements (RFC 3137) and removed F-let_else Issues related to let-else statements (RFC 3137) I-lang-nominated Nominated for discussion during a lang team meeting. labels Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. F-let_else Issues related to let-else statements (RFC 3137)
Projects
None yet
Development

No branches or pull requests

6 participants