Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reorder nesting scopes and declare bindings without drop schedule #101410

Merged
merged 6 commits into from
Sep 15, 2022
+221 −65
Merged

Reorder nesting scopes and declare bindings without drop schedule #101410

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1b87ce0
reorder nesting scopes and declare bindings without drop schedule
dingxiangfei2009 Sep 4, 2022
af591eb
add test for #99975
dingxiangfei2009 Sep 5, 2022
34f0c45
supplement for the missing or incomplete comments
dingxiangfei2009 Sep 5, 2022
635b57c
enclose else block in terminating scope
dingxiangfei2009 Sep 12, 2022
c7d1c9b
add explanatory note
dingxiangfei2009 Sep 13, 2022
4a5d2a5
add diagram to explain the MIR structure
dingxiangfei2009 Sep 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
188 changes: 166 additions & 22 deletions compiler/rustc_mir_build/src/build/block.rs
View file
Open in desktop
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),
} => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It honestly would be helpful to either have a test with mir output that covers this, or an example with the expected mir output in comments here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me try both. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Speeking too soon. It seems not possible to dump MIR in ui tests yet. I have added explanatory comments here instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a list of MIR based tests in src/test/mir-opt. Despite the name of the directory, not all are about optimization. You can check out src/test/mir-opt/storage_ranges.rs for an example that just checks the generated MIR. It is also explained here.

// 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
Open in desktop
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
Open in desktop
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
Open in desktop
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