Let-else does not drop temporaries at the end of the statement

[camsteffen]

This bug has already been identified but I wanted to open an issue for it.

The following code should print I've been dropped first. (playground)

#![feature(let_else)]

#[derive(Default)]
struct Droppy { inner: u32 }

impl Drop for Droppy {
    fn drop(&mut self) {
        println!("I've been dropped");
    }
}

fn main() {
    let 0 = Droppy::default().inner else { return };
    println!("Should have dropped 👆");
}

I believe there is an unanswered question: Should temporaries be dropped before the else block?

    let 1 = Droppy::default().inner else {
        println!("Should have dropped?????");
        return;
    };

@nikomatsakis said this issue is being investigated at #93628 (comment).

This issue has been assigned to @dingxiangfei2009 via this comment.

[camsteffen]

@rustbot assign @dingxiangfei2009 (I'm pretty sure you are working on this at #98574)

[camsteffen]

Should temporaries be dropped before the else block?

I think they should, since this would be consistent with if/else. (playground)

[est31]

Originally reported in: #93628 (comment)

There are also some variations of it: #93628 (comment)

I've made a little test case for it that doesn't rely on printf but instead can be ran as a run-pass test:

// run-pass
#![feature(let_else)]
#![allow(irrefutable_let_patterns)]

use std::cell::RefCell;

thread_local! {
    pub static LAST_DROPPED: RefCell<u32> = RefCell::new(0);
}

fn reset_last_dropped() {
    LAST_DROPPED.with(|d| {
        *d.borrow_mut() = 0;
    });
}

fn get_last_dropped() -> u32 {
    LAST_DROPPED.with(|d| *d.borrow())
}

struct Droppy(u32);

impl Drop for Droppy {
    fn drop(&mut self) {
        LAST_DROPPED.with(|d| {
            *d.borrow_mut() = self.0;
        });
    }
}

fn test_simple() {
    {
        reset_last_dropped();
        const LINE: u32 = line!();
        let _x = Droppy(LINE).0;
        assert_eq!(get_last_dropped(), LINE);
    }
    {
        reset_last_dropped();
        const LINE: u32 = line!();
        let _x = Droppy(LINE).0 else { return };
        assert_eq!(get_last_dropped(), LINE);
    }
}

fn test_borrow() {
    {
        reset_last_dropped();
        const LINE: u32 = line!();
        let _x = &Droppy(LINE).0;
        assert_eq!(get_last_dropped(), LINE);
    }
    {
        reset_last_dropped();
        const LINE: u32 = line!();
        let _x = &Droppy(LINE).0 else { return };
        assert_eq!(get_last_dropped(), LINE);
    }
}

fn main() {
    test_simple();
    test_borrow();
}

[dingxiangfei2009]

I believe the two tests in test_borrow shall be comparing to 0. The reason is that let _x = &(<expr>).0; actually extends the temporary <expr> to the end of the block at the moment, which is the desired outcome.

cc @est31

[est31]

@dingxiangfei2009 good point. What's important is that both the let-else and let-else free version are doing the same thing. Maybe one can add an additional block around the let _x = ... assert_eq!(...); and then add a second assert_eq to ensure it dropped after the block terminated?

Also note that when I wrote that test I didn't know about compiletest storing/checking the stdout of run-pass tests, so maybe you can simplify it a little by removing the thread local variable as well as the code that manages it, and instead printing to stdout at important places (and adding println!("Visited line {}", line!()) to a few places). Edit: nvm, it's not possible as the .stdout files are for compiler output, not runtime output of the tests.

[camsteffen]

Here's another way to test, using Rc

#![feature(let_else)]

use std::rc::Rc;

pub fn main() {
    {
        // test let-else drops temps after statement
        let rc = Rc::new(0);
        let 0 = *rc.clone() else { unreachable!() };
        Rc::try_unwrap(rc).unwrap();
    }
    {
        // test let-else drops temps before else block
        let rc = Rc::new(0);
        let 1 = *rc.clone() else {
            Rc::try_unwrap(rc).unwrap();
            return;
        };
        unreachable!();
    }
}

[est31]

It might also be an idea to add a test for the inconsistency between if and if let: https://hackmd.io/@nikomatsakis/r1CpZqc8c#Side-note (no matter which side of the divide let else is on, make sure that there is no breakage of it in the future)

[camsteffen]

The above test for "drop before else block" failed on master. Reopening.

[dingxiangfei2009]

@camsteffen @est31 Allow me to notify that I am currently looking at how to drop values earlier inside the else block.

[est31]

So I was rebasing #99291 onto latest master and I wanted to share this as part of the diff:

There has been great improvement by #99518 but it was no full fix, especially this still post-drops:

#![feature(let_else)]

#[allow(unused)]
enum Droppy {
    A(u32),
    B,
}

impl Drop for Droppy {
    fn drop(&mut self) {
        println!("I dropped");
    }
}

fn foo() {
    let &Droppy::A(_v) = &Droppy::B else {
        println!("Should have dropped");
        return;
    };
}

fn main() {
    foo();
}

prints:

Should have dropped
I dropped

If you remove the & it works now thanks to #99518.

[dingxiangfei2009]

@est31 I think this is because the borrowed Droppy::B is assigned with extended lifetime. #99518 only breaks out towards the remainder scope. It turns out this is not sufficient. I will try to break out to further one scope up, say the parent remainder scope of the current one. I feel that this should suffice.

[camsteffen]

This is correct behavior I think. Anything accessible in the pattern needs to be in scope till the end of the block. Consider you can write let x @ &Droppy(_) = . It shouldn't depend on whether the pattern matches since that is not known at compile time generally.

[est31]

@camsteffen yeah it may not be dropped before the code after the let else construct terminates, but it can be dropped before the else block.

[camsteffen]

I see. I think this is the non-temporary case, when the pattern doesn't match.