Skip to content

Add more methods in Cell for non-Copy types #39715

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Add more methods in Cell for non-Copy types #39715

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
19c0c7b
Add more methods in Cell for non-Copy types
Feb 10, 2017
3775c4d
ensure panic safety
F001 Feb 10, 2017
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
111 changes: 109 additions & 2 deletions src/libcore/cell.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,7 @@ use fmt::{self, Debug, Display};
use marker::Unsize;
use mem;
use ops::{Deref, DerefMut, CoerceUnsized};
use ptr;

/// A mutable memory location.
///
Expand Down Expand Up @@ -280,6 +281,19 @@ unsafe impl<T> Send for Cell<T> where T: Send {}
#[stable(feature = "rust1", since = "1.0.0")]
impl<T> !Sync for Cell<T> {}

#[unstable(feature = "move_cell", issue = "39264")]
impl<T:Clone> Clone for Cell<T> {
#[inline]
default fn clone(&self) -> Cell<T> {
let temp = unsafe { mem::uninitialized() };
let inner = self.replace(temp);
// If `clone` panics, let it crash.
Copy link
Contributor

@hanna-kruppe hanna-kruppe Feb 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Isn't this super unsafe? If a panic occurs here, *self will be dropped (not in this scope, but later during stack unwinding) which will drop uninitialized memory which will cause UB.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. AssertUnwindSafe is not unsafe, and for good reason. Safe code using AssertUnwindSafe unwisely may get very undesirable behavior, but not UB.
  2. AssertUnwindSafe is irrelevant, as this problem occurs without any catch_unwind calls. In fact, a well-placed catch_unwind can stop a panic in this clone call from dropping *self (by stopping the unwinding before it gets to the stack frame where the Cell would be dropped).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, you are right. I need to figure out a solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

@F001: You'll probably want to use something similar to the Hole structure described in the nomicon, which writes a valid value back into the original cell in case of a panic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or rather, is there a reason, you can't just use self.value.get().clone()?

Copy link
Contributor

Choose a reason for hiding this comment

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

@TimNN @F001 I haven't thought through the details but https://users.rust-lang.org/t/why-does-cell-require-copy-instead-of-clone/5769/3?u=rkruppe seems relevant.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rkruppe: Thanks for the link! After having a quick read through it and the comments on the original RFC, I think that safely implementing the methods proposed by this PR is fundamentally incompatible with "MoveCells", since we are not allowed to call any method on the contained Type, as I understand it. (This is not a problem for "CopyCells" since we can create a new copy of the value without calling any methods on it).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your comments!

@TimNN I don't agree that these methods are fundamentally incompatible with "MoveCells". Cell is not intended to prohibit methods of the contained type. The most important guarantee is "disallowing any reference (mutable or immutable) to the data contained in the cell."

Copy link
Contributor

Choose a reason for hiding this comment

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

The most important guarantee is "disallowing any reference (mutable or immutable) to the data contained in the cell."

I agree with that statement. However I think exactly this is happening here: As soon as you call clone or partial_eq, you provide those methods with a reference to the data in the cell, which then allows for bad things to happen, as can be seen by the example in the post linked by @rkruppe.

let another = inner.clone();
self.set(inner);
Cell::new(another)
}
}

#[stable(feature = "rust1", since = "1.0.0")]
impl<T:Copy> Clone for Cell<T> {
#[inline]
Expand All @@ -289,14 +303,26 @@ impl<T:Copy> Clone for Cell<T> {
}

#[stable(feature = "rust1", since = "1.0.0")]
impl<T:Default + Copy> Default for Cell<T> {
impl<T:Default> Default for Cell<T> {
/// Creates a `Cell<T>`, with the `Default` value for T.
#[inline]
fn default() -> Cell<T> {
Cell::new(Default::default())
}
}

#[unstable(feature = "move_cell", issue = "39264")]
impl<T:PartialEq> PartialEq for Cell<T> {
#[inline]
default fn eq(&self, other: &Cell<T>) -> bool {
if ptr::eq(self, other) {
true
} else {
unsafe { Cell::take_and_restore(self, other, |l, r| l == r) }
}
}
}

#[stable(feature = "rust1", since = "1.0.0")]
impl<T:PartialEq + Copy> PartialEq for Cell<T> {
#[inline]
Expand All @@ -305,9 +331,60 @@ impl<T:PartialEq + Copy> PartialEq for Cell<T> {
}
}

#[unstable(feature = "move_cell", issue = "39264")]
impl<T:Eq> Eq for Cell<T> {}

#[stable(feature = "cell_eq", since = "1.2.0")]
impl<T:Eq + Copy> Eq for Cell<T> {}

#[unstable(feature = "move_cell", issue = "39264")]
impl<T:PartialOrd> PartialOrd for Cell<T> {
#[inline]
default fn partial_cmp(&self, other: &Cell<T>) -> Option<Ordering> {
if ptr::eq(self, other) {
Some(Ordering::Equal)
} else {
unsafe { Cell::take_and_restore(self, other, |l, r| l.partial_cmp(r)) }
}
}

#[inline]
default fn lt(&self, other: &Cell<T>) -> bool {
if ptr::eq(self, other) {
true
} else {
unsafe { Cell::take_and_restore(self, other, |l, r| l < r) }
}
}

#[inline]
default fn le(&self, other: &Cell<T>) -> bool {
if ptr::eq(self, other) {
true
} else {
unsafe { Cell::take_and_restore(self, other, |l, r| l <= r) }
}
}

#[inline]
default fn gt(&self, other: &Cell<T>) -> bool {
if ptr::eq(self, other) {
true
} else {
unsafe { Cell::take_and_restore(self, other, |l, r| l > r) }
}
}

#[inline]
default fn ge(&self, other: &Cell<T>) -> bool {
if ptr::eq(self, other) {
true
} else {
unsafe { Cell::take_and_restore(self, other, |l, r| l >= r) }
}
}
}

#[stable(feature = "cell_ord", since = "1.10.0")]
impl<T:PartialOrd + Copy> PartialOrd for Cell<T> {
#[inline]
Expand Down Expand Up @@ -336,6 +413,18 @@ impl<T:PartialOrd + Copy> PartialOrd for Cell<T> {
}
}

#[unstable(feature = "move_cell", issue = "39264")]
impl<T:Ord> Ord for Cell<T> {
#[inline]
default fn cmp(&self, other: &Cell<T>) -> Ordering {
if ptr::eq(self, other) {
Ordering::Equal
} else {
unsafe { Cell::take_and_restore(self, other, |l, r| l.cmp(r)) }
}
}
}

#[stable(feature = "cell_ord", since = "1.10.0")]
impl<T:Ord + Copy> Ord for Cell<T> {
#[inline]
Expand All @@ -345,7 +434,7 @@ impl<T:Ord + Copy> Ord for Cell<T> {
}

#[stable(feature = "cell_from", since = "1.12.0")]
impl<T: Copy> From<T> for Cell<T> {
impl<T> From<T> for Cell<T> {
fn from(t: T) -> Cell<T> {
Cell::new(t)
}
Expand Down Expand Up @@ -422,6 +511,24 @@ impl<T> Cell<T> {
pub fn into_inner(self) -> T {
unsafe { self.value.into_inner() }
}

/// Private function used for implementing other methods when `T` is not `Copy`.
///
/// This is `unsafe` because `left` and `right` may point to the same object.
#[unstable(feature = "move_cell", issue = "39264")]
unsafe fn take_and_restore<Result, F>(left: &Self, right: &Self, f: F) -> Result
where F: FnOnce(&T, &T) -> Result
{
let temp_left = mem::uninitialized();
let temp_right = mem::uninitialized();
let lhs = left.replace(temp_left);
let rhs = right.replace(temp_right);
// If panic happens in `f`, just let it crash.
Copy link
Contributor

@hanna-kruppe hanna-kruppe Feb 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

For the record: same UB issue here as in the Clone impl, so this affects most of the other impls added in this PR too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. Updated.

let result = f(&lhs, &rhs);
left.set(lhs);
right.set(rhs);
return result;
}
}

impl<T: Default> Cell<T> {
Expand Down