Rc::clone(&r) or r.clone()?

libs
1

We've been told that we should always write Rc::clone(&r) and not r.clone().

In the book we read:

By using Rc::clone for reference counting, we can visually distinguish between the deep-copy kinds of clones and the kinds of clones that increase the reference count. When looking for performance problems in the code, we only need to consider the deep-copy clones and can disregard calls to Rc::clone

https://doc.rust-lang.org/book/ch15-04-rc.html#using-rct-to-share-data

On the other hand, the document of the Clone trait says:

[Clone] differs from Copy in that Copy is implicit and an inexpensive bit-wise copy, while Clone is always explicit and may or may not be expensive...

...but you may reimplement Clone and run arbitrary code.

And to some extent it clarifies that clone is not necessarily a deep copy. However, currently in the book in several places clone is used like a deep copy:

https://doc.rust-lang.org/book/ch04-01-what-is-ownership.html#variables-and-data-interacting-with-clone

As we know, when we deep copy object a and create object b, that means we'll get two independent objects a and b. There should be no way that manipulating a's state affects b's state and vice versa.

When we clone an Rc, as long as the data to which it is pointing is immutable, we'll get an independent copy. However, if we make that data mutable, the clone created by Rc::clone will not be an independent copy. If a program assumes that an object which implements the Clone trait makes independent copies with its clone function, an Rc which contains a RefCell can become problematic. (like this example or this example)

Is it really necessary and is it a good idea to recommend calling clone like an associated function for shared smart pointers? (i.e. like Rc::clone(&r) and not r.clone()) This is not just about Rc. Any struct that contains a shared smart pointer will be subject to this recommendation. In my opinion this is just adding unnecessary complexity to the rust development. Now a programmer can be a "bad rust developer" just because he's used r.clone() in his code. This type of recommendations are always troublesome.

For this reason, I suggest that one of these solutions be chosen:

  1. We can introduce a new trait for deep copy and then deprecate all current implementations of the Clone trait that are doing a deep copy. (the new trait will replace them)
  2. We can introduce a trait for costume copy semantics or even without introducing a new trait, we deprecate all implementations of the Clone trait that are performing some type of costume/shallow copy. For example Rc::clone should be deprecated and get replaced with a function like Rc::share

This will reduce the probability of misunderstandings.

2

It must not be a very strong convention, because I use r.clone() in my code all the time without being aware of this rule, and cargo clippy didn't tell me not to. I don't see a lint for this in the clippy lints at all, not even a "pedantic" (allowed by default) one. (Should there be, I wonder?)

If you only need one level of duplication: Rc::new((*r).clone()).

Rc::make_mut may also be relevant, depending on what you're doing.

A true "deep clone" trait doesn't necessarily seem as valuable for general-purpose programming, since there are a lot of different possible choices about how deeply you want to clone, how to handle cycles, etc.

3 Likes
3

I think the book's advice is good. It's useful to have visual distinction in the code.

Arc/Rc implementing Clone is still useful, because there are generic methods that work on all cloneable types. If Rc::clone was something else, it wouldn't compose nicely with other cloneable types.

5 Likes
4

Here: https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_ref_ptr

Seems like it used to be on by default but has (had?) a difficult to detect edge case that caused it to be moved to restriction: Fix #2048: Move `clone_on_ref_ptr` to the restriction lints by alusch · Pull Request #2355 · rust-lang/rust-clippy · GitHub

I think that it's a good practise but it's definitely unfortunate that clippy has trouble enforcing it reliably, especially when the book calls it out as "Rust’s convention".

5

Oh, I see! I missed it when searching the list, thanks for pointing it out!

6

let's say that I have a library that assumes it receives a cloneble object. If I I give it an Rc which contains a RefCell, that will not break anything probably?

They could have implemented something like share for Rc. If the programmer needed to use it like a cloneable he could just define a wrapper type and implement the clone trait. Anyways, the visual distinction will not be important if we state clearly that clone does not necessarily deepCopy and for deep copy another trait should be used. Moreover, since the copy trait can not be implemented we need a trait to implement customized copy logic. In the current circumstances the Clone trait looks like a good choice.

This type of recommendations are really troublesome. They are not easy to follow. Many programmers may ignore them. It is not good if a language needs a strong linter to tell you what to do and what not to do and you can not write the appropriate code intuitively.

7

It'll be safe, since an Rc is a different type than an Rc. Is there a chance you may make the wrong assumptions about what you're cloning? Yeah.

Rust made some choices about trading clarity for ergonomics when it comes to references/pointers/smart pointers and how the deref trait works.

I love the ergonomics, but the loss of clarity can be a problem. This is one such case. Which clone is being called!? You just have to know.

This recommendation simply aims to return some of that clarity, but results may vary. At the end of the day, recommendations are just that. Nobody is enforcing them and if they're not helpful, any project can disregard them. If they really are helpful, they tend to find their way back in regardless.

8

Sorry, I'm not sure I understand. Could you explain that?

9

I was surprised to read this, because I thought the documentation already said so – but looking back at the documentation for Clone, it actually doesn't make any comments on how deep the copy is. I must have just assumed, because I learned about Rc being cloneable early on when I read the Rust Book.

Worse, the Rust book explicitly says (emphasis mine):

The Clone trait allows you to explicitly create a deep copy of a value, and the duplication process might involve running arbitrary code and copying heap data. See the “Ways Variables and Data Interact: Clone” section in Chapter 4 for more information on Clone.

This is misleading, and should be fixed! The precise thing to say is that deriving Clone calls the clone method on all fields, but the Clone trait is otherwise not opinionated on whether the clone will be by value, shared pointer, etc.

4 Likes
10

