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

Tracking Issue for RFC 3137: let-else statements #87335

Closed
6 of 7 tasks
joshtriplett opened this issue Jul 21, 2021 · 79 comments · Fixed by #93628
Closed
6 of 7 tasks

Tracking Issue for RFC 3137: let-else statements #87335

joshtriplett opened this issue Jul 21, 2021 · 79 comments · Fixed by #93628
Assignees
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. F-let-else Issues related to let-else statements (RFC 3137) S-tracking-impl-incomplete Status: The implementation is incomplete. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@joshtriplett
Copy link
Member

joshtriplett commented Jul 21, 2021

This is a tracking issue for RFC 3137: let-else statements.
The feature gate for the issue is #![feature(let_else)].

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

Unresolved Questions

  • Readability in practice: Will let ... else { ... }; be clear enough to humans in practical code, or will some introducer syntax be desirable?
  • Conflicts with if-let-chains: Does this conflict too much with the if-let-chains RFC or vice-versa? Neither this feature nor that feature should be stabilized without considering the other.
  • Amount of special cases: Are there too many special-case interactions with other features?
  • Grammar clarity: Does the grammar need to be clarified? This RFC has some slightly unusual grammar requirements.

Implementation history

@joshtriplett joshtriplett added T-lang Relevant to the language team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. F-let-else Issues related to let-else statements (RFC 3137) labels Jul 21, 2021
@inquisitivecrystal

This comment has been minimized.

@LeSeulArtichaut
Copy link
Contributor

Is there already someone who wants to implement the RFC? Otherwise I could maybe take a stab at it

@camsteffen
Copy link
Contributor

I volunteered on Zulip to implement a while ago and I have a start on it. @Fishrock123 are you still okay with me taking it since you were also interested?

@Fishrock123
Copy link
Contributor

I'm interested but I'm clueless and pretty busy. I'd say you should just take it.

@matklad
Copy link
Member

matklad commented Aug 18, 2021

An interesting example which I didn't see come up in the RFC text/thread (quick search for mut). In today's Rust, let-else won't work for a case of updating variables. An example here is computing a path in a graph after DFS returns a map of parents:

let parents: HashMap<u32, u32> = dfs();
let mut curr: u32 = 0;
let mut path: Vec<u32> = Vec::new();
loop {
    path.push(curr);
    let Some(&curr) = parents.get(curr) else { break }
}

Here, let won't re-bind curr, it'll shadow it instead.

Just as an observation, Kotlin-style elvis operator would avoid this issue, as it doesn't require a pattern:

let parents: HashMap<u32, u32> = dfs();
let mut curr: u32 = 0;
let mut path: Vec<u32> = Vec::new();
loop {
    path.push(curr);
    curr = *(parents.get(curr) ?: break);
}

@joshtriplett

This comment has been minimized.

@matklad

This comment has been minimized.

@matklad
Copy link
Member

matklad commented Aug 18, 2021

Hypothetically, we can allow the following (else without let):

let parents: HashMap<u32, u32> = dfs();
let mut curr: u32 = 0;
let mut path: Vec<u32> = Vec::new();
loop {
    path.push(curr);
    Some(&curr) = parents.get(curr) else { break }
}

This would be a natural combination of let-else and #71126. While #71126 is still unstable, it might be worth is to think about the interactions between the two. Will the above example just work, or are there any problems with it?

@camsteffen
Copy link
Contributor

I also mentioned this in the RFC. Probably no technical limitation but just readability concerns. I think we can safely wait until let-else and destructuring-assignment have matured before expecting them to procreate (forgive me).

@Fishrock123
Copy link
Contributor

Fishrock123 commented Aug 18, 2021

Ah, I seem to have missed that thing about destructuring_assignment, yes that seems like a future possibility, with the above mentioned caveats.

bors added a commit to rust-lang-ci/rust that referenced this issue Sep 1, 2021
Introduce `let...else`

Tracking issue: rust-lang#87335

