About retained ownership and `.clone()` _vs._ `{Ar,R}c::clone(&`?

On a GH issue, @trentj mentioned:

[…] the common recommendation to use Arc::clone(&x) instead of simply x.clone() . I have mixed feelings about that convention

This post is not gonna revolve exclusively about this, nor about calling out Trentj :grinning_face_with_smiling_eyes:, since, on the contrary, I always appreciate when something "given" is questioned every now and then, to make sure we don't operate off sheer inertia :slightly_smiling_face:

And it turns out that I've been having "mixed feelings" about the clone situation myself (although maybe for different reasons):

  • On the one hand, writing .clone() is handy (shorter to type than ARc::clone(&), accessible (in the prelude, ARc is not), and actually plays a bit better with some corner case situations where type coercion or subtyping is involved (c.f. the aforementioned issue);

  • On the other hand, .clone() does not let one know about whether the operation is trivial / cheap, such as a counter increment (even an atomic one), or if one is actually duplicating a long string, a long vector, or some other big collection, at least not until looking at the actual types in play and what their Clone implementations are about. I believe it's a similar sentiment to the one that motivated the addition of the .copied() adaptor: one feels "better" when knowing they'll just be performing Copyes of the elements rather than Clones (assuming the type sizes are not too big, of course!).

    I believe some people have asked for a CheapClone (marker?) trait. But I don't think that's really the meaningful point here. Indeed, besides the "performance" point of view, there is also the point of view of semantics (although the shared vs unique access distinction in Rust makes this aspect less error-prone, which, btw, is one of the excellent things about Rust!):

    • while a "classic" .clone() duplicates memory, thus making mutations of the original not be visible on the newly obtained owned handle,

    • a "shared ownership"-clone, that is, a retain shared ownership operation, yields an owned handle which refers to the same entity as the clonee's, so that mutations through either owned handle can be observed by the other.

    I believe it is this aspect which motivates writing {Ar,R}c::clone(&…) rather than ….clone() when possible.

So, at this point, two things transpire:

  • method syntax can be a bit more convenient than associated-function syntax;

  • there is a semantical difference between duplicating an entity and retaining shared ownership of it.

And in between those two points lingers a rather thorny situation. Indeed, consider:

#[derive(Clone)]
struct NewType(Arc<…>);

or any similar thing.

This pattern is both idiomatic for a number of reasons, and yet plays quite poorly w.r.t. the "semantics of .clone()" situation:

  • Since this is a new type, I don't think anybody will be expecting to see NewType::clone(&…) syntax, that would be weird.
    So my_new_type_instance.clone() it is.
    And yet… we are not duplicating data, here, we are retaining ownership of it!

    This means that if we see, in the middle of code, something like:

    let my_instance2 = my_instance.clone();
    …
    my_instance2.clear();
    …
    stuff(my_instance);
    

    Without knowing the actual semantics of my_instance's type's Clone implementation, we don't know if my_instance will be in a cleared state or not: if it is some kind of wrapper around shared retained ownership, such as NewType, then it will be cleared (through some interior mutability obviously), and if it is, on the contrary, something like a DashMap, then it won't be.

    Some people may blame the interior mutability here —especially when named like that: such mutability, imho, ought to be rather named shared mutability— since, indeed, Rust is about exploiting the beauty of aliasing NAND mutation (not xor, btw: you can have neither), but the reality is that, for instance, async codebases that .spawn() stuff lead to a lot of ownership (either duplicated or retained), and this, in turn, leads to shared mutability being necessary: let's not over-demonize shared mutability, please.

    So my_thing.clone(); is an ambiguous thing to write, when my_thing may be featuring retained ownership.

  • There is also an API issue: is the retained clone part of the public API? Technically, it isn't, and besides documentation, there is nothing that can express this property. This means that if a downstream user of NewType needs, on their own end, to have retained ownership semantics, they don't have a choice but to re-wrap NewType within their own Arc, to guarantee those semantics. That seems a bit silly, at least for the situation where the author of NewType was intending to keep featuring the retained ownership implementation.


I hope that by this point, I've managed to "spread around" this feeling of uneasy-ness around Clone's over-terse semantics / contract / API.

Note that I am not criticizing Clone per se: such a trait, and the usage done in generic contexts such as the .cloned(), or .repeat() iterator adaptors, or the .resize() method on Vec, is indeed necessary. But the key word here is generics, and, from there, the general abstract contract that Clone is about.

And this is the "problematic" part: either Clone is not the best name for the functionality at play, here, or Arc / Rc implementing Clone are kind of abusing the contract. Whatever it may be since Arc / Rc (and newtypes wrapping it) implement Clone, this leads to an empirical definition of the actual Clone contract:

  • A type is Clone if it can feature a &Self -> Self operation.

    • I'll add that there may be kind of expected subcontracts, especially for Eq types, such as thing.clone() == thing, but even that is quite tacit (although conveyed by the Clone name, and compatible with retained ownership, so let's not dismiss it).

    And that's it. Clone is nowadays rather a OwnedFromRef, or, in more Rusty parlance, ToOwned<Owned = Self>. I like this ToOwned<Owned = Self> vision way more than the Clone name (although I'm not at all advocating to actually change the names, obviously! I just want the conceptual view to broaden a bit): we no longer name / assume "how" the ref-to-owned operation is achieved.

