Auto merge of rust-lang#101410 - dingxiangfei2009:fix-let-else-scoping, r=jackh726

Reorder nesting scopes and declare bindings without drop schedule Fix rust-lang#99228 Fix rust-lang#99975 Storages are previously not declared before entering the `else` block of a `let .. else` statement. However, when breaking out of the pattern matching into the `else` block, those storages are recorded as scheduled for drops. This is not expected. This MR fixes this issue by not scheduling the drops for those storages. cc `@est31`

Author: bors

compiler/rustc_mir_build/src/build/block.rs

@@ -109,13 +109,170 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
  )
  );
  }
+ StmtKind::Let {
+ remainder_scope,
+ init_scope,
+ pattern,
+ initializer: Some(initializer),
+ lint_level,
+ else_block: Some(else_block),
+ } => {
+ // When lowering the statement `let <pat> = <expr> else { <else> };`,
+ // the `<else>` block is nested in the parent scope enclosing this statment.
+ // That scope is usually either the enclosing block scope,
+ // or the remainder scope of the last statement.
+ // This is to make sure that temporaries instantiated in `<expr>` are dropped
+ // as well.
+ // In addition, even though bindings in `<pat>` only come into scope if
+ // the pattern matching passes, in the MIR building the storages for them
+ // are declared as live any way.
+ // This is similar to `let x;` statements without an initializer expression,
+ // where the value of `x` in this example may or may be assigned,
+ // because the storage for their values may not be live after all due to
+ // failure in pattern matching.
+ // For this reason, we declare those storages as live but we do not schedule
+ // any drop yet- they are scheduled later after the pattern matching.
+ // The generated MIR will have `StorageDead` whenever the control flow breaks out
+ // of the parent scope, regardless of the result of the pattern matching.
+ // However, the drops are inserted in MIR only when the control flow breaks out of
+ // the scope of the remainder scope associated with this `let .. else` statement.
+ // Pictorial explanation of the scope structure:
+ // ┌─────────────────────────────────┐
+ // │ Scope of the enclosing block, │
+ // │ or the last remainder scope │
+ // │ ┌───────────────────────────┐ │
+ // │ │ Scope for <else> block │ │
+ // │ └───────────────────────────┘ │
+ // │ ┌───────────────────────────┐ │
+ // │ │ Remainder scope of │ │
+ // │ │ this let-else statement │ │
+ // │ │ ┌─────────────────────┐ │ │
+ // │ │ │ <expr> scope │ │ │
+ // │ │ └─────────────────────┘ │ │
+ // │ │ extended temporaries in │ │
+ // │ │ <expr> lives in this │ │
+ // │ │ scope │ │
+ // │ │ ┌─────────────────────┐ │ │
+ // │ │ │ Scopes for the rest │ │ │
+ // │ │ └─────────────────────┘ │ │
+ // │ └───────────────────────────┘ │
+ // └─────────────────────────────────┘
+ // Generated control flow:
+ // │ let Some(x) = y() else { return; }
+ // │
+ // ┌────────▼───────┐
+ // │ evaluate y() │
+ // └────────┬───────┘
+ // │ ┌────────────────┐
+ // ┌────────▼───────┐ │Drop temporaries│
+ // │Test the pattern├──────►in y() │
+ // └────────┬───────┘ │because breaking│
+ // │ │out of <expr> │
+ // ┌────────▼───────┐ │scope │
+ // │Move value into │ └───────┬────────┘
+ // │binding x │ │
+ // └────────┬───────┘ ┌───────▼────────┐
+ // │ │Drop extended │
+ // ┌────────▼───────┐ │temporaries in │
+ // │Drop temporaries│ │<expr> because │
+ // │in y() │ │breaking out of │
+ // │because breaking│ │remainder scope │
+ // │out of <expr> │ └───────┬────────┘
+ // │scope │ │
+ // └────────┬───────┘ ┌───────▼────────┐
+ // │ │Enter <else> ├────────►
+ // ┌────────▼───────┐ │block │ return;
+ // │Continue... │ └────────────────┘
+ // └────────────────┘
+
+ let ignores_expr_result = matches!(pattern.kind, PatKind::Wild);
+ this.block_context.push(BlockFrame::Statement { ignores_expr_result });
+
+ // Lower the `else` block first because its parent scope is actually
+ // enclosing the rest of the `let .. else ..` parts.
+ let else_block_span = this.thir[*else_block].span;
+ // This place is not really used because this destination place
+ // should never be used to take values at the end of the failure
+ // block.
+ let dummy_place = this.temp(this.tcx.types.never, else_block_span);
+ let failure_entry = this.cfg.start_new_block();
+ let failure_block;
+ unpack!(
+ failure_block = this.ast_block(
+ dummy_place,
+ failure_entry,
+ *else_block,
+ this.source_info(else_block_span),
+ )
+ );
+ this.cfg.terminate(
+ failure_block,
+ this.source_info(else_block_span),
+ TerminatorKind::Unreachable,
+ );
+
+ // Declare the bindings, which may create a source scope.
+ let remainder_span = remainder_scope.span(this.tcx, this.region_scope_tree);
+ this.push_scope((*remainder_scope, source_info));
+ let_scope_stack.push(remainder_scope);
+
+ let visibility_scope =
+ Some(this.new_source_scope(remainder_span, LintLevel::Inherited, None));
+
+ let init = &this.thir[*initializer];
+ let initializer_span = init.span;
+ this.declare_bindings(
+ visibility_scope,
+ remainder_span,
+ pattern,
+ ArmHasGuard(false),
+ Some((None, initializer_span)),
+ );
+ this.visit_primary_bindings(
+ pattern,
+ UserTypeProjections::none(),
+ &mut |this, _, _, _, node, span, _, _| {
+ this.storage_live_binding(block, node, span, OutsideGuard, false);
+ },
+ );
+ let failure = unpack!(
+ block = this.in_opt_scope(
+ opt_destruction_scope.map(|de| (de, source_info)),
+ |this| {
+ let scope = (*init_scope, source_info);
+ this.in_scope(scope, *lint_level, |this| {
+ this.ast_let_else(
+ block,
+ init,
+ initializer_span,
+ *else_block,
+ &last_remainder_scope,
+ pattern,
+ )
+ })
+ }
+ )
+ );
+ this.cfg.goto(failure, source_info, failure_entry);
+
+ if let Some(source_scope) = visibility_scope {
+ this.source_scope = source_scope;
+ }
+ last_remainder_scope = *remainder_scope;
+ }
+ StmtKind::Let { init_scope, initializer: None, else_block: Some(_), .. } => {
+ span_bug!(
+ init_scope.span(this.tcx, this.region_scope_tree),
+ "initializer is missing, but else block is present in this let binding",
+ )
+ }
  StmtKind::Let {
  remainder_scope,
  init_scope,
  ref pattern,
  initializer,
  lint_level,
- else_block,
+ else_block: None,
  } => {
  let ignores_expr_result = matches!(pattern.kind, PatKind::Wild);
  this.block_context.push(BlockFrame::Statement { ignores_expr_result });
@@ -141,27 +298,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
  |this| {
  let scope = (*init_scope, source_info);
  this.in_scope(scope, *lint_level, |this| {
- if let Some(else_block) = else_block {
- this.ast_let_else(
- block,
- init,
- initializer_span,
- *else_block,
- visibility_scope,
- last_remainder_scope,
- remainder_span,
- pattern,
- )
- } else {
- this.declare_bindings(
- visibility_scope,
- remainder_span,
- pattern,
- ArmHasGuard(false),
- Some((None, initializer_span)),
- );
- this.expr_into_pattern(block, pattern, init) // irrefutable pattern
- }
+ this.declare_bindings(
+ visibility_scope,
+ remainder_span,
+ pattern,
+ ArmHasGuard(false),
+ Some((None, initializer_span)),
+ );
+ this.expr_into_pattern(block, &pattern, init) // irrefutable pattern
  })
  },
  )

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

@@ -699,7 +699,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
  self.cfg.push(block, Statement { source_info, kind: StatementKind::StorageLive(local_id) });
  // Although there is almost always scope for given variable in corner cases
  // like #92893 we might get variable with no scope.
- if let Some(region_scope) = self.region_scope_tree.var_scope(var.0.local_id) && schedule_drop{
+ if let Some(region_scope) = self.region_scope_tree.var_scope(var.0.local_id) && schedule_drop {
  self.schedule_drop(span, region_scope, local_id, DropKind::Storage);
  }
  Place::from(local_id)
@@ -2274,23 +2274,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
  init: &Expr<'tcx>,
  initializer_span: Span,
  else_block: BlockId,
- visibility_scope: Option<SourceScope>,
- remainder_scope: region::Scope,
- remainder_span: Span,
+ let_else_scope: &region::Scope,
  pattern: &Pat<'tcx>,
- ) -> BlockAnd<()> {
+ ) -> BlockAnd<BasicBlock> {
  let else_block_span = self.thir[else_block].span;
- let (matching, failure) = self.in_if_then_scope(remainder_scope, |this| {
+ let (matching, failure) = self.in_if_then_scope(*let_else_scope, |this| {
  let scrutinee = unpack!(block = this.lower_scrutinee(block, init, initializer_span));
  let pat = Pat { ty: init.ty, span: else_block_span, kind: PatKind::Wild };
  let mut wildcard = Candidate::new(scrutinee.clone(), &pat, false);
- this.declare_bindings(
- visibility_scope,
- remainder_span,
- pattern,
- ArmHasGuard(false),
- Some((None, initializer_span)),
- );
  let mut candidate = Candidate::new(scrutinee.clone(), pattern, false);
  let fake_borrow_temps = this.lower_match_tree(
  block,
@@ -2321,28 +2312,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
  None,
  None,
  );
- this.break_for_else(failure, remainder_scope, this.source_info(initializer_span));
+ this.break_for_else(failure, *let_else_scope, this.source_info(initializer_span));
  matching.unit()
  });
-
- // This place is not really used because this destination place
- // should never be used to take values at the end of the failure
- // block.
- let dummy_place = self.temp(self.tcx.types.never, else_block_span);
- let failure_block;
- unpack!(
- failure_block = self.ast_block(
- dummy_place,
- failure,
- else_block,
- self.source_info(else_block_span),
- )
- );
- self.cfg.terminate(
- failure_block,
- self.source_info(else_block_span),
- TerminatorKind::Unreachable,
- );
- matching.unit()
+ matching.and(failure)
  }
 }

compiler/rustc_typeck/src/check/region.rs

@@ -126,6 +126,29 @@ fn resolve_block<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, blk: &'tcx h
 
  for (i, statement) in blk.stmts.iter().enumerate() {
  match statement.kind {
+ hir::StmtKind::Local(hir::Local { els: Some(els), .. }) => {
+ // Let-else has a special lexical structure for variables.
+ // First we take a checkpoint of the current scope context here.
+ let mut prev_cx = visitor.cx;
+
+ visitor.enter_scope(Scope {
+ id: blk.hir_id.local_id,
+ data: ScopeData::Remainder(FirstStatementIndex::new(i)),
+ });
+ visitor.cx.var_parent = visitor.cx.parent;
+ visitor.visit_stmt(statement);
+ // We need to back out temporarily to the last enclosing scope
+ // for the `else` block, so that even the temporaries receiving
+ // extended lifetime will be dropped inside this block.
+ // We are visiting the `else` block in this order so that
+ // the sequence of visits agree with the order in the default
+ // `hir::intravisit` visitor.
+ mem::swap(&mut prev_cx, &mut visitor.cx);
+ visitor.terminating_scopes.insert(els.hir_id.local_id);
+ visitor.visit_block(els);
+ // From now on, we continue normally.
+ visitor.cx = prev_cx;
+ }
  hir::StmtKind::Local(..) | hir::StmtKind::Item(..) => {
  // Each declaration introduces a subscope for bindings
  // introduced by the declaration; this subscope covers a
@@ -138,10 +161,10 @@ fn resolve_block<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, blk: &'tcx h
  data: ScopeData::Remainder(FirstStatementIndex::new(i)),
  });
  visitor.cx.var_parent = visitor.cx.parent;
+ visitor.visit_stmt(statement)
  }
- hir::StmtKind::Expr(..) | hir::StmtKind::Semi(..) => {}
+ hir::StmtKind::Expr(..) | hir::StmtKind::Semi(..) => visitor.visit_stmt(statement),
  }
