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

AngelicosPhosphoros · 2022-06-24T15:57:23Z

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.

rust-highfive · 2022-06-24T15:57:26Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

rust-highfive · 2022-06-24T15:57:27Z

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

AngelicosPhosphoros · 2022-07-03T11:31:28Z

r? @m-ou-se
We had already discussed this so maybe you would review?

joshtriplett · 2022-07-20T19:30:58Z

Since this is a change to the documented guarantees (or lack thereof):

@rfcbot merge

Also, @Amanieu wanted to propose a different wording to avoid the term "safe" (which in this context is unrelated to the usual Rust safe/unsafe distinction).

@rfcbot concern reword

rfcbot · 2022-07-20T19:30:59Z

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

~~reword~~ resolved by Document lack of panic safety guarantees of Clone::clone_from #98461 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

Amanieu · 2022-07-20T19:47:13Z

The term "panic safe" generally refers to ensure unsafe code remains sound in the face of panics. Its use here is not completely accurate: clone_from must be panic-safe since it is a safe function. However what it can do is leave the contents of self as indeterminate if a panic occurs.

yaahc · 2022-07-26T01:12:11Z

library/core/src/clone.rs

+    /// ```no_run
+    /// use std::sync::atomic::{AtomicBool, Ordering};
+    ///
+    /// // Ensure that static vector accessed only once
+    /// static WAS_CALLED: AtomicBool = AtomicBool::new(false);
+    /// if WAS_CALLED.fetch_or(true, Ordering::Acquire){
+    ///     return;
+    /// }
+    ///
+    /// #[derive(PartialEq, Eq, Debug)]
+    /// struct IAmPanicDuringClone;
+    ///
+    /// impl Clone for IAmPanicDuringClone {
+    ///     fn clone(&self) -> Self {
+    ///         panic!()
+    ///     }
+    /// }
+    ///
+    /// // Here little complicated
+    /// // because we need to pass it through panic boundary for demo purposes.
+    /// // Hacky and improper way to do this is usage of static variable.
+    /// // Don't write like that in production.
+    /// static mut A: Vec<IAmPanicDuringClone> = Vec::new();
+    /// let a = unsafe{
+    ///     // Safe because we have AtomicBool check before.
+    ///     &mut A
+    /// };
+    ///
+    /// *a = vec![
+    ///         IAmPanicDuringClone,
+    ///         IAmPanicDuringClone,
+    ///         IAmPanicDuringClone,
+    ///     ];
+    /// let b = vec![
+    ///     IAmPanicDuringClone,
+    ///     IAmPanicDuringClone,
+    ///     IAmPanicDuringClone,
+    /// ];
+    /// let c = vec![IAmPanicDuringClone, IAmPanicDuringClone];
+    ///
+    /// // This succeedes.
+    /// assert_eq!(a, &b);
+    /// std::panic::catch_unwind(|| {
+    ///     unsafe { A.clone_from(&c) };
+    /// }).expect_err("Must panic");
+    ///
+    /// // This fails at Rust 1.61.
+    /// assert_eq!(unsafe { &A }, &b);
+    /// ```


I'm not comfortable with using this as an example (it feels a bit contrived), how do you feel about using this version instead?

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b0df895179ea9769302d6b866baec535

Absolutely agree, your version is much more cleaner and has less noise in it.

Would fix PR later.

