From d665f28cd7c7447209045a12b056d41b96f8cd2c Mon Sep 17 00:00:00 2001 From: AngelicosPhosphoros Date: Fri, 24 Jun 2022 18:51:29 +0300 Subject: [PATCH] Document lack of panic safety guarantees of `Clone::clone_from` It isn't panic safe de-facto (e.g. for `Vec`) so I just document current behaviour. Panic safety was mentioned by @scottmcm when discussing [PR with deriving clone_from](https://github.com/rust-lang/rust/pull/98445#issuecomment-1165371469). Co-authored-by: Jane Losare-Lusby --- library/core/src/clone.rs | 56 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 53 insertions(+), 3 deletions(-) diff --git a/library/core/src/clone.rs b/library/core/src/clone.rs index fd5624812f554..1856ed1de43f1 100644 --- a/library/core/src/clone.rs +++ b/library/core/src/clone.rs @@ -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 + /// of `a` to avoid unnecessary allocations. + /// + /// # Panic behaviour + /// + /// Due to it's nature, this method cannot guarantee that it would be transactional. + /// If call panics, `self` can be left in inconsistent state. + /// 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: + /// ```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(|| { + /// 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); + /// ``` #[inline] #[stable(feature = "rust1", since = "1.0.0")] #[cfg_attr(bootstrap, default_method_body_is_const)]