So, assuming that conceptual OwnedFromRef API, (the generic APIs would conceptually be built around that trait), and Clone would actually be a "marker" (sub)trait that would "taint" the owned_from_ref operation with "duplication" semantics.

And while this is conceptual, we'd now be able to feature a sibling of Clone: another subtrait which would, instead, "taint" the owned_from_ref operation with "retain" semantics.

Conceptual view / what the Rust stdlib could have been
//! Conceptual view: nevermind the names

/// Non-derivable (mainly to avoid overlapping impls issues with generic derives).
trait OwnedFromRef : Sized {
   fn owned_from_ref(&self) -> Self;
}

trait Clone : OwnedFromRef {
    fn clone(&self) -> Self;
}
impl<T : Clone> OwnedFromRef for T {
    fn owned_from_ref(&self) -> Self {
        self.clone()
    }
}

/// Derivable.
trait Retain : OwnedFromRef {
    fn retain(&self) -> Self;
}
impl<T : Retain> OwnedFromRef for T {
    fn owned_from_ref(&self) -> Self {
        self.retain()
    }
}

impl<T> Vec<T> {
    fn resize (self: &'_ mut Vec<T>, new_len: usize, value: T)
    where
        T : OwnedFromRef,
    {

Again, I'm not advocating for this design, nor that the stdlib should have been using it should we have a time machine: I just want people to keep an open mind w.r.t. engineering APIs and the semantics that go with it.

trait Retain : Clone

Now, back to the real world, that Retain trait is the one which I think deserves to be more than a conceptual / theoretical view of the mind: I believe that a Retain trait could be a nice addition to improve the quality / readability of our APIs, according to the code > documentation principle:

/// "Marker" trait to express shared ownership semantics
/// (_e.g._, through reference-counting).
trait Retain : Clone { // also added to the prelude.
    fn retain(&self) -> Self;
}

/// Prevent the `.retain()` implementation from being overridden,
/// giving it "marker" trait semantics.
partial      // <- current keyword in nightly is `default`, but it's misleading.
impl<T : Retain> Retain for T {
    #[inline]
    fn retain(&self) -> Self {
        self.clone()
    }
}

impl<T> Retain for Arc<T> {}

#[derive(
    Clone,
    Retain, // publicly express that this inner `Arc` is part of the public contract
)]
struct NewType(Arc<…>);
  • Indeed, if such a trait were to be added to the prelude, we'd have all the advantages of .clone() syntax, and none of its ambiguity.