The trickiest part for me was enforcing the diverging else block with clear diagnostics. Perhaps the obvious solution is to expand to `let _: ! = ..`, but I decided against this because, when a "mismatched type" error is found in typeck, there is no way to trace where in the HIR the expected type originated, AFAICT. In order to pass down this information, I believe we should introduce `Expectation::LetElseNever(HirId)` or maybe add `HirId` to `Expectation::HasType`, but I left that as a future enhancement. For now, I simply assert that the block is `!` with a custom `ObligationCauseCode`, and I think this is clear enough, at least to start. The downside here is that the error points at the entire block rather than the specific expression with the wrong type. I left a todo to this effect.

Overall, I believe this PR is feature-complete with regard to the RFC.
@Fishrock123
Copy link
Contributor

@joshtriplett #87688 has been merged, can you tick off "implement the RFC"?

@Fishrock123
Copy link
Contributor

@camsteffen @cjgillot did the implementation answer the "Grammar clarity" question at all?

@camsteffen
Copy link
Contributor

camsteffen commented Sep 1, 2021

did the implementation answer the "Grammar clarity" question at all?

See expr_trailing_brace - I think the "slightly unusual grammar requirements" are contained there.

@Fishrock123
Copy link
Contributor

My understanding of the state of implementation is:

@nikomatsakis
Copy link
Contributor

@Fishrock123 cool! Once you've had a chance to test, we should consider moving to feature complete status and writing a blog post as a way to get more feedback. =)

@nagisa
Copy link
Member

nagisa commented Oct 9, 2021

#89688 is a blocker for a stabilization. Without this fixed, our implementation does not match the behaviour as prescribed by the RFC.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Sep 16, 2022
…plett

Stabilize `let else`