@scottmcm

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](rust-lang#98445 (comment)). Co-authored-by: Jane Losare-Lusby <[email protected]>

AngelicosPhosphoros · 2022-08-26T18:12:17Z

@m-ou-se Reworded comment, updated example as @yaahc requested (also added her as co-author because I copied her example).

@rustbot label: +S-waiting-on-review -S-waiting-on-author

m-ou-se · 2023-02-28T15:21:38Z

library/core/src/clone.rs

+    /// let dest = Mutex::new(vec![]);
+    ///
+    /// // clone from src into dest
+    /// std::panic::catch_unwind(|| {


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

m-ou-se · 2023-02-28T15:23:51Z

library/core/src/clone.rs

+    /// // dest is left in an incomplete state with only half of the clone having
+    /// // completed successfully
+    /// assert_eq!(vec![PanicAtTheClone(false)], *guard);


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

m-ou-se · 2023-02-28T15:23:59Z

library/core/src/clone.rs

+    /// so it cannot preserve old data stored in those resources.
+    ///
+    /// That example shows one case of such behaviour:
+    /// ```no_run


Why is this no_run?

m-ou-se · 2023-02-28T15:25:46Z

ping @joshtriplett for your rfcbot concern.

Dylan-DPC · 2023-07-21T12:55:36Z

@AngelicosPhosphoros any updates on this? thanks

joshtriplett · 2023-08-19T05:55:36Z

@rfcbot resolve reword

Sorry to have missed the ping here.

rfcbot · 2023-08-19T05:55:40Z

🔔 This is now entering its final comment period, as per the review above. 🔔

joshlf

Just a few tiny drive-by suggestions!

joshlf · 2023-08-24T06:41:48Z

library/core/src/clone.rs

+    ///
+    /// # Panic behaviour
+    ///
+    /// Due to it's nature, this method cannot guarantee that it would be transactional.


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

joshlf · 2023-08-24T06:42:23Z

library/core/src/clone.rs

+    /// # 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.


Tiny nit: "If a call panics"

joshlf · 2023-08-24T06:43:19Z

library/core/src/clone.rs

+    /// `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:


Tiny nit: "This example"

kpreid · 2023-08-25T04:04:35Z

library/core/src/clone.rs

+    /// `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


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.

rfcbot · 2023-08-29T06:00:43Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

apiraino · 2024-02-12T09:02:04Z

@AngelicosPhosphoros seems this PR needs a tiny bit of polish and then it's gold. Any capacity to drive this to the finish line? (No pressure, just asking as part of the periodic reviews we do on pending contributions). Thanks 🙂

JohnCSimon · 2024-04-14T17:26:02Z

Author hasn't responded in months...

@AngelicosPhosphoros

Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you are going to continue please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

rust-highfive assigned kennytm Jun 24, 2022

rustbot added the T-libs label Jun 24, 2022

rust-highfive added the S-waiting-on-review label Jun 24, 2022

AngelicosPhosphoros mentioned this pull request Jun 27, 2022

[Experiment] [WIP] Add clone_from to clone derive #98445

Closed

rust-highfive assigned m-ou-se and unassigned kennytm Jul 3, 2022

m-ou-se added I-libs-api-nominated S-waiting-on-team T-libs-api and removed S-waiting-on-review T-libs labels Jul 3, 2022

joshtriplett removed the I-libs-api-nominated label Jul 20, 2022

rfcbot added proposed-final-comment-period disposition-merge labels Jul 20, 2022

yaahc requested changes Jul 26, 2022

View reviewed changes

m-ou-se added S-waiting-on-author and removed S-waiting-on-team labels Jul 27, 2022

AngelicosPhosphoros force-pushed the document_panic_safety_of_clone_from branch from c51267f to 67251ab Compare August 26, 2022 18:00

AngelicosPhosphoros force-pushed the document_panic_safety_of_clone_from branch from 67251ab to d665f28 Compare August 26, 2022 18:08

rustbot added S-waiting-on-review and removed S-waiting-on-author labels Aug 26, 2022

JohnCSimon removed the S-waiting-on-review label Oct 8, 2022

JohnCSimon added the S-waiting-on-review label Oct 8, 2022

m-ou-se reviewed Feb 28, 2023

View reviewed changes

KittyBorgX added S-waiting-on-author and removed S-waiting-on-review labels Apr 29, 2023

rfcbot added final-comment-period and removed proposed-final-comment-period labels Aug 19, 2023

joshlf reviewed Aug 24, 2023

View reviewed changes

kpreid reviewed Aug 25, 2023

View reviewed changes

rfcbot added finished-final-comment-period to-announce and removed final-comment-period labels Aug 29, 2023

apiraino removed the to-announce label Sep 1, 2023

JohnCSimon added S-waiting-on-author and removed S-waiting-on-author labels Nov 12, 2023

JohnCSimon closed this Apr 14, 2024

rustbot added the S-inactive label Apr 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

AngelicosPhosphoros commented Jun 24, 2022

rust-highfive commented Jun 24, 2022

rust-highfive commented Jun 24, 2022

AngelicosPhosphoros commented Jul 3, 2022

joshtriplett commented Jul 20, 2022

rfcbot commented Jul 20, 2022 •

edited

Loading

Amanieu commented Jul 20, 2022

yaahc Jul 26, 2022 •

edited

Loading

AngelicosPhosphoros Jul 26, 2022

AngelicosPhosphoros commented Aug 26, 2022

m-ou-se Feb 28, 2023

m-ou-se Feb 28, 2023

m-ou-se Feb 28, 2023

m-ou-se commented Feb 28, 2023

Dylan-DPC commented Jul 21, 2023

joshtriplett commented Aug 19, 2023

rfcbot commented Aug 19, 2023

joshlf left a comment

joshlf Aug 24, 2023

joshlf Aug 24, 2023

joshlf Aug 24, 2023

kpreid Aug 25, 2023 •

edited

Loading

rfcbot commented Aug 29, 2023

apiraino commented Feb 12, 2024

JohnCSimon commented Apr 14, 2024

		/// `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

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

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

Conversation

AngelicosPhosphoros commented Jun 24, 2022

rust-highfive commented Jun 24, 2022

rust-highfive commented Jun 24, 2022

AngelicosPhosphoros commented Jul 3, 2022

joshtriplett commented Jul 20, 2022

rfcbot commented Jul 20, 2022 • edited Loading

Amanieu commented Jul 20, 2022

yaahc Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

AngelicosPhosphoros Jul 26, 2022

Choose a reason for hiding this comment

AngelicosPhosphoros commented Aug 26, 2022

m-ou-se Feb 28, 2023

Choose a reason for hiding this comment

m-ou-se Feb 28, 2023

Choose a reason for hiding this comment

m-ou-se Feb 28, 2023

Choose a reason for hiding this comment

m-ou-se commented Feb 28, 2023

Dylan-DPC commented Jul 21, 2023

joshtriplett commented Aug 19, 2023

rfcbot commented Aug 19, 2023

joshlf left a comment

Choose a reason for hiding this comment

joshlf Aug 24, 2023

Choose a reason for hiding this comment

joshlf Aug 24, 2023

Choose a reason for hiding this comment

joshlf Aug 24, 2023

Choose a reason for hiding this comment

kpreid Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

rfcbot commented Aug 29, 2023

apiraino commented Feb 12, 2024

JohnCSimon commented Apr 14, 2024

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

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

rfcbot commented Jul 20, 2022 •

edited

Loading

yaahc Jul 26, 2022 •

edited

Loading

kpreid Aug 25, 2023 •

edited

Loading