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

Drop temporaries created in a condition, even if it's a let chain #102998

Merged
merged 3 commits into from
Oct 15, 2022
Merged

Drop temporaries created in a condition, even if it's a let chain #102998

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ad8b242
Let chains should still drop temporaries
nathanwhit Oct 12, 2022
709a08a
Lower condition directly from AST to HIR
nathanwhit Oct 14, 2022
4e1c09d
Validate MIR in the `drop_order` test
nathanwhit Oct 14, 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
65 changes: 45 additions & 20 deletions compiler/rustc_ast_lowering/src/expr.rs
View file
Open in desktop
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(
nathanwhit marked this conversation as resolved.
Show resolved Hide resolved
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
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
// run-pass
nathanwhit marked this conversation as resolved.
Show resolved Hide resolved
// 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
Copy link
Contributor

Choose a reason for hiding this comment

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

The drop order is a bit surprising. IIUC, we start by dropping the first regular conditions in reverse source order, then other regular conditions in forward source order, then the block, then the let conditions.

Why the forward/reverse mix?

Copy link
Member Author

@nathanwhit nathanwhit Oct 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah that's definitely a quirk. It's pretty much because for that condition we go from an AST like

// Fake notation - the numbers correspond to the argument to `option_loud_drop`
Binary(And, Binary(And, Binary(And, Let(18), Let(17)), MethodCall(14)), MethodCall(15)

to hir like

Binary(And, Binary(And, Binary(And, Let(18), Let(17)), DropTemps(MethodCall(14))), DropTemps(MethodCall(15)))

Ideally we could group

self.option_loud_drop(14).is_some() // 1
            && self.option_loud_drop(15).is_some() // 2

into the same DropTemps and then they would drop in reverse source order, but due to the nesting, I can't see a way to wrap them in the same DropTemps without the let conditions being lumped in.

If the regular conditions come first, like

self.option_loud_drop(14).is_some()
    && self.option_loud_drop(15).is_some()
    && let Some(_d) = self.option_loud_drop(18)
    && let Some(_e) = self.option_loud_drop(17)

we can (and do) wrap them in the same DropTemps, and so they drop in reverse source order.

Copy link
Member

Choose a reason for hiding this comment

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

FTR there is also pretty weird behaviour for vanilla non-let if chains. Drop behaviour is currently like:

        if self.option_loud_drop(5).is_some()
            && self.option_loud_drop(1).is_some()
            && self.option_loud_drop(2).is_some()
            && self.option_loud_drop(3).is_some()
            && self.option_loud_drop(4).is_some() {
            self.print(6);
        }

Same goes if you put the expression into a let _= ..., or replace the && with a || (while changing the is_some to is_none so that all partial exprs get evaluated). So second sub-expression is first, then it goes in normal direction, until the end. Then comes the first sub-expression. Non-short-circuiting & and | drop more consistently in inverse order (5,4,3,2,1). I'm not sure how to resolve this, and how to integrate this with let chains. There is the danger of furthering technical debt on one hand, on the other hand there is the danger of adding exceptions.

A bit unrelated: Generally I think that dropping the temporaries of the if let expression after the else block is entered was a mistake. Ideally it should be changed to always pre-drop, like what if is doing. I think let chains give us a good opportunity to switch to the new behaviour, at least for let chains, and I'm very happy that currently this is what this PR seems to be doing: drop all temporaries, from let or from an expression, before the else block is entered. Just as a positive feedback :).

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();
}