Rollup merge of #121928 - Zalathar:then-else-args, r=Nadrieril

Extract an arguments struct for `Builder::then_else_break`

Most of this method's arguments are usually or always forwarded as-is to recursive invocations.

Wrapping them in a dedicated struct allows us to document each struct field, and lets us use struct-update syntax to indicate which arguments are being modified when making a recursive call.

---

While trying to understand the lowering of `if` expressions, I found it difficult to keep track of the half-dozen arguments passed through to every call to `then_else_break`. I tried switching over to an arguments struct, and I found that it really helps to make sense of what each argument does, and how each call is modifying the arguments.

I have some further ideas for how to streamline these recursive calls, but I've kept those out of this PR so that it's a pure refactoring with no behavioural changes.

Author: matthiaskrgr

compiler/rustc_mir_build/src/build/expr/into.rs

@@ -81,10 +81,10 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                                     let then_blk = unpack!(this.then_else_break(
                                         block,
                                         cond,
-                                        Some(condition_scope),
+                                        Some(condition_scope), // Temp scope
                                         condition_scope,
                                         source_info,
-                                        true,
+                                        true, // Declare `let` bindings normally
                                     ));
 
                                     this.expr_into_dest(destination, then_blk, then)
@@ -146,9 +146,11 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                         this.then_else_break(
                             block,
                             lhs,
-                            Some(condition_scope),
+                            Some(condition_scope), // Temp scope
                             condition_scope,
                             source_info,
+                            // This flag controls how inner `let` expressions are lowered,
+                            // but either way there shouldn't be any of those in here.
                             true,
                         )
                     });

compiler/rustc_mir_build/src/build/matches/mod.rs

@@ -30,76 +30,92 @@ mod util;
 use std::borrow::Borrow;
 use std::mem;
 
