Auto merge of rust-lang#115677 - matthewjasper:let-expr-recovery, r=b…

…-naber

Improve invalid let expression handling

- Move all of the checks for valid let expression positions to parsing.
- Add a field to ExprKind::Let in AST/HIR to mark whether it's in a valid location.
- Suppress some later errors and MIR construction for invalid let expressions.
- Fix a (drop) scope issue that was also responsible for rust-lang#104172.

Fixes rust-lang#104172
Fixes rust-lang#104868

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, _) => {

compiler/rustc_hir/src/hir.rs

@@ -19,6 +19,7 @@ use rustc_macros::HashStable_Generic;
 use rustc_span::hygiene::MacroKind;
 use rustc_span::source_map::Spanned;
 use rustc_span::symbol::{kw, sym, Ident, Symbol};
+use rustc_span::ErrorGuaranteed;
 use rustc_span::{def_id::LocalDefId, BytePos, Span, DUMMY_SP};
 use rustc_target::asm::InlineAsmRegOrRegClass;
 use rustc_target::spec::abi::Abi;
@@ -1415,6 +1416,9 @@ pub struct Let<'hir> {
     pub pat: &'hir Pat<'hir>,
     pub ty: Option<&'hir Ty<'hir>>,
     pub init: &'hir Expr<'hir>,
+    /// `Some` when this let expressions is not in a syntanctically valid location.
+    /// Used to prevent building MIR in such situations.
+    pub is_recovered: Option<ErrorGuaranteed>,
 }
 
 #[derive(Debug, Clone, Copy, HashStable_Generic)]

compiler/rustc_hir_analysis/src/check/region.rs

@@ -149,7 +149,7 @@ fn resolve_block<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, blk: &'tcx h
                     // From now on, we continue normally.
                     visitor.cx = prev_cx;
                 }
-                hir::StmtKind::Local(..) | hir::StmtKind::Item(..) => {
+                hir::StmtKind::Local(..) => {
                     // Each declaration introduces a subscope for bindings
                     // introduced by the declaration; this subscope covers a
                     // suffix of the block. Each subscope in a block has the
@@ -163,6 +163,10 @@ fn resolve_block<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, blk: &'tcx h
                     visitor.cx.var_parent = visitor.cx.parent;
                     visitor.visit_stmt(statement)
                 }
+                hir::StmtKind::Item(..) => {
+                    // Don't create scopes for items, since they won't be
+                    // lowered to THIR and MIR.
+                }
                 hir::StmtKind::Expr(..) | hir::StmtKind::Semi(..) => visitor.visit_stmt(statement),
             }
         }

compiler/rustc_hir_typeck/src/expr.rs

@@ -1203,7 +1203,12 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         // otherwise check exactly as a let statement
         self.check_decl(let_expr.into());
         // but return a bool, for this is a boolean expression
-        self.tcx.types.bool
+        if let Some(error_guaranteed) = let_expr.is_recovered {
+            self.set_tainted_by_errors(error_guaranteed);
+            Ty::new_error(self.tcx, error_guaranteed)
+        } else {
+            self.tcx.types.bool
+        }
     }
 
     fn check_expr_loop(

compiler/rustc_hir_typeck/src/gather_locals.rs

@@ -50,7 +50,7 @@ impl<'a> From<&'a hir::Local<'a>> for Declaration<'a> {
 
 impl<'a> From<&'a hir::Let<'a>> for Declaration<'a> {
     fn from(let_expr: &'a hir::Let<'a>) -> Self {
-        let hir::Let { hir_id, pat, ty, span, init } = *let_expr;
+        let hir::Let { hir_id, pat, ty, span, init, is_recovered: _ } = *let_expr;
         Declaration { hir_id, pat, ty, span, init: Some(init), origin: DeclOrigin::LetExpr }
     }
 }

compiler/rustc_lint/src/unused.rs

@@ -816,8 +816,7 @@ trait UnusedDelimLint {
         let (value, ctx, followed_by_block, left_pos, right_pos, is_kw) = match e.kind {
             // Do not lint `unused_braces` in `if let` expressions.
             If(ref cond, ref block, _)
-                if !matches!(cond.kind, Let(_, _, _))
-                    || Self::LINT_EXPR_IN_PATTERN_MATCHING_CTX =>
+                if !matches!(cond.kind, Let(..)) || Self::LINT_EXPR_IN_PATTERN_MATCHING_CTX =>
             {
                 let left = e.span.lo() + rustc_span::BytePos(2);
                 let right = block.span.lo();
@@ -826,8 +825,7 @@ trait UnusedDelimLint {
 
             // Do not lint `unused_braces` in `while let` expressions.
             While(ref cond, ref block, ..)
-                if !matches!(cond.kind, Let(_, _, _))
-                    || Self::LINT_EXPR_IN_PATTERN_MATCHING_CTX =>
+                if !matches!(cond.kind, Let(..)) || Self::LINT_EXPR_IN_PATTERN_MATCHING_CTX =>
             {
                 let left = e.span.lo() + rustc_span::BytePos(5);
                 let right = block.span.lo();
@@ -1003,7 +1001,7 @@ impl UnusedDelimLint for UnusedParens {
                     self.emit_unused_delims_expr(cx, value, ctx, left_pos, right_pos, is_kw)
                 }
             }
-            ast::ExprKind::Let(_, ref expr, _) => {
+            ast::ExprKind::Let(_, ref expr, _, _) => {
                 self.check_unused_delims_expr(
                     cx,
                     expr,
@@ -1067,7 +1065,7 @@ impl EarlyLintPass for UnusedParens {
         }
 
         match e.kind {
-            ExprKind::Let(ref pat, _, _) | ExprKind::ForLoop(ref pat, ..) => {
+            ExprKind::Let(ref pat, _, _, _) | ExprKind::ForLoop(ref pat, ..) => {
                 self.check_unused_parens_pat(cx, pat, false, false, (true, true));
             }
             // We ignore parens in cases like `if (((let Some(0) = Some(1))))` because we already
@@ -1311,7 +1309,7 @@ impl UnusedDelimLint for UnusedBraces {
                     }
                 }
             }
-            ast::ExprKind::Let(_, ref expr, _) => {
+            ast::ExprKind::Let(_, ref expr, _, _) => {
                 self.check_unused_delims_expr(
                     cx,
                     expr,