Clone in return with?

code review
#1

Hi,

in the function below:

pub fn a(a: Vec<u8>) -> Result<u8, Vec<u8>> {
    let x = a.get(8).ok_or(a.clone())?;
    Ok(*x)
}

I need to clone a or the compiler will complain.

Instead I could do:

pub fn b(a: Vec<u8>) -> Result<u8, Vec<u8>> {
    let x = a.get(8);
    match x {
        Some(z) => Ok(*z),
        None => Err(a),
    }
}

I'm wondering which is the best method?
The method 1 generate more code (Compiler Explorer) but it actually clone only in the cold path. Method 1 simplify a lot the code if a function can fail in several point and if fail cases are nested.

I'm also wondering if I'm missing something and there is a third way to early return error without boilerplate and without cloning in situations like the one above.

Ty

#2

This is incorrect - the argument of ok_or is always evaluated, as mentioned in the docs:

Arguments passed to ok_or are eagerly evaluated; if you are passing the result of a function call, it is recommended to use ok_or_else , which is lazily evaluated.

So your first function is unconditionally doing a clone of the Vec - it only uses that clone in the cold path.

2 Likes
#3

The following is the shorter alternative, it uses Option::copied

pub fn a(a: Vec<u8>) -> Result<u8, Vec<u8>> {
    a.get(8).copied().ok_or(a)
}
9 Likes
#4

To elaborate on why @matklad's version of a works and yours doesn't:

Calling get on a Vec<T> returns an Option<&T>. In your case, a doesn't dereference this reference until the very end of the function, where you do *x. This means that the reference to the Vec continues to be held until the end of the function, preventing you from moving the Vec itself.

Adding copied turns the Option<&u8> into an Option<u8> by copying the value (basically the same as what you're doing by dereferencing it), which releases the borrow, allowing you to move the Vec again.

This (or your b example) is definitely preferable to cloning the Vec in an ok_or_else, if you don't actually need two versions of the same Vec.

4 Likes