Skip to content

Commit

Permalink
Auto merge of #101410 - dingxiangfei2009:fix-let-else-scoping, r=jack…
Browse files Browse the repository at this point in the history 
…h726

Reorder nesting scopes and declare bindings without drop schedule

Fix #99228
Fix #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`
  • Loading branch information
@bors
bors committed Sep 15, 2022
2 parents 294f0ee + 4a5d2a5 commit 35a0407
Show file tree
Hide file tree
Showing 6 changed files with 221 additions and 65 deletions.
188 changes: 166 additions & 22 deletions compiler/rustc_mir_build/src/build/block.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
Expand All @@ -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
})
},
)
Expand Down
40 changes: 6 additions & 34 deletions compiler/rustc_mir_build/src/build/matches/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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)
}
}
35 changes: 27 additions & 8 deletions compiler/rustc_typeck/src/check/region.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
}
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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.
///
Expand Down Expand Up @@ -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() {
Expand All @@ -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)
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/async-await/async-await-let-else.no-drop-tracking.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
|
Expand Down
Loading

0 comments on commit 35a0407

Please sign in to comment.