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

Recover from for-else and while-else #108427

Merged
merged 1 commit into from Mar 1, 2023
Merged

Conversation

y21
Copy link
Member

@y21 y21 commented Feb 24, 2023

This recovers from attempts at writing for-else or while-else loops, which might help users coming from e.g. Python.

for _ in 0..0 {
  // ...
} else {
  // ...
}

Combined with trying to store it in a let binding, the current diagnostic can be a bit confusing. It mentions let-else and suggests wrapping the loop in parentheses, which the user probably doesn't want. let-else doesn't make sense for for and while loops, as they are of type () (which already is an irrefutable pattern and doesn't need let-else).

Current diagnostic
error: right curly brace `}` before `else` in a `let...else` statement not allowed
 --> src/main.rs:4:5
  |
4 |     } else {
  |     ^
  |
help: wrap the expression in parentheses
  |
2 ~     let _x = (for _ in 0..0 {
3 |         
4 ~     }) else {
  |

Some questions:

  • Can the wording for the error message be improved? Would "for...else loops are not allowed" fit better?
  • Should we be more "conservative" in case we want to support this in the future (i.e. say "for...else loops are currently not allowed/supported")?
  • Is there a better way than storing a &'static str for the loop type? It is used for substituting the placeholder in the locale file (since it can emit either for...else or while...else). Maybe there is an enum I could use that I couldn't find

@rustbot
Copy link
Collaborator

rustbot commented Feb 24, 2023

r? @michaelwoerister

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 24, 2023
Copy link
Member

@Nilstrieb Nilstrieb left a comment

Choose a reason for hiding this comment

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

Thanks! Having a dedicated error is great, but I don't like the suggestion.

compiler/rustc_parse/src/errors.rs Outdated Show resolved Hide resolved
@michaelwoerister
Copy link
Member

r? diagnostics

@mejrs
Copy link
Contributor

mejrs commented Feb 24, 2023

When this sort of thing comes up I tend to recommend https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.try_for_each, and ControlFlow, which seems to work for most people.

Maybe there is an enum I could use that I couldn't find

You can invent one, and implement IntoDiagnosticArg if you want to insert it into the string.

Or you can change your error type to:

pub enum LoopElseNotSupported{
    While { .. },
    For { .. }
}

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

Thanks! This looks great. One suggestion?

compiler/rustc_parse/src/parser/expr.rs Show resolved Hide resolved
@mejrs
Copy link
Contributor

mejrs commented Feb 24, 2023

As for the suggestion, remember that the user themselves might not know what they want. When you ask people what they think Python's for..else does you'll get a split of:

  • The else body is executed if the loop body is never executed
  • The else body is executed if the loop manages to complete fully (i.e. without break).

In Rust, a reasonable interpretation might also be "the else branch is executed if the loop never breaks with a value" i.e. they wrote

for _ in iterator {
  break value
} else {
  fallback_value
}

@y21
Copy link
Member Author

y21 commented Feb 24, 2023

By the way, I intentionally did not add this recovery to loop {} else {} (only for and while) because I don't think such code actually makes sense to write (there is no condition that can become false, you can only manually break out of it).
Should I add it anyway, for consistency?

@compiler-errors
Copy link
Member

I don't see why not, especially since pointing out the loop keyword that the else is associated might help the user realized they got their braces wrong.

@rust-log-analyzer

This comment has been minimized.

@y21
Copy link
Member Author

y21 commented Feb 24, 2023

There was a failing ui test under let-else that was checking for the old error when let pat = loop {} else {}; is encountered.
I moved it to its own file, because the new error isn't about let-else anymore. Not sure if that test makes sense anymore tho or if I should just remove it entirely.

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 1, 2023

📌 Commit 13b8497 has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 1, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 1, 2023
…r-errors

Recover from for-else and while-else

This recovers from attempts at writing for-else or while-else loops, which might help users coming from e.g. Python.
```rs
for _ in 0..0 {
  // ...
} else {
  // ...
}
```
Combined with trying to store it in a let binding, the current diagnostic can be a bit confusing. It mentions let-else and suggests wrapping the loop in parentheses, which the user probably doesn't want. let-else doesn't make sense for `for` and `while` loops, as they are of type `()` (which already is an irrefutable pattern and doesn't need let-else).
<details>
<summary>Current diagnostic</summary>

```rs
error: right curly brace `}` before `else` in a `let...else` statement not allowed
 --> src/main.rs:4:5
  |
4 |     } else {
  |     ^
  |
help: wrap the expression in parentheses
  |
2 ~     let _x = (for _ in 0..0 {
3 |
4 ~     }) else {
  |
```
</details>

Some questions:
- Can the wording for the error message be improved? Would "for...else loops are not allowed" fit better?
- Should we be more "conservative" in case we want to support this in the future (i.e. say "for...else loops are **currently** not allowed/supported")?
- Is there a better way than storing a `&'static str` for the loop type? It is used for substituting the placeholder in the locale file (since it can emit either `for...else` or `while...else`). Maybe there is an enum I could use that I couldn't find
@y21
Copy link
Member Author

y21 commented Mar 1, 2023

Looks like this failed in a rollup because Parser::parse_else_expr was removed, or, I guess renamed to parse_expr_else.
Rebased/updated the PR.

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 1, 2023

📌 Commit 0758c05 has been approved by compiler-errors

It is now in the queue for this repository.

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 1, 2023
Rollup of 7 pull requests

Successful merges:

 - rust-lang#108143 (rustdoc: search by macro when query ends with `!`)
 - rust-lang#108394 (Make `x doc --open` work on every book)
 - rust-lang#108427 (Recover from for-else and while-else)
 - rust-lang#108462 (Fix `VecDeque::append` capacity overflow for ZSTs)
 - rust-lang#108568 (Make associated_item_def_ids for traits use an unstable option to also return associated types for RPITITs)
 - rust-lang#108604 (Add regression test for rust-lang#107280)
 - rust-lang#108605 (Add regression test for rust-lang#105821)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 27fa7b5 into rust-lang:master Mar 1, 2023
@rustbot rustbot added this to the 1.69.0 milestone Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants