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

libs
41

You don't "have to", because the Rc::clone thing is just an extra bit of bonus clarity, which just happens to be easy to deliver because many users recognize the name Rc. If the convention was motivated by a serious bug-prevention need, then it would to be much more thorough, yes. But it isn't, so it doesn't.

This is also reflected by how highly the Rust devs have prioritized "enforcing" the convention (i.e. not very highly – even if it wasn't for the implementation difficulties in clippy, being a clippy lint is significantly lower importance than being in rustc by default.)

1 Like
42

By independent copy, I'm going to assume you mean "no (mutable) shared state possible".

Rust does not support independent copies [1]. You can't assume you can ever get an independent copy in generic context.

(There may be some exceptions with heavy gymnastics, but they're not something you'll see in day-to-day programming and won't really be what you're talking about here.)

Getting an independent copy is not the contract of Clone.

The entire point of having an Rc is shared ownership. Not getting a deep clone is not dangerous, it's the point of the data type.

The recommendation is because you can't prevent trait methods that take &self from being called on the implementing type method-call style, so Rc can't do what it normally does and have fn clone(this: &Self) here to emphasize you're not calling clone on the enclosed object.

I personally use rc.clone() unless there's something more confusing going on that requires type annotations to make clear. If you don't know what cloning an Rc does, I think you have larger problems.

If by correct/safe cloning, you mean "guaranteed to generate independent copies", that's what the "magical deep clone" you dismissed explored. Maybe you could get something less complicated if you limited the types severely enough -- you seem fine not implementing it for RefCell for example. However if by "must not be misused", you want an actual guarantee, then it still must be an unsafe trait.

One could explore the idea of a good-faith trait (not unsafe so no guarantee). But needing a deep clone in the context of an Rc is an exception, not the rule.

If the developer doesn't understand Rc, they're going to misinterpret a lot of things, sure. There's presumably a reason Rc is being used and if they don't understand it and especially if they start to try to circumvent it, a lot of things could go wrong. They should learn about Rc and why it's being used.

Maybe it's just being used for performance... but if there's interior mutability within, it's probably being used because shared state is desired. In that case, the developer is going to have to learn about interior mutability.

The "independent copy" misconception is not an Rc specific thing though:

fn foo<T: Mutable>(mut thing: T) {
    // I received ownership so no one can observe my changes right?
    thing.mutate();
}

fn bar(mimut: &MyInteriorMutabilityUsingType) {
   // Wrong.
   foo(mimut);
}

In a generic context, you can't prevent the possibility of someone handing you a type containing, or indirectly using, interior mutability.

Rust doesn't support giving you a way to prevent that.

  1. outside of knowing all the concrete types... and how they are used ↩︎

5 Likes
43

A DeepClone trait wouldn't help here. Even if the trait existed, a beginner who doesn't know anything about Rc probably wouldn't be familiar with a new niche trait either. And even if they were, Clone and DeepClone would have identical implementations in the vast majority of cases, so without knowing anything about Rc it would be fair to assume that its .clone() is the same as its .deep_clone(), just like with Vec, String and all the other types they're familiar with.

3 Likes
44

I think it's fair to say that the fully qualified form being the only idiomatic way is no longer the case. [1] In fact, that was agreed to by FCP.

The history is somewhat interesting if you're into that kind of thing.

I saw these:

And then looking up where the doc changes came from,

That points the origin of the recommendation, clippy lint, and book changes

And the motivation was to emphasize that it was a shallow and inexpensive operation.

With that in mind, here's what I would consider reasonable changes (modulo my ability to word good) in the linked book sections and some std documentation:

  • In the last paragraph here, don't imply it's not the only idiomatic way, and ditch the "deep-copy" talk.
     We could have called `a.clone()` rather than `Rc::clone(&a)`,
-but Rust’s convention is to use `Rc::clone` in this case.
+and doing so is one convention.  However,  we're following a
+convention that uses the fully qualified form to emphasize the
+low performance cost and shallow nature of cloning the `Rc`.
-The implementation of `Rc::clone` doesn’t make a deep copy of
-all the data like most types’ implementations of `clone` do.
 The call to `Rc::clone` only increments the reference count,
 which doesn’t take much time.
-Deep copies of data can take a lot of time.
 By using `Rc::clone` for reference counting, we can visually
-distinguish between the deep-copy kinds of clones and
+distinguish between potentially expensive 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`.
  • In the last paragraph here, seems like a good place to note the lack of DeepClone.
     When you see a call to clone, you know that some arbitrary
 code is being executed and that code may be expensive. It’s a
 visual indicator that something different is going on.
+Note that while it is common for clone to give a deep copy of
+the owned data, this is not always the case, as we will explore
+in chapter 15.  There is no trait in the standard library for
+making a recursively deep copy.
  • And also here.
     The `Clone` trait allows you to explicitly
-create a deep copy of a value,
+duplicate 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`.

    +// And right before the talk about `Copy`...
+
+Note that while a typical implementation of `Clone` may perform
+a deep copy, `Clone` is intended to logically duplicate values
+and does not *guarantee* a deep copy.  As one example,
+references themselves implement `Clone`.  As another example,
+shared ownership smart pointers such as `Rc` and `Arc` only
+update their reference counts and create new smart pointers to
+the same shared data.
+
+Derived and other recursive implementations of `Clone` also
+typically respect the `Clone` implementations of their contained
+types.  A clone of a `Vec<Rc<Widget>>` will newly allocated `Vec`,
+for example, but the `Rc<Widget>`s in the new `Vec` will be smart
+pointers to the same set of shared `Widget`s.
+
+Rust has no trait exclusively for deep copies.
  • Remove the "idiomatic" language here.
  • Add some language about the lack of deep copies here.
    +Note that `Clone` does not necessarily provide a "deep copy"
+of a type's data.  `Rc` and `Arc`, for example, increment
+reference counters and return a shallow copy of themselves
+in order to provided shared ownership.  Rust has no trait
+exclusively for deep copies.

  1. I'm not sure it ever actually idiomatic, really:

    The recommendation is also not followed in this repo. It is hard to figure out how many calls there are of the .clone() form, but there are 1550 uses of Arc and only 65 uses of Arc::clone. The recommendation has existed for over two years.

    ↩︎
5 Likes
45

Those changes seem great! Just a couple of minor edits to your edits:

– After "As one example, references themselves implement Clone", I'd add "by copying the reference rather than the value." to make sure to be totally explicit.

– "A clone of a Vec<Rc<Widget>> will newly allocated Vec" is a mistake (perhaps you meant should be "will newly allocate" or "will return a newly allocated"

1 Like
46

Yeah, that's a very good point and you are right.

When I was writing the post I had two different options:

  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 copy. For example Rc::clone will be deprecated and get replaced with a function like Rc::share

(When we deprecate something it is not removed from the API it is just marked as deprecated and using it is discouraged)

When I was writing the post I thought with myself if I go with option 1, because a deep copy implementation of Clone is not harmful there will be no pressure for the deprecation phase and that would be easier. However I did not consider that Clone is implemented for all primitive types and there will be lots of confusion. So maybe option 2 is the right way for doing it.

47

Isn't this just strictly worse than the current situation... rather than being (maybe) confusing to new-comers, it's now just finicky for everybody?

Rc::clone follows the same semantics as copy/clone for usize. Would we deprecate clone for some of the numeric types and not others? In many ways, anything that can be used to index into memory needs to have clone deprecated (which is just about everything?)

Right now clone does more of less the thing you're most likely to want for a given type. This might be confusing in the face of generic code, but it's straight forward 99% of the time. It's pretty clear why cloning an Rc increments the reference count. It would less clear why a numeric type has a deprecated clone.

If I arena allocate a linked list, I may follow usize pointers as I clone. I couldn't follow usize in general because I can't know if a usize is being used as an index in general. So an arena allocated data structure can use the added context of its type to decide what cloning means. That's back to custom cloning (I'm pretty sure it's always going to come back to custom cloning).

In part, the confusion stems from the fact that nobody has yet figured out how to say what a deep clone even is. The closest we've come is a trait that would prevent a programmer what accidentally using shared mutability but...

This is why clone doesn't have the teeth you may want. I think that's fine. Again a trait that does the thing you're most likely to want from clone in the context of the type given still seems the best approach for a standard library. Trying to staple on extra guarantees is fragile work - perhaps best left to a third party crate.

I'd like to see a real-life worked example where this is really a problem. The examples so far have all been small and (perhaps not fairly) unconvincing. Let's find a project with multiple contributors and a decent code size that would really benefit from this change :slight_smile:

48

A example:

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

fn main() {
    let mut v1: Vec<Rc<RefCell<i32>>> = Vec::new();
    v1.push(Rc::new(RefCell::new(1)));
    v1.push(Rc::new(RefCell::new(2)));
   
    let v2 = v1.clone();
    println!("v2: {:?}", v2);
  
    *v1[0].borrow_mut() += 1;

    println!("v2: {:?}", v2);
}

(Playground)

Output:

v2: [RefCell { value: 1 }, RefCell { value: 2 }]
v2: [RefCell { value: 2 }, RefCell { value: 2 }]
49

This is exactly the output I would expect with Rc<RefCell<<i32>>>

I mean I'd like to see a real-life worked example where this is really a problem. The examples so far have all been small and (perhaps not fairly) unconvincing. Let's find a project with multiple contributors and a decent code size that would really benefit from this change :slight_smile:

2 Likes
50

We are not talking about a compiler bug here. In any example you will be able to predict and prevent the bug.

51

Many compiling pieces of code may have logic bugs. Preventing these systemically where possible makes sense.

You're suggesting what amounts to a fairly large change in the standard library. The hope (I assume) is that this effort would prevent some logic bugs. Sure, I'm on board! If we're going to convince others, we should probably first assess whether this is a problem that manifests in actual code bases.

Finding as many in use examples of the problem we're aiming to solve as we can will also help us design the solution. We may learn more about the problem and resulting bugs from doing a survey of some public code. If we examine how other code bases work around the problem when it appears, we might find inspiration there too.

We could do a quick survey of how other systems languages manage cloning. Not that Rust needs to copy them, but it might be instructive too.

Anyway, I'd like to see a real-life worked example where this is really a problem. The examples so far have all been small and (perhaps not fairly) unconvincing. Let's find a project with multiple contributors and a decent code size that would really benefit from this change :slight_smile:

2 Likes
52

The RFC that got declined was about adding some method with a different name. Since that resulted in the Rc::clone recommendation but that recommendation was later removed, maybe libs would reconsider.

The deprecation isn't going to happen. For one, it would be too disruptive -- especially as a 180 over the recommendation that made it the preferred style for some. But for two, it can't even apply and doesn't even matter in a generic setting where you don't know you have an Rc. It does nothing for your &impl Clone use cases.

And with regards to those use cases: Clone is working exactly as intended. There is no bug here. Duplicating an Rc with shared ownership gives you something with the same shared ownership. Allowing things like your example is entirely intentional. There are some learning potholes and questionable naming choices though, certainly.

Incidentally, I thought of another example of why you don't really want independent copies (no shared mutable state): You couldn't have anything that allocates, as that's manipulating state global to the allocator. [1] You can use this to do more than just allocating. [2]

[3]

  1. Something similar is true of many other OS objects incidentally, like files and streams -- all those &T implementations. ↩︎

  2. And incidentally, the RFC that resulted in a defaulted type parameter for an allocation type on things like Vec is an example of something that would be impossible if adding interior mutability to a type was a breaking change. Being able to do these things is also intentional. ↩︎

  3. One could ponder a good-faith DeepClone that doesn't care about the shared state thing, but I personally don't think there's enough demand, because Duplicate Clone is usually what you want unless you know the specific types anyway, so you're asking an entire ecosystem to add a derive for something almost no one needs. ↩︎

2 Likes
53

In all these cases (even trait objects) the compiler will be able to generate a warning which tells you that a deprecated function is used in your code. The warning will be in the code that is using the generic function where you've passed the Rc in.

54

It doesn't work that way, but since it won't happen, it's moot.

55

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.