Provide a more explicit way to "clone" a standard reference counted pointer.

[nical]

This is my first RFC.

This RFC proposes a very small addition to the standard reference counted pointer types, that aims solving a recurrent paper-cut-type of annoyance related the lack of clarity of code calling clone().

Rendered.

[SimonSapin]

I’ve sometimes used Arc::clone(&some_arc) instead of some_arc.clone() for this reason. Type::trait_method works as another shorthand for <Type as Trait>::trait_method when Trait is in scope.

[burdges]

Isn't there a case for putting new_ref into a trait too, perhaps along with Weak, so as to be generic-ish over Send/Sync? I once considered using something like this to ensure my Arcs did not really need Send/Sync in my own code :

trait RcClone<T> : Deref<Target=T> + Borrow<T> + AsRef<T> + Clone {
    type Weak;
    fn downgrade(&self) -> Weak;
}

impl<T> RcClone<T> for rc::Rc<T> { 
    type Weak = rc::Weak<T>;
    fn downgrade(&self) -> rc::Weak<T> { rc::Rc::downgrade(self) }
}
impl<T> RcClone<T> for sync::Arc<T> { 
    type Weak = sync::Weak<T>;
    fn downgrade(&self) -> sync::Weak<T> { rc::Arc::downgrade(self) }
}

trait RcWeak<T> : Clone {
    type Strong;
    fn upgrade(&self) -> Option<Strong>
}

impl<T> RcWeak<T> for rc::Weak<T> {
    type Strong = rc::Rc<T>;
    fn upgrade(&self) -> Option<rc::Rc<T>> { rc::Weak::upgrade(self) }
}
impl<T> RcWeak<T> for snyc::Weak<T> {
    type Strong = sync::Arc<T>;
    fn upgrade(&self) -> Option<sync::Arc<T>> { sync::Weak::upgrade(self) }
}

[kennytm]

Weal ↦ Weak.

[kennytm]

Better define it as

fn new_ref(this: &Self) -> Self;

to let user call it as

let p1 = Rc::new(vec![0u8; 1000]);
let p2 = Rc::new_ref(&p1);   // <-- instead of p1.new_ref()

as explained in the doc of Rc:

The inherent methods of Rc are all associated functions, which means that you have to call them as e.g. Rc::get_mut(&value) instead of value.get_mut(). This avoids conflicts with methods of the inner type T.

(The .clone() method on Rc is precisely causing such a conflict.)

[nical]

Thanks a lot for the feedback so far. I had not though about it before, but the concern about new_ref conflicting with the pointee type potentially having a new_ref method of its own is pretty serious and quite obvious in retrospect.

I added it to the RFC under Drawbacks and Alternative, although depending on how the discussion goes it might become a reformulation of the main proposal. It would be great, though, if we could find a solution that is at least almost as convenient as clone, otherwise I fear that what will/may come out of this RFC won't be used in practice, which would defeat part of the purpose.

[hadronized]

I’m balanced. I think the distinction is important. I saw the example in some gfx codebase lately in which
Buffer<_> – defined as a wrapper type over Arc<RawBuffer<_>> – implements Clone.

impl<T: _> Clone for Buffer<T> { _ }

What do we expect a buf.clone() to do? I’d say it actually creates a new buffer holding the exact same data as the source, but living in a different place in memory. Well, that’s not the case. It just clones the inner arc. And that semantic is plain wrong.

On the other side, I think that Arc should have an implementation of Clone creating a new T instead. But you suggest is to default the implementation to new_ref. Which wouln’t solve the buffer problem I mentionned above.

It’s all about confusion. In a perfect world, I’d rather prefer not having an impl Clone for ref counted pointers, and have a dedicated method for that.

[oli-obk]

I'm 100% behind the motivation for this RFC, but as @SimonSapin stated, we already have an explicit version (Arc::clone(&some_arc)). I'd prefer if the compiler simply told me that some_arc.clone() is easy to misinterpret and Arc::clone(&some_arc) should be used instead. Writing a lint that does this is pretty easy.

If we don't add such a lint, the new_ref method doesn't add any benefit to existing code and to new code written without knowledge of that method.

The "how do we teach this" section is insufficient imo, since the change of adding new_ref goes by pretty silently except for documentation changes. If we consider some_arc.clone() to be unidiomatic, we should deprecate or lint it.

[nical]

Very good point about the lint. Is this something that should go into the compiler, or is clippy the place for this type of thing?
I would be surprised that we have the luxury to completely deprecate Rc::clone and friends at this point (as in, move towards making it not compile), considering that most likely all of the code bases that use standard reference counted pointers in rust clone them at some point (otherwise why use reference counting).

[oli-obk]

Well... in clippy we can insert this without an RFC. For the compiler, this RFC would need to continue.

Deprecation is not breaking the code. Deprecation means you start getting warnings when you use it. So basically the same as adding a lint.

[sfackler]

