From ad8b24272428b28770471f222c19fa3154a65819 Mon Sep 17 00:00:00 2001
From: Nathan Whitaker <nathan.whitaker01@gmail.com>
Date: Wed, 12 Oct 2022 15:29:08 -0700
Subject: [PATCH 1/3] Let chains should still drop temporaries by the end of
 the condition's execution

---
 compiler/rustc_ast_lowering/src/expr.rs | 45 +++++++++++++-----
 src/test/ui/drop/drop_order.rs          | 63 +++++++++++++++++++++++++
 2 files changed, 97 insertions(+), 11 deletions(-)

diff --git a/compiler/rustc_ast_lowering/src/expr.rs b/compiler/rustc_ast_lowering/src/expr.rs
index c55b490630200..104010f8d435e 100644
--- a/compiler/rustc_ast_lowering/src/expr.rs
+++ b/compiler/rustc_ast_lowering/src/expr.rs
@@ -388,7 +388,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
         else_opt: Option<&Expr>,
     ) -> hir::ExprKind<'hir> {
         let lowered_cond = self.lower_expr(cond);
-        let new_cond = self.manage_let_cond(lowered_cond);
+        let new_cond = self.wrap_cond_in_drop_scope(lowered_cond);
         let then_expr = self.lower_block_expr(then);
         if let Some(rslt) = else_opt {
             hir::ExprKind::If(new_cond, self.arena.alloc(then_expr), Some(self.lower_expr(rslt)))
@@ -397,9 +397,9 @@ impl<'hir> LoweringContext<'_, 'hir> {
         }
     }
 
-    // If `cond` kind is `let`, returns `let`. Otherwise, wraps and returns `cond`
-    // in a temporary block.
-    fn manage_let_cond(&mut self, cond: &'hir hir::Expr<'hir>) -> &'hir hir::Expr<'hir> {
+    // Wraps a condition (i.e. `cond` in `if cond` or `while cond`) in a terminating scope
+    // so that temporaries created in the condition don't live beyond it.
+    fn wrap_cond_in_drop_scope(&mut self, cond: &'hir hir::Expr<'hir>) -> &'hir hir::Expr<'hir> {
         fn has_let_expr<'hir>(expr: &'hir hir::Expr<'hir>) -> bool {
             match expr.kind {
                 hir::ExprKind::Binary(_, lhs, rhs) => has_let_expr(lhs) || has_let_expr(rhs),
@@ -407,12 +407,35 @@ impl<'hir> LoweringContext<'_, 'hir> {
                 _ => false,
             }
         }
-        if has_let_expr(cond) {
-            cond
-        } else {
-            let reason = DesugaringKind::CondTemporary;
-            let span_block = self.mark_span_with_reason(reason, cond.span, None);
-            self.expr_drop_temps(span_block, cond, AttrVec::new())
+
+        // 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 mantain 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 {
+            hir::ExprKind::Binary(
+                op @ Spanned { node: hir::BinOpKind::And | hir::BinOpKind::Or, .. },
+                lhs,
+                rhs,
+            ) if has_let_expr(cond) => {
+                let lhs = self.wrap_cond_in_drop_scope(lhs);
+                let rhs = self.wrap_cond_in_drop_scope(rhs);
+
+                self.arena.alloc(self.expr(
+                    cond.span,
+                    hir::ExprKind::Binary(op, lhs, rhs),
+                    AttrVec::new(),
+                ))
+            }
+            hir::ExprKind::Let(_) => cond,
+            _ => {
+                let reason = DesugaringKind::CondTemporary;
+                let span_block = self.mark_span_with_reason(reason, cond.span, None);
+                self.expr_drop_temps(span_block, cond, AttrVec::new())
+            }
         }
     }
 
@@ -440,7 +463,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
         opt_label: Option<Label>,
     ) -> hir::ExprKind<'hir> {
         let lowered_cond = self.with_loop_condition_scope(|t| t.lower_expr(cond));
-        let new_cond = self.manage_let_cond(lowered_cond);
+        let new_cond = self.wrap_cond_in_drop_scope(lowered_cond);
         let then = self.lower_block_expr(body);
         let expr_break = self.expr_break(span, AttrVec::new());
         let stmt_break = self.stmt_expr(span, expr_break);
