From 7396fd1fa06f0dd0ce0c8c92783736420e154495 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Wed, 6 Mar 2024 14:56:13 +1100 Subject: [PATCH 1/3] Clarify how lowering `if` produces then/else blocks This makes it easier to see that the call to `in_scope` returns both the then block and the else block. The rather confusing `unpack!` step is confined to its own separate line. (This patch reindents several lines, so using "ignore whitespace" is recommended in order to focus on the actual changes.) --- .../rustc_mir_build/src/build/expr/into.rs | 61 ++++++++++--------- 1 file changed, 31 insertions(+), 30 deletions(-) diff --git a/compiler/rustc_mir_build/src/build/expr/into.rs b/compiler/rustc_mir_build/src/build/expr/into.rs index 9a6d44983520b..8440db208e72e 100644 --- a/compiler/rustc_mir_build/src/build/expr/into.rs +++ b/compiler/rustc_mir_build/src/build/expr/into.rs @@ -58,42 +58,43 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { this.thir[scrutinee].span, ), ExprKind::If { cond, then, else_opt, if_then_scope } => { - let then_blk; let then_span = this.thir[then].span; let then_source_info = this.source_info(then_span); let condition_scope = this.local_scope(); - let mut else_blk = unpack!( - then_blk = this.in_scope( - (if_then_scope, then_source_info), - LintLevel::Inherited, - |this| { - let source_info = if this.is_let(cond) { - let variable_scope = - this.new_source_scope(then_span, LintLevel::Inherited, None); - this.source_scope = variable_scope; - SourceInfo { span: then_span, scope: variable_scope } - } else { - this.source_info(then_span) - }; - let (then_block, else_block) = - this.in_if_then_scope(condition_scope, then_span, |this| { - let then_blk = unpack!(this.then_else_break( - block, - cond, - Some(condition_scope), // Temp scope - condition_scope, - source_info, - true, // Declare `let` bindings normally - )); - - this.expr_into_dest(destination, then_blk, then) - }); - then_block.and(else_block) - }, - ) + let then_and_else_blocks = this.in_scope( + (if_then_scope, then_source_info), + LintLevel::Inherited, + |this| { + let source_info = if this.is_let(cond) { + let variable_scope = + this.new_source_scope(then_span, LintLevel::Inherited, None); + this.source_scope = variable_scope; + SourceInfo { span: then_span, scope: variable_scope } + } else { + this.source_info(then_span) + }; + let (then_block, else_block) = + this.in_if_then_scope(condition_scope, then_span, |this| { + let then_blk = unpack!(this.then_else_break( + block, + cond, + Some(condition_scope), // Temp scope + condition_scope, + source_info, + true, // Declare `let` bindings normally + )); + + this.expr_into_dest(destination, then_blk, then) + }); + then_block.and(else_block) + }, ); + // Unpack `BlockAnd` into `(then_blk, else_blk)`. + let (then_blk, mut else_blk); + else_blk = unpack!(then_blk = then_and_else_blocks); + else_blk = if let Some(else_opt) = else_opt { unpack!(this.expr_into_dest(destination, else_blk, else_opt)) } else { From 3402f39bcb684fd2edf9e7cae21b1ae68c720886 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Wed, 6 Mar 2024 15:11:21 +1100 Subject: [PATCH 2/3] Clarify lowering the `else` arm into the else block --- compiler/rustc_mir_build/src/build/expr/into.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_mir_build/src/build/expr/into.rs b/compiler/rustc_mir_build/src/build/expr/into.rs index 8440db208e72e..7e1a066e105a0 100644 --- a/compiler/rustc_mir_build/src/build/expr/into.rs +++ b/compiler/rustc_mir_build/src/build/expr/into.rs @@ -95,15 +95,15 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { let (then_blk, mut else_blk); else_blk = unpack!(then_blk = then_and_else_blocks); - else_blk = if let Some(else_opt) = else_opt { - unpack!(this.expr_into_dest(destination, else_blk, else_opt)) + // If there is an `else` arm, lower it into `else_blk`. + if let Some(else_expr) = else_opt { + unpack!(else_blk = this.expr_into_dest(destination, else_blk, else_expr)); } else { - // Body of the `if` expression without an `else` clause must return `()`, thus - // we implicitly generate an `else {}` if it is not specified. + // There is no `else` arm, so we know both arms have type `()`. + // Generate the implicit `else {}` by assigning unit. let correct_si = this.source_info(expr_span.shrink_to_hi()); this.cfg.push_assign_unit(else_blk, correct_si, destination, this.tcx); - else_blk - }; + } let join_block = this.cfg.start_new_block(); this.cfg.goto(then_blk, source_info, join_block); From 250e697834b2c48db7f71d9f2926eda51f3486a6 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Wed, 6 Mar 2024 15:22:39 +1100 Subject: [PATCH 3/3] Additional comments for lowering `if` --- compiler/rustc_mir_build/src/build/expr/into.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/compiler/rustc_mir_build/src/build/expr/into.rs b/compiler/rustc_mir_build/src/build/expr/into.rs index 7e1a066e105a0..69f3d3101fa21 100644 --- a/compiler/rustc_mir_build/src/build/expr/into.rs +++ b/compiler/rustc_mir_build/src/build/expr/into.rs @@ -66,6 +66,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { (if_then_scope, then_source_info), LintLevel::Inherited, |this| { + // FIXME: Does this need extra logic to handle let-chains? let source_info = if this.is_let(cond) { let variable_scope = this.new_source_scope(then_span, LintLevel::Inherited, None); @@ -74,6 +75,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { } else { this.source_info(then_span) }; + + // Lower the condition, and have it branch into `then` and `else` blocks. let (then_block, else_block) = this.in_if_then_scope(condition_scope, then_span, |this| { let then_blk = unpack!(this.then_else_break( @@ -85,8 +88,11 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { true, // Declare `let` bindings normally )); + // Lower the `then` arm into its block. this.expr_into_dest(destination, then_blk, then) }); + + // Pack `(then_block, else_block)` into `BlockAnd`. then_block.and(else_block) }, ); @@ -105,6 +111,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { this.cfg.push_assign_unit(else_blk, correct_si, destination, this.tcx); } + // The `then` and `else` arms have been lowered into their respective + // blocks, so make both of them meet up in a new block. let join_block = this.cfg.start_new_block(); this.cfg.goto(then_blk, source_info, join_block); this.cfg.goto(else_blk, source_info, join_block);