collections: Add shallow_copy method to Cow which always reborrows data

[ipetkov]

Found myself needing a method to unconditionally borrow the underlying data inside of a Cow (there by doing a "shallow copy" instead of a full clone), and I think it could be useful to others.

Its a small addition so I figured I'd start the discussion here instead of going through an RFC first.

Currently feature gated behind cow_shallow_copy.

[rust-highfive]

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

[birkenfeld]

Is there an advantage over just using the deref'd &B?

[ipetkov]

@birkenfeld You continue to have clone-on-write semantics without having to manually wrap the &B. Thus you can shallow_copy (but also borrow) the original Cow and any sub-copies as much as you want but only cloning when absolutely needed.

[aturon]

@ipetkov My apoogies -- I totally missed this PR in my inbox.

This looks like a very plausible addition! Before we land, I'll let @rust-lang/libs weigh in.

[alexcrichton]

I'd personally be a little confused by this method as it seems like it's not actually copying anything? That is, this is the equivalent of Cow::Borrowed(&*foo), right?

[brson]

I can see the use. "shallow copy" is a strange thing to say in Rust since all copies are shallow - "shallow clone" would make more sense, but also less.

[ipetkov]

@alexcrichton pretty much. The idea for this stemmed from my misunderstanding (for quite a while actually) around cloning a Cow<T> (I thought it reborrowed, when it actually clones the underlying data). Figured seeing a method like this would clear the confusion for others, especially since the docs don't make it explicit how cloning a Cow behaves unless you look at the source. But there may be better ways to get that point across if this addition doesn't bring in a lot of utility.

@brson I originally called it shallow_clone but changed my mind since I wanted to highlight that this is as cheap as a copy (and avoid confusion between .clone() which usually has "deep clone" semantics in rust).

[alexcrichton]

Hm actually cloning a Cow kinda makes sense to me to do an operation like this, but we probably can't do that now due to backwards compatibility. I think another reason this feels a little off to me as this is basically a fancy wrapper around &'a T so couldn't that just be used instead?

[ipetkov]

@alexcrichton You could, but you'd have to manually rewrap it (or the harder step, realize that's all you need to maintain clone-on-write semantics).

I was using this somewhat in a crate of mine and I figured this could have been useful upstream (i.e. downstream crates use this method from std and not from my export), but since then I've moved away from Cows for unrelated reasons. I don't feel too strongly about not including this in std for now, but I would suggest we include a doc example to highlight it is possible to have "cheap copies whose mutation does not affect the original".

[brson]

Hm actually cloning a Cow kinda makes sense to me to do an operation like this, but we probably can't do that now due to backwards compatibility

This does look like the obviously right thing for clone to just do in retrospect. Maybe a 2.0 wishlist?

[bluss]

Using 'a like this is usually a red flag that deserves some scrutiny (when 'a is the parameter used in the type, and not a method level parameter). I think fn shallow_copy(&self) -> Cow<B> would be a more natural

[alexcrichton]

Ah as @bluss pointed out in #34284, this is not the same as clone because the lifetime is tied to the receiver, not the 'a parameter on the Cow type itself. That makes me a little more wary to add a dedicated method for this as I'm not sure what the utility of having a shortcut for Cow::Borrowed is in that case...

[ipetkov]

@alexcrichton seems like there isn't much we can do at the current moment to improve this, besides adding documentation to avoid pitfalls and misconceptions.

Thanks to everyone for the feedback, feel free to close if there is nothing further to discuss.

[alexcrichton]

The libs team got a chance to discuss this today and the conclusion was that we're going to close this for now. There are enough open questions about this API that it doesn't seem quite appropriate for the standard library just yet, but thanks regardless for the PR @ipetkov!

[JustAPerson]

I was also looking for something like this today as well. What kind of documentation would help? I'd be willing to open a PR. An explanation that to duplicate a Cow, you should just Cow::Borrowed(&**cow)? I had assumed this is what .clone() did but I went to double check then became confused why this wasn't the case, so at least some mention would be nice.

There are enough open questions about this API that it doesn't seem quite appropriate for the standard library just yet

Is requiring &'a self the open question?

[bluss]

I'll try to put the &'a self question to rest 😄

I don't think requiring &'a self is a workable solution. An exercise would be to add a method with that signature using an extension trait, and then try to use it in a realistic scenario.

The natural signature is:

impl<'a, T> Cow<'a, T> {
   fn shallow_copy<'b>(&'b self) -> Cow<'b, T>;
}

(This matches the current state of the PR) And this signature is general enough that it allows 'b == 'a if you can and would have use for that.

[getreu]

Has someone implemented @bluss signature proposal?

[getreu]

I suggest to reopen this. I find even the restricted version very useful. Here is my use case:

use std::borrow::Cow;

pub trait CloneExt<'b> {
    /// Clone a `Cow` without memory allocation.
    /// Note, the original must outlive the clone! Use case:
    /// ```
    /// use tpnote_lib::clone_ext::CloneExt;
    /// use std::borrow::Cow;
    /// fn do_something_or_nothing(v: Cow<str>) -> Cow<str> {
    ///     if v.len() > 3 {
    ///         let s = "Hello ".to_string() + &*v;
    ///         Cow::Owned(s)
    ///     } else {
    ///         v
    ///     }
    /// }
    /// // Sometimes, we only have a `&Cow`, but we need a `Cow`!
    /// let a: &Cow<str> = &Cow::Owned("world!".to_string());
    /// let b: Cow<str>  = a.shallow_clone();
    /// assert_eq!(do_something_or_nothing(b), "Hello world!");
    ///
    /// let a: &Cow<str> = &Cow::Owned("ld!".to_string());
    /// let b: Cow<str>  = a.shallow_clone();
    /// assert_eq!(do_something_or_nothing(b), "ld!");
    /// ```
    fn shallow_clone(&'b self) -> Cow<'b, str>;
}

impl<'b> CloneExt<'b> for Cow<'b, str> {
    fn shallow_clone(&'b self) -> Cow<'b, str> {
        match *self {
            Self::Borrowed(b) => Self::Borrowed(b),
            Self::Owned(ref o) => Self::Borrowed(o.as_ref()),
        }
    }
}

Add a shallow_clone method to Cow (reopen #33777) - libs - Rust Internals

[ChrisDenton]

@getreu Please open an API change proposal (ACP) with the motivation and a proposal that the libs-api team can consider. It's been many years since this was closed so a fresh proposal is warranted. Feel free to also link back to this issue and the internals thread.

[getreu]

Add `shallow_clone()` method to Cow which always reborrows data · Issue #283 · rust-lang/libs-team