diff --git a/src/test/ui/drop/drop_order.rs b/src/test/ui/drop/drop_order.rs
index e42150dcc09d2..bf740b6a9ab67 100644
--- a/src/test/ui/drop/drop_order.rs
+++ b/src/test/ui/drop/drop_order.rs
@@ -1,4 +1,5 @@
 // run-pass
+#![feature(let_chains)]
 
 use std::cell::RefCell;
 use std::convert::TryInto;
@@ -116,6 +117,58 @@ impl DropOrderCollector {
         }
     }
 
+    fn let_chain(&self) {
+        // take the "then" branch
+        if self.option_loud_drop(2).is_some() // 2
+            && self.option_loud_drop(1).is_some() // 1
+            && let Some(_d) = self.option_loud_drop(4) { // 4
+            self.print(3); // 3
+        }
+
+        // take the "else" branch
+        if self.option_loud_drop(6).is_some() // 2
+            && self.option_loud_drop(5).is_some() // 1
+            && let None = self.option_loud_drop(7) { // 3
+            unreachable!();
+        } else {
+            self.print(8); // 4
+        }
+
+        // let exprs interspersed
+        if self.option_loud_drop(9).is_some() // 1
+            && let Some(_d) = self.option_loud_drop(13) // 5
+            && self.option_loud_drop(10).is_some() // 2
+            && let Some(_e) = self.option_loud_drop(12) { // 4
+            self.print(11); // 3
+        }
+
+        // let exprs first
+        if let Some(_d) = self.option_loud_drop(18) // 5
+            && let Some(_e) = self.option_loud_drop(17) // 4
+            && self.option_loud_drop(14).is_some() // 1
+            && self.option_loud_drop(15).is_some() { // 2
+                self.print(16); // 3
+            }
+
+        // let exprs last
+        if self.option_loud_drop(20).is_some() // 2
+            && self.option_loud_drop(19).is_some() // 1
+            && let Some(_d) = self.option_loud_drop(23) // 5
+            && let Some(_e) = self.option_loud_drop(22) { // 4
+                self.print(21); // 3
+        }
+    }
+
+    fn while_(&self) {
+        let mut v = self.option_loud_drop(4);
+        while let Some(_d) = v
+            && self.option_loud_drop(1).is_some()
+            && self.option_loud_drop(2).is_some() {
+            self.print(3);
+            v = None;
+        }
+    }
+
     fn assert_sorted(self) {
         assert!(
             self.0
@@ -142,4 +195,14 @@ fn main() {
     let collector = DropOrderCollector::default();
     collector.match_();
     collector.assert_sorted();
+
+    println!("-- let chain --");
+    let collector = DropOrderCollector::default();
+    collector.let_chain();
+    collector.assert_sorted();
+
+    println!("-- while --");
+    let collector = DropOrderCollector::default();
+    collector.while_();
+    collector.assert_sorted();
 }

From 709a08ade6a8873dad8dc3987e842ed26523ef1f Mon Sep 17 00:00:00 2001
From: Nathan Whitaker <nathan.whitaker01@gmail.com>
Date: Thu, 13 Oct 2022 18:20:39 -0700
Subject: [PATCH 2/3] Lower condition directly from AST to HIR

---
 compiler/rustc_ast_lowering/src/expr.rs | 46 +++++++++++++------------
 1 file changed, 24 insertions(+), 22 deletions(-)

diff --git a/compiler/rustc_ast_lowering/src/expr.rs b/compiler/rustc_ast_lowering/src/expr.rs
index 104010f8d435e..ec9c393502056 100644
--- a/compiler/rustc_ast_lowering/src/expr.rs
+++ b/compiler/rustc_ast_lowering/src/expr.rs
@@ -387,23 +387,26 @@ impl<'hir> LoweringContext<'_, 'hir> {
         then: &Block,
         else_opt: Option<&Expr>,
     ) -> hir::ExprKind<'hir> {
-        let lowered_cond = self.lower_expr(cond);
-        let new_cond = self.wrap_cond_in_drop_scope(lowered_cond);
+        let lowered_cond = self.lower_cond(cond);
         let then_expr = self.lower_block_expr(then);
         if let Some(rslt) = else_opt {
-            hir::ExprKind::If(new_cond, self.arena.alloc(then_expr), Some(self.lower_expr(rslt)))
+            hir::ExprKind::If(
+                lowered_cond,
+                self.arena.alloc(then_expr),
+                Some(self.lower_expr(rslt)),
+            )
         } else {
-            hir::ExprKind::If(new_cond, self.arena.alloc(then_expr), None)
+            hir::ExprKind::If(lowered_cond, self.arena.alloc(then_expr), None)
         }
     }
 
