Cow is not Copy-On-Write

[Ekleog]

So. I hope I'm misreading the code or reading at the wrong place. But…

If I can read https://doc.rust-lang.org/src/alloc/borrow.rs.html#159-182 correctly, the implementation of Clone for Cow is like this:

    fn clone(&self) -> Cow<'a, B> {
        match *self {
            Borrowed(b) => Borrowed(b),
            Owned(ref o) => {
                let b: &B = o.borrow();
                Owned(b.to_owned())
            }
        }
    }

So this copies the data when the Cow is copied.

I think it should instead be written like this:

    fn clone(&self) -> Cow<'a, B> {
        match *self {
            Borrowed(b) => Borrowed(b),
            Owned(ref o) => Borrowed(o.borrow()),
        }
    }

(and a similar fix for clone_from)

If I didn't misunderstand the code or its purpose, I can try to write up a PR to fix this.
(and plz gimme kudos for the clickbait title)

[SoniEx2]

clone does an actual clone. it sucks, but isn't a bug.

there should be an issue for Cow.borrowed() tho? you can always use Cow::Borrowed(&*other) tho

[retep998]

Doesn't Clone require that the result have the same lifetime as the original value? If so, then the suggested change would not work.

[Ekleog]

@SoniEx2 At least in my mind, when a type is COW it's supposed to mean cloning it is a lazy operation that'll happen on next modification, and as such having .clone without .borrowed (or .borrow or similar) is treacherous.

@retep007 You're right, my suggested change appears not to work (the rustc build just ended, somehow it doesn't pass all the tests locally even with vanilla, but all the tests in src/libstd do pass.

And actually you're right, there is a conceptual reason why this shouldn't work: Cow::Owned means the lifetime parameter can be 'static if need be, which means cloning it by borrowing it would need to trigger a 'static borrow on the owned object, which isn't a priori possible.

So indeed having a .borrowed (or .borrow or anything else) with a type signature like:

impl<'a, T> Cow<'a, T> {
    pub fn borrowed<'b>(&'b self) -> Cow<'b, T> where 'a: 'b {
        Cow::Borrowed(self.o.borrow())
    }
}

could help show that .clone() is not designed to be used for borrowing (I'd even think deriving Clone for Cow is a misfeature, but removing it would be a retrocompatibility breakage, so…).

Or at least put it up front in the docs that .clone() is not copy-on-write, and that it should be done manually with Cow::Borrowed(&*other).

(now, I'm going to just git grep my code for .clone(), figure out which uses are for Cow, and replace them with another way of borrowing)

[hrvolapeter]

You probably wanted to mention @retep998 😃. Now he will get also notification.

[Ekleog]

Oh, indeed, sorry I mistabbed!

[oli-obk]

I'm going to just git grep my code for .clone(), figure out which uses are for Cow, and replace them with another way of borrowing

That should be a clippy lint!

[SimonSapin]

"Copy on write" refers to the behavior of the to_mut method: https://doc.rust-lang.org/std/borrow/enum.Cow.html#method.to_mut

[Mark-Simulacrum]

Seems like the suggestions here -- beyond a potential clippy lint, rust-lang/rust-clippy#2692 -- are either not viable (backwards compatibility wise) or just not implementable. Closing.