Add convenient constructor for building DropGuard<(), FnOnce(())>

[tisonkun]

This refers to #144426.

cc @andylokandy @jplatte

Updated with:

#150027 (comment)

.. plus:

/// Create a new instance of `DropGuard` with a cleanup closure.
///
/// # Example
///
/// ```rust
/// # #![allow(unused)]
/// #![feature(drop_guard)]
///
/// use std::mem::defer;
///
/// let _guard = defer! { println!("Goodbye, world!") };
/// ```
#[unstable(feature = "drop_guard", issue = "144426")]
pub macro defer($($t:tt)*) {
    $crate::mem::DropGuard::new((), |()| { $($t)* })
}

---

Unfortunately, it's hard to convert impl FnOnce() to the input type param F: FnOnce(T) where T = ().

I tried:

impl<F> DropGuard<(), F>
where
    F: FnOnce(()),
{
    pub const fn new<G: FnOnce()>(f: G) -> Self {
        let f = move |()| f();
        Self { inner: ManuallyDrop::new(()), f: ManuallyDrop::new(f) }
    }
}

But it failed because the new closure has a different type of F.

[rustbot]

r? @scottmcm

rustbot has assigned @scottmcm.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[tisonkun]

Suggested change

	/// let _guard = DropGuard::new(|()| println!("Goodbye, world!"));
	/// let _guard = DropGuard::new(|| println!("Goodbye, world!"));

It's more proper to write || without (). But it seems quite hard to fine-tune the type parameter F: FnOnce(T) even if we are sure that T = (). impl FnOnce(()) is different from impl FnOnce().

[nxsaken]

I think the proposed API would be nice to have if it got rid of the unit argument, but as it is it feels like a half-measure.

I don't know if it would be popular, but maybe we could provide a defer! { ... } macro like the scopeguard crate does:

macro_rules! defer {
    ($($t:tt)*) => {
        let _guard = DropGuard::new((), |()| { $($t)* });
    };
}

This works too:

const fn defer<F: FnOnce()>(f: F) -> DropGuard<(), impl FnOnce(())> {
    DropGuard {
        inner: ManuallyDrop::new(()),
        f: ManuallyDrop::new(move |()| f()),
    }
}

[tisonkun]

const fn defer

This looks like a good solution. Let me try it out.

So far we export DropGuard as core::mem::DropGuard. A mem level core::mem::defer/core::mem::guard may be too generic, though.

[tisonkun]

Updated with:

// core::mem::defer
pub const fn defer(f: impl FnOnce()) -> DropGuard<(), impl FnOnce(())> { }
// core::mem::guard
pub const fn guard<T, F>(value: T, f: F) -> DropGuard<T, F> { }

Maybe we should keep DropGuard::new and remove guard. But anyway the functionality is here and now we can consider the naming and API :|

[nxsaken]

I think we should keep DropGuard::new, and that defer should be a macro if it exists. Returning DropGuard<(), impl FnOnce(())> makes the guard's type unnameable and impossible to store in a struct, which seems too limiting for the standard library.

The macro is not perfect, e.g. I don't think you can have both defer! { foo() } and defer!(foo). Also, scopeguard's version of defer binds the guard to an unused variable. This guarantees that defer does what it says, but doesn't allow users to dismiss the guard. We could just return the guard and rely on #[must_use].

[tisonkun]

@nxsaken Great suggestion! Updated.

[tisonkun]

Not sure if |()| { ... } can proper move value when needed.

Added defer_moved_value test case and it seems to work properly.

[tisonkun]

r? libs-api

[andylokandy]

I'd lean toward keeping defer as a function rather than a macro. Macros tend to be heavier in terms of complexity — for example, they don't play as nicely with formatters like rustfmt, and they can be harder to debug or reason about compared to plain functions.

If we go with a macro, one advantage of a macro is that it can wrap the entire let _guard = DropGuard:: new((), |()| { ... }); pattern, which isn't possible with a function. However, if we take this route, we should make sure the design aligns with Rust's general philosophy for standard library macros.

[jplatte]

I think the macro makes a lot of sense, if it includes the let _guard part. That would make it impossible to misuse even if the user disabled unused-must-use, and it's also a decent amount shorter than any other version of this.

Any usage pattern where the lifetime of the guards ends early in some branch via drop (or equivalent) instead of at the end of the scope seems very niche in comparison and IMHO should be fine with DropGuard::new, or some custom locally-defined free function for creating the guard.

[tisonkun]

5 out of 10 in my use cases need to store the DropGuard::new((), |()| { .. }) into a field to hold it during the app lifetime.

[jplatte]

5 out of 10 in my use cases need to store the DropGuard::new((), |()| { .. }) into a field to hold it during the app lifetime.

Do the closures in that case not capture anything from their environment? Or are you boxing them / using the unstable type-alias-impl-trait to be able to name the DropGuard type?