-    // Wraps a condition (i.e. `cond` in `if cond` or `while cond`) in a terminating scope
+    // 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 wrap_cond_in_drop_scope(&mut self, cond: &'hir hir::Expr<'hir>) -> &'hir hir::Expr<'hir> {
-        fn has_let_expr<'hir>(expr: &'hir hir::Expr<'hir>) -> bool {
-            match expr.kind {
-                hir::ExprKind::Binary(_, lhs, rhs) => has_let_expr(lhs) || has_let_expr(rhs),
-                hir::ExprKind::Let(..) => true,
+    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,
             }
         }
@@ -415,14 +418,13 @@ impl<'hir> LoweringContext<'_, 'hir> {
         // In order to mantain 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 {
-            hir::ExprKind::Binary(
-                op @ Spanned { node: hir::BinOpKind::And | hir::BinOpKind::Or, .. },
-                lhs,
-                rhs,
-            ) if has_let_expr(cond) => {
-                let lhs = self.wrap_cond_in_drop_scope(lhs);
-                let rhs = self.wrap_cond_in_drop_scope(rhs);
+        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,
@@ -430,8 +432,9 @@ impl<'hir> LoweringContext<'_, 'hir> {
                     AttrVec::new(),
                 ))
             }
-            hir::ExprKind::Let(_) => cond,
+            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, AttrVec::new())
@@ -462,14 +465,13 @@ impl<'hir> LoweringContext<'_, 'hir> {
         body: &Block,
         opt_label: Option<Label>,
     ) -> hir::ExprKind<'hir> {
-        let lowered_cond = self.with_loop_condition_scope(|t| t.lower_expr(cond));
-        let new_cond = self.wrap_cond_in_drop_scope(lowered_cond);
+        let lowered_cond = self.with_loop_condition_scope(|t| t.lower_cond(cond));
         let then = self.lower_block_expr(body);
         let expr_break = self.expr_break(span, AttrVec::new());
         let stmt_break = self.stmt_expr(span, expr_break);
         let else_blk = self.block_all(span, arena_vec![self; stmt_break], None);
         let else_expr = self.arena.alloc(self.expr_block(else_blk, AttrVec::new()));
-        let if_kind = hir::ExprKind::If(new_cond, self.arena.alloc(then), Some(else_expr));
+        let if_kind = hir::ExprKind::If(lowered_cond, self.arena.alloc(then), Some(else_expr));
         let if_expr = self.expr(span, if_kind, AttrVec::new());
         let block = self.block_expr(self.arena.alloc(if_expr));
         let span = self.lower_span(span.with_hi(cond.span.hi()));

From 4e1c09dcd64507c03ded02844c1e5022031b9e38 Mon Sep 17 00:00:00 2001
From: Nathan Whitaker <nathan.whitaker01@gmail.com>
Date: Thu, 13 Oct 2022 18:29:25 -0700
Subject: [PATCH 3/3] Validate MIR in the `drop_order` test

---
 src/test/ui/drop/drop_order.rs | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/test/ui/drop/drop_order.rs b/src/test/ui/drop/drop_order.rs
index bf740b6a9ab67..ba1ac53aa7ce8 100644
--- a/src/test/ui/drop/drop_order.rs
+++ b/src/test/ui/drop/drop_order.rs
@@ -1,4 +1,5 @@
 // run-pass
+// compile-flags: -Z validate-mir
 #![feature(let_chains)]
 
 use std::cell::RefCell;