take on bool

[sanbox-irl]

This RFC proposes adding a method called take on bool, which will return the value of the bool while also setting bool to false, similar to Option::take.

Rendered

[kpreid]

Prior art: AtomicBool::compare_exchange, which is used for similar purposes. It is a more complex operation, because it allows specifying memory orderings (irrelevant here) and because it can set the boolean to either true or false, or act effectively as a read without modifying. But, it is closely related in that, for example, code being migrated towards or away from Sync support might replace one with the other.

Probably it doesn't make sense to name this operation bool::compare_exchange, though, since that would be misleading to users familiar with atomics and reduce the parallel to Option::take and mem::take. But I think it's worth mentioning.

[mbartlett21]

This should perhaps note that using core::mem::take does not actually optimize quite as well. It has an extra instruction compared:

         mov     al, byte ptr [rdi]
+        and     al, 1
         mov     byte ptr [rdi], 0
         ret

(Godbolt)

[sanbox-irl]

Yup -- I didn't want to get too in the weed here, but that's a great point. It mostly goes away once you call the method though

[scottmcm]

That's not a reason to add this method, though, because it's a problem with every single two-variant enum, not just bool: https://rust.godbolt.org/z/1Wjs1bf3Y.

Its root cause is LLVM discarding information about the possible values of the discriminant. rust-lang/rust#85133 (comment) This "just" needs to get fixed, since it's actually hurting substantial amounts of Option- and Result-using code too.

[mbartlett21]

The comparison to Option::take is a little confusing. A more direct comparison would be to mem::take:

Something like:

All .take() does is return the value of a boolean while also setting that value internally to false. It is just like mem::take, except it is called as a method instead of a free function. In places where booleans are commonly read and then reset, like dirty flags, this method is useful.

[sanbox-irl]

This is a great point. I'll make this edit.

[joshtriplett]

Micro-optimization aside, is there a reason to want this rather than mem::take?

A similar method could appear on any type that implements Default, but I don't think we should add such a method to every such type.

[jhpratt]

mem::take can likely be improved internally, no?

[jdahlstrom]

A similar method could appear on any type that implements Default, but I don't think we should add such a method to every such type.

Hmm, actually, maybe this could be a provided method on Default? A known problem with the mem:: functions are that they're not very discoverable and perhaps feel like second-class citizens compared to methods. It occurs to me that to C++ programmers .take() would feel familiar, "like C++ move semantics, leaving the source object in a valid 'default' state".

[jhpratt]

Given that Default is in the prelude, I'd be strongly opposed to that.

[Havvy]

If adding it to Default is too hard, it could be done via an extension trait trait DefaultTake {} impl<T: Default> DefaultTake for T {}

[sanbox-irl]

Micro-optimization aside, is there a reason to want this rather than mem::take?

Yes, for two reasons:

1. Adding mem into a project implies a more complex operation than .take. mem::take, imo, is used primarily in patterns where you don't want to do the borrowchecker dance -- principally, when the type isn't Copy like bool is, and so actually taking ownership and replacing it with something else requires non-trivial unsafe code. In general, the mem module has a collection of relatively low-level operations on memory patterns, and mentally, I, and I suspect many Rust programmers, go to it when we're doing memory manipulation. As @jdahlstrom put it, it's a bit of a "second-class citizen compared to methods". I believe there are two patterns where a "Take" pattern is used that is more about API usability, rather than memory handling: Options and bool. For the former, we already have the .take method (and in my Rust experience, anecdotally, is used often). We don't have anything for the latter.

2. It's a common enough operation that importing the use declaration can be annoying enough to justify its existence.

As a general statement here though, I suspect this PR divides users cleanly into "I have always wanted this and wished it existed" and "I have never wanted that, and I think it's bloat."

[sanbox-irl]

Also updated with feedback from @kpreid and @mbartlett21, thank you both!

[jquesada2016]

Adding take() for all T where T: Default is a splendid idea!

[mbartlett21]

1. Adding mem into a project implies a more complex operation than .take. mem::take, imo, is used primarily in patterns where you don't want to do the borrowchecker dance -- principally, when the type isn't Copy like bool is, and so actually taking ownership and replacing it with something else requires non-trivial unsafe code. In general, the mem module has a collection of relatively low-level operations on memory patterns, and mentally, I, and I suspect many Rust programmers, go to it when we're doing memory manipulation. As @jdahlstrom put it, it's a bit of a "second-class citizen compared to methods". I believe there are two patterns where a "Take" pattern is used that is more about API usability, rather than memory handling: Options and bool. For the former, we already have the .take method (and in my Rust experience, anecdotally, is used often). We don't have anything for the latter.

Perhaps you could put this under the Motivation section, saying why mem::take, though it has the same functionality, is not as suitable for the task?

[sanbox-irl]

1. Adding mem into a project implies a more complex operation than .take. mem::take, imo, is used primarily in patterns where you don't want to do the borrowchecker dance -- principally, when the type isn't Copy like bool is, and so actually taking ownership and replacing it with something else requires non-trivial unsafe code. In general, the mem module has a collection of relatively low-level operations on memory patterns, and mentally, I, and I suspect many Rust programmers, go to it when we're doing memory manipulation. As @jdahlstrom put it, it's a bit of a "second-class citizen compared to methods". I believe there are two patterns where a "Take" pattern is used that is more about API usability, rather than memory handling: Options and bool. For the former, we already have the .take method (and in my Rust experience, anecdotally, is used often). We don't have anything for the latter.

Perhaps you could put this under the Motivation section, saying why mem::take, though it has the same functionality, is not as suitable for the task?

thank you!

[programmerjake]

adding a replace method on bool would also be a good idea, since I mentally put take and replace in the same category.

[tanriol]

To me a take feels out of place on a bool just like it would look weird on an u32, f64 or Duration. All these types have a Default impl, but none of them have that default strong enough to justify a dedicated "copy and reset to default" operation.

Depending on the scale of the problem, in my code I'd use one of:

• mem::replace and/or mem::take
• Option<()>
• struct Flag(bool) with methods set, reset and take

and I'd prefer any of these to bool::take.

[scottmcm]

➕💯 to @tanriol's post here. I agree that most Default types shouldn't have take, and that mem::take is perfect for the comparatively-rare situations where it's what's needed.

That said, my opposition here is basically entirely to the precedent from naming this take.

If this was instead phrased as bool-terminology methods, then I'm more in the "sure, why not" camp. Perhaps this:

impl bool {
    #[must_use = "If you don't need the return value, just use `= false`"]
    pub reset(&mut self) -> bool {
        mem::replace(self, false)
    }

    #[must_use = "If you don't need the return value, just use `= true`"]
    pub set(&mut self) -> bool {
        mem::replace(self, true)
    }
}

[nrc]

As a reader, I prefer this version. It is only one line longer and it is much clearer what is going on. take is not immediately obvious unless you are very familiar with the option version, and even then I think you might stop for a minute to consider the semantics (looking just at self.dirty.take() I might assume dirty is being treated as some kind of pointer and left as null).

(I also have a preference against functions which have both an important side effect and return an important value, they tend to be 'too clever' and to surprise readers).

[joshtriplett]

@rfcbot close

[rfcbot]

Team member @joshtriplett has proposed to close this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period, with a disposition to close, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This is now closed.