- visitor.visit_stmt(statement)
  }
  walk_list!(visitor, visit_expr, &blk.expr);
  }
@@ -460,7 +483,6 @@ fn resolve_local<'tcx>(
  visitor: &mut RegionResolutionVisitor<'tcx>,
  pat: Option<&'tcx hir::Pat<'tcx>>,
  init: Option<&'tcx hir::Expr<'tcx>>,
- els: Option<&'tcx hir::Block<'tcx>>,
 ) {
  debug!("resolve_local(pat={:?}, init={:?})", pat, init);
 
@@ -547,9 +569,6 @@ fn resolve_local<'tcx>(
  if let Some(pat) = pat {
  visitor.visit_pat(pat);
  }
- if let Some(els) = els {
- visitor.visit_block(els);
- }
 
  /// Returns `true` if `pat` match the `P&` non-terminal.
  ///
@@ -766,7 +785,7 @@ impl<'tcx> Visitor<'tcx> for RegionResolutionVisitor<'tcx> {
  // (i.e., `'static`), which means that after `g` returns, it drops,
  // and all the associated destruction scope rules apply.
  self.cx.var_parent = None;
- resolve_local(self, None, Some(&body.value), None);
+ resolve_local(self, None, Some(&body.value));
  }
 
  if body.generator_kind.is_some() {
@@ -793,7 +812,7 @@ impl<'tcx> Visitor<'tcx> for RegionResolutionVisitor<'tcx> {
  resolve_expr(self, ex);
  }
  fn visit_local(&mut self, l: &'tcx Local<'tcx>) {
- resolve_local(self, Some(&l.pat), l.init, l.els)
+ resolve_local(self, Some(&l.pat), l.init)
  }
 }
 

src/test/ui/async-await/async-await-let-else.no-drop-tracking.stderr

@@ -35,7 +35,7 @@ LL | bar2(Rc::new(())).await
  | |
  | has type `Rc<()>` which is not `Send`
 LL | };
- |  - `Rc::new(())` is later dropped here
+ | - `Rc::new(())` is later dropped here
 note: required by a bound in `is_send`
  --> $DIR/async-await-let-else.rs:19:15
  |