Auto merge of rust-lang#119122 - matthewjasper:if-let-guard-scoping, r=TaKO8Ki

Give temporaries in if let guards correct scopes

Temporaries in if-let guards have scopes that escape the match arm, this causes problems because the drops might be for temporaries that are not storage live. This PR changes the scope of temporaries in if-let guards to be limited to the arm:

```rust
_ if let Some(s) = std::convert::identity(&Some(String::new())) => {}
//                Temporary for Some(String::new()) is dropped here ^
```

We also now deduplicate temporaries between copies of the guard created for or-patterns:

```rust
// Only create a single Some(String::new()) temporary variable
_ | _ if let Some(s) = std::convert::identity(&Some(String::new())) => {}
```

This changes MIR building to pass around `ExprId`s rather than `Expr`s so that we have a way to index different expressions.

cc rust-lang#51114
Closes rust-lang#116079

Author: bors

Diff for: compiler/rustc_hir_analysis/src/check/region.rs

@@ -179,10 +179,10 @@ fn resolve_block<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, blk: &'tcx h
 fn resolve_arm<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, arm: &'tcx hir::Arm<'tcx>) {
     let prev_cx = visitor.cx;
 
-    visitor.enter_scope(Scope { id: arm.hir_id.local_id, data: ScopeData::Node });
-    visitor.cx.var_parent = visitor.cx.parent;
+    visitor.terminating_scopes.insert(arm.hir_id.local_id);
 
-    visitor.terminating_scopes.insert(arm.body.hir_id.local_id);
+    visitor.enter_node_scope_with_dtor(arm.hir_id.local_id);
+    visitor.cx.var_parent = visitor.cx.parent;
 
     if let Some(hir::Guard::If(expr)) = arm.guard {
         visitor.terminating_scopes.insert(expr.hir_id.local_id);

Diff for: compiler/rustc_mir_build/src/build/block.rs

@@ -15,7 +15,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
     ) -> BlockAnd<()> {
         let Block { region_scope, span, ref stmts, expr, targeted_by_break, safety_mode } =
             self.thir[ast_block];
-        let expr = expr.map(|expr| &self.thir[expr]);
         self.in_scope((region_scope, source_info), LintLevel::Inherited, move |this| {
             if targeted_by_break {
                 this.in_breakable_scope(None, destination, span, |this| {
@@ -49,7 +48,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         mut block: BasicBlock,
         span: Span,
         stmts: &[StmtId],
-        expr: Option<&Expr<'tcx>>,
+        expr: Option<ExprId>,
         safety_mode: BlockSafety,
         region_scope: Scope,
     ) -> BlockAnd<()> {
@@ -90,7 +89,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                     let si = (*scope, source_info);
                     unpack!(
                         block = this.in_scope(si, LintLevel::Inherited, |this| {
-                            this.stmt_expr(block, &this.thir[*expr], Some(*scope))
+                            this.stmt_expr(block, *expr, Some(*scope))
                         })
                     );
                 }
@@ -205,8 +204,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                     let visibility_scope =
                         Some(this.new_source_scope(remainder_span, LintLevel::Inherited, None));
 
-                    let init = &this.thir[*initializer];
-                    let initializer_span = init.span;
+                    let initializer_span = this.thir[*initializer].span;
                     let scope = (*init_scope, source_info);
                     let failure = unpack!(
                         block = this.in_scope(scope, *lint_level, |this| {
@@ -232,7 +230,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                             );
                             this.ast_let_else(
                                 block,
-                                init,
+                                *initializer,
                                 initializer_span,
                                 *else_block,
                                 &last_remainder_scope,
@@ -276,9 +274,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                         Some(this.new_source_scope(remainder_span, LintLevel::Inherited, None));
 
                     // Evaluate the initializer, if present.
-                    if let Some(init) = initializer {
-                        let init = &this.thir[*init];
-                        let initializer_span = init.span;
+                    if let Some(init) = *initializer {
+                        let initializer_span = this.thir[init].span;
                         let scope = (*init_scope, source_info);
 
                         unpack!(
@@ -334,13 +331,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         // of the block, which is stored into `destination`.
         let tcx = this.tcx;
         let destination_ty = destination.ty(&this.local_decls, tcx).ty;
-        if let Some(expr) = expr {
+        if let Some(expr_id) = expr {
+            let expr = &this.thir[expr_id];
             let tail_result_is_ignored =
                 destination_ty.is_unit() || this.block_context.currently_ignores_tail_results();
             this.block_context
                 .push(BlockFrame::TailExpr { tail_result_is_ignored, span: expr.span });
 
-            unpack!(block = this.expr_into_dest(destination, block, expr));
+            unpack!(block = this.expr_into_dest(destination, block, expr_id));
             let popped = this.block_context.pop();
 
             assert!(popped.is_some_and(|bf| bf.is_tail_expr()));

Diff for: compiler/rustc_mir_build/src/build/expr/as_operand.rs

@@ -17,10 +17,10 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
     pub(crate) fn as_local_operand(
         &mut self,
         block: BasicBlock,
-        expr: &Expr<'tcx>,
+        expr_id: ExprId,
     ) -> BlockAnd<Operand<'tcx>> {
         let local_scope = self.local_scope();
-        self.as_operand(block, Some(local_scope), expr, LocalInfo::Boring, NeedsTemporary::Maybe)
+        self.as_operand(block, Some(local_scope), expr_id, LocalInfo::Boring, NeedsTemporary::Maybe)
     }
 
     /// Returns an operand suitable for use until the end of the current scope expression and
@@ -76,7 +76,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
     pub(crate) fn as_local_call_operand(
         &mut self,
         block: BasicBlock,
-        expr: &Expr<'tcx>,
+        expr: ExprId,
     ) -> BlockAnd<Operand<'tcx>> {
         let local_scope = self.local_scope();
         self.as_call_operand(block, Some(local_scope), expr)
@@ -101,17 +101,18 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         &mut self,
         mut block: BasicBlock,
         scope: Option<region::Scope>,
-        expr: &Expr<'tcx>,
+        expr_id: ExprId,
         local_info: LocalInfo<'tcx>,
         needs_temporary: NeedsTemporary,
     ) -> BlockAnd<Operand<'tcx>> {
         let this = self;
 
+        let expr = &this.thir[expr_id];
         if let ExprKind::Scope { region_scope, lint_level, value } = expr.kind {
             let source_info = this.source_info(expr.span);
             let region_scope = (region_scope, source_info);
             return this.in_scope(region_scope, lint_level, |this| {
-                this.as_operand(block, scope, &this.thir[value], local_info, needs_temporary)
+                this.as_operand(block, scope, value, local_info, needs_temporary)
             });
         }
 
@@ -126,7 +127,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 block.and(Operand::Constant(Box::new(constant)))
             }
             Category::Constant | Category::Place | Category::Rvalue(..) => {
-                let operand = unpack!(block = this.as_temp(block, scope, expr, Mutability::Mut));
+                let operand = unpack!(block = this.as_temp(block, scope, expr_id, Mutability::Mut));
                 // Overwrite temp local info if we have something more interesting to record.
                 if !matches!(local_info, LocalInfo::Boring) {
                     let decl_info =
@@ -144,16 +145,17 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         &mut self,
         mut block: BasicBlock,
         scope: Option<region::Scope>,
-        expr: &Expr<'tcx>,
+        expr_id: ExprId,
     ) -> BlockAnd<Operand<'tcx>> {
-        debug!("as_call_operand(block={:?}, expr={:?})", block, expr);
         let this = self;
+        let expr = &this.thir[expr_id];
+        debug!("as_call_operand(block={:?}, expr={:?})", block, expr);
 
         if let ExprKind::Scope { region_scope, lint_level, value } = expr.kind {
             let source_info = this.source_info(expr.span);
             let region_scope = (region_scope, source_info);
             return this.in_scope(region_scope, lint_level, |this| {
-                this.as_call_operand(block, scope, &this.thir[value])
+                this.as_call_operand(block, scope, value)
             });
         }
 
@@ -171,9 +173,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 // type, and that value is coming from the deref of a box.
                 if let ExprKind::Deref { arg } = expr.kind {
                     // Generate let tmp0 = arg0
-                    let operand = unpack!(
-                        block = this.as_temp(block, scope, &this.thir[arg], Mutability::Mut)
-                    );
+                    let operand = unpack!(block = this.as_temp(block, scope, arg, Mutability::Mut));
 
                     // Return the operand *tmp0 to be used as the call argument
                     let place = Place {
@@ -186,6 +186,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             }
         }
 
-        this.as_operand(block, scope, expr, LocalInfo::Boring, NeedsTemporary::Maybe)
+        this.as_operand(block, scope, expr_id, LocalInfo::Boring, NeedsTemporary::Maybe)
     }
 }

Diff for: compiler/rustc_mir_build/src/build/expr/as_place.rs

@@ -354,9 +354,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
     pub(crate) fn as_place(
         &mut self,
         mut block: BasicBlock,
-        expr: &Expr<'tcx>,
+        expr_id: ExprId,
     ) -> BlockAnd<Place<'tcx>> {
-        let place_builder = unpack!(block = self.as_place_builder(block, expr));
+        let place_builder = unpack!(block = self.as_place_builder(block, expr_id));
         block.and(place_builder.to_place(self))
     }
 
@@ -365,9 +365,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
     pub(crate) fn as_place_builder(
         &mut self,
         block: BasicBlock,
-        expr: &Expr<'tcx>,
+        expr_id: ExprId,
     ) -> BlockAnd<PlaceBuilder<'tcx>> {
-        self.expr_as_place(block, expr, Mutability::Mut, None)
+        self.expr_as_place(block, expr_id, Mutability::Mut, None)
     }
 
     /// Compile `expr`, yielding a place that we can move from etc.
@@ -378,9 +378,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
     pub(crate) fn as_read_only_place(
         &mut self,
         mut block: BasicBlock,
-        expr: &Expr<'tcx>,
+        expr_id: ExprId,
     ) -> BlockAnd<Place<'tcx>> {
-        let place_builder = unpack!(block = self.as_read_only_place_builder(block, expr));
+        let place_builder = unpack!(block = self.as_read_only_place_builder(block, expr_id));
         block.and(place_builder.to_place(self))
     }
 
@@ -393,18 +393,19 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
     fn as_read_only_place_builder(
         &mut self,
         block: BasicBlock,
-        expr: &Expr<'tcx>,
+        expr_id: ExprId,
     ) -> BlockAnd<PlaceBuilder<'tcx>> {
-        self.expr_as_place(block, expr, Mutability::Not, None)
+        self.expr_as_place(block, expr_id, Mutability::Not, None)
     }
 
     fn expr_as_place(
         &mut self,
         mut block: BasicBlock,
-        expr: &Expr<'tcx>,
+        expr_id: ExprId,
         mutability: Mutability,
         fake_borrow_temps: Option<&mut Vec<Local>>,
     ) -> BlockAnd<PlaceBuilder<'tcx>> {
+        let expr = &self.thir[expr_id];
         debug!("expr_as_place(block={:?}, expr={:?}, mutability={:?})", block, expr, mutability);
 
         let this = self;
@@ -413,31 +414,29 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         match expr.kind {
             ExprKind::Scope { region_scope, lint_level, value } => {
                 this.in_scope((region_scope, source_info), lint_level, |this| {
-                    this.expr_as_place(block, &this.thir[value], mutability, fake_borrow_temps)
+                    this.expr_as_place(block, value, mutability, fake_borrow_temps)
                 })
             }
             ExprKind::Field { lhs, variant_index, name } => {
-                let lhs = &this.thir[lhs];
+                let lhs_expr = &this.thir[lhs];
                 let mut place_builder =
                     unpack!(block = this.expr_as_place(block, lhs, mutability, fake_borrow_temps,));
-                if let ty::Adt(adt_def, _) = lhs.ty.kind() {
+                if let ty::Adt(adt_def, _) = lhs_expr.ty.kind() {
                     if adt_def.is_enum() {
                         place_builder = place_builder.downcast(*adt_def, variant_index);
                     }
                 }
                 block.and(place_builder.field(name, expr.ty))
             }
             ExprKind::Deref { arg } => {
-                let place_builder = unpack!(
-                    block =
-                        this.expr_as_place(block, &this.thir[arg], mutability, fake_borrow_temps,)
-                );
+                let place_builder =
+                    unpack!(block = this.expr_as_place(block, arg, mutability, fake_borrow_temps,));
                 block.and(place_builder.deref())
             }
             ExprKind::Index { lhs, index } => this.lower_index_expression(
                 block,
-                &this.thir[lhs],
-                &this.thir[index],
+                lhs,
+                index,
                 mutability,
                 fake_borrow_temps,
                 expr.temp_lifetime,
@@ -461,12 +460,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
 
             ExprKind::PlaceTypeAscription { source, ref user_ty } => {
                 let place_builder = unpack!(
-                    block = this.expr_as_place(
-                        block,
-                        &this.thir[source],
-                        mutability,
-                        fake_borrow_temps,
-                    )
+                    block = this.expr_as_place(block, source, mutability, fake_borrow_temps,)
                 );
                 if let Some(user_ty) = user_ty {
                     let annotation_index =
@@ -494,9 +488,10 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 block.and(place_builder)
             }
             ExprKind::ValueTypeAscription { source, ref user_ty } => {
-                let source = &this.thir[source];
-                let temp =
-                    unpack!(block = this.as_temp(block, source.temp_lifetime, source, mutability));
+                let source_expr = &this.thir[source];
+                let temp = unpack!(
+                    block = this.as_temp(block, source_expr.temp_lifetime, source, mutability)
+                );
                 if let Some(user_ty) = user_ty {
                     let annotation_index =
                         this.canonical_user_type_annotations.push(CanonicalUserTypeAnnotation {
@@ -562,7 +557,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 // these are not places, so we need to make a temporary.
                 debug_assert!(!matches!(Category::of(&expr.kind), Some(Category::Place)));
                 let temp =
-                    unpack!(block = this.as_temp(block, expr.temp_lifetime, expr, mutability));
+                    unpack!(block = this.as_temp(block, expr.temp_lifetime, expr_id, mutability));
                 block.and(PlaceBuilder::from(temp))
             }
         }
@@ -591,8 +586,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
     fn lower_index_expression(
         &mut self,
         mut block: BasicBlock,
-        base: &Expr<'tcx>,
-        index: &Expr<'tcx>,
+        base: ExprId,
+        index: ExprId,
         mutability: Mutability,
         fake_borrow_temps: Option<&mut Vec<Local>>,
         temp_lifetime: Option<region::Scope>,
@@ -609,7 +604,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         // Making this a *fresh* temporary means we do not have to worry about
         // the index changing later: Nothing will ever change this temporary.
         // The "retagging" transformation (for Stacked Borrows) relies on this.
-        let idx = unpack!(block = self.as_temp(block, temp_lifetime, index, Mutability::Not,));
+        let idx = unpack!(block = self.as_temp(block, temp_lifetime, index, Mutability::Not));
 
         block = self.bounds_check(block, &base_place, idx, expr_span, source_info);