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

Document lack of panic safety guarantees of Clone::clone_from #98461

Closed
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
56 changes: 53 additions & 3 deletions library/core/src/clone.rs
Original file line number Diff line number Diff line change
Expand Up @@ -124,9 +124,59 @@ pub trait Clone: Sized {

/// Performs copy-assignment from `source`.
///
/// `a.clone_from(&b)` is equivalent to `a = b.clone()` in functionality,
/// but can be overridden to reuse the resources of `a` to avoid unnecessary
/// allocations.
/// `a.clone_from(&b)` is equivalent to `a = b.clone()` in functionality except cases
/// when it panics in the middle of operation. It can be overridden to reuse the resources
Comment on lines +127 to +128
Copy link
Contributor

@kpreid kpreid Aug 25, 2023

Choose a reason for hiding this comment

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

I think this sentence could be readily misinterpreted as “clone_from is equivalent to clone except that in some cases it may panic”, since this is the first mention of panicking in the text. I'm not sure exactly what to do about that, but how about "…in functionality (except in the event of a panic).” which emphasizes that it's about the consequences of a panic, not causing a panic. The “in the middle” is explained below so it doesn't need to be brought up here.

/// of `a` to avoid unnecessary allocations.
///
/// # Panic behaviour
///
/// Due to it's nature, this method cannot guarantee that it would be transactional.
Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny grammar nit: This should read "Due to its" (not "it's" with an apostrophe)

/// If call panics, `self` can be left in inconsistent state.
Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny nit: "If a call panics"

/// This is different from `a = b.clone()` because in that case `a` is overwritten
/// only if `clone()` succeedes. Therefore, if you need transactional behaviour,
/// you shouldn't use this method.
///
/// `clone_from` is intended to preserve resources owned by `self`
/// so it cannot preserve old data stored in those resources.
///
/// That example shows one case of such behaviour:
Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny nit: "This example"

/// ```no_run
Copy link
Member

Choose a reason for hiding this comment

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

Why is this no_run?

/// use std::sync::Mutex;
///
/// #[derive(PartialEq, Eq, Debug)]
/// struct PanicAtTheClone(bool);
///
/// impl Clone for PanicAtTheClone {
/// fn clone(&self) -> Self {
/// if self.0 {
/// panic!()
/// } else {
/// PanicAtTheClone(false)
/// }
/// }
/// }
///
/// // first element will copy just fine, second element will panic if cloned!
/// let src = vec![PanicAtTheClone(false), PanicAtTheClone(true)];
/// // start with an empty vec
/// let dest = Mutex::new(vec![]);
///
/// // clone from src into dest
/// std::panic::catch_unwind(|| {
Comment on lines +162 to +165
Copy link
Member

Choose a reason for hiding this comment

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

Instead of a Mutex, you can use catch_unwind(AssertUnwindSafe(..)).

/// dest.lock().unwrap().clone_from(&src);
/// })
/// .expect_err("Must panic");
///
/// // handle the poisoned mutex (without the mutex even attempting this produces a compiler error!)
/// let guard = dest
/// .lock()
/// .expect_err("mutex should be poisoned by the panic")
/// .into_inner();
///
/// // dest is left in an incomplete state with only half of the clone having
/// // completed successfully
/// assert_eq!(vec![PanicAtTheClone(false)], *guard);
Comment on lines +176 to +178
Copy link
Member

Choose a reason for hiding this comment

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

Maybe change this to just assert_ne!(src, dest);, since we don't want to make promises about the resulting state.

/// ```
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
#[cfg_attr(bootstrap, default_method_body_is_const)]
Expand Down