  • It also scales well for other newtypes around Retained types, such that they are, themselves, Retained as well, but only if the author opts into that, through that Retain derive (equivalent to impl Retain for NewType {} + assertions that the every field is Retain).

I personally find the idea quite aesthetically pleasant, and will soon be publishing a PoC crate of the trait and the derive. I just wanted to gather some thoughts from the community, to see if they shared those feelings or not, so as to see if this would be (pre?)-RFC-worthy, and everything else: even though it's easy for a third-party crate to feature this abstraction, I find that not having it in the prelude, and not even in the standard library, defeats like 90% of the reasons to use this crate to begin with, which means that, as a third-party crate, it is quite doomed to fail.

Be it as it may, the actual discussions w.r.t. an stdlib addition ought to be deferred to that potential follow-up (pre?)-RFC. **The topic here is rather to gather feelings about .clone() vs. Arc::clone(), feelings about this Retain marker trait idea (I could imagine name bikeshedding, such as a RefCounted trait with an .increment_ref_count()), maybe even feelings about the tangentially related CheapClone trait (I personally find that such a trait would come with many more corner cases than the very simple Retain abstraction).

So, what are your thoughts about all this?


Updates:

  • The derive is debatable;

  • The name .retain() conflicts too much with what is used in the standard library, so methods named like .retained() or .reown() could be more suitable:

    • trait RetainOwnership {
          fn retained(&self) -> Self;
      }
      
      impl<T> RetainOwnership for Arc<T> {}
      
    • trait SharedOwnership {
          fn reown(&self) -> Self;
      }
      
      impl<T> SharedOwnership for Arc<T> {}
      
7 Likes

Haven’t read through your whole post in detail (yet) but I think you’re missing the fact that Arc implements Deref. This means that almost all method calls on an Arc<T> are expected to be forwarded to T itself. But .clone() isn’t. This is crucial; this is why Arc::clone(&…) has a huge clarity advantage. And this is also why ….clone() on a NewType(Arc<…>) is not a problem at all (unless that type implements Deref as-well).

6 Likes

That's a good point!

  • Nit:

    at all seems like a rather strong / blunt way of putting it, since I do think I have proven that .clone() on a NewType is not 100% optimal. But yeah, at least .clone() on NewType doesn't feature the problems of Deref.

It's indeed something very specific the retainable smart pointers. That being said, I find that for the vast majority of cases, .retain() actually helps disambiguate through Deref a fair amount!

  • Let's consider, for instance:

    let counter = Arc::new(AtomicCell::new(0));
    let handle = ::std::thread::spawn({
        let counter = (*counter).clone(); // <- typo here.
        move || {
            counter.fetch_add(1);
        }
    });
    counter.fetch_add(1);
    let () = handle.join().expect("Thread panicked");
    assert_eq!(counter.load(), 2); // Fails!
    

    In that case, what we wanted, again, was to express a shared/retained ownership of the AtomicCell, and a .retain() method would thus disambiguate that as well:

