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

Document let else statements #1156

Merged
merged 3 commits into from
Oct 5, 2022
Merged

Document let else statements #1156

merged 3 commits into from
Oct 5, 2022

Conversation

est31
Copy link
Member

@est31 est31 commented Feb 3, 2022

For the stabilization of let else statements.

Tracking issue: rust-lang/rust#87335

@ehuss ehuss added the S-waiting-on-stabilization Waiting for a stabilization PR to be merged in the main Rust repository label Feb 3, 2022
Copy link

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

I would probably write this documentation a bit differently. I think to the developer there is not much different between let and let else. Consider that you can already match against irrefutable pattern in let, and this is essentially just an extension to let in practice.

But perhaps its direct relation to let as a natural extension belongs more in a guide level explanation.

src/statements.md Outdated Show resolved Hide resolved
@est31
Copy link
Member Author

est31 commented Feb 3, 2022

Consider that you can already match against irrefutable pattern in let, and this is essentially just an extension to let in practice.

Originally I tried to extend the let section as you suggest. There are commonalities, but there are also a lot of differences. For let, the pattern must be irrefutable, for let else it must be refutable. For let, the = expr is optional, for let else it isn't. Explaining those differences would have made it harder to explain/comprehend how the thing works in the first place.

@cormacrelf
Copy link

for let else it must be refutable.

No, by default it is only a warning to have an irrefutable binding in there.

warning: irrefutable `let...else` pattern
 --> src/main.rs:4:5
  |
4 | /     let x = 5 else {
5 | |         return;
6 | |     };
  | |______^
  |
  = note: `#[warn(irrefutable_let_patterns)]` on by default
  = note: this pattern will always match, so the `else` clause is useless
  = help: consider removing the `else` clause

@est31
Copy link
Member Author

est31 commented Feb 4, 2022

Good point, it's only a warning. Should I change the text of the reference to extend it to all patterns?

@cormacrelf
Copy link

Yes. But refutable patterns deserve a mention, something along the lines of

If the pattern is a refutable pattern, then the else block will be executed when the pattern is not matched. The else block must escape [/return/whatever we call it] ...

I haven't read much of the reference before so not sure whether it's appropriate to mention that refutable patterns are the whole point of it.

src/statements.md Outdated Show resolved Hide resolved
src/statements.md Outdated Show resolved Hide resolved
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request 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.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Sep 17, 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.
@est31
Copy link
Member Author

est31 commented Sep 18, 2022

With let else stable, this is ready for review!

@ehuss
Copy link
Contributor

ehuss commented Sep 20, 2022

I think I too would prefer to see this defined as a single statement. I understand it can be a little awkward since it changes the semantics quite a bit, but I think it shouldn't be too difficult to describe. As it is, it looks like there is a lot of duplication.

To make it fit in the original let statements section, I think it should be sufficient to:

  • Remove the word "irrefutable".
  • Add a paragraph describing the else block behavior. Here's a rough draft, but feel free to adjust:

When an else block follows the initializer, then the pattern may be a refutable pattern.
When the pattern does not match, the else block is executed.
The type of the else block must be the never type.
If there is no else block, then the pattern must be irrefutable.

  • The grammar for Expression in the initializer needs to reject forbidden expressions. I would probably just add a dagger (†) that links to a sentence in the grammar section that explains it. Like:

Syntax
… [Expression]

† When an else block is specified, the Expression must not be a LazyBooleanExpression or end with a }.

I'm also a bit surprised this is how it works, as this adds unprecedented complexity to the grammar and I think would be quite difficult for many parsers.

  • Add a note about irrefutability:

Note: rustc will emit a warning when using an irrefutable pattern with an else block since that makes it impossible for the else block to execute.

  • Add an example. Illustrate both a basic let binding, and one with an else block.

@est31 est31 force-pushed the let_else branch 5 times, most recently from d69ffae to 5ab52b7 Compare September 20, 2022 02:37
@est31
Copy link
Member Author

est31 commented Sep 20, 2022

@ehuss okay I gave up, I refactored the section so that it is unified with let. My second attempt of unifying feels less complicated than what I remember of my first attempt, so this was a good idea already, and I'm quite open towards this approach now.

