-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Add core::mem::DropGuard
#144236
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.
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
base: master
Are you sure you want to change the base?
Add core::mem::DropGuard
#144236
Conversation
rustbot has assigned @Mark-Simulacrum. Use |
this appears to be revisiting rust-lang/libs-team#622 |
This comment has been minimized.
This comment has been minimized.
Oh you mean the naming? I guess I missed that issue since it's just a few days old. Most of the names come from the mini-scopeguard crate I put together in preparation for this PR earlier this year. To give some rationale for the names:
This to me seemed like a reasonable starting point for a contribution. The part I'm least sure about is the |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Some things missing in this PR from my ACP (which makes sense since it was apparently done in parallel):
pub struct WithDrop<T, F: FnOnce(T) = fn(T)> { This is similar to how it is defaulted in
There are also some differences:
|
Thanks @orlp, @purplesyringa, @bjorn3, @lukaslueg - you've raised some really good points. I'll try and address your comments either later today or tomorrow. But I wanted to take a moment to say thank you before then ^^ |
This comment has been minimized.
This comment has been minimized.
F: FnOnce(T), | ||
{ | ||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
fmt::Debug::fmt(&**self, f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little wary of making wrappers transparent in Debug
. orlp's suggestion in rust-lang/libs-team#622 was to print DropGuard(...)
here via debug_tuple
, and personally I find that more reasonable. Did you implement Debug
transparently deliberately or is this just an omission?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't necessarily love this either - but this is what we seem to be doing for most other wrapper types in the stdlib. I lifted this impl from Mutex
, but for example the Pin
type does the same thing.
If we want to change this, I think we should probably argue it for all wrapper types in the stdlib. I'd rather not break precedent on an addition like this, even if I agree that I prefer your suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took inspiration from LazyLock
which does a debug print with a tuple wrapper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please correct me if I'm wrong, but I think the rule of thumb here is "Debug
is transparent if the wrapper type has no runtime behavior". E.g. types like Mutex
, LazyLock
, ManuallyDrop
, etc. implement Debug
non-transparently, and Pin
, Unique
are transparent. I think DropGuard
is closer to the former than the latter so we wouldn't break any conventions here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what the rules here are honestly, but I like where you're going with this. While the the impl for Mutex
is intransparent, the impl for MutexGuard
is transparent. I think it's fair to say that MutexGuard
does have its own runtime behavior. How would that fit into this framework?
This comment has been minimized.
This comment has been minimized.
|
||
// This will run the closure, which will panic when dropped. This should | ||
// run the destructor of the value we passed, which we validate. | ||
let _ = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm understanding this failure correctly, this line isn't actually catching the unwind the way it should.
Or maybe the test runner in CI mode is getting confused that we're unwinding and catching it. I'm not actually sure what is happening?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried recreating this locally using ./x.py test library/coretests
, and I can't seem to. All tests are passing, and I'm not quite sure what's different. I'm now re-running all tests locally to see if that makes a difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't reproduce locally (not as part of coretest, standalone). The panic still gets printed as we haven't installed a panic_handler
but it should get caught.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, you're just testing on cranelift with panic = "abort"
. Add #[cfg(panic = "unwind")]
to the test. Sorry, noticed this CI failure earlier but thought you'd resolve it yourself 😅
The job Click to see the possible cause of the failure (guessed by this bot)
|
1.0 Summary
This PR introduces a new type
core::mem::DropGuard
which wraps a value and runs a closure when the value is dropped.2.0 Motivation
A number of programming languages include constructs like
try..finally
ordefer
to run code as the last piece of a particular sequence, regardless of whether an error occurred. This is typically used to clean up resources, like closing files, freeing memory, or unlocking resources. In Rust we use theDrop
trait instead, allowing us to never having to manually close sockets.While
Drop
(and RAII in general) has been working incredibly well for Rust in general, sometimes it can be a little verbose to setup. In particular when upholding invariants are local to functions, having a quick inline way to setup animpl Drop
can be incredibly convenient. We can see this in use in the Rust stdlib, which has a number of privateDropGuard
impls used internally:3.0 Design
This PR implements what can be considered about the simplest possible design:
DropGuard
which takes both a generic typeT
and a closureF
.Deref
+DerefMut
impls to make it easy to work with theT
in the guard.impl Drop
on the guard which calls the closureF
on drop.fn into_inner
which takes the typeT
out of the guard without calling the closureF
.Notably this design does not allow divergent behavior based on the type of drop that has occurred. The
scopeguard
crate includes additionalon_success
andon_onwind
variants which can be used to branch on unwind behavior instead. However in a lot of cases this doesn’t seem necessary, and using the arm/disarm pattern seems to provide much the same functionality:DropGuard
combined with this pattern seems like it should cover the vast majority of use cases for quick, inline destructors. It certainly seems like it should cover all existing uses in the stdlib, as well as all existing uses in crates like hashbrown.4.0 Acknowledgements
This implementation is based on the mini-scopeguard crate which in turn is based on the scopeguard crate. The implementations only differ superficially; because of the nature of the problem there is only really one obvious way to structure the solution. And the scopeguard crate got that right!
5.0 Conclusion
This PR adds a new type
core::mem::DropGuard
to the stdlib which adds a small convenience helper to create inline destructors with. This would bring the majority of the functionality of thescopeguard
crate into the stdlib, which is the 49th most downloaded crate on crates.io (387 million downloads).Given the actual implementation of
DropGuard
is only around 60 lines, it seems to hit that sweet spot of low-complexity / high-impact that makes for a particularly efficient stdlib addition. Which is why I’m putting this forward for consideration; thanks!