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

Confusing error now saying let (not if let) is an expression, rather than a statement, which contradicts the book #65254

Closed
carols10cents opened this issue Oct 10, 2019 · 14 comments · Fixed by #65893
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints D-confusing Diagnostics: Confusing error or lint that should be reworked. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. F-let_chains `#![feature(let_chains)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@carols10cents
Copy link
Member

carols10cents commented Oct 10, 2019

cc @Centril

Code first. This code:

fn main() {
    let x = (let y = 6);
}

on current stable Rust 1.38, gives this set of error messages:

error[E0658]: `let` expressions in this position are experimental
 --> src/main.rs:2:14
  |
2 |     let x = (let y = 6);
  |              ^^^^^^^^^
  |
  = note: for more information, see https://github.com/rust-lang/rust/issues/53667

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

warning: unnecessary parentheses around assigned value
 --> src/main.rs:2:13
  |
2 |     let x = (let y = 6);
  |             ^^^^^^^^^^^ help: remove these parentheses
  |
  = note: `#[warn(unused_parens)]` on by default

The part that I'm most concerned about is:

error: `let` expressions are not supported here

Before eRFC "if- and while-let-chains, take 2" (rust-lang/rfcs#2497, #53667, #53668), this code USED to result in this error message:

error: expected expression, found statement (`let`)
 --> src/main.rs:2:14
  |
2 |     let x = (let y = 6);
  |              ^^^
  |
  = note: variable declaration using `let` is a statement

The reason I know this is because we have the old error message in the book, in the section where we're trying to explain the difference between statements and expressions. The error message I'm seeing now is muddying the waters by saying "let expressions".

Based on my reading of the eRFC, it's only supposed to change if let, but as kind of a side effect let is now sort-of an expression? The updates to the reference don't clear it up for me, as they only describe if let, not plain lets.

What I expected is that even though the eRFC has been accepted and implemented, plain let y = 6 would still be considered a statement and the error message wouldn't talk about "let expressions".

If my expectation is valid, then the compiler error message is a bug. If my expectation is invalid, please let me know so that I can work on updating the book. Thanks!

This issue has been assigned to @jafern14 via this comment.

@Centril
Copy link
Contributor

Centril commented Oct 10, 2019

Also added notes in #docs if sync discussion is desired.


So the new compiler error, introduced in #60861, here is by design and tested in https://github.com/rust-lang/rust/blob/master/src/test/ui/rfc-2497-if-let-chains/disallowed-positions.rs which has more notes. Specifically, the error here comes from HIR lowering.

The idea is that "let" $pat "=" $expr is a syntactically valid expression in all cases (on nightly). The distinction between semantic and syntactic can be noticed via macros and conditional compilation. Semantically however, let is only allowed in the cases that disallowed-positions.rs notes, namely the cases generated by the grammar Expr:

Expr =
  | ...
  | "if" ExprWithLet Block {"else" Block}?
  | {Label ":"}? "while" ExprWithLet Block
  ;

ExprWithLet =
  | "let" PatTop "=" Expr
  | ExprWithLet "&&" ExprWithLet 
  | "(" ExprWithLet ")"
  | Expr
  ;

Eventually however, e.g. with a follow-up RFC, I would like to turn let into a true semantic expression typed at bool such that e.g. if !let Foo::Bar = x { ... } becomes legal. So overall the goal is to "muddy the waters".

The reference has not been updated because we only make updates to it once a feature becomes stable.


With all that said, I'm open to tweaks to the error message (it's optimized for suggesting that e.g. if foo || let A(x) = bar { ... } is not legal). For example, we could have a different note for stable compilers or when the feature gate is not active. cc @estebank

@Centril Centril added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. D-confusing Diagnostics: Confusing error or lint that should be reworked. F-let_chains `#![feature(let_chains)]` labels Oct 10, 2019
@carols10cents
Copy link
Member Author

I would be happy with a different error message for stable compilers that continued to call let outside of if let a statement rather than an expression.

@Centril
Copy link
Contributor

Centril commented Oct 10, 2019

Should be easy to implement by tweaking the HIR lowering code just a tiny bit. Will try to do that sometime during the week but no firm promises. :)

@Centril Centril self-assigned this Oct 10, 2019
@Centril
Copy link
Contributor

Centril commented Oct 10, 2019

...but if anyone wants to work-steal this from me then this would be a good first issue.

You'll want to tweak LoweringContext::lower_expr_let by keeping the current wording when self.sess.parse_sess.unstable_features.is_nightly_build() holds and changing it to @carols10cents's suggested wording (the old one) otherwise.

@Centril Centril added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Oct 10, 2019
@estebank
Copy link
Contributor

There's precedent for different messages depending on whether the current compiler is stable or not and whether there's a feature flag enabled or not. I don't think all three cases are needed here and just gate on the feature being enabled.

@Manishearth
Copy link
Member

I'd go further to say that we should be doing what @estebank describes most of the time: unless a feature is enabled, and the old diagnostics were useful, don't spit out new "you tried to use this feature but it's unstable" diagnostics that aren't helpful.

@RobbieMcKinstry
Copy link

I'll take a stab at it. First time contributor, so I might not know all the hurtles, but I'll take a swing at it.

@Centril
Copy link
Contributor

Centril commented Oct 10, 2019

@rustbot assign @RobbieMcKinstry

@rustbot rustbot assigned rustbot and unassigned Centril Oct 10, 2019
@Aloso
Copy link
Contributor

Aloso commented Oct 13, 2019

Eventually however, e.g. with a follow-up RFC, I would like to turn let into a true semantic expression typed at bool such that e.g. if !let Foo::Bar = x { ... } becomes legal.

@Centril I'm not sure if this is a good idea, because let is usually used to create variable bindings. But with let expressions, the scope of these variables can get much more complicated. Example:

if let Some(x) = foo && !let Some(y) = bar {
    // we can access x, but not y
} else {
    // what about this scope???
}

@Centril
Copy link
Contributor

Centril commented Oct 13, 2019

@Aloso It's probably best to discuss that once an RFC is made but let's not do it here. :)

@jafern14
Copy link

@rustbot claim

@rustbot rustbot assigned rustbot and unassigned rustbot Oct 28, 2019
@jafern14
Copy link

I've got the change locally now with exactly what @Centril had mentioned in a previous message. The only thing I'm getting right now is that the compiler is highlighting the entire statement and not just the let keyword like before.

I'm looking for the method where I can get this. I'll put up a PR in a bit to show where I'm at along with some terminal output to demonstrate a bit better what I'm talking about if it doesn't make sense already.

@jafern14
Copy link

Opened #65893

@estebank
Copy link
Contributor

Don't worry about the span, it is ok as it is in your pr.

@bors bors closed this as completed in 30431a3 Oct 29, 2019
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 D-confusing Diagnostics: Confusing error or lint that should be reworked. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. F-let_chains `#![feature(let_chains)]` 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.

8 participants