Rc/Arc needs a better name than clone

[prasannavl]

Seeing clone all over the codebases makes me cringe with no way to know if it's an expensive one or if it's just a reference increment. One has to dig back all the way to type to understand what it actually means.

Would it not make more sense to have a method like ref or similar that can be used instead of clone ? So, that's its explicit and clear. The ref method could just as well call clone internally. Or perhaps, new_ref. Or a name that makes more sense. Just not clone, (though that has become accepted today in the ecosystem) but the actual semantics are rather different.

Edit: Make it clear that I'm not suggesting that clone be renamed - That's neither my intent nor a solution to the issue. Just looking for potential way to mitigate this.

[Diggsey]

Clone is a trait method, not an inherent method on Rc/Arc, so you can't rename it on Rc/Arc without also renaming it on every other type. It doesn't makes sense to have separate traits because then you can't abstract over anything that can be cloned. Clone as it stands is already a "shallow clone", as much as it can be.

[stefano-pogliani]

I believe the solution to this issue is Arc::clone(&from).
From the standard library documentation:

The Arc::clone(&from) syntax is the most idiomatic because it conveys more explicitly the meaning of the code. In the example above, this syntax makes it easier to see that this code is creating a new reference rather than copying the whole content of foo.

See https://doc.rust-lang.org/std/sync/struct.Arc.html#cloning-references

Would converting all uses of item.clone() to Arc::clone(&item) help?
Note this can be done incrementally if the codebase is too large for a one off replace task as the two calls have the same effect and only different "appearance".

[prasannavl]

@Diggsey, No, not a direct rename - I'm just trying to see if there's a better solution to this. However, separate traits may not be the worst idea though - You could have one (say, RefCount for the sake of the argument that has method ref) that's implemented only by Arc/Rc and types that's supposed to have similar semantics, that just forwards it to Clone. So, remains unchanged as it is.

Or perhaps a marker type, that the compiler simply forwards to Clone even. I think it can solved in a nice way if it's acknowledged by the community as a problem.

PS: This is not too different from Borrow which exists mostly for semantic clarity as well.

@stefano-pogliani, I think that would would be a good solution. If this is indeed the best solution, perhaps adding to clippy, and/or compiler warnings would help mitigate this.

[Nemo157]

A clippy lint for this already exists, it's just not on by default: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#clone_on_ref_ptr

[mehcode]

I strongly support the idea of a new trait. Clone screams at me expensive. Copy is "free". But a ref increase with Rc is neither "free" nor expensive.

With just a Arc<T> you can (and are pushed to with clippy) use Arc::clone which is good but what about types that themselves contain Arc/Rcs?

[nical]

See also #1954

[Diggsey]

what about types that themselves contain Arc/Rcs?

What about types that themselves contain Arc/Rcs?

The Clone trait currently means "shallow clone". When you clone an Arc it won't clone the thing inside the Arc so whether that type contains further Arcs is irrelevant.

There are two possible things that could be added:

• A new "lightweight clone" trait (let's call it FastClone: Clone for lack of a better name). This would be implemented for Rc, Arc, and... what else? Maybe Copy types too? You could conceivably have it represent things clonable without a heap allocation.

• A new "heavyweight clone" trait (let's call it DeepClone: Clone). This would behave the same as Clone except where Rc and Arc are concerned, where it would instead recurse into them and clone their interiors.

The first trait has very limited usefulness: it's a pretty vague description, and even if you go with "heap allocation" as your distinguishing factor, heap allocation is often pretty fast, and in some cases may be faster than cloning an Arc.

The second trait is potentially more useful, but can easily be implemented in a library.

[burdges]

We should add a clippy lint warning against cloning HashMap/Set instead suggesting inherent method that does

hm.iter().map(|(k,v)| (k.clone(),v.clone())).collect::<HashMap<L,V>>()

because there is a DoS vector arising from the fact that HashMaps necessarily keep the same secret. We discussed routes to fixing this problem outright, but they all appeared problematic.

I believe HashMap: Clone represents a more fundamental problem than Arc: Clone, but lies far outside the spectrum being discussed here because rehashing costs far more than reallocation and itself represents an attack vector. If you actually encountered the lint on HashMap: Clone then you'd make a security argument either for or against the slow clone, or else you'd swap out HashMap for some transactional or layered data structure.

I suspect lints might be the correct option here too: If deep clones are usually the correct behavior for MyType due to Arcs, then put some lint against MyType: Clone that recommend an inherent method with exactly the correct semantics.

[scottmcm]

Another possibility would be to allow foo.Arc::clone() or bar.Rc::clone() to make it clearer what's being cloned, using something like Idea: Paths in method names.

[burdges]

As an aside, I suspect Arc: Clone and Rc: Clone already provide the correct behavior almost anytime they're used without additional "real" interior mutability, ala Mutex, RefCell, etc. Arc has no into_inner so this only make_mut or get_mut can run into problems in this case. We're mostly discussing the usual case where Arc wraps "real" interior mutability here.

[phlopsi]

IMHO, Arc/Rc implementing Clone was a mistake. When looking at how Arc does equality, it's easy to see how it could've been done better, instead. Arc::ptr_eq is a thing. Consistency suggests Arc::ptr_clone would've been the obvious choice. Then, Arc::clone would refer to the interior type's clone method or end up as a compile error, if T doesn't implement Clone. Since Arc implements shared ownership, Arc::share could've been a fitting name, as well. Other names coming to my mind are Arc::duplicate or Arc::dup for short. Sadly, I think we're stuck with the status quo forever unless unimplementing traits for specified editions becomes possible.

[Diggsey]

@phlopsi the Arc/Rc Clone implementation is used by every single crate that use Arc/Rc: the type is useless without the Clone implementation.

It would become impossible to #[derive(Clone)] on types containing an Arc/Rc. The entire idea is a non-starter even if backwards compatibility were not a concern.

[phlopsi]

@phlopsi […]

It would become impossible to #[derive(Clone)] on types containing an Arc/Rc. […]

You can implement Clone manually. I'd even go as far and say, that not being able to derive Clone on a type containing Arc is an advantage, because only you can decide whether it's correct to clone the pointer or the interior value and make a new Arc. In my eyes, this prevents a logic bug caused by a combination of an ambiguous Clone implementation and laziness.

[Lokathor]

Regardless of the weight of that argument, it's a massive breaking change to Rust and so it won't be done for that reason alone.

[burdges]

I'm confident Rc/Arc: Clone being shallow represents the correct decision. As a fun example, closures auto derive Clone, making Arc/Rc usable in Clone + Fn*, an ergonomics benefit for closures.

Also Rc/Arcs internals should never be cloned except for mutation: If T avoids interior mutability, then Arc<T>: Clone should always be shallow for performance. If you want to mutate T directly, then you need let mut fresh: T = Arc::deref(foo).clone(); or maybe let mut fresh: T = MutexGuard::deref( foo.lock() ? ).clone() if using Arc<Mutex<T>>, not some mess with Arc::into_inner.

Afaik, we've no clear reason for a clone_inner convenience method on MutexGuard, but maybe some pedagogical reason exists, or maybe MutexGuard::deref( .. ).clone() should appear in some example code somewhere.

As an aside, I do still think Rust would benefit from lints against some borderline impls, but for HashMap: Clone and some user traits, not for Rc/Arc. That's a clippy topic however.

[shepmaster]

If you want to mutate T directly

IMO it’s clearer to say

InnerType::clone(&some_arc)