-
Notifications
You must be signed in to change notification settings - Fork 13.4k
[experiment] Make Cell<T>::update
work for T: Default | Copy
.
#89357
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -435,8 +435,15 @@ impl<T: Copy> Cell<T> { | |
// but `Cell` is `!Sync` so this won't happen. | ||
unsafe { *self.value.get() } | ||
} | ||
} | ||
|
||
/// Updates the contained value using a function and returns the new value. | ||
impl<T> Cell<T> { | ||
/// Updates the contained value using a function. | ||
/// | ||
/// If `T` implements [`Copy`], the function is called on a copy of the | ||
/// contain value. Otherwise, if `T` implements [`Default`], the value | ||
/// contained in the cell is temporarily replaced by [`Default::default()]` | ||
m-ou-se marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// while the function runs. | ||
/// | ||
/// # Examples | ||
/// | ||
|
@@ -445,22 +452,78 @@ impl<T: Copy> Cell<T> { | |
/// | ||
/// use std::cell::Cell; | ||
/// | ||
/// let c = Cell::new(5); | ||
/// let new = c.update(|x| x + 1); | ||
/// let a = Cell::new(5); | ||
/// a.update(|x| x + 1); | ||
/// | ||
/// assert_eq!(new, 6); | ||
/// assert_eq!(c.get(), 6); | ||
/// let b = Cell::new("Hello".to_string()); | ||
/// b.update(|x| x + " World!"); | ||
/// | ||
/// assert_eq!(a.get(), 6); | ||
/// assert_eq!(b.take(), "Hello World!"); | ||
/// ``` | ||
#[inline] | ||
#[unstable(feature = "cell_update", issue = "50186")] | ||
pub fn update<F>(&self, f: F) -> T | ||
pub fn update<F>(&self, f: F) | ||
where | ||
F: FnOnce(T) -> T, | ||
T: get_or_take::CopyOrDefault, | ||
{ | ||
let old = self.get(); | ||
let old = <T as get_or_take::CopySpec>::get_or_take(self); | ||
let new = f(old); | ||
self.set(new); | ||
new | ||
} | ||
} | ||
|
||
#[unstable(feature = "cell_update_specializations", issue = "none")] | ||
mod get_or_take { | ||
use super::Cell; | ||
|
||
#[rustc_on_unimplemented( | ||
label = "`{Self}` implements neither Copy nor Default", | ||
message = "`Cell<{Self}>::update()` requires either `{Self}: Copy` or `{Self}: Default`" | ||
)] | ||
#[marker] | ||
pub trait CopyOrDefault: Sized {} | ||
|
||
impl<T: Copy> CopyOrDefault for T {} | ||
impl<T: Default> CopyOrDefault for T {} | ||
|
||
#[rustc_unsafe_specialization_marker] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Who would be the right person to evaluate soundness of hacks like this? @matthewjasper maybe? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My notes so far: thanks to However, that means IOW, when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is right: In that regard, I find that writing the overriding impl before the default one, and slightly changing the naming of the things involved, can improve the readability of the code, as I've showcased in this Playground (I've also nested some of the trait definitions and impls, which as a bonus scopes the Extra comments / remarks regarding the soundness hereSo, in the linked Playground one can see that the "parent impl" for the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It may be sound in this particular case, but isn't this basically a… I guess you could say soundness hole, in
So it's not completely sound, but it's in some sense 'less unsound' than unrestricted specialization because you can't use it to call methods. Except you are using it to call methods, by taking advantage of the supertrait With that ability, how is this any different from unrestricted specialization? In other words, what dangerous thing could you do by specializing on I note that in the case of some existing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this might be a soundness hole in the current There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've opened #89413 for this |
||
trait IsDefault: Default {} | ||
impl<T: Default> IsDefault for T {} | ||
|
||
pub(super) trait CopySpec { | ||
fn get_or_take(cell: &Cell<Self>) -> Self; | ||
} | ||
|
||
trait DefaultSpec { | ||
fn get_or_take(cell: &Cell<Self>) -> Self; | ||
} | ||
|
||
impl<T> CopySpec for T { | ||
default fn get_or_take(cell: &Cell<Self>) -> Self { | ||
<T as DefaultSpec>::get_or_take(cell) | ||
} | ||
} | ||
|
||
impl<T: Copy> CopySpec for T { | ||
fn get_or_take(cell: &Cell<Self>) -> Self { | ||
cell.get() | ||
} | ||
} | ||
|
||
impl<T> DefaultSpec for T { | ||
default fn get_or_take(_: &Cell<Self>) -> Self { | ||
// This is unreachable because we'd only get here for types that | ||
// implement CopyOrDefault, but not Copy or Default, which isn't possible. | ||
unreachable!() | ||
} | ||
} | ||
|
||
impl<T: IsDefault> DefaultSpec for T { | ||
fn get_or_take(cell: &Cell<Self>) -> Self { | ||
cell.take() | ||
} | ||
} | ||
} | ||
|
||
|
Uh oh!
There was an error while loading. Please reload this page.