    ::std::thread::spawn({
        let counter = (*counter).retain(); // <- typo here => Error, no `.retain()` for `AtomicCell`
    

    So since Arc<impl Retain> would be a very rare thing do see (it would be an antipattern of sorts), using .retain() would solve the ambiguity issue almost as well as Arc::clone(), with the added ergonomic benefit of also handling auto-{de,}ref for us :slight_smile:

    And in the pathological / redundant case of something like Arc<NewType>, where NewType : Retain, then the code will not be buggy whether we .retain() at the outer or inner level (except from very niche reference-count-based algorithms I can't think of right now; in that very unlikely case, yes, Arc::… disambiguation would be needed, even despite the Retain trait being around).

How far are these derives going to go? Do you have Retain for Vec<T> where T: Retain? Cloning a Vec<Arc<…>> does create shared ownership on one level and duplicates data and allocation on another level.

1 Like

I think the name “retain” might be suboptimal. I don’t have an alternative suggestion, but “retain” sounds too much like Vec::retain / String::retain / HashMap::retain / HashSet::retain / BTreeMap::retain / BTreeSet::retain / VecDeque::retain / BinaryHeap::retain(nightly).

You’ve mentioned Arc<DashMap>, DashMap has a retain method, too, and it’s even &self.

I know, the number of arguments is different, but it might still be a potential source of confusion.

1 Like

I don't know if I can explain my position very well but I will ramble a bit.

My feeling about Arc::clone has always been that it marks the wrong thing, and is so not really helpful for disambiguation except perhaps in a handful of contrived cases which, taken in the context of all the possible code we might write, don't present a compelling case for marking Arc::clone.

(I'm using "mark" here in the linguistics sense, that the "unmarked" version of a word or phrase is the normal or usual way to say it, and a "marked" version bears some additional meaning or weight; in Rust, foo.clone() is the unmarked way to clone something, and so T::clone(&foo) is marked. In the case of Arc, since the marked and unmarked versions are semantically identical, I don't find a good reason to prefer the marked one. Similarly, we don't use transmute for pointer casts, or use scan where map will do, because the more complicated solution is unnecessary.)

Considering this example:

let my_instance2 = my_instance.clone();
…
my_instance2.clear();
…
stuff(my_instance);

It actually doesn't matter where my_instance2 comes from: even if it's not a clone of my_instance, my_instance2.clear() may clear my_instance if the type is a wrapper around some kind of shared state. So this function has the same problem:

fn whatever(my_instance: NewType, my_instance2: NewType) {
    my_instance2.clear();
    stuff(my_instance);
}

You still can't tell from looking at whatever whether my_instance and my_instance2 might share some state. The code that you have to read to understand what's happening here could be arbitrarily far away and might not even involve clone. So I don't find the argument compelling that Arc::clone may disambiguate some cases; that might be true when the clone happens "close" to the usage site, but in cases where each clone goes its own merry way (which is probably the majority of Arc uses, since shared ownership is rarely necessary inside the scope of a single function) there is no reason to believe that context will be present at the point where the shared state actually becomes relevant.

To play devil's advocate, surely the real problem is autoderef: instead of writing Arc::clone(&my_instance) and my_instance2.clear(), we should write my_instance.clone() and NewType::clear(&my_instance2). Because the second thing is the more surprising one, the one which actually breaks the naive assumption, it should be marked.

Or (continuing to play devil's advocate) perhaps we should go hard the other way. If cloning an Arc deserves marking because it can lead to spooky action at a distance, surely, nearly anything you can do with an Arc (that doesn't autoderef) is equally deserving of a marker. Should we write stuff(my_instance as Arc<_>)? Should we allow my_instance2 to be implicitly dropped at the end of the scope, or do we need to explicitly call mem::drop::<Arc<_>>(my_instance2), so that a reader won't be confused and think that NewType is being dropped?

In short, I don't find a significant difference in readability between var.clone() and Arc::clone(&var) to justify making a blanket recommendation to use the second version. To be honest, I don't think I've ever seen a motivating example from real world code; people usually appeal to our friends foo and bar to illustrate why you should do Arc::clone, which is understandable since real code is often messy, but also makes me a bit uneasy.

One might expect that I would then be opposed to Retain, since I don't think the readability difference justifies the effort, but actually I'm feeling rather ambivalent. I think the major disadvantage of Arc::clone(&foo) is that it's an unusual syntax for a normal thing, and you have to learn to read it without paying attention to the part of your brain that knows it could be written more cleanly as foo.clone(). Whereas foo.retain() is already in simplest syntactic form and changing it to clone is more of a semantic difference, so it's not as bad. Anyway, that's where my brain lands at the moment.

Here are some alternative name ideas. I find "retain" to be a bit obscure, not the worst, but when you take into account the existence of all the other retains it needs to be something different.

  • .alias()
  • .share()
  • .link()
  • .another()
  • .reown() (by analogy to "reborrow")
3 Likes

Good question, I have been myself thinking about the structural case (say a struct with two Arc fields), and although I was leaning towards allowing them, the mutation argument you pointed out is indeed on point. Maybe the derive doesn't make that much sense and it would have to be on a "common sense basis" / let the develop choose whether to implement this (since it's just a marker-like impl). Kind of like CheapClone, finding a strict line appears to be hard.

Yeah, I'm not 100% fond of the name, although it had the advantage to come with the background of other ref-counted abstractions in other languages. But the examples you provided definitely rule that name out. It would have to be renamed retain_ownership to disambiguate, and at that point the name is too long for an expected pervasive method. Although :thinking: what about .retained()?

Oh, I quite like that name! :slightly_smiling_face:

Maybe it's a large fuzzy reasoning, but I'm personally perfectly okay with newtype.clone() having "silent reown" semantics with shared mutability.

When a type doesn't have shared mutability, there's clearly nothing wrong with this; the reference count is solely a fully encapsulated detail. It's then a question of whether you consider interior shared mutability an encapsulation breaking property.

In my experience, though, it comes down to a naming problem. If a type exposes shared interior mutability, that should be clear from the type. Most times, I'll see something like XHandle for a shared mutability handle. If a type doesn't make clear that it represents a handle to some shared resource, that's a failing of the type, not of .clone().

So with that argument, why do I still often use Arc::clone? Because Arc is a Deref smart pointer; it's supposed to act like a reference, where you just use it as if it were just &T with different sharing semantics. Because Arc is a smart pointer, .clone() is human-ambiguous as to whether we wanted an interior clone of the pointee or a clone of the handle. Using a UFCS T::clone clarifies this intent to the reader.

4 Likes

I wrote up my thoughts and they were rather rambling, so I thought it would be better to condense. But after a couple days, nothing really condensed and no other conversation happened, so I'm posting my rambling thoughts anyway.


I think @steffahn really pin-pointed what this is about: safe, clear, and ergonomic ways to use types that implement Deref.

  • All of Rc's inherent "methods" are actually associated functions to avoid shadowing their contained type
  • This is also why Rc::clone(...) is suggested in the first place, for consistency and to avoid confusion where someone may think rc.clone() should clone the contents
  • And additionally why the straightforward way to disambiguate (just put a reown method on Rc) is undesirable
  • So a trait is desired which Rc implements but it's contents probably don't, as this would give Rc an unambiguous method
  • Thus the Reown trait, to distinguish from Clone

You spend awhile talking about the surprise of interior mutability and the unease that induces, but I think that's actually a distinct issue. To see why, consider when should you implement Reown for your own type.

As I interpret it, the considerations are similar to "should I implement Copy" -- you should only implement it if it really is a "cheap" operation, and isn't likely to set off footguns. Which types would meet those requirements? Probably only those which are themselves shared-ownership implementations, or thin wrappers around shared-ownership implementations. These are the types most likely to implement Deref to yield their contents. (And if you don't implement Deref, you can just add an inherent method to give the unambiguous option... though this doesn't help in generic situations.)

This means that types which contain shared-mutability types (shared-ownership or otherwise), but also contain other data and implement Clone, shouldn't implement Reown. And thus they will still have the semantic ambiguity you talk about. I believe you're mostly in agreement here, as you said "every field is Retain". (I think that's actually a bit too strong; maybe I need to carry around a handle to my interner or something.)

All of that was a long winded way of saying: I think "may contain shared-mutability" and "is a shared-ownership type (and not much else)" are distinct concerns. The former is more like the "no UnsafeCell trait" or some sort of effect system that is occasionally advocated for, in my mind.

So really we're talking about "I want shared-ownership to be a trait".

The "higher-ranked bound that contains Rc and Arc" idea comes up occasionally. Perhaps this is a generalization of that. I'm not sure if bundling them together would work well, or if Reown would better be a supertrait of the family, say.


Back to Deref and avoiding ambiguity. In the general case, not just with Clone, if you want to avoid ambiguity between the wrapper and the contents, you want the wrapper's clarifying traits to be as special-purposed as possible. To this end, it would be nice to lock it down somewhat from the start, e.g.

// This includes all shared references (and mutable ones are `!Clone`)
impl<T: Copy> !Reown for T;

I call out references specifically as they can be confusing with Clone. (I've seen more realistic cases when hitting a &&T in an iterator chain, say.)

Speaking of locking things down...

/// Prevent the `.retain()` implementation from being overridden,
/// giving it "marker" trait semantics.

I hope that (a) we get partial impl and not default impl, and (b) function bodies in partial impl can be final (or not), but as I'm sure you're aware, that's not the implementation today (partial implementations are always overridable).
:crossed_fingers:


Thought experiment that doesn't go much of anywhere

On the original issue, I considered: what if you could disavow the ability of a type to fulfill a trait within a given scope? Something like:

fn sloppy_cloner<T: Clone>(t: T)
where
    // Forget that &T can `Clone` in here
    // Which means &&T can't `Clone` either, etc
    for<'t> &'t T: ?Clone,
{
    let x = &t;
    let y = &&t;
    let z /* : T */ = y.clone();
}

On the surface it seems like it might help with the "accidentally cloning a reference" case. But in reality, it probably would be more harmful than helpful, as (logically) a reference would no longer be Copy either.

And I guess it's not really great for this case...

fn main() where AArc /* = Arc<dyn A + Send + Sync> */ : ?Clone {
    let mut c = Coll::new();
    let s = Arc::new(S::new(0));
    c.add(Arc::clone(&s));
    // That all worked as we hoped, but now you can't clone
    // the `AArc` in `c` if you want to, which you likely do?
}

Or especially here:

fn main()
where
    ???
// `new` isn't a trait method we can "block" with `?Trait`
// And we need `Pin<&mut i32>` to unsize-coerce
// And for that we need `&mut i32` to unsize-coerce
// So there's nothing we can "block" to guide inference
{
    let mut a = 1;
    let _x = Foo(Pin::new(&mut a)); // ~Error
}

So it's probably a dead end, though maybe there's something salvageable if the "un-bound" were in some way shallow. Even then, probably not worth it. And on the other hand, the "don't clone my reference" case requires some sort of non-shallowness.

Probably diagnostics to suggest as _ is all that's really needed.

1 Like

I agree about the ambiguity of semantics of Clone. There are two orthogonal questions:

  • Is it cheap?
  • Is it shared?

The sharing aspect affects behavior of the program, and in a subtle way that could lead to difficult bugs and data corruption when developer misunderstands what is shared and what isn't.

Types with inner Arc are especially confusing. If a type represents a resource, does cloning it create a new instance, or a new handle to the same instance?

And cheap clones don't always imply something is shared. True copy-on-write types can have cheap clones and still semantically behave like separate instances by lazily cloning only on modification (Rust's Cow isn't it, because it does expensive clones. Arc::make_mut and im are closer, but a true COW behavior is common in dynamic languages).

2 Likes

Could you explain what differentiates Arc<T> (T without interior mutability!) from such cow types “in dynamic languages”?

For example PHP arrays have a true COW behavior. They behave like passed by value all the time (Rust's Copy types), and the foreach statement avoids iterator invalidation problem by iterating a copy of the array. However, all copies are implemented via refcount increments (as fast as Arc). Only array mutation behaves like Arc::make_mut and creates a copy if refcount > 1.

I'm usually a huge fan of being explicit, and writing clear code. However, cloning refcounted smart pointers is not something I have strong feelings about. I understand, appreciate, and somewhat agree with the clarity argument of Rc::clone(), but I have yet to encounter a situation in which not doing that and just .clone() ing the pointer leads to an error. In my experience, this kind of mistake is trivially caught by the type system.

To sum up: if your project's coding guidelines mandate using the UFCS version, I'm happy to. In my own code, I may or may not use it, depending on how many fatiguing hours of DNS issues I had to debug during the day.

4 Likes

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.