Skip to content

Commit

Permalink
Rollup merge of #102998 - nathanwhit:let-chains-drop-order, r=eholk
Browse files Browse the repository at this point in the history
Drop temporaries created in a condition, even if it's a let chain

Fixes #100513.

During the lowering from AST to HIR we wrap expressions acting as conditions in a `DropTemps` expression so that any temporaries created in the condition are dropped after the condition is executed. Effectively this means we transform

```rust
if Some(1).is_some() { .. }
```

into (roughly)

```rust
if { let _t = Some(1).is_some(); _t } { .. }
```

so that if we create any temporaries, they're lifted into the new scope surrounding the condition, so for example something along the lines of

```rust
if { let temp = Some(1); let _t = temp.is_some(); _t }.
```

Before this PR, if the condition contained any let expressions we would not introduce that new scope, instead leaving the condition alone. This meant that in a let-chain like

```rust
if get_drop("first").is_some() && let None = get_drop("last") {
        println!("second");
} else { .. }
```

the temporary created for `get_drop("first")` would be lifted into the _surrounding block_, which caused it to be dropped after the execution of the entire `if` expression.

After this PR, we wrap everything but the `let` expression in terminating scopes. The upside to this solution is that it's minimally invasive, but the downside is that in the worst case, an expression with `let` exprs interspersed like

```rust
if get_drop("first").is_some()
    && let Some(_a) = get_drop("fifth")
    && get_drop("second").is_some()
    && let Some(_b) = get_drop("fourth") { .. }
```

gets _multiple_ new scopes, roughly

```rust
if { let _t = get_drop("first").is_some(); _t }
    && let Some(_a) = get_drop("fifth")
    && { let _t = get_drop("second").is_some(); _t }
    && let Some(_b) = get_drop("fourth") { .. }
```

so instead of all of the temporaries being dropped at the end of the entire condition, they will be dropped right after they're evaluated (before the subsequent `let` expr). So while I'd say the drop behavior around let-chains is _less_ surprising after this PR, it still might not exactly match what people might expect.

For tests, I've just extended the drop order tests added in #100526. I'm not sure if that's the best way to go about it, though, so suggestions are welcome.
  • Loading branch information
Dylan-DPC authored Oct 15, 2022
2 parents 59e0af6 + 4e1c09d commit b79ad57
Show file tree
Hide file tree
Showing 2 changed files with 109 additions and 20 deletions.
65 changes: 45 additions & 20 deletions compiler/rustc_ast_lowering/src/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -387,32 +387,58 @@ impl<'hir> LoweringContext<'_, 'hir> {
then: &Block,
else_opt: Option<&Expr>,
) -> hir::ExprKind<'hir> {
let lowered_cond = self.lower_expr(cond);
let new_cond = self.manage_let_cond(lowered_cond);
let lowered_cond = self.lower_cond(cond);
let then_expr = self.lower_block_expr(then);
if let Some(rslt) = else_opt {
hir::ExprKind::If(new_cond, self.arena.alloc(then_expr), Some(self.lower_expr(rslt)))
hir::ExprKind::If(
lowered_cond,
self.arena.alloc(then_expr),
Some(self.lower_expr(rslt)),
)
} else {
hir::ExprKind::If(new_cond, self.arena.alloc(then_expr), None)
hir::ExprKind::If(lowered_cond, self.arena.alloc(then_expr), None)
}
}

