rust-lang / rfcs

RFCs for changes to Rust
https://rust-lang.github.io/rfcs/
Apache License 2.0
5.93k stars 1.57k forks source link

Support resetting a value to it's default by adding `Default::to_default()` #3511

Closed t3hmrman closed 1 year ago

t3hmrman commented 1 year ago

Motivation

Sometimes it's useful to be able to mutate a value to it's Default implementation, and it would be useful to have that in the standard library (ideally as a part of Default, but possibly as another trait).

The code that would be generated would be of this form:

impl Default for T {
    fn to_default(&mut self) {
        self.x = Default::default();
        // ... other fields ...
    }
}

This is distinct from just swapping (via mem::swap) an existing instance because I want to alter the existing instance specifically, effectively hollowing it out (see example existing use case).

Alternatives

Users could continue to write this code themselves fairly simply themselves.

Alternatively, someone could create a small crate that reflects this functionality -- in writing an unrelated library it was fairly simple to write a macro that did this, so I could release that as an independent crate, and call the trait ResetDefault/ToDefault or something like that.

Existing use-cases

SOF3 commented 1 year ago

Let me summarize the proposal quickly: The default implementation in the trait can be written as fn to_default(&mut self) { *self = Self::default() }, and the only motivation to derive this with the fields instead of using the default implementation is to avoid calling the Drop impl.

But the problem is, is it really desirable to be able to reset all fields to the default value without calling Drop? Automatically doing so for all existing std Default derive macro calls may break some assumptions in traits that explicitly rely on running Drop but still derived Default. For example consider a simplified Vec implementation:

#[derive(Default)]
struct ByteVec {
    ptr: Option<NonNull<u8>>,
}
impl Drop for ByteVec {
    fn drop(&mut self) {
        if let Some(ptr) = self.ptr {
            drop_items(ptr);
            free(ptr);
        }
    }
}

Now, adding a to_default() method that simply sets self.ptr = None; would leak the allocated buffer.

Since mem::forget() is safe, automatically deriving to_default for the fields should not expose new possibilities of UB, but it may lead to unintended leaking.

SOF3 commented 1 year ago

By the way, referring to https://rust-lang.github.io/api-guidelines/naming.html, to_* should be used for &T -> U methods, not &mut T -> (). A more appropriate name would be set_default. And to avoid polluting the prelude namespace, this should not be part of the Default trait since Default is in the prelude.

t3hmrman commented 1 year ago

Hey @SOF3 Thanks for the detailed consideration -- I think you've captured the gist & details accurately.

But the problem is, is it really desirable to be able to reset all fields to the default value without calling Drop? Automatically doing so for all existing std Default derive macro calls may break some assumptions in traits that explicitly rely on running Drop but still derived Default. For example consider a simplified Vec implementation:

Thanks for noting, this is a great point and it is indeed troublesome -- with your example it's pretty clear that automatically doing it for all Default derive macros is too risky given this approach.

Users that write code which must make an assumption of a custom Drop function should probably not be using this, and making that clear in set_default() seems a bit onerous, but not completely unheard of.

I'm unaware if there's any way to ensure that using set_default() could be disabled for custom implementations of Default, but if there isn't, then it could definitely become a tripping point for people writing code with custom Drop implementations that they expect to be called.

That said, I think this is something users would specifically look to do (when looking to re-use/repopulate some object) rather than at most situations.

Maybe the somewhat niche aspect of this makes it a much better fit for a standalone derive macro crate.

By the way, referring to https://rust-lang.github.io/api-guidelines/naming.html, to_* should be used for &T -> U methods, not &mut T -> (). A more appropriate name would be set_default. And to avoid polluting the prelude namespace, this should not be part of the Default trait since Default is in the prelude.

I definitely agree here, since the name I used in my code is reset_to_default. I checked the API guidelines but was torn between to_ and into_ rather than considering set.

It looks like if this shouldn't be in the prelude namespace and Default isn't the right place, then this is another sign (to me) that it belongs in a crate -- adding a new Trait to the stdlib with only this functionality feels wrong to me.

t3hmrman commented 1 year ago

Very open to hearing more comments, but closing this for now so we can have one less issue on rust-lang/rfcs!