:tada:  **Stabilizes the `let else` feature, added by [RFC 3137](rust-lang/rfcs#3137 🎉

Reference PR: rust-lang/reference#1156

closes rust-lang#87335 (`let else` tracking issue)

FCP: rust-lang#93628 (comment)

----------

## Stabilization report

### Summary

The feature allows refutable patterns in `let` statements if the expression is
followed by a diverging `else`:

```Rust
fn get_count_item(s: &str) -> (u64, &str) {
    let mut it = s.split(' ');
    let (Some(count_str), Some(item)) = (it.next(), it.next()) else {
        panic!("Can't segment count item pair: '{s}'");
    };
    let Ok(count) = u64::from_str(count_str) else {
        panic!("Can't parse integer: '{count_str}'");
    };
    (count, item)
}
assert_eq!(get_count_item("3 chairs"), (3, "chairs"));
```

### Differences from the RFC / Desugaring

Outside of desugaring I'm not aware of any differences between the implementation and the RFC. The chosen desugaring has been changed from the RFC's [original](https://rust-lang.github.io/rfcs/3137-let-else.html#reference-level-explanations). You can read a detailed discussion of the implementation history of it in `@cormacrelf` 's [summary](rust-lang#93628 (comment)) in this thread, as well as the [followup](rust-lang#93628 (comment)). Since that followup, further changes have happened to the desugaring, in rust-lang#98574, rust-lang#99518, rust-lang#99954. The later changes were mostly about the drop order: On match, temporaries drop in the same order as they would for a `let` declaration. On mismatch, temporaries drop before the `else` block.

### Test cases

In chronological order as they were merged.

Added by df9a2e0 (rust-lang#87688):

* [`ui/pattern/usefulness/top-level-alternation.rs`](https://github.com/rust-lang/rust/blob/1.58.1/src/test/ui/pattern/usefulness/top-level-alternation.rs) to ensure the unreachable pattern lint visits patterns inside `let else`.

Added by 5b95df4 (rust-lang#87688):

* [`ui/let-else/let-else-bool-binop-init.rs`](https://github.com/rust-lang/rust/blob/1.58.1/src/test/ui/let-else/let-else-bool-binop-init.rs) to ensure that no lazy boolean expressions (using `&&` or `||`) are allowed in the expression, as the RFC mandates.
* [`ui/let-else/let-else-brace-before-else.rs`](https://github.com/rust-lang/rust/blob/1.58.1/src/test/ui/let-else/let-else-brace-before-else.rs) to ensure that no `}` directly preceding the `else` is allowed in the expression, as the RFC mandates.
* [`ui/let-else/let-else-check.rs`](https://github.com/rust-lang/rust/blob/1.58.1/src/test/ui/let-else/let-else-check.rs) to ensure that `#[allow(...)]` attributes added to the entire `let` statement apply for the `else` block.
* [`ui/let-else/let-else-irrefutable.rs`](https://github.com/rust-lang/rust/blob/1.58.1/src/test/ui/let-else/let-else-irrefutable.rs) to ensure that the `irrefutable_let_patterns` lint fires.
* [`ui/let-else/let-else-missing-semicolon.rs`](https://github.com/rust-lang/rust/blob/1.58.1/src/test/ui/let-else/let-else-missing-semicolon.rs) to ensure the presence of semicolons at the end of the `let` statement.
* [`ui/let-else/let-else-non-diverging.rs`](https://github.com/rust-lang/rust/blob/1.58.1/src/test/ui/let-else/let-else-non-diverging.rs) to ensure the `else` block diverges.
* [`ui/let-else/let-else-run-pass.rs`](https://github.com/rust-lang/rust/blob/1.58.1/src/test/ui/let-else/let-else-run-pass.rs) to ensure the feature works in some simple test case settings.
* [`ui/let-else/let-else-scope.rs`](https://github.com/rust-lang/rust/blob/1.58.1/src/test/ui/let-else/let-else-scope.rs) to ensure the bindings created by the outer `let` expression are not available in the `else` block of it.

Added by bf7c32a (rust-lang#89965):

* [`ui/let-else/issue-89960.rs`](https://github.com/rust-lang/rust/blob/1.58.1/src/test/ui/let-else/issue-89960.rs) as a regression test for the ICE-on-error bug rust-lang#89960 . Later in 102b912 this got removed in favour of more comprehensive tests.

Added by 8565419 (rust-lang#89974):

* [`ui/let-else/let-else-if.rs`](https://github.com/rust-lang/rust/blob/1.58.1/src/test/ui/let-else/let-else-if.rs) to test for the improved error message that points out that `let else if` is not possible.

Added by 9b45713:

* [`ui/let-else/let-else-allow-unused.rs`](https://github.com/rust-lang/rust/blob/1.61.0/src/test/ui/let-else/let-else-allow-unused.rs) as a regression test for rust-lang#89807, to ensure that `#[allow(...)]` attributes added to the entire `let` statement apply for bindings created by the `let else` pattern.

Added by 61bcd8d (rust-lang#89841):

* [`ui/let-else/let-else-non-copy.rs`](https://github.com/rust-lang/rust/blob/1.61.0/src/test/ui/let-else/let-else-non-copy.rs) to ensure that a copy is performed out of non-copy wrapper types. This mirrors `if let` behaviour. The test case bases on rustc internal changes originally meant for rust-lang#89933 but then removed from the PR due to the error prior to the improvements of rust-lang#89841.
* [`ui/let-else/let-else-source-expr-nomove-pass.rs `](https://github.com/rust-lang/rust/blob/1.61.0/src/test/ui/let-else/let-else-source-expr-nomove-pass.rs) to ensure that while there is a move of the binding in the successful case, the `else` case can still access the non-matching value. This mirrors `if let` behaviour.

Added by 102b912 (rust-lang#89841):

* [`ui/let-else/let-else-ref-bindings.rs`](https://github.com/rust-lang/rust/blob/1.61.0/src/test/ui/let-else/let-else-ref-bindings.rs) and [`ui/let-else/let-else-ref-bindings-pass.rs `](https://github.com/rust-lang/rust/blob/1.61.0/src/test/ui/let-else/let-else-ref-bindings-pass.rs) to check `ref` and `ref mut` keywords in the pattern work correctly and error when needed.

Added by 2715c5f (rust-lang#89841):

* Match ergonomic tests adapted from the `rfc2005` test suite.

Added by fec8a50 (rust-lang#89841):

* [`ui/let-else/let-else-deref-coercion-annotated.rs`](https://github.com/rust-lang/rust/blob/1.61.0/src/test/ui/let-else/let-else-deref-coercion-annotated.rs) and [`ui/let-else/let-else-deref-coercion.rs`](https://github.com/rust-lang/rust/blob/1.61.0/src/test/ui/let-else/let-else-deref-coercion.rs) to check deref coercions.

#### Added since this stabilization report was originally written (2022-02-09)

Added by 76ea566 (rust-lang#94211):

* [`ui/let-else/let-else-destructuring.rs`](https://github.com/rust-lang/rust/blob/1.63.0/src/test/ui/let-else/let-else-destructuring.rs) to give a nice error message if an user tries to do an assignment with a (possibly refutable) pattern and an `else` block, like asked for in rust-lang#93995.

Added by e7730dc (rust-lang#94208):

* [`ui/let-else/let-else-allow-in-expr.rs`](https://github.com/rust-lang/rust/blob/1.61.0/src/test/ui/let-else/let-else-allow-in-expr.rs) to test whether `#[allow(unused_variables)]` works in the expr, as well as its non presence, as well as putting it on the entire `let else` *affects* the expr, too. This was adding a missing test as pointed out by the stabilization report.
* Expansion of `ui/let-else/let-else-allow-unused.rs` and `ui/let-else/let-else-check.rs` to ensure that non-presence of `#[allow(unused)]` does issue the unused lint. This was adding a missing test case as pointed out by the stabilization report.

Added by 5bd7106 (rust-lang#94208):

* [`ui/let-else/let-else-slicing-error.rs`](https://github.com/rust-lang/rust/blob/1.61.0/src/test/ui/let-else/let-else-slicing-error.rs), a regression test for rust-lang#92069, which got fixed without addition of a regression test. This resolves a missing test as pointed out by the stabilization report.

Added by 5374688 (rust-lang#98574):

* [`src/test/ui/async-await/async-await-let-else.rs`](https://github.com/rust-lang/rust/blob/master/src/test/ui/async-await/async-await-let-else.rs) to test the interaction of async/await with `let else`

Added by 6c529de (rust-lang#98574):

* [`src/test/ui/let-else/let-else-temporary-lifetime.rs`](https://github.com/rust-lang/rust/blob/master/src/test/ui/let-else/let-else-temporary-lifetime.rs) as a (partial) regression test for rust-lang#98672

Added by 9b56640 (rust-lang#99518):

* [`src/test/ui/let-else/let-else-temp-borrowck.rs`](https://github.com/rust-lang/rust/blob/master/src/test/ui/let-else/let-else-temporary-lifetime.rs) as a regression test for rust-lang#93951
* Extension of `src/test/ui/let-else/let-else-temporary-lifetime.rs` to include a partial regression test for rust-lang#98672 (especially regarding `else` drop order)

Added by baf9a7c (rust-lang#99518):

* Extension of `src/test/ui/let-else/let-else-temporary-lifetime.rs` to include a partial regression test for rust-lang#93951, similar to `let-else-temp-borrowck.rs`

Added by 60be2de (rust-lang#99518):

* Extension of `src/test/ui/let-else/let-else-temporary-lifetime.rs` to include a program that can now be compiled thanks to borrow checker implications of rust-lang#99518

Added by 47a7a91 (rust-lang#100132):

* [`src/test/ui/let-else/issue-100103.rs`](https://github.com/rust-lang/rust/blob/master/src/test/ui/let-else/issue-100103.rs), as a regression test for rust-lang#100103, to ensure that there is no ICE when doing `Err(...)?` inside else blocks.

Added by e3c5bd6 (rust-lang#100443):

* [`src/test/ui/let-else/let-else-then-diverge.rs`](https://github.com/rust-lang/rust/blob/master/src/test/ui/let-else/let-else-then-diverge.rs), to verify that there is no unreachable code error with the current desugaring.

Added by 9818526 (rust-lang#100443):

* [`src/test/ui/let-else/issue-94176.rs`](https://github.com/rust-lang/rust/blob/master/src/test/ui/let-else/issue-94176.rs), to make sure that a correct span is emitted for a missing trailing expression error. Regression test for rust-lang#94176.

Added by e182d12 (rust-lang#100434):

* [src/test/ui/unpretty/pretty-let-else.rs](https://github.com/rust-lang/rust/blob/master/src/test/ui/unpretty/pretty-let-else.rs), as a regression test to ensure pretty printing works for `let else` (this bug surfaced in many different ways)

Added by e262856 (rust-lang#99954):

* [`src/test/ui/let-else/let-else-temporary-lifetime.rs`](https://github.com/rust-lang/rust/blob/master/src/test/ui/let-else/let-else-temporary-lifetime.rs) extended to contain & borrows as well, as this was identified as an earlier issue with the desugaring: rust-lang#98672 (comment)

Added by 2d8460e (rust-lang#99291):

* [`src/test/ui/let-else/let-else-drop-order.rs`](https://github.com/rust-lang/rust/blob/master/src/test/ui/let-else/let-else-drop-order.rs) a matrix based test for various drop order behaviour of `let else`. Especially, it verifies equality of `let` and `let else` drop orders, [resolving](rust-lang#93628 (comment)) a [stabilization blocker](rust-lang#93628 (comment)).

Added by 1b87ce0 (rust-lang#101410):

* Edit to `src/test/ui/let-else/let-else-temporary-lifetime.rs` to add the `-Zvalidate-mir` flag, as a regression test for rust-lang#99228

Added by af591eb (rust-lang#101410):

* [`src/test/ui/let-else/issue-99975.rs`](https://github.com/rust-lang/rust/blob/master/src/test/ui/let-else/issue-99975.rs) as a regression test for the ICE rust-lang#99975.

Added by this PR:

* `ui/let-else/let-else.rs`, a simple run-pass check, similar to `ui/let-else/let-else-run-pass.rs`.

### Things not currently tested

* ~~The `#[allow(...)]` tests check whether allow works, but they don't check whether the non-presence of allow causes a lint to fire.~~ → *test added by e7730dc*
* ~~There is no `#[allow(...)]` test for the expression, as there are tests for the pattern and the else block.~~ → *test added by e7730dc*
* ~~`let-else-brace-before-else.rs` forbids the `let ... = {} else {}` pattern and there is a rustfix to obtain `let ... = ({}) else {}`. I'm not sure whether the `.fixed` files are checked by the tooling that they compile. But if there is no such check, it would be neat to make sure that `let ... = ({}) else {}` compiles.~~ → *test added by e7730dc*
* ~~rust-lang#92069 got closed as fixed, but no regression test was added. Not sure it's worth to add one.~~ → *test added by 5bd7106*
* ~~consistency between `let else` and `if let` regarding lifetimes and drop order: rust-lang#93628 (comment) → *test added by 2d8460e*

Edit: they are all tested now.

### Possible future work / Refutable destructuring assignments

[RFC 2909](https://rust-lang.github.io/rfcs/2909-destructuring-assignment.html) specifies destructuring assignment, allowing statements like `FooBar { a, b, c } = foo();`.
As it was stabilized, destructuring assignment only allows *irrefutable* patterns, which before the advent of `let else` were the only patterns that `let` supported.
So the combination of `let else` and destructuring assignments gives reason to think about extensions of the destructuring assignments feature that allow refutable patterns, discussed in rust-lang#93995.

A naive mapping of `let else` to destructuring assignments in the form of `Some(v) = foo() else { ... };` might not be the ideal way. `let else` needs a diverging `else` clause as it introduces new bindings, while assignments have a default behaviour to fall back to if the pattern does not match, in the form of not performing the assignment. Thus, there is no good case to require divergence, or even an `else` clause at all, beyond the need for having *some* introducer syntax so that it is clear to readers that the assignment is not a given (enums and structs look similar). There are better candidates for introducer syntax however than an empty `else {}` clause, like `maybe` which could be added as a keyword on an edition boundary:

```Rust
let mut v = 0;
maybe Some(v) = foo(&v);
maybe Some(v) = foo(&v) else { bar() };
```

Further design discussion is left to an RFC, or the linked issue.
@bors bors closed this as completed in 3ad81e0 Sep 17, 2022
seanyoung added a commit to seanyoung/solang that referenced this issue Mar 24, 2023
scale-info v2.4.0 fails to compile with:

 error[E0658]: `let...else` statements are unstable
   --> /Users/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/scale-info-2.4.0/src/portable.rs:124:17
    |
124 | /                 let Some(ty) = param.ty() else {
125 | |                     continue
126 | |                 };
    | |__________________^
    |
    = note: see issue #87335 <rust-lang/rust#87335> for more information

Signed-off-by: Sean Young <sean@mess.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. F-let-else Issues related to let-else statements (RFC 3137) S-tracking-impl-incomplete Status: The implementation is incomplete. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
Status: Feature Complete
Development

Successfully merging a pull request may close this issue.