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

Misplaced parenthesis in if let produces unhelpful error message #82827

Closed
mcclure opened this issue Mar 6, 2021 · 5 comments · Fixed by #82854
Closed

Misplaced parenthesis in if let produces unhelpful error message #82827

mcclure opened this issue Mar 6, 2021 · 5 comments · Fixed by #82854
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-parser Area: The parsing of Rust source code to an AST. C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@mcclure
Copy link

mcclure commented Mar 6, 2021

I am learning Rust. I haven't read the full manual yet, I am just bumbling through. Tonight I hit an error message that confused me. I am using Rust 1.50.0 on OS X 10.13.6, but I can reproduce the problem on Godbolt. Here's my Godbolt repro:

// Type your code here, or load an example.
pub fn main() {
  let x = Some(3);
  if (let Some(y) = x) {
    println!("{}", y)
  } else {
    println!("None")
  }
}

This is wrong because the parenthesis between if and let confuses Rust completely. If you remove the parenthesis (Godbolt link) it works fine. But I didn't know that.

The wrong version produces the following error messages:

error[E0658]: `let` expressions in this position are experimental
 --> <source>:4:7
  |
4 |   if (let Some(y) = x) {
  |       ^^^^^^^^^^^^^^^
  |
  = note: see issue #53667 <https://github.com/rust-lang/rust/issues/53667> for more information

error: expected expression, found statement (`let`)
 --> <source>:4:7
  |
4 |   if (let Some(y) = x) {
  |       ^^^^^^^^^^^^^^^
  |
  = note: variable declaration using `let` is a statement

warning: unnecessary parentheses around `if` condition
 --> <source>:4:6
  |
4 |   if (let Some(y) = x) {
  |      ^^^^^^^^^^^^^^^^^ help: remove these parentheses
  |
  = note: `#[warn(unused_parens)]` on by default

error: aborting due to 2 previous errors; 1 warning emitted

In my opinion, this is not as helpful as it could be.

The "experimental" error mislead me badly. First off, saying something is "experimental" does not mean it is wrong, it could mean "this will be legal in the next released version". Second off, the "experimental" error does not say what is experimental. Is it destructuring which is not allowed? Is it destructuring *in this position* that is not allowed? (In context-- IE not this minimal example-- I had the if embedded as an expression inside a match.) The linked github issue is over two years old and is not enlightening, in fact, it's just frankly kinda scary.

The "expected expression, found statement" error is potentially helpful, but confused me a little, because I'd used "if let" before— and I wasn't paying as much attention to it anyway, because (C habit that I shouldn't be bringing with me into Rust?) if I get two errors on the same line, I assume the first one is the "real" error and the second one is just the compiler being thrown off by the first error.

The warning is a good hint (I had a conversation on twitter about this) but first off it looks like a style recommendation, not a functional one; and also I did not see it.

My expected behavior:

  • Because putting the parenthesis in the wrong place when doing an if let is a likely new-user "gotcha" (especially for C++ programmers, where the equivalent of if (let x = ...) has been a syntax since C++03), it would be great if the compiler would detect this would have been correct syntax without the parenthesis and print a "hint: there are no parenthesis in if let, recommend remove parenthesis" there.
  • A more nebulous thought: You should present those "is experimental" errors in a very different way, if you display them at all! They're scary, they're not actionable, they could be misleading. One possibility (I assume this has been well hashed out, but) would be to treat the experimental features as like a third kind of error besides error and warning, something like "note: this would have been legal in the 0.51 beta" (and maybe they shouldn't be printed with the errors— maybe they should be sorted down with the warnings? I wouldn't have focused on the "experimental" error, when I got it, if it weren't first).

Postscript: I had some thoughts about that parenthesis warning. I thought they would be distracting in this issue, and those thoughts were more like a feature request or a question than a bug report, so as your issue form suggests I made a forum thread about that instead.

@mcclure mcclure added the C-bug Category: This is a bug. label Mar 6, 2021
@SNCPlay42
Copy link
Contributor

Oddly enough, simply switching to a nightly compiler changes the second, "expected expression, found statement" error (not because the code has been changed, but because the compiler actually checks if it's a nightly build):

error: `let` expressions are not supported here
 --> src/main.rs:3:7
  |
3 |   if (let Some(y) = x) {
  |       ^^^^^^^^^^^^^^^
  |
  = note: only supported directly in conditions of `if`- and `while`-expressions
  = note: as well as when nested within `&&` and parenthesis in those conditions

Which is maybe clearer (if the user realizes "directly" means "without parentheses"; the second note does hint that parentheses are significant). I think the intent of changing the message like this is to avoid referring to "let expressions" on the stable compiler - they don't exist in stable syntax, if let and while let are their own special form of if/while statement as far as stable syntax is concerned - but the stable message has forgotten that if let and while let exist at all. "variable declaration using let is a statement" is misleading by omission.

I think it would be possible to avoid emitting the "experimental" error in any situation that causes the second error to be emitted - whether the second error is emitted isn't changed by the presence of nightly features (only its text), so any code that causes the second error is invalid regardless of whether the feature is enabled.

The "unnecessary parentheses" warning really is a style lint and it's sort of a coincidence that it's a helpful hint here. One of the previous two errors should be made to recognize that this is valid syntax without the parentheses and suggest removing them itself.

@rustbot label A-parser A-diagnostics T-compiler

@rustbot rustbot added A-diagnostics Area: Messages for errors, warnings, and lints A-parser Area: The parsing of Rust source code to an AST. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 6, 2021
@estebank
Copy link
Contributor

estebank commented Mar 7, 2021

#82854:

error[E0658]: `let` expressions in this position are experimental
 --> f10.rs:4:9
  |
4 |     if (let Some(x) = y) {
  |         ^^^^^^^^^^^^^^^
  |
  = note: see issue #53667 <https://github.com/rust-lang/rust/issues/53667> for more information
  = help: add `#![feature(let_chains)]` to the crate attributes to enable

error: invalid parentheses around `let` expression in `if let`
 --> f10.rs:4:8
  |
4 |     if (let Some(x) = y) {
  |        ^               ^
  |
  = note: only supported directly without parentheses in conditions of `if`- and `while`-expressions, as well as in `let` chains within parentheses
help: `if let` needs to be written without parentheses
  |
4 |     if let Some(x) = y {
  |       --             --

error: aborting due to 2 previous errors

The PR accounts for any number of unnecessary parentheses.

I think there would still be value in eliminating the first error entirely, but that will require a larger refactoring than I wish to embark on right now 😅

estebank added a commit to estebank/rust that referenced this issue Mar 7, 2021
@bors bors closed this as completed in 6a55aa1 Mar 9, 2021
@atsuzaki
Copy link
Contributor

IMHO, I think this issue should stay open until this error message is eliminated:

error[E0658]: `let` expressions in this position are experimental
 --> f10.rs:4:9
  |
4 |     if (let Some(x) = y) {
  |         ^^^^^^^^^^^^^^^
  |
  = note: see issue #53667 <https://github.com/rust-lang/rust/issues/53667> for more information
  = help: add `#![feature(let_chains)]` to the crate attributes to enable

I just ran into this and got really confused before I ran into this issue.

@estebank
Copy link
Contributor

@atsuzaki in nightly, the output is

error[E0658]: `let` expressions in this position are experimental
 --> src/main.rs:4:7
  |
4 |   if (let Some(y) = x) {
  |       ^^^^^^^^^^^^^^^
  |
  = note: see issue #53667 <https://github.com/rust-lang/rust/issues/53667> for more information
  = help: add `#![feature(let_chains)]` to the crate attributes to enable
  = help: you can write `matches!(<expr>, <pattern>)` instead of `let <pattern> = <expr>`

error: invalid parentheses around `let` expression in `if let`
 --> src/main.rs:4:6
  |
4 |   if (let Some(y) = x) {
  |      ^               ^
  |
help: `if let` needs to be written without parentheses
  |
4 |   if let Some(y) = x {
  |     --             --

We close tickets when the problem has been fixed in master, we can't keep tickets open beyond that because the logistics to close them would become too onerous (but maybe we could fix that in the future with tooling).

The lint now mentions that you might have wanted matches!(y, Some(x)) instead there, and the second error gives you the right solution. We have multiple cases where we emit more errors than strictly necessary, and we do consider that a problem.

If you don't mind, could you open a new ticket dedicated only to the silencing of the lint in this case (linking to this ticket)? That makes it easier to track what still needs to be done, instead of changing the labels after the fact to change its categorization. Doing it that way makes traceability of the work done on the project possible.

@atsuzaki
Copy link
Contributor

If you don't mind, could you open a new ticket dedicated only to the silencing of the lint in this case (linking to this ticket)? That makes it easier to track what still needs to be done, instead of changing the labels after the fact to change its categorization. Doing it that way makes traceability of the work done on the project possible.

Thanks a bunch, I've opened #83274!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-parser Area: The parsing of Rust source code to an AST. C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants