OnDrop: Simple Drop guard

[camsteffen]

Proposal

Problem statement

Sometimes I want to run a function upon the end of a function or block. This may be for resource clean-up or just any side effect. This function does not necessarily own data. Or it might reference data with interior mutability.

We want a straightforward and idiomatic alternative to golang's defer.

Motivating examples or use cases

thread_local! {
    static RECURSION_COUNT: Cell<u32> = Cell::new(0);
}

fn my_function() {
    RECURSION_COUNT.update(|n| n + 1);
    let _guard = OnDrop::new(|| RECURSION_COUNT.update(|n| n - 1));
    println!("my_function is being executed {} times", RECURSION_COUNT.get());
    
    // insert any complex logic here, with early returns, recursion and what have you
}

Solution sketch

struct OnDrop<F: FnOnce()>(ManuallyDrop<F>);

impl<F: FnOnce()> OnDrop<F> {
    fn new(f: F) -> Self {
        OnDrop(ManuallyDrop::new(f))
    }

    fn into_inner(self) -> F;
}

impl<F: FnOnce()> Drop for OnDrop<F> {
    fn drop(&mut self) {
        let f = unsafe { ManuallyDrop::take(&mut self.0) };
        f();
    }
}

Alternatives

One alternative is to add another constructor function to WithDrop with no data.

let _guard = WithDrop::from_fn(|| todo!());
/// ...is equivalent to...
let _guard = WithDrop::new((), |_| todo!());

I would say this feels like deciding not to have HashSet because we already have HashMap. It would be awkward to write the signature WithDrop<(), ...>. A key feature of WithDrop is that it wraps a value, and this feels like a distinct use case.

I estimate that this utility is about equally as useful as WithDrop. So, at the risk of a slippy slope fallacy, I think if WithDrop is accepted then this should be too.

Links and related work

• ACP: Add core::mem::WithDrop to associate a custom drop fn with some data #622
• scopeguard
• defer
• https://internals.rust-lang.org/t/idea-drop-guard-or-scope-guard-helper-function/16312
• https://internals.rust-lang.org/t/a-defer-discussion/20387/1

[camsteffen]

Some thoughts on naming. If this is added, it changes discussion of what WithDrop and OnDrop should be named. The two abstractions are similar but different, so their names should be similar but different, accentuating the difference. I think WithDrop and OnDrop accomplishes that. WithDrop can be described as "decorates a value with Drop", and in that way that name makes a lot of sense.

[ChrisDenton]

I'm really not seeing the motivation. How often would you write out the signature? The whole point of WithDrop (or similar) is that they don't escape their scope and so there's no need to manually write it.

Another constructor function could be useful but it seems fairly minor and not really worth a new type with a confusingly similar name.

[camsteffen]

I could imagine keeping just one type, but I would want to tweak the API so that the no-value use case is given primacy and doesn't feel like an afterthought. Instead of WithDrop I'd probably name it OnDrop or DropGuard.

let _guard = OnDrop::new(|| println!("just a side effect");
let foo = OnDrop::value(foo, |foo| println!("with a value {foo}");

How often would you write out the signature?

I have a project where I actually have a lot of helper functions for constructing a drop guard because the specifics of the guard are dynamic.

[kennytm]

Instead of WithDrop I'd probably name it OnDrop or DropGuard.

The accepted #622 is actually implemented in rust-lang/rust#144426 under the name "DropGuard", not "WithDrop". (The name may also be changed before stabilization). I think that nullifies all reason invoking awkwardness about why this ACP should be a different type than #622.

I would say this feels like deciding not to have HashSet because we already have HashMap.

IMO this ACP is more like deciding we need a distinct MutexLock type because Mutex<()> is ugly. The fact that the motivating example is "wrapping" a single resource RECURSION_COUNT makes it closer to the Mutex situation than the HashSet situation. The motivating example may as well be like

    let counter = DropGuard::new(&RECURSION_COUNT, |c| c.update(|n| n - 1));
    counter.update(|n| n+1);
    println!("my_function is being executed {} times", counter.get());

(Also HashSet has some API like symmetric_difference which makes no sense for HashMap, but there is nothing special about DropGuard<(), _> beyond construction)

[Amanieu]

We discussed this in the @rust-lang/libs-api meeting. We don't believe this provides sufficient value over the existing DropGuard type to justify adding an entirely new type. We might consider an additional constructor method on DropGuard which don't take a value, but even then the value is questionable since you're only saving a few characters and it doesn't impact readability.