Something like this should be possible, but the default on F seems potentially problematic (fn(T) seems like a more reasonable default, and I wouldn't expect UnitFn<fn()>-to-fn(()) akin to non-capturing-closure-to-fn(...) to be worth the complexity)

pub struct DropGuard<T = (), F = UnitFn<fn()>>
where
    F: FnOnce(T),
{
    inner: ManuallyDrop<T>,
    f: ManuallyDrop<F>,
}

#[unstable(...)] // perma-unstable
struct UnitFn<F>(F);

impl<F> FnOnce<((),)> for UnitFn<F>
where
    F: FnOnce<()>,
{
    type Output = <F as FnOnce<Args>>::Output;

    extern "rust-call" fn call_once(self, _: ((),)) -> Self::Output {
        <F as FnOnce<Args>>::call_once(self.0, ())
    }
}

impl<F> DropGuard<(), UnitFn<F>>
where
    F: FnOnce(),
{
    pub const fn new(f: F) -> Self {
        Self {
            inner: ManuallyDrop::new(()),
            f: ManuallyDrop::new(UnitFn(f)),
        }
    }
}

impl<T, F> DropGuard<T, F>
where
    F: FnOnce(T),
{
    pub const fn with_value(value: T, f: F) -> Self {
        // ...
    }
}

// plus the deref macro including the `let _guard` bit, for convenience

[tisonkun]

@jplatte Sounds reasonable. Let me give it a try tomorrow.

[tisonkun]

@jplatte Updated.

[tisonkun]

Should be permanently unstable. But we don't export it anyway.

[nxsaken]

Note that UnitFn<fn()> wraps a function pointer. Closures are monomorphized/inlined, function pointers are not. You still have to write out the whole type if you want to store a guard with an actual closure, but that would either leak UnitFn into the public API, or it would prevent using the ergonomic constructor.

struct Struct<Fn0, Fn1>
where
    Fn0: FnOnce(()),
    Fn1: FnOnce(),
{
    good: DropGuard<(), Fn0>,
    bad: DropGuard<(), UnitFn<Fn1>>, // ERROR: unstable UnitFn
    bad2: DropGuard,                 // stores a function pointer!
}

impl<Fn0, Fn1> Struct<Fn0, Fn1>
where
    Fn0: FnOnce(()),
    Fn1: FnOnce(),
{
    fn new(fn0: Fn0, fn1: Fn1) -> Self {
        Self {
            // ERROR: expected FnOnce(), found FnOnce(())
            good: DropGuard::new(fn1),
            bad: DropGuard::new(fn1),
            // ERROR: expected fn(), found FnOnce()
            bad2: DropGuard::new(fn1),
        }
    }
}

DropGuard::new((), |_| { ... }) seems to be the optimal API for the standard library.

[jplatte]

As I see it, this is unrelated to the entire discussion above.

Yes, with current Rust you can not name a DropGuard that holds a closure, without coercing that closure into a function pointer (which only works if that closure doesn't capture anything).

However, DropGuard::new as this PR has it does not always return a DropGuard<(), UnitFn<fn()>>. If type inference doesn't force the passed FnOnce to be coerced to a fn pointer, it won't be.

DropGuard::new only gets rid of the weird unit parameter, that's it. And the default for F only allows naming the type of DropGuard obtained from DropGuard::new (at the expense of the function pointer indirection), nothing more or less.

[tisonkun]

cc @yoshuawuyts (due to being the issue author)

[tisonkun]

r? libs

[yoshuawuyts]

I’m not sure whether this change carries its weight. It makes the main intended use case of this API harder by making main constructor harder to remember. DropGuard::new is pretty obvious, DropGuard::with_value does not have any precedent in the stdlib, so people will need to either memorize it or look it up:

let _guard = DropGuard::new(x, |x| ..);        // ← current
let _guard = DropGuard::with_value(x, |x| ..); // ← proposed

I would like to see evidence presented that constructing DropGuard without any values is so common that we would want to make it the default. In rust-lang/rust we have a total of 42 references of the DropGuard pattern, of which 7 don’t take any arguments (ref). That’s a hit rate of 16%, which to me seems too low to change the default. Especially since that 16% of cases is already supported today by writing:

let _guard = DropGuard::new((), |_| ..);

Perhaps there is a way to introduce a convenience function that doesn’t change the default? For example something like DropGuard::with_default that defaults to passing (). But that’s something which could always be considered later.

[yoshuawuyts]

This is a new feature which should not be tracked as part of the drop guard tracking issue. I'd also suggest filing this in a separate PR, since introducing this macro is a different question from whether we should add this convenience function.

[yoshuawuyts]

I don't understand how this works. Since this is public, I would expected to be documented. Probably extra so since the expectation is to keep this perma-unstable.

[tisonkun]

An alternative is to implement the convenient constructor as a free function:

#![feature(drop_guard)]

use std::mem::DropGuard;

fn main() {
    let drop_guard = drop_guard(|| {
        println!("Dropped!");
    });
    println!("Doing some work...");
}

fn drop_guard(f: impl FnOnce()) -> DropGuard<(), impl FnOnce(())> {
    DropGuard::new((), |()| f())
}

But the FQN is hard to decide in this case.

[tisonkun]

In rust-lang/rust we have a total of 42 references of the DropGuard pattern, of which 7 don’t take any arguments (ref)

I suppose this is a bias in the stdlib where we test many cases that having a value. In the broader ecosystem, this DropGuard pattern is commonly used as a defer block.

[jhpratt]

While I agree there is likely a bias, perhaps that's best for the tracking issue? Up to you, as you're the PR author.