Auto merge of rust-lang#143213 - dianne:lower-cond-tweaks, r=cjgillot

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

There was some overlap between `rustc_ast_lowering::LoweringContext::lower_cond` and `rustc_hir_analysis::check::region::resolve_expr`, so I've removed the former and migrated its logic to the latter, with some simplifications.

Consequences:
- For `while` and `if` expressions' `let`-chains, this changes the `HirId`s for the `&&`s to properly correspond to their AST nodes. This is how guards were handled already.
- This makes match guards share previously-duplicated logic with `if`/`while` expressions. This will also be used by guard pattern[^1] guards.
- Aside from legacy syntax extensions (e.g. some builtin macros) that directly feed AST to the compiler, it's currently impossible to put attributes directly on `&&` operators in `let` chains[^2]. Nonetheless, attributes on `&&` operators in `let` chains in `if`/`while` expression conditions are no longer silently ignored and will be lowered.
- This no longer wraps conditions in `DropTemps`, so the HIR and THIR will be slightly smaller.
- `DesugaringKind::CondTemporary` is now gone. It's no longer applied to any spans, and all uses of it were dead since they were made to account for `if` and `while` being desugared to `match` on a boolean scrutinee.
- Should be a marginal perf improvement beyond that due to leveraging [`ScopeTree` construction](https://github.com/rust-lang/rust/blob/5e749eb66f93ee998145399fbdde337e57cd72ef/compiler/rustc_hir_analysis/src/check/region.rs#L312-L355)'s clever handling of `&&` and `||`:
  - This removes some unnecessary terminating scopes that were placed around top-level `&&` and `||` operators in conditions. When lowered to MIR, logical operator chains don't create intermediate boolean temporaries, so there's no temporary to drop. The linked snippet handles wrapping the operands in terminating scopes as necessary, in case they create temporaries.
  - The linked snippet takes care of letting `let` temporaries live and terminating other operands, so we don't need separate traversals of `&&` chains for that.

[^1]: rust-lang#129967
[^2]: Case-by-case, here's my justification: `#[attr] e1 && e2` applies the attribute to `e1`. In `#[attr] (e1 && e2)` , the attribute is on the parentheses in the AST, plus it'd fail to parse if `e1` or `e2` contains a `let`. In `#[attr] expands_to_let_chain!()`, the attribute would already be ignored (rust-lang#63221) and it'd fail to parse anyway; even if the expansion site is a condition, the expansion wouldn't be parsed with `Restrictions::ALLOW_LET`. If it *was* allowed, the notion of a "reparse context" from rust-lang#61733 (comment) would be necessary in order to make `let`-chains left-associative; multiple places in the compiler assume they are.

Author: bors

compiler/rustc_ast_lowering/src/expr.rs

@@ -528,7 +528,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(
@@ -541,44 +541,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:
     //
     // ```
@@ -602,7 +564,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);
@@ -2091,7 +2053,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

@@ -167,23 +167,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);
 
@@ -340,7 +356,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);
@@ -355,7 +371,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)