Move let expression checking to parsing

There was an incomplete version of the check in parsing and a second
version in AST validation. This meant that some, but not all, invalid
uses were allowed inside macros/disabled cfgs. It also means that later
passes have a hard time knowing when the let expression is in a valid
location, sometimes causing ICEs.

- Add a field to ExprKind::Let in AST/HIR to mark whether it's in a
  valid location.
- Suppress later errors and MIR construction for invalid let
  expressions.

Author: matthewjasper

compiler/rustc_ast/src/ast.rs

@@ -33,7 +33,7 @@ use rustc_macros::HashStable_Generic;
 use rustc_serialize::{Decodable, Decoder, Encodable, Encoder};
 use rustc_span::source_map::{respan, Spanned};
 use rustc_span::symbol::{kw, sym, Ident, Symbol};
-use rustc_span::{Span, DUMMY_SP};
+use rustc_span::{ErrorGuaranteed, Span, DUMMY_SP};
 use std::fmt;
 use std::mem;
 use thin_vec::{thin_vec, ThinVec};
@@ -1426,7 +1426,7 @@ pub enum ExprKind {
     /// of `if` / `while` expressions. (e.g., `if let 0 = x { .. }`).
     ///
     /// `Span` represents the whole `let pat = expr` statement.
-    Let(P<Pat>, P<Expr>, Span),
+    Let(P<Pat>, P<Expr>, Span, Option<ErrorGuaranteed>),
     /// An `if` block, with an optional `else` block.
     ///
     /// `if expr { block } else { expr }`

compiler/rustc_ast/src/mut_visit.rs

@@ -1366,7 +1366,7 @@ pub fn noop_visit_expr<T: MutVisitor>(
             vis.visit_ty(ty);
         }
         ExprKind::AddrOf(_, _, ohs) => vis.visit_expr(ohs),
-        ExprKind::Let(pat, scrutinee, _) => {
+        ExprKind::Let(pat, scrutinee, _, _) => {
             vis.visit_pat(pat);
             vis.visit_expr(scrutinee);
         }

compiler/rustc_ast/src/util/classify.rs

@@ -36,7 +36,7 @@ pub fn expr_trailing_brace(mut expr: &ast::Expr) -> Option<&ast::Expr> {
             | AssignOp(_, _, e)
             | Binary(_, _, e)
             | Break(_, Some(e))
-            | Let(_, e, _)
+            | Let(_, e, _, _)
             | Range(_, Some(e), _)
             | Ret(Some(e))
             | Unary(_, e)

compiler/rustc_ast/src/visit.rs

@@ -827,7 +827,7 @@ pub fn walk_expr<'a, V: Visitor<'a>>(visitor: &mut V, expression: &'a Expr) {
             visitor.visit_expr(subexpression);
             visitor.visit_ty(typ)
         }
-        ExprKind::Let(pat, expr, _) => {
+        ExprKind::Let(pat, expr, _, _) => {
             visitor.visit_pat(pat);
             visitor.visit_expr(expr);
         }

compiler/rustc_ast_lowering/src/expr.rs

@@ -152,13 +152,14 @@ impl<'hir> LoweringContext<'_, 'hir> {
                     let ohs = self.lower_expr(ohs);
                     hir::ExprKind::AddrOf(*k, *m, ohs)
                 }
-                ExprKind::Let(pat, scrutinee, span) => {
+                ExprKind::Let(pat, scrutinee, span, is_recovered) => {
                     hir::ExprKind::Let(self.arena.alloc(hir::Let {
                         hir_id: self.next_id(),
                         span: self.lower_span(*span),
                         pat: self.lower_pat(pat),
                         ty: None,
                         init: self.lower_expr(scrutinee),
+                        is_recovered: *is_recovered,
                     }))
                 }
                 ExprKind::If(cond, then, else_opt) => {
@@ -558,13 +559,14 @@ impl<'hir> LoweringContext<'_, 'hir> {
     fn lower_arm(&mut self, arm: &Arm) -> hir::Arm<'hir> {
         let pat = self.lower_pat(&arm.pat);
         let guard = arm.guard.as_ref().map(|cond| {
-            if let ExprKind::Let(pat, scrutinee, span) = &cond.kind {
+            if let ExprKind::Let(pat, scrutinee, span, is_recovered) = &cond.kind {
                 hir::Guard::IfLet(self.arena.alloc(hir::Let {
                     hir_id: self.next_id(),
                     span: self.lower_span(*span),
                     pat: self.lower_pat(pat),
                     ty: None,
                     init: self.lower_expr(scrutinee),
+                    is_recovered: *is_recovered,
                 }))
             } else {
                 hir::Guard::If(self.lower_expr(cond))

compiler/rustc_ast_passes/messages.ftl

@@ -117,16 +117,6 @@ ast_passes_forbidden_default =
     `default` is only allowed on items in trait impls
     .label = `default` because of this
 
-ast_passes_forbidden_let =
-    `let` expressions are not supported here
-    .note = only supported directly in conditions of `if` and `while` expressions
-    .not_supported_or = `||` operators are not supported in let chain expressions
-    .not_supported_parentheses = `let`s wrapped in parentheses are not supported in a context with let chains
-
-ast_passes_forbidden_let_stable =
-    expected expression, found statement (`let`)
-    .note = variable declaration using `let` is a statement
-
 ast_passes_forbidden_lifetime_bound =
     lifetime bounds cannot be used in this context
 

compiler/rustc_ast_passes/src/ast_validation.rs

@@ -14,14 +14,12 @@ use rustc_ast::{walk_list, StaticItem};
 use rustc_ast_pretty::pprust::{self, State};
 use rustc_data_structures::fx::FxIndexMap;
 use rustc_feature::Features;
-use rustc_macros::Subdiagnostic;
 use rustc_parse::validate_attr;
 use rustc_session::lint::builtin::{
     DEPRECATED_WHERE_CLAUSE_LOCATION, MISSING_ABI, PATTERNS_IN_FNS_WITHOUT_BODY,
 };
 use rustc_session::lint::{BuiltinLintDiagnostics, LintBuffer};
 use rustc_session::Session;
-use rustc_span::source_map::Spanned;
 use rustc_span::symbol::{kw, sym, Ident};
 use rustc_span::Span;
 use rustc_target::spec::abi;
@@ -69,9 +67,6 @@ struct AstValidator<'a> {
     /// or `Foo::Bar<impl Trait>`
     is_impl_trait_banned: bool,
 
-    /// See [ForbiddenLetReason]
-    forbidden_let_reason: Option<ForbiddenLetReason>,
-
     lint_buffer: &'a mut LintBuffer,
 }
 
@@ -118,26 +113,6 @@ impl<'a> AstValidator<'a> {
         self.with_tilde_const(Some(ctx), f)
     }
 
-    fn with_let_management(
-        &mut self,
-        forbidden_let_reason: Option<ForbiddenLetReason>,
-        f: impl FnOnce(&mut Self, Option<ForbiddenLetReason>),
-    ) {
-        let old = mem::replace(&mut self.forbidden_let_reason, forbidden_let_reason);
-        f(self, old);
-        self.forbidden_let_reason = old;
-    }
-
-    /// Emits an error banning the `let` expression provided in the given location.
-    fn ban_let_expr(&self, expr: &'a Expr, forbidden_let_reason: ForbiddenLetReason) {
-        let sess = &self.session;
-        if sess.opts.unstable_features.is_nightly_build() {
-            sess.emit_err(errors::ForbiddenLet { span: expr.span, reason: forbidden_let_reason });
-        } else {
-            sess.emit_err(errors::ForbiddenLetStable { span: expr.span });
-        }
-    }
-
     fn check_type_alias_where_clause_location(
         &mut self,
         ty_alias: &TyAlias,
@@ -779,67 +754,6 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
         validate_attr::check_attr(&self.session.parse_sess, attr);
     }
 
-    fn visit_expr(&mut self, expr: &'a Expr) {
-        self.with_let_management(Some(ForbiddenLetReason::GenericForbidden), |this, forbidden_let_reason| {
-            match &expr.kind {
-                ExprKind::Binary(Spanned { node: BinOpKind::Or, span }, lhs, rhs) => {
-                    let local_reason = Some(ForbiddenLetReason::NotSupportedOr(*span));
-                    this.with_let_management(local_reason, |this, _| this.visit_expr(lhs));
-                    this.with_let_management(local_reason, |this, _| this.visit_expr(rhs));
-                }
-                ExprKind::If(cond, then, opt_else) => {
-                    this.visit_block(then);
-                    walk_list!(this, visit_expr, opt_else);
-                    this.with_let_management(None, |this, _| this.visit_expr(cond));
-                    return;
-                }
-                ExprKind::Let(..) if let Some(elem) = forbidden_let_reason => {
-                    this.ban_let_expr(expr, elem);
-                },
-                ExprKind::Match(scrutinee, arms) => {
-                    this.visit_expr(scrutinee);
-                    for arm in arms {
-                        this.visit_expr(&arm.body);
-                        this.visit_pat(&arm.pat);
-                        walk_list!(this, visit_attribute, &arm.attrs);
-                        if let Some(guard) = &arm.guard {
-                            this.with_let_management(None, |this, _| {
-                                this.visit_expr(guard)
-                            });
-                        }
-                    }
-                }
-                ExprKind::Paren(local_expr) => {
-                    fn has_let_expr(expr: &Expr) -> bool {
-                        match &expr.kind {
-                            ExprKind::Binary(_, lhs, rhs) => has_let_expr(lhs) || has_let_expr(rhs),
-                            ExprKind::Let(..) => true,
-                            _ => false,
-                        }
-                    }
-                    let local_reason = if has_let_expr(local_expr) {
-                        Some(ForbiddenLetReason::NotSupportedParentheses(local_expr.span))
-                    }
-                    else {
-                        forbidden_let_reason
-                    };
-                    this.with_let_management(local_reason, |this, _| this.visit_expr(local_expr));
-                }
-                ExprKind::Binary(Spanned { node: BinOpKind::And, .. }, ..) => {
-                    this.with_let_management(forbidden_let_reason, |this, _| visit::walk_expr(this, expr));
-                    return;
-                }
-                ExprKind::While(cond, then, opt_label) => {
-                    walk_list!(this, visit_label, opt_label);
-                    this.visit_block(then);
-                    this.with_let_management(None, |this, _| this.visit_expr(cond));
-                    return;
-                }
-                _ => visit::walk_expr(this, expr),
-            }
-        });
-    }
-
     fn visit_ty(&mut self, ty: &'a Ty) {
         self.visit_ty_common(ty);
         self.deny_anon_struct_or_union(ty);
@@ -1601,26 +1515,9 @@ pub fn check_crate(
         outer_impl_trait: None,
         disallow_tilde_const: None,
         is_impl_trait_banned: false,
-        forbidden_let_reason: Some(ForbiddenLetReason::GenericForbidden),
         lint_buffer: lints,
     };
     visit::walk_crate(&mut validator, krate);
 
     validator.has_proc_macro_decls
 }
-
-/// Used to forbid `let` expressions in certain syntactic locations.
-#[derive(Clone, Copy, Subdiagnostic)]
-pub(crate) enum ForbiddenLetReason {
-    /// `let` is not valid and the source environment is not important
-    GenericForbidden,
-    /// A let chain with the `||` operator
-    #[note(ast_passes_not_supported_or)]
-    NotSupportedOr(#[primary_span] Span),
-    /// A let chain with invalid parentheses
-    ///
-    /// For example, `let 1 = 1 && (expr && expr)` is allowed
-    /// but `(let 1 = 1 && (let 1 = 1 && (let 1 = 1))) && let a = 1` is not
-    #[note(ast_passes_not_supported_parentheses)]
-    NotSupportedParentheses(#[primary_span] Span),
-}

compiler/rustc_ast_passes/src/errors.rs

@@ -5,27 +5,8 @@ use rustc_errors::AddToDiagnostic;
 use rustc_macros::{Diagnostic, Subdiagnostic};
 use rustc_span::{symbol::Ident, Span, Symbol};
 
-use crate::ast_validation::ForbiddenLetReason;
 use crate::fluent_generated as fluent;
 
-#[derive(Diagnostic)]
-#[diag(ast_passes_forbidden_let)]
-#[note]
-pub struct ForbiddenLet {
-    #[primary_span]
-    pub span: Span,
-    #[subdiagnostic]
-    pub(crate) reason: ForbiddenLetReason,
-}
-
-#[derive(Diagnostic)]
-#[diag(ast_passes_forbidden_let_stable)]
-#[note]
-pub struct ForbiddenLetStable {
-    #[primary_span]
-    pub span: Span,
-}
-
 #[derive(Diagnostic)]
 #[diag(ast_passes_keyword_lifetime)]
 pub struct KeywordLifetime {

compiler/rustc_ast_pretty/src/pprust/state/expr.rs

@@ -352,7 +352,7 @@ impl<'a> State<'a> {
                 self.end();
                 self.word(")");
             }
-            ast::ExprKind::Let(pat, scrutinee, _) => {
+            ast::ExprKind::Let(pat, scrutinee, _, _) => {
                 self.print_let(pat, scrutinee);
             }
             ast::ExprKind::If(test, blk, elseopt) => self.print_if(test, blk, elseopt.as_deref()),

compiler/rustc_builtin_macros/src/assert/context.rs

@@ -241,7 +241,7 @@ impl<'cx, 'a> Context<'cx, 'a> {
                 self.manage_cond_expr(prefix);
                 self.manage_cond_expr(suffix);
             }
-            ExprKind::Let(_, local_expr, _) => {
+            ExprKind::Let(_, local_expr, _, _) => {
                 self.manage_cond_expr(local_expr);
             }
             ExprKind::Match(local_expr, _) => {