de-duplicate condition scoping logic between AST→HIR lowering and ScopeTree construction

compiler/rustc_ast_lowering/src/expr.rs

@@ -532,7 +532,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
         then: &Block,
         else_opt: Option<&Expr>,
     ) -> hir::ExprKind<'hir> {
-        let lowered_cond = self.lower_cond(cond);
+        let lowered_cond = self.lower_expr(cond);
         let then_expr = self.lower_block_expr(then);
         if let Some(rslt) = else_opt {
             hir::ExprKind::If(
@@ -545,44 +545,6 @@ impl<'hir> LoweringContext<'_, 'hir> {
         }
     }
 
-    // Lowers a condition (i.e. `cond` in `if cond` or `while cond`), wrapping it in a terminating scope
-    // so that temporaries created in the condition don't live beyond it.
-    fn lower_cond(&mut self, cond: &Expr) -> &'hir hir::Expr<'hir> {
-        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,
-            }
-        }
-
-        // We have to take special care for `let` exprs in the condition, e.g. in
-        // `if let pat = val` or `if foo && let pat = val`, as we _do_ want `val` to live beyond the
-        // condition in this case.
-        //
-        // In order to maintain the drop behavior for the non `let` parts of the condition,
-        // we still wrap them in terminating scopes, e.g. `if foo && let pat = val` essentially
-        // gets transformed into `if { let _t = foo; _t } && let pat = val`
-        match &cond.kind {
-            ExprKind::Binary(op @ Spanned { node: ast::BinOpKind::And, .. }, lhs, rhs)
-                if has_let_expr(cond) =>
-            {
-                let op = self.lower_binop(*op);
-                let lhs = self.lower_cond(lhs);
-                let rhs = self.lower_cond(rhs);
-
-                self.arena.alloc(self.expr(cond.span, hir::ExprKind::Binary(op, lhs, rhs)))
-            }
-            ExprKind::Let(..) => self.lower_expr(cond),
-            _ => {
-                let cond = self.lower_expr(cond);
-                let reason = DesugaringKind::CondTemporary;
-                let span_block = self.mark_span_with_reason(reason, cond.span, None);
-                self.expr_drop_temps(span_block, cond)
-            }
-        }
-    }
-
     // We desugar: `'label: while $cond $body` into:
     //
     // ```
@@ -606,7 +568,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
         body: &Block,
         opt_label: Option<Label>,
     ) -> hir::ExprKind<'hir> {
-        let lowered_cond = self.with_loop_condition_scope(|t| t.lower_cond(cond));
+        let lowered_cond = self.with_loop_condition_scope(|t| t.lower_expr(cond));
         let then = self.lower_block_expr(body);
         let expr_break = self.expr_break(span);
         let stmt_break = self.stmt_expr(span, expr_break);
@@ -2095,7 +2057,8 @@ impl<'hir> LoweringContext<'_, 'hir> {
     /// In terms of drop order, it has the same effect as wrapping `expr` in
     /// `{ let _t = $expr; _t }` but should provide better compile-time performance.
     ///
-    /// The drop order can be important in e.g. `if expr { .. }`.
+    /// The drop order can be important, e.g. to drop temporaries from an `async fn`
+    /// body before its parameters.
     pub(super) fn expr_drop_temps(
         &mut self,
         span: Span,

compiler/rustc_hir_analysis/src/check/region.rs

@@ -176,23 +176,39 @@ fn resolve_block<'tcx>(
     visitor.cx = prev_cx;
 }
 
