Skip to content
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

[experiment] Make Cell<T>::update work for T: Default | Copy. #89357

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 71 additions & 8 deletions library/core/src/cell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
/// contained value. Otherwise, if `T` implements [`Default`], the value
/// contained in the cell is temporarily replaced by [`Default::default()`]
/// while the function runs.
///
/// # Examples
///
Expand All @@ -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]
Copy link
Member Author

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My notes so far: thanks to CopyOrDefault, the only way Cell::update could be unsound is if both traits are implemented, but with different imposed relationships on lifetimes.

However, that means Copy is used, and Default isn't called. So this is just as unsound as any other specialization on Copy.

IOW, when Default::default gets called, T: CopyOrDefault is equivalent to T: Default and any requirements from the Default impl will be imposed on the caller of Cell::update.

Copy link
Contributor

@danielhenrymantilla danielhenrymantilla Sep 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is right: IsDefault here is used to specialize over nothing.

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 unsafe_specialization_marker, since I can fathom the branches better that way, but I understand that that's debatable. I stand by the order of my impls with the /* + !Trait */ annotations, though)

Extra comments / remarks regarding the soundness here

So, in the linked Playground one can see that the "parent impl" for the IsDefault specialization is CopyOrDefault, with the specialized Copy impls stripped. This means that we are only dealing with Default types in practice (modulo Copy specialization issues), so the specialization is effectively specializing over the whole subset / it is as if there effectively were a Default supertrait. That's why I consider this IsDefault specialization, when contained to this very usage, sound. But that does assume that the Copy specialization is fine to begin with, which it may technically not be.

  • For instance, if the Copy specialization were not to kick in for a Copy + !Default type, could we reach the unreachable! impl? 🤔

  • So, if anything, the only potential issue / area I'm not that confident about is whether a "flaw" with the Copy specialization could be "amplified" by allowing a different "flaw" with the IsDefault specialization.

  • And even then, I could only see that causing "unsoundness" if we were to guarantee, at the API level, that for Copy + Default types update() shall necessarily Copy (because a hypothetical failed application of Copy specialization could break that guarantee). That's why, unless this situation is cleared up of all suspicion, I think we could loosen that guarantee to something like mem::needs_drop() and other such stuff:

    • For Copy + Default types, the "using get()" optimization is "likely" to be favored, but it is not guaranteed to do so: unsafe code cannot rely on this for soundness.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 rustc_unsafe_specialization_marker? To quote the PR that created the attribute:

This is unsound but we allow it in the short term because it can't cause use after frees with purely safe code in the same way as specializing on traits methods can.

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 Default, from a specialized impl (impl<T: IsDefault> DefaultSpec for T) whose parent impl does not have a Default bound.

With that ability, how is this any different from unrestricted specialization? In other words, what dangerous thing could you do by specializing on Default (which is not allowed) that you cannot do by creating a wrapper trait like this IsDefault and specializing on that? It seems like the answer is 'nothing', in which case the idea of rustc_unsafe_specialization_marker being limited to marker traits that have no methods is meaningless. But I may be missing something.

I note that in the case of some existing rustc_unsafe_specialization_marker traits, such as FusedIterator and TrustedLen, they do have supertraits – in both cases, Iterator – but impls that specialize on them always(?) have parent impls that are already bounded on Iterator. On the other hand, another such trait, MarkerEq, as used in RcEqIdent and ArcEqIdent, is the same kind of wrapper as IsDefault. It even has a comment saying:

Hack to allow specializing on Eq even though Eq has a method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might be a soundness hole in the current min_specialization impl that I didn't get around to fixing.

Copy link
Contributor

Choose a reason for hiding this comment

The 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()
}
}
}

Expand Down
1 change: 1 addition & 0 deletions library/core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@
#![feature(lang_items)]
#![feature(link_llvm_intrinsics)]
#![feature(llvm_asm)]
#![feature(marker_trait_attr)]
#![feature(min_specialization)]
#![feature(negative_impls)]
#![feature(never_type)]
Expand Down
9 changes: 7 additions & 2 deletions library/core/tests/cell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,16 @@ fn smoketest_cell() {
fn cell_update() {
let x = Cell::new(10);

assert_eq!(x.update(|x| x + 5), 15);
x.update(|x| x + 5);
assert_eq!(x.get(), 15);

assert_eq!(x.update(|x| x / 3), 5);
x.update(|x| x / 3);
assert_eq!(x.get(), 5);

let x = Cell::new("abc".to_string());

x.update(|x| x + "123");
assert_eq!(x.take(), "abc123");
}

#[test]
Expand Down