// If `cond` kind is `let`, returns `let`. Otherwise, wraps and returns `cond`
// in a temporary block.
fn manage_let_cond(&mut self, cond: &'hir hir::Expr<'hir>) -> &'hir hir::Expr<'hir> {
fn has_let_expr<'hir>(expr: &'hir hir::Expr<'hir>) -> bool {
match expr.kind {
hir::ExprKind::Binary(_, lhs, rhs) => has_let_expr(lhs) || has_let_expr(rhs),
hir::ExprKind::Let(..) => true,
// Lowers a condition (i.e. `cond` in `if cond` or `while cond`), wrapping it in a terminating scope
// so that temporaries created in the condition don't live beyond it.
fn lower_cond(&mut self, cond: &Expr) -> &'hir hir::Expr<'hir> {
fn has_let_expr(expr: &Expr) -> bool {
match &expr.kind {
ExprKind::Binary(_, lhs, rhs) => has_let_expr(lhs) || has_let_expr(rhs),
ExprKind::Let(..) => true,
_ => false,
}
}
if has_let_expr(cond) {
cond
} else {
let reason = DesugaringKind::CondTemporary;
let span_block = self.mark_span_with_reason(reason, cond.span, None);
self.expr_drop_temps(span_block, cond, AttrVec::new())

// We have to take special care for `let` exprs in the condition, e.g. in
// `if let pat = val` or `if foo && let pat = val`, as we _do_ want `val` to live beyond the
// condition in this case.
//
// In order to mantain the drop behavior for the non `let` parts of the condition,
// we still wrap them in terminating scopes, e.g. `if foo && let pat = val` essentially
// gets transformed into `if { let _t = foo; _t } && let pat = val`
match &cond.kind {
ExprKind::Binary(op @ Spanned { node: ast::BinOpKind::And, .. }, lhs, rhs)
if has_let_expr(cond) =>
{
let op = self.lower_binop(*op);
let lhs = self.lower_cond(lhs);
let rhs = self.lower_cond(rhs);

self.arena.alloc(self.expr(
cond.span,
hir::ExprKind::Binary(op, lhs, rhs),
AttrVec::new(),
))
}
ExprKind::Let(..) => self.lower_expr(cond),
_ => {
let cond = self.lower_expr(cond);
let reason = DesugaringKind::CondTemporary;
let span_block = self.mark_span_with_reason(reason, cond.span, None);
self.expr_drop_temps(span_block, cond, AttrVec::new())
}
}
}

Expand All @@ -439,14 +465,13 @@ impl<'hir> LoweringContext<'_, 'hir> {
body: &Block,
opt_label: Option<Label>,
) -> hir::ExprKind<'hir> {
let lowered_cond = self.with_loop_condition_scope(|t| t.lower_expr(cond));
let new_cond = self.manage_let_cond(lowered_cond);
let lowered_cond = self.with_loop_condition_scope(|t| t.lower_cond(cond));
let then = self.lower_block_expr(body);
let expr_break = self.expr_break(span, AttrVec::new());
let stmt_break = self.stmt_expr(span, expr_break);
let else_blk = self.block_all(span, arena_vec![self; stmt_break], None);
let else_expr = self.arena.alloc(self.expr_block(else_blk, AttrVec::new()));
let if_kind = hir::ExprKind::If(new_cond, self.arena.alloc(then), Some(else_expr));
let if_kind = hir::ExprKind::If(lowered_cond, self.arena.alloc(then), Some(else_expr));
let if_expr = self.expr(span, if_kind, AttrVec::new());
let block = self.block_expr(self.arena.alloc(if_expr));
let span = self.lower_span(span.with_hi(cond.span.hi()));
Expand Down
64 changes: 64 additions & 0 deletions src/test/ui/drop/drop_order.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
// run-pass
// compile-flags: -Z validate-mir
#![feature(let_chains)]

use std::cell::RefCell;
use std::convert::TryInto;
Expand Down Expand Up @@ -116,6 +118,58 @@ impl DropOrderCollector {
}
}

fn let_chain(&self) {
// take the "then" branch
if self.option_loud_drop(2).is_some() // 2
&& self.option_loud_drop(1).is_some() // 1
&& let Some(_d) = self.option_loud_drop(4) { // 4
self.print(3); // 3
}

// take the "else" branch
if self.option_loud_drop(6).is_some() // 2
&& self.option_loud_drop(5).is_some() // 1
&& let None = self.option_loud_drop(7) { // 3
unreachable!();
} else {
self.print(8); // 4
}

// let exprs interspersed
if self.option_loud_drop(9).is_some() // 1
&& let Some(_d) = self.option_loud_drop(13) // 5
&& self.option_loud_drop(10).is_some() // 2
&& let Some(_e) = self.option_loud_drop(12) { // 4
self.print(11); // 3
}

// let exprs first
if let Some(_d) = self.option_loud_drop(18) // 5
&& let Some(_e) = self.option_loud_drop(17) // 4
&& self.option_loud_drop(14).is_some() // 1
&& self.option_loud_drop(15).is_some() { // 2
self.print(16); // 3
}

// let exprs last
if self.option_loud_drop(20).is_some() // 2
&& self.option_loud_drop(19).is_some() // 1
&& let Some(_d) = self.option_loud_drop(23) // 5
&& let Some(_e) = self.option_loud_drop(22) { // 4
self.print(21); // 3
}
}

fn while_(&self) {
let mut v = self.option_loud_drop(4);
while let Some(_d) = v
&& self.option_loud_drop(1).is_some()
&& self.option_loud_drop(2).is_some() {
self.print(3);
v = None;
}
}

fn assert_sorted(self) {
assert!(
self.0
Expand All @@ -142,4 +196,14 @@ fn main() {
let collector = DropOrderCollector::default();
collector.match_();
collector.assert_sorted();

println!("-- let chain --");
let collector = DropOrderCollector::default();
collector.let_chain();
collector.assert_sorted();

println!("-- while --");
let collector = DropOrderCollector::default();
collector.while_();
collector.assert_sorted();
}

0 comments on commit b79ad57

Please sign in to comment.