Comment on lines 57 to 58
> ( `:` [_Type_] )<sup>?</sup> ( | `=` [_Expression_] | `=` [_Expression_]
> `else` [_BlockExpression_]) `;`
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems a little tricky the unravel since it is duplicating the expression. I'm also not clear what the leading | is. Perhaps that is to imply the ε empty rule? That isn't a style we've used in the reference that I can recall, and I would kinda like to avoid it just to try to keep things simple. I think just making it an optional sub-expression doesn't look too bad, as in:

image

Or maybe even better, place it as a separate rule, as in:

image

Also, it is important to include the direct note about the syntax limitations of the initializer expression. Since it is part of the syntax, it must be documented as part of the grammar rules. The restrictions described in the English text are only for post-parsing restrictions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you know how to do the note in source code?

Copy link
Member Author

Choose a reason for hiding this comment

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

(also yes the leading | was to indicate the epsilon case, but I have changed it to your suggestion)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, sorry, I should have included the source from the comment above:

> **<sup>Syntax</sup>**\
> … [_Expression_] [†](#let-else-restriction) …
> 
> <span id="let-else-restriction">† When an `else` block is specified, the _Expression_ must not be a [_LazyBooleanExpression_](expressions/operator-expression.md#lazy-boolean-operators) or end with a `}`.</span>

Just make it a link, and use a <span> to set the target.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I've tried out your code, it causes an error in CI:

error in ../src/statements.md (line 57): broken Reference link (reference )
some style checks failed

Apparently pulldown-cmark does not parse the inline html.

I have tried using a footnote, but that shows a 1, which in this context can be very easily misunderstood as a repetition:
Screenshot_20220924_031548

However, it is possible to hide the number and put a † on its stead. I've pushed a commit to do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to not change the behavior of footnotes. Those are used in other places of the reference.

I would also prefer to avoid using them in this situation. This isn't really a footnote, but more of an attempt to stuff a bunch of information in a small amount of space.

I tried the suggestion I posted above, and the style-check and the linkchecker both passed. It should look exactly like:

> **<sup>Syntax</sup>**\
> _LetStatement_ :\
> &nbsp;&nbsp; [_OuterAttribute_]<sup>\*</sup> `let` [_PatternNoTopAlt_]
>     ( `:` [_Type_] )<sup>?</sup> (`=` [_Expression_] [](#let-else-restriction)
>     ( `else` [_BlockExpression_]) <sup>?</sup> ) <sup>?</sup> `;`
>
> <span id="let-else-restriction">† When an `else` block is specified, the _Expression_ must not be a [_LazyBooleanExpression_](expressions/operator-expr.md#lazy-boolean-operators) or end with a `}`.</span>

Copy link
Member Author

@est31 est31 Oct 4, 2022

Choose a reason for hiding this comment

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

@ehuss regarding the CI failure, please see this CI run, ran from this commit. If you click on the prev button you will get to the commit that has precisely the state that you suggest.

    Compiling style-check v0.1.0 (/home/runner/work/reference/reference/style-check)
    Finished dev [unoptimized + debuginfo] target(s) in 37.63s
     Running `target/debug/style-check ../src`
error in ../src/statements.md (line 57): broken Reference link (reference `†`)
some style checks failed
> &nbsp;&nbsp; [_OuterAttribute_]<sup>\*</sup> `let` [_PatternNoTopAlt_]
>     ( `:` [_Type_] )<sup>?</sup> (`=` [_Expression_][](#let-else-restriction)
>     ( `else` [_BlockExpression_]) <sup>?</sup> ) <sup>?</sup> `;`
<span id="let-else-restriction">† When an `else` block is specified, the
_Expression_ must not be a [_LazyBooleanExpression_], or end with a `}`.</span>

I can try it again, but I doubt it will be any different.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's because in that commit you have:

[_Expression_][†](#let-else-restriction)

The text I have above has a space:

[_Expression_] [†](#let-else-restriction)

Without the space, markdown thinks the [†] is a reference link for the preceding [_Expression_].

Copy link
Member Author

Choose a reason for hiding this comment

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

Ohhhh... I see... Will adjust it right away.

src/statements.md Outdated Show resolved Hide resolved
src/statements.md Outdated Show resolved Hide resolved
src/statements.md Outdated Show resolved Hide resolved
Comment on lines 73 to 74
If an `else` block is present, restrictions apply on the expression:
It might not be a [_LazyBooleanExpression_], or end in a `}` token.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be up in the grammar section since it is part of the grammar.

Also, similar to the must/should/may terminology:

Suggested change
If an `else` block is present, restrictions apply on the expression:
It might not be a [_LazyBooleanExpression_], or end in a `}` token.
If an `else` block is present, restrictions apply on the expression:
It must not be a [_LazyBooleanExpression_], or end in a `}` token.

Although I do lean towards the earlier wording I suggested. I think it can sometimes be a little difficult to follow when using "it" when it is not very clear what "it" is. Here it is referring to an "expression", but there are different expressions at play (such as the block expression, and all the expressions within the block). Up in the grammar, the word Expression refers to a specific expression (there is only one of that form; that is a rule with a capital E and italicized).

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's clear because there is only one expression in the grammar. It is also mentioned in the first paragraph.

@est31 est31 force-pushed the let_else branch 2 times, most recently from c60764b to 3eacf00 Compare September 24, 2022 00:38
@est31
Copy link
Member Author

est31 commented Sep 24, 2022

r? @ehuss

Havvy
Havvy previously requested changes Sep 26, 2022
Copy link
Contributor

@Havvy Havvy left a comment

Choose a reason for hiding this comment

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

On https://doc.rust-lang.org/reference/items/functions.html#function-parameters we mention that let patterns are always irrefutable. This is no longer true, so that section needs to be updated as well.

@est31
Copy link
Member Author

est31 commented Sep 26, 2022

@Havvy good catch! I've updated that sentence in the last commit.

@Havvy Havvy dismissed their stale review September 26, 2022 08:44

Feedback has been applied. Feedback was not a full review.

@ehuss ehuss removed the S-waiting-on-stabilization Waiting for a stabilization PR to be merged in the main Rust repository label Oct 3, 2022
irrefutable [pattern]. The pattern is followed optionally by a type
annotation and then optionally by an initializer expression. When no
type annotation is given, the compiler will infer the type, or signal
<span id="let-else-restriction">† When an `else` block is specified, the
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this and the previous blank line include the > prefix to keep it in the blockquote? Since this is part of the grammar, I'd like to keep it as a single block. That would also help with future efforts to have a consolidated grammar appendix, where otherwise the dagger link would not work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Thanks for the patience with the reviews.

@ehuss ehuss merged commit 3d1ca95 into rust-lang:master Oct 5, 2022
@est31
Copy link
Member Author

est31 commented Oct 5, 2022

No problem! It is great that consistency is important to you.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Oct 12, 2022
Update books

## nomicon

1 commits in f53bfa056929217870a5d2df1366d2e7ba35096d..9c73283775466d22208a0b28afcab44db4c0cc10
2022-09-05 07:19:02 -0700 to 2022-09-30 07:31:22 +0900
- Fix typo (rust-lang/nomicon#380)

## reference

9 commits in a7cdac33ca7356ad49d5c2b5e2c5010889b33eee..f6ed74f582bddcec73f753eafaab3749c4f7df61
2022-09-19 17:39:58 -0700 to 2022-10-08 02:43:26 -0700
- Typo 'a' -&gt; 'an' (rust-lang/reference#1280)
- One line one sentence for expressions and statements main chapters (rust-lang/reference#1277)
- Document let else statements (rust-lang/reference#1156)
- Document `label_break_value` in the reference (rust-lang/reference#1263)
- Document target_has_atomic (rust-lang/reference#1171)
- update 'unsafe' (rust-lang/reference#1278)
- Update tokens.md (rust-lang/reference#1276)
- One sentence, one line Patterns chapter (rust-lang/reference#1275)
- Use semver-compliant example version (rust-lang/reference#1272)

## rust-by-example

9 commits in 767a6bd9727a596d7cfdbaeee475e65b2670ea3a..5e7b296d6c345addbd748f242aae28c42555c015
2022-09-14 09:17:18 -0300 to 2022-10-05 08:24:45 -0300
- Make it clear that rustdoc uses the commonmark spec (rust-lang/rust-by-example#1622)
- Update defaults.md (rust-lang/rust-by-example#1615)
- added "see also" for the @ binding sigil (rust-lang/rust-by-example#1612)
- add more precision to the effects of --bin flag (rust-lang/rust-by-example#1607)
- create bar project in cargo/dependencies example (rust-lang/rust-by-example#1606)
- use consistent wording about type annotation (rust-lang/rust-by-example#1603)
- cast.md improvements (rust-lang/rust-by-example#1599)
- Fix typo in macros.md (rust-lang/rust-by-example#1598)
- Corrected mistaken "The" instead of "There" (rust-lang/rust-by-example#1617)

## rustc-dev-guide

2 commits in 9a86c0467bbe42056f73fdf5b03fff757d7c4a9b..7518c3445dc02df0d196f5f84e568d633c5141fb
2022-10-07 18:34:51 +0200 to 2022-10-08 12:29:47 +0200
- Update debugging.md
- Use llvm subdomain for compiler-explorer link

## embedded-book

1 commits in 4ce51cb7441a6f02b5bf9b07b2eb755c21ab7954..c533348edd69f11a8f4225d633a05d7093fddbf3
2022-09-15 08:53:09 +0000 to 2022-10-10 10:16:49 +0000
- Fix a typo in registers.md  (rust-embedded/book#330)
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Oct 12, 2022
Update books

## nomicon

1 commits in f53bfa056929217870a5d2df1366d2e7ba35096d..9c73283775466d22208a0b28afcab44db4c0cc10
2022-09-05 07:19:02 -0700 to 2022-09-30 07:31:22 +0900
- Fix typo (rust-lang/nomicon#380)

## reference

9 commits in a7cdac33ca7356ad49d5c2b5e2c5010889b33eee..f6ed74f582bddcec73f753eafaab3749c4f7df61
2022-09-19 17:39:58 -0700 to 2022-10-08 02:43:26 -0700
- Typo 'a' -&gt; 'an' (rust-lang/reference#1280)
- One line one sentence for expressions and statements main chapters (rust-lang/reference#1277)
- Document let else statements (rust-lang/reference#1156)
- Document `label_break_value` in the reference (rust-lang/reference#1263)
- Document target_has_atomic (rust-lang/reference#1171)
- update 'unsafe' (rust-lang/reference#1278)
- Update tokens.md (rust-lang/reference#1276)
- One sentence, one line Patterns chapter (rust-lang/reference#1275)
- Use semver-compliant example version (rust-lang/reference#1272)

## rust-by-example

9 commits in 767a6bd9727a596d7cfdbaeee475e65b2670ea3a..5e7b296d6c345addbd748f242aae28c42555c015
2022-09-14 09:17:18 -0300 to 2022-10-05 08:24:45 -0300
- Make it clear that rustdoc uses the commonmark spec (rust-lang/rust-by-example#1622)
- Update defaults.md (rust-lang/rust-by-example#1615)
- added "see also" for the @ binding sigil (rust-lang/rust-by-example#1612)
- add more precision to the effects of --bin flag (rust-lang/rust-by-example#1607)
- create bar project in cargo/dependencies example (rust-lang/rust-by-example#1606)
- use consistent wording about type annotation (rust-lang/rust-by-example#1603)
- cast.md improvements (rust-lang/rust-by-example#1599)
- Fix typo in macros.md (rust-lang/rust-by-example#1598)
- Corrected mistaken "The" instead of "There" (rust-lang/rust-by-example#1617)

## rustc-dev-guide

2 commits in 9a86c0467bbe42056f73fdf5b03fff757d7c4a9b..7518c3445dc02df0d196f5f84e568d633c5141fb
2022-10-07 18:34:51 +0200 to 2022-10-08 12:29:47 +0200
- Update debugging.md
- Use llvm subdomain for compiler-explorer link

## embedded-book

1 commits in 4ce51cb7441a6f02b5bf9b07b2eb755c21ab7954..c533348edd69f11a8f4225d633a05d7093fddbf3
2022-09-15 08:53:09 +0000 to 2022-10-10 10:16:49 +0000
- Fix a typo in registers.md  (rust-embedded/book#330)
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.

6 participants