Would this also deprecate deriving Clone for any type containing a reference counted pointer?

[oli-obk]

Would this also deprecate deriving Clone for any type containing a reference counted pointer?

We're not deprecating the Clone impl of the reference counted pointers. Only the method call syntax for clone on these pointers.

[sfackler]

Is the derived impl going to suppress that deprecation warning then?

[eddyb]

@sfackler Those use $crate::clone::Clone::clone to disambiguate, not method call syntax.

[Kixunil]

This sounds like a good idea. Maybe add clone_inner() for T: Clone too? It might be more readable than (*rc).clone().

[jfager]

Without weighing in on the specific solution, I just want to voice support for this being a problem that needs solving. I've always hated calling clone on pointer types, though seeing this RFC is the first articulation of the reason for that.

Minor noncommittal bike shed: share?

[plumenator]

Typo: repsective -> respective

[eddyb]

Ugh, sorry, mobile UI has no confirmation on acidental press.

[scottmcm]

I think "use Arc::clone" (from @SimonSapin) deserves to be explicitly noted as an alternative in the RFC text. All the clarity and linting points could be done immediately with that, no need for a stabilization period. And at least the two data.clone() calls in the webrender example file linked from the RFC don't call methods after the .clone(), so don't need method call syntax for ergonomics.

I think whether new_ref calls clone or clone calls new_ref is irrelevant, as it's invisible "when reading or reviewing code".

Bikeshed: I'm not a fan of "new" in the name, since that has a somewhat different connotation in its usual use. What about just clone_ref? I also like @jfager's share.

Also, if the name is going to include ref, it should probably work for &. Of course that's somewhat silly non-generically, so implies a trait.

Is this always going to be on something where clone is shallow anyway? Clone being shallow for Arc is certainly more fundamental, since a deep clone is Arc::new(p.deref().clone()). And "deep clone" on a & can't return a &, so is called to_owned. Weak is even weirder, since returning a Weak is possible but fundamentally useless, so it's good that nothing "deep clone"-like is provided there.

Overall, that could turn the proposal to something like this:

trait CloneRef : Clone {
    fn clone_ref(&self) -> Self { self.clone() }
}

impl<'a, T: ?Sized> CloneRef for &'a T {}
impl<T: ?Sized> CloneRef for std::rc::Rc<T> {}
impl<T: ?Sized> CloneRef for std::rc::Weak<T> {}
impl<T: ?Sized> CloneRef for std::sync::Arc<T> {}
impl<T: ?Sized> CloneRef for std::sync::Weak<T> {}

Another alternative: Put these methods (and possibly trait) in a crate, since there are no coherence issues. See how broadly the crate is used before putting into std.

On the lint: warning on Arc::clone feels quite aggressive for rustc. I think it should live in clippy.

[nical]

I added the suggestion made by @SimonSapin to the alternatives section as well as something equivalent to the CloneRef trait suggested by @scottmcm.

[aturon]

The libs team discussed this RFC a bit in triage yesterday, and the general consensus was that the Arc::clone approach would be preferable to us, as a more minimal shift. There was also agreement with @scottmcm's point that linting here should probably go through clippy, as it is indeed quite aggressive.

[nical]

I take it that the decision is made to go for Arc::clone(&foo).

So, taking this approach, what is the way forward from here? Is there a separate RFC process for clippy? Should I re-write this RFC to formalize that Arc::clone is the idiomatic way to clone references (is it actually part of the consensus)? What about updating the documentation? (sorry, lots of questions, I am new to these things).

I'm about to go offline for two weeks but I'll be happy to help with these when I come back.

I don't think that this approach will solve the problem (as in, the vast majority of code will still be written foo.clone() and most people won't know about the possibility to do otherwise. But I understand the decision to minimize change and it's fine because I suppose we can easily come back in a year and revisit the question if it turns out the problem still exists.

[kennytm]

@nical You just need to file an issue to https://github.com/Manishearth/rust-clippy/issues. Typical reports seem to be pretty short (e.g. https://github.com/Manishearth/rust-clippy/issues/1594).

[nical]

Thanks, I filed a clippy issue. It would be good to clarify what the official position is with respect to the idiomatic way to clone a reference counted pointer before we close this pull request.

[Kixunil]

What is considered as idiomatic way of cloning inner value then? Is it just (*my_rc).clone() or something else?

[oli-obk]

What is considered as idiomatic way of cloning inner value then? Is it just (*my_rc).clone() or something else?

Technically you could do T::clone(&*my_rc) to be very sure about what's going on.

[SimonSapin]

you could do T::clone(&*my_rc)

Or T::clone(&my_rc), with implicit autoderef.

[sfackler]

I have always used the T::foo syntax when I want to be explicit about what type I want the method called on.

[aturon]

Yep, T::clone is idiomatic.

@nical, do you want to close?

[nical]

Alright! I'll look into updating the docs as a follow up soon-ish.