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

[nathanwhit]

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

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

into (roughly)

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

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

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

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

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.

[rust-highfive]

r? @davidtwco

(rust-highfive has picked a reviewer for you, use r? to override)

[nathanwhit]

Hmmm not sure what that ICE in CI is about, I can’t reproduce it locally. I’ll have to take a deeper look later.

[Nilstrieb]

Does this also fix #100276?

[davidtwco]

r? @compiler-errors

[compiler-errors]

I am not qualified to approve this PR i think 😂

r? compiler

also returning to author to fix the bug

@rustbot author

[eholk]

Hmmm not sure what that ICE in CI is about, I can’t reproduce it locally. I’ll have to take a deeper look later.

I ran into an issue recently that only showed up in CI. I was able to repro it locally by runner the tests in docker: https://rustc-dev-guide.rust-lang.org/tests/docker.html

Maybe that would help here?

[nathanwhit]

Does this also fix #100276?

Hmm unfortunately not, it seems to be a separate bug. (Though now that I'm looking at it, I think I actually might know the problem there - I'll try to open a separate PR, hopefully soon-ish).

[nathanwhit]

Hmmm not sure what that ICE in CI is about, I can’t reproduce it locally. I’ll have to take a deeper look later.

I ran into an issue recently that only showed up in CI. I was able to repro it locally by runner the tests in docker: https://rustc-dev-guide.rust-lang.org/tests/docker.html

Maybe that would help here?

Ah, I was actually able to fix it based on @compiler-errors comment, but thanks for the pointer! I'm sure it'll come in handy in the future.

With the latest commits I think this should be ready for review, so
@rustbot ready

[joshtriplett]

The behavior change seems correct to me. (No comment on the implementation.)

[eholk]

The implementation seems reasonable to me, so this PR seems good to go.

@bors r+

[bors]

📌 Commit 4e1c09d has been approved by eholk

It is now in the queue for this repository.

[cjgillot]

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?

[nathanwhit]

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.

[est31]

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 :).