-fn resolve_arm<'tcx>(visitor: &mut ScopeResolutionVisitor<'tcx>, arm: &'tcx hir::Arm<'tcx>) {
-    fn has_let_expr(expr: &Expr<'_>) -> bool {
-        match &expr.kind {
-            hir::ExprKind::Binary(_, lhs, rhs) => has_let_expr(lhs) || has_let_expr(rhs),
-            hir::ExprKind::Let(..) => true,
-            _ => false,
-        }
-    }
+/// Resolve a condition from an `if` expression or match guard so that it is a terminating scope
+/// if it doesn't contain `let` expressions.
+fn resolve_cond<'tcx>(visitor: &mut ScopeResolutionVisitor<'tcx>, cond: &'tcx hir::Expr<'tcx>) {
+    let terminate = match cond.kind {
+        // Temporaries for `let` expressions must live into the success branch.
+        hir::ExprKind::Let(_) => false,
+        // Logical operator chains are handled in `resolve_expr`. Since logical operator chains in
+        // conditions are lowered to control-flow rather than boolean temporaries, there's no
+        // temporary to drop for logical operators themselves. `resolve_expr` will also recursively
+        // wrap any operands in terminating scopes, other than `let` expressions (which we shouldn't
+        // terminate) and other logical operators (which don't need a terminating scope, since their
+        // operands will be terminated). Any temporaries that would need to be dropped will be
+        // dropped before we leave this operator's scope; terminating them here would be redundant.
+        hir::ExprKind::Binary(
+            source_map::Spanned { node: hir::BinOpKind::And | hir::BinOpKind::Or, .. },
+            _,
+            _,
+        ) => false,
+        // Otherwise, conditions should always drop their temporaries.
+        _ => true,
+    };
+    resolve_expr(visitor, cond, terminate);
+}
 
+fn resolve_arm<'tcx>(visitor: &mut ScopeResolutionVisitor<'tcx>, arm: &'tcx hir::Arm<'tcx>) {
     let prev_cx = visitor.cx;
 
     visitor.enter_node_scope_with_dtor(arm.hir_id.local_id, true);
     visitor.cx.var_parent = visitor.cx.parent;
 
     resolve_pat(visitor, arm.pat);
     if let Some(guard) = arm.guard {
-        resolve_expr(visitor, guard, !has_let_expr(guard));
+        resolve_cond(visitor, guard);
     }
     resolve_expr(visitor, arm.body, false);
 
@@ -420,7 +436,7 @@ fn resolve_expr<'tcx>(
             };
             visitor.enter_scope(Scope { local_id: then.hir_id.local_id, data });
             visitor.cx.var_parent = visitor.cx.parent;
-            visitor.visit_expr(cond);
+            resolve_cond(visitor, cond);
             resolve_expr(visitor, then, true);
             visitor.cx = expr_cx;
             resolve_expr(visitor, otherwise, true);
@@ -435,7 +451,7 @@ fn resolve_expr<'tcx>(
             };
             visitor.enter_scope(Scope { local_id: then.hir_id.local_id, data });
             visitor.cx.var_parent = visitor.cx.parent;
-            visitor.visit_expr(cond);
+            resolve_cond(visitor, cond);
             resolve_expr(visitor, then, true);
             visitor.cx = expr_cx;
         }

compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs

@@ -51,12 +51,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         };
 
         match span.desugaring_kind() {
-            // If span arose from a desugaring of `if` or `while`, then it is the condition
-            // itself, which diverges, that we are about to lint on. This gives suboptimal
-            // diagnostics. Instead, stop here so that the `if`- or `while`-expression's
-            // block is linted instead.
-            Some(DesugaringKind::CondTemporary) => return,
-
             // Don't lint if the result of an async block or async function is `!`.
             // This does not affect the unreachable lints *within* the body.
             Some(DesugaringKind::Async) => return,

compiler/rustc_hir_typeck/src/lib.rs

@@ -428,11 +428,9 @@ fn report_unexpected_variant_res(
                 }
                 hir::Node::Expr(hir::Expr { kind: hir::ExprKind::Binary(..), hir_id, .. }) => {
                     suggestion.push((expr.span.shrink_to_lo(), "(".to_string()));
-                    if let hir::Node::Expr(drop_temps) = tcx.parent_hir_node(*hir_id)
-                        && let hir::ExprKind::DropTemps(_) = drop_temps.kind
-                        && let hir::Node::Expr(parent) = tcx.parent_hir_node(drop_temps.hir_id)
+                    if let hir::Node::Expr(parent) = tcx.parent_hir_node(*hir_id)
                         && let hir::ExprKind::If(condition, block, None) = parent.kind
-                        && condition.hir_id == drop_temps.hir_id
+                        && condition.hir_id == *hir_id
                         && let hir::ExprKind::Block(block, _) = block.kind
                         && block.stmts.is_empty()
                         && let Some(expr) = block.expr

compiler/rustc_hir_typeck/src/pat.rs

@@ -24,7 +24,6 @@ use rustc_session::lint::builtin::NON_EXHAUSTIVE_OMITTED_PATTERNS;
 use rustc_session::parse::feature_err;
 use rustc_span::edit_distance::find_best_match_for_name;
 use rustc_span::edition::Edition;
-use rustc_span::hygiene::DesugaringKind;
 use rustc_span::source_map::Spanned;
 use rustc_span::{BytePos, DUMMY_SP, Ident, Span, kw, sym};
 use rustc_trait_selection::infer::InferCtxtExt;
@@ -902,16 +901,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         // then that's equivalent to there existing a LUB.
         let cause = self.pattern_cause(ti, span);
         if let Err(err) = self.demand_suptype_with_origin(&cause, expected, pat_ty) {
-            err.emit_unless(
-                ti.span
-                    .filter(|&s| {
-                        // In the case of `if`- and `while`-expressions we've already checked
-                        // that `scrutinee: bool`. We know that the pattern is `true`,
-                        // so an error here would be a duplicate and from the wrong POV.
-                        s.is_desugaring(DesugaringKind::CondTemporary)
-                    })
-                    .is_some(),
-            );
+            err.emit();
         }
 
         pat_ty

compiler/rustc_span/src/hygiene.rs

@@ -1191,11 +1191,6 @@ impl AstPass {
 /// The kind of compiler desugaring.
 #[derive(Clone, Copy, PartialEq, Debug, Encodable, Decodable, HashStable_Generic)]
 pub enum DesugaringKind {
-    /// We desugar `if c { i } else { e }` to `match $ExprKind::Use(c) { true => i, _ => e }`.
-    /// However, we do not want to blame `c` for unreachability but rather say that `i`
-    /// is unreachable. This desugaring kind allows us to avoid blaming `c`.
-    /// This also applies to `while` loops.
-    CondTemporary,
     QuestionMark,
     TryBlock,
     YeetExpr,
@@ -1230,7 +1225,6 @@ impl DesugaringKind {
     /// The description wording should combine well with "desugaring of {}".
     pub fn descr(self) -> &'static str {
         match self {
-            DesugaringKind::CondTemporary => "`if` or `while` condition",
             DesugaringKind::Async => "`async` block or function",
             DesugaringKind::Await => "`await` expression",
             DesugaringKind::QuestionMark => "operator `?`",
@@ -1253,7 +1247,6 @@ impl DesugaringKind {
     /// like `from_desugaring = "QuestionMark"`
     pub fn matches(&self, value: &str) -> bool {
         match self {
-            DesugaringKind::CondTemporary => value == "CondTemporary",
             DesugaringKind::Async => value == "Async",
             DesugaringKind::Await => value == "Await",
             DesugaringKind::QuestionMark => value == "QuestionMark",

src/tools/clippy/clippy_lints/src/booleans.rs

@@ -1,5 +1,6 @@
 use clippy_config::Conf;
 use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_hir_and_then};
+use clippy_utils::higher::has_let_expr;
 use clippy_utils::msrvs::{self, Msrv};
 use clippy_utils::source::SpanRangeExt;
 use clippy_utils::sugg::Sugg;
@@ -646,7 +647,9 @@ impl<'tcx> Visitor<'tcx> for NonminimalBoolVisitor<'_, 'tcx> {
     fn visit_expr(&mut self, e: &'tcx Expr<'_>) {
         if !e.span.from_expansion() {
             match &e.kind {
-                ExprKind::Binary(binop, _, _) if binop.node == BinOpKind::Or || binop.node == BinOpKind::And => {
+                ExprKind::Binary(binop, _, _)
+                    if binop.node == BinOpKind::Or || binop.node == BinOpKind::And && !has_let_expr(e) =>
+                {
                     self.bool_expr(e);
                 },
                 ExprKind::Unary(UnOp::Not, inner) => {

src/tools/clippy/clippy_lints/src/if_not_else.rs

@@ -51,7 +51,6 @@ declare_lint_pass!(IfNotElse => [IF_NOT_ELSE]);
 impl LateLintPass<'_> for IfNotElse {
     fn check_expr(&mut self, cx: &LateContext<'_>, e: &Expr<'_>) {
         if let ExprKind::If(cond, cond_inner, Some(els)) = e.kind
-            && let ExprKind::DropTemps(cond) = cond.kind
             && let ExprKind::Block(..) = els.kind
         {
             let (msg, help) = match cond.kind {

src/tools/clippy/clippy_lints/src/implicit_saturating_add.rs

@@ -41,8 +41,7 @@ declare_lint_pass!(ImplicitSaturatingAdd => [IMPLICIT_SATURATING_ADD]);
 impl<'tcx> LateLintPass<'tcx> for ImplicitSaturatingAdd {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
         if let ExprKind::If(cond, then, None) = expr.kind
-            && let ExprKind::DropTemps(expr1) = cond.kind
-            && let Some((c, op_node, l)) = get_const(cx, expr1)
+            && let Some((c, op_node, l)) = get_const(cx, cond)
             && let BinOpKind::Ne | BinOpKind::Lt = op_node
             && let ExprKind::Block(block, None) = then.kind
             && let Block {
@@ -66,7 +65,7 @@ impl<'tcx> LateLintPass<'tcx> for ImplicitSaturatingAdd {
             && Some(c) == get_int_max(ty)
             && let ctxt = expr.span.ctxt()
             && ex.span.ctxt() == ctxt
-            && expr1.span.ctxt() == ctxt
+            && cond.span.ctxt() == ctxt
             && clippy_utils::SpanlessEq::new(cx).eq_expr(l, target)
             && AssignOpKind::AddAssign == op1.node
             && let ExprKind::Lit(lit) = value.kind

src/tools/clippy/clippy_lints/src/let_if_seq.rs

@@ -62,15 +62,8 @@ impl<'tcx> LateLintPass<'tcx> for LetIfSeq {
             if let hir::StmtKind::Let(local) = stmt.kind
                 && let hir::PatKind::Binding(mode, canonical_id, ident, None) = local.pat.kind
                 && let hir::StmtKind::Expr(if_) = next.kind
-                && let hir::ExprKind::If(
-                    hir::Expr {
-                        kind: hir::ExprKind::DropTemps(cond),
-                        ..
-                    },
-                    then,
-                    else_,
-                ) = if_.kind
-                && !is_local_used(cx, *cond, canonical_id)
+                && let hir::ExprKind::If(cond, then, else_) = if_.kind
+                && !is_local_used(cx, cond, canonical_id)
                 && let hir::ExprKind::Block(then, _) = then.kind
                 && let Some(value) = check_assign(cx, canonical_id, then)
                 && !is_local_used(cx, value, canonical_id)

src/tools/clippy/clippy_utils/src/higher.rs

@@ -54,7 +54,7 @@ impl<'tcx> ForLoop<'tcx> {
     }
 }
 
-/// An `if` expression without `DropTemps`
+/// An `if` expression without `let`
 pub struct If<'hir> {
     /// `if` condition
     pub cond: &'hir Expr<'hir>,
@@ -66,16 +66,10 @@ pub struct If<'hir> {
 
 impl<'hir> If<'hir> {
     #[inline]
-    /// Parses an `if` expression
+    /// Parses an `if` expression without `let`
     pub const fn hir(expr: &Expr<'hir>) -> Option<Self> {
-        if let ExprKind::If(
-            Expr {
-                kind: ExprKind::DropTemps(cond),
-                ..
-            },
-            then,
-            r#else,
-        ) = expr.kind
+        if let ExprKind::If(cond, then, r#else) = expr.kind
+            && !has_let_expr(cond)
         {
             Some(Self { cond, then, r#else })
         } else {
@@ -198,18 +192,10 @@ impl<'hir> IfOrIfLet<'hir> {
     /// Parses an `if` or `if let` expression
     pub const fn hir(expr: &Expr<'hir>) -> Option<Self> {
         if let ExprKind::If(cond, then, r#else) = expr.kind {
-            if let ExprKind::DropTemps(new_cond) = cond.kind {
-                return Some(Self {
-                    cond: new_cond,
-                    then,
-                    r#else,
-                });
-            }
-            if let ExprKind::Let(..) = cond.kind {
-                return Some(Self { cond, then, r#else });
-            }
+            Some(Self { cond, then, r#else })
+        } else {
+            None
         }
-        None
     }
 }
 
@@ -343,15 +329,7 @@ impl<'hir> While<'hir> {
             Block {
                 expr:
                     Some(Expr {
-                        kind:
-                            ExprKind::If(
-                                Expr {
-                                    kind: ExprKind::DropTemps(condition),
-                                    ..
-                                },
-                                body,
-                                _,
-                            ),
+                        kind: ExprKind::If(condition, body, _),
                         ..
                     }),
                 ..
@@ -360,6 +338,7 @@ impl<'hir> While<'hir> {
             LoopSource::While,
             span,
         ) = expr.kind
+            && !has_let_expr(condition)
         {
             return Some(Self { condition, body, span });
         }
@@ -493,3 +472,13 @@ pub fn get_vec_init_kind<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -
     }
     None
 }
+
+/// Checks that a condition doesn't have a `let` expression, to keep `If` and `While` from accepting
+/// `if let` and `while let`.
+pub const fn has_let_expr<'tcx>(cond: &'tcx Expr<'tcx>) -> bool {
+    match &cond.kind {
+        ExprKind::Let(_) => true,
+        ExprKind::Binary(_, lhs, rhs) => has_let_expr(lhs) || has_let_expr(rhs),
+        _ => false,
+    }
+}

src/tools/clippy/tests/ui/author/if.stdout

@@ -1,8 +1,7 @@
 if let StmtKind::Let(local) = stmt.kind
     && let Some(init) = local.init
     && let ExprKind::If(cond, then, Some(else_expr)) = init.kind
-    && let ExprKind::DropTemps(expr) = cond.kind
-    && let ExprKind::Lit(ref lit) = expr.kind
+    && let ExprKind::Lit(ref lit) = cond.kind
     && let LitKind::Bool(true) = lit.node
     && let ExprKind::Block(block, None) = then.kind
     && block.stmts.len() == 1

src/tools/clippy/tests/ui/author/struct.stdout

@@ -2,8 +2,7 @@ if let ExprKind::Struct(qpath, fields, None) = expr.kind
     && fields.len() == 1
     && fields[0].ident.as_str() == "field"
     && let ExprKind::If(cond, then, Some(else_expr)) = fields[0].expr.kind
-    && let ExprKind::DropTemps(expr1) = cond.kind
-    && let ExprKind::Lit(ref lit) = expr1.kind
+    && let ExprKind::Lit(ref lit) = cond.kind
     && let LitKind::Bool(true) = lit.node
     && let ExprKind::Block(block, None) = then.kind
     && block.stmts.is_empty()