yeah, it kinda says that, but not very strongly. I've quoted that part, it says the clone's code could be arbitrary, but it doesn't say clearly that it may not be a deep and independent copy.

11

Emphasis mine:

The Clone trait allows you to explicitly create a deep copy of a value, and the duplication process might involve running arbitrary code and copying heap data.

That being said, I agree it's not too clear. On the very next line, the book does say

Deriving Clone implements the clone method, which when implemented for the whole type, calls clone on each of the parts of the type. This means all the fields or values in the type must also implement Clone to derive Clone .

Which is pretty explicit.

Even so, I agree this could be better/more clearly worded!

12

ooph, my bad. I think the markup swallowed some of the text.

Rc<T> and Rc<R> are different types and their safety properties can be different depending on whether they are sized, send, sync, etc.

2 Likes
13

Well... We were talking about functions that accept something like item: &impl Clone.

14

Consider the following situation, where the type of T carried forward in some manner (Which, if it has only Clone as a trait bound, there's not much else that can happen).

<T: Clone>(item: T) -> SomeType<T>

It depends on what you mean by break, I suppose. What I meant with my answer is that Rust's safety properties should be fine under those conditions. Though you may confuse some business logic. Realistically though, T: Clone isn't very useful on it's own. When that generic gets carried forward, you're likely to be writing code that works on T's type more concretely. At which point the Rc<InnerType> resurfaces.

1 Like
15

This is really simple. When we deep copy object a and create object b, that means we'll get two independent objects a and b.(arguably some people say clone should always do that) There should be no way that a's state can be affected by b's state.

When you clone an Rc, as long as the data it is pointing to is immutable, you'll get an independent copy. But if you make that data mutable, the clone created by Rc::clone will not be an independent copy anymore. So if a program assumes that clone is making independent copies will probably break.

That's why that I'm suggesting that a new trait DeepCopy be introduced to represent the concept of independent copies, and the Clone trait officially becomes a trait for implementing costume and arbitrary copy semantics. In this way there will be no need for writing Rc::clone either.

16

I had not written that part very well. Of course I was not talking about "possibilities". We were talking about how an API looks and what it offers.

17

I have hard time imagining a practical case where you would have code that is both so generic that it only uses the Clone bound that allows many types including unwanted ones like Rc<RefCell>, but also manages to depend on deep copying and independence of the types so much that it breaks.

3 Likes
18

Do you have any examples in mind? I can come up with contrived examples wherein I do something like:

trait Behaviour {
    fn mutate_self(&mut self);
}

impl<T> Behaviour for Rc<RefCell<T>>
where
    T: Behaviour,
{
    fn mutate_self(&mut self) {
        self.borrow_mut().mutate_self()
    }
}

So that now I can use Behaviour as a trait bound and hide whether we have a plain T or Rc<RefCell<T>>. Due to the orphan rule, you can't really do this for a trait not in your module and a library that assumes clone has a specific cloning semantics would not create such a blanket impl for traits they rely on.

I just don't see this happening in practice.

I think there's a bigger issue here though:

There should be no way that a 's state can be affected by b 's state.

This is true of the interior mutability pattern. Pragmatically it doesn't feel that way, but that's what the indirection accomplishes. The problem isn't the state of a or b, it's that I can use this unchanging state to mutate something else.

Consider this simplification:

fn main() {
    let mut alphabet = vec!['a', 'b', 'c', 'd', 'e'];
    let c = 2usize;
    let copy_c = c.clone();

    print!("{} ", alphabet[copy_c]);
    alphabet[c] = 'z';
    print!("{}", alphabet[copy_c]);
}

This prints c z because c could be used to mutate the result of later using copy_c. Despite the two being independent objects.

Here's the quandary, the only reason that c.clone() is not a deep clone is because it is used to index an array. Really, you'd be forgiven for saying that either this is the deep clone of a usize or that usize (or to generalize, this is true for any type under the right conditions) can never be deep cloned. The issue is that data of all sorts end up acting suspiciously like pointers and pointers so often end up acting suspiciously like data. Match statements on enums make variants feel like virtual functions.

Any example of a deep clone can be trivialized to mutate some other state. RefCell is a nice abstraction around that pattern/problem.

Generally, when somebody talks about a deep clone, what they mean is specific to their use case. I may want everything in memory copied (follow all pointers and datatypes masquerading as pointers), but not go so far as to copy entire servers/databases just because I own a handle to them. On the other hand, maybe cloning an entire database is exactly what I mean by deep cloning.

I think Cloning is a complex enough topic that a one-size-fits-all solution isn't possible. It's also common enough that you want at least some std coverage. It makes sense to err on the side of conservative cloning. You can then create your own traits for your use cases. This might be preferable over championing a new trait in the std library.

Just my 2 cents of'c

:slight_smile:

19

You don't really have to go into Rc and interior mutability land to run into "shared state". &T is Clone and Copy for any T: ?Sized (as are raw pointers).

4 Likes
20

This is really simple. The programmer has an assumption which is not correct and that will cause trouble:

use std::cell::RefCell;
use std::rc::Rc;

impl Mutable for Rc<RefCell<i32>> {
    fn mutate(&mut self) {
        *self.borrow_mut() += 1;
    }
}

trait Mutable {
    fn mutate(&mut self);
}

fn doesnt_change_input(input: &(impl Clone + Mutable)) {
    let mut my_own_copy = input.clone();
    my_own_copy.mutate();
    // I did not change the input, I only changed my own copy!
}

fn main() {
    let r = Rc::new(RefCell::new(6));
    doesnt_change_input(&r);
    println!("r should be 6 but is {:?}", r);
}

(Playground)

Output:

r should be 6 but is RefCell { value: 7 }