+/// Arguments to [`Builder::then_else_break_inner`] that are usually forwarded
+/// to recursive invocations.
+#[derive(Clone, Copy)]
+struct ThenElseArgs {
+    /// Used as the temp scope for lowering `expr`. If absent (for match guards),
+    /// `self.local_scope()` is used.
+    temp_scope_override: Option<region::Scope>,
+    /// Scope to pass to [`Builder::break_for_else`]. Must match the scope used
+    /// by the enclosing call to [`Builder::in_if_then_scope`].
+    break_scope: region::Scope,
+    variable_source_info: SourceInfo,
+    /// Forwarded to [`Builder::lower_let_expr`] when lowering [`ExprKind::Let`].
+    /// When false (for match guards), `let` bindings won't be declared.
+    declare_let_bindings: bool,
+}
+
 impl<'a, 'tcx> Builder<'a, 'tcx> {
     /// Lowers a condition in a way that ensures that variables bound in any let
     /// expressions are definitely initialized in the if body.
     ///
-    /// If `declare_bindings` is false then variables created in `let`
+    /// If `declare_let_bindings` is false then variables created in `let`
     /// expressions will not be declared. This is for if let guards on arms with
     /// an or pattern, where the guard is lowered multiple times.
     pub(crate) fn then_else_break(
         &mut self,
-        mut block: BasicBlock,
+        block: BasicBlock,
         expr_id: ExprId,
         temp_scope_override: Option<region::Scope>,
         break_scope: region::Scope,
         variable_source_info: SourceInfo,
-        declare_bindings: bool,
+        declare_let_bindings: bool,
+    ) -> BlockAnd<()> {
+        self.then_else_break_inner(
+            block,
+            expr_id,
+            ThenElseArgs {
+                temp_scope_override,
+                break_scope,
+                variable_source_info,
+                declare_let_bindings,
+            },
+        )
+    }
+
+    fn then_else_break_inner(
+        &mut self,
+        block: BasicBlock, // Block that the condition and branch will be lowered into
+        expr_id: ExprId,   // Condition expression to lower
+        args: ThenElseArgs,
     ) -> BlockAnd<()> {
         let this = self;
         let expr = &this.thir[expr_id];
         let expr_span = expr.span;
 
         match expr.kind {
             ExprKind::LogicalOp { op: LogicalOp::And, lhs, rhs } => {
-                let lhs_then_block = unpack!(this.then_else_break(
-                    block,
-                    lhs,
-                    temp_scope_override,
-                    break_scope,
-                    variable_source_info,
-                    declare_bindings,
-                ));
-
-                let rhs_then_block = unpack!(this.then_else_break(
-                    lhs_then_block,
-                    rhs,
-                    temp_scope_override,
-                    break_scope,
-                    variable_source_info,
-                    declare_bindings,
-                ));
-
+                let lhs_then_block = unpack!(this.then_else_break_inner(block, lhs, args));
+                let rhs_then_block = unpack!(this.then_else_break_inner(lhs_then_block, rhs, args));
                 rhs_then_block.unit()
             }
             ExprKind::LogicalOp { op: LogicalOp::Or, lhs, rhs } => {
                 let local_scope = this.local_scope();
                 let (lhs_success_block, failure_block) =
                     this.in_if_then_scope(local_scope, expr_span, |this| {
-                        this.then_else_break(
+                        this.then_else_break_inner(
                             block,
                             lhs,
-                            temp_scope_override,
-                            local_scope,
-                            variable_source_info,
-                            true,
+                            ThenElseArgs {
+                                break_scope: local_scope,
+                                declare_let_bindings: true,
+                                ..args
+                            },
                         )
                     });
-                let rhs_success_block = unpack!(this.then_else_break(
+                let rhs_success_block = unpack!(this.then_else_break_inner(
                     failure_block,
                     rhs,
-                    temp_scope_override,
-                    break_scope,
-                    variable_source_info,
-                    true,
+                    ThenElseArgs { declare_let_bindings: true, ..args },
                 ));
 
                 // Make the LHS and RHS success arms converge to a common block.
                 // (We can't just make LHS goto RHS, because `rhs_success_block`
                 // might contain statements that we don't want on the LHS path.)
                 let success_block = this.cfg.start_new_block();
-                this.cfg.goto(lhs_success_block, variable_source_info, success_block);
-                this.cfg.goto(rhs_success_block, variable_source_info, success_block);
+                this.cfg.goto(lhs_success_block, args.variable_source_info, success_block);
+                this.cfg.goto(rhs_success_block, args.variable_source_info, success_block);
                 success_block.unit()
             }
             ExprKind::Unary { op: UnOp::Not, arg } => {
@@ -111,50 +127,38 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                         if this.tcx.sess.instrument_coverage() {
                             this.cfg.push_coverage_span_marker(block, this.source_info(expr_span));
                         }
-                        this.then_else_break(
+                        this.then_else_break_inner(
                             block,
                             arg,
-                            temp_scope_override,
-                            local_scope,
-                            variable_source_info,
-                            true,
+                            ThenElseArgs {
+                                break_scope: local_scope,
+                                declare_let_bindings: true,
+                                ..args
+                            },
                         )
                     });
-                this.break_for_else(success_block, break_scope, variable_source_info);
+                this.break_for_else(success_block, args.break_scope, args.variable_source_info);
                 failure_block.unit()
             }
             ExprKind::Scope { region_scope, lint_level, value } => {
                 let region_scope = (region_scope, this.source_info(expr_span));
                 this.in_scope(region_scope, lint_level, |this| {
-                    this.then_else_break(
-                        block,
-                        value,
-                        temp_scope_override,
-                        break_scope,
-                        variable_source_info,
-                        declare_bindings,
-                    )
+                    this.then_else_break_inner(block, value, args)
                 })
             }
-            ExprKind::Use { source } => this.then_else_break(
-                block,
-                source,
-                temp_scope_override,
-                break_scope,
-                variable_source_info,
-                declare_bindings,
-            ),
+            ExprKind::Use { source } => this.then_else_break_inner(block, source, args),
             ExprKind::Let { expr, ref pat } => this.lower_let_expr(
                 block,
                 expr,
                 pat,
-                break_scope,
-                Some(variable_source_info.scope),
-                variable_source_info.span,
-                declare_bindings,
+                args.break_scope,
+                Some(args.variable_source_info.scope),
+                args.variable_source_info.span,
+                args.declare_let_bindings,
             ),
             _ => {
-                let temp_scope = temp_scope_override.unwrap_or_else(|| this.local_scope());
+                let mut block = block;
+                let temp_scope = args.temp_scope_override.unwrap_or_else(|| this.local_scope());
                 let mutability = Mutability::Mut;
                 let place =
                     unpack!(block = this.as_temp(block, Some(temp_scope), expr_id, mutability));
@@ -166,7 +170,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
 
                 let source_info = this.source_info(expr_span);
                 this.cfg.terminate(block, source_info, term);
-                this.break_for_else(else_block, break_scope, source_info);
+                this.break_for_else(else_block, args.break_scope, source_info);
 
                 then_block.unit()
             }
@@ -2105,10 +2109,10 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                     this.then_else_break(
                         block,
                         guard,
-                        None,
+                        None, // Use `self.local_scope()` as the temp scope
                         match_scope,
                         this.source_info(arm.span),
-                        false,
+                        false, // For guards, `let` bindings are declared separately
                     )
                 });