rustmove-semantics

How not to capture or move a String in a match branch or ok_or argument


I'm working on a function that converts a json value into an i32, returning an error if any step of the conversion fails.

fn json_to_block_height(height: Value) -> Result<i32, RPCError> {
    let err_msg = "Invalid height".to_string();
    let height = match height {
        Value::Number(h) => Ok(h),
        _ => Err(RPCError::CustomError(1, err_msg.clone())),
    }?;
    let height = height
        .as_i64()
        .ok_or(RPCError::CustomError(1, err_msg.clone()))?;
    if height < 0 {
        return Err(RPCError::CustomError(1, err_msg));
    }
    i32::try_from(height).map_err(|_| RPCError::CustomError(1, err_msg))
}

Is there any good way to prevent the unnecessary cloning of the error message in the match branch and in the ok_or argument, short of unrolling the whole code with explicit if...else... branches? Technically if any of these error conditions are met the following code will be unreachable, so we should never really need to move this String in a perfect world.

I tried replacing the ok_or(RPCError::CustomError(1, err_msg.clone()) method with ok_or_else(|| RPCError::CustomError(1, err_msg.clone()), but the err_msg still seems to be captured and moved in the closure.

Bonus question: would there be any performance improvement by making the code more verbose with explicit branching to avoid the unnecessary copy, or is the "idiomatic rust" solution more performant despite these copies?


Solution

  • Explicit clones can be removed like so, though I don't expect this to have any performance implications:

    #![allow(dead_code)]
    
    enum RPCError {
        CustomError(usize, String),
    }
    
    use serde_json::Value; // 1.0.133
    
    fn json_to_block_height(height: Value) -> Result<i32, RPCError> {
        let err_msg = "Invalid height".to_string();
        let height = match height {
            Value::Number(h) => h,
            _ => return Err(RPCError::CustomError(1, err_msg)),
        };
        let height = match height.as_i64() {
            Some(h) => h,
            None => return Err(RPCError::CustomError(1, err_msg)), 
        };
        if height < 0 {
            return Err(RPCError::CustomError(1, err_msg));
        }
        i32::try_from(height).map_err(|_| RPCError::CustomError(1, err_msg))
    }
    

    or the more functional:

    #![allow(dead_code)]
    
    enum RPCError {
        CustomError(usize, String),
    }
    
    use serde_json::Value; // 1.0.133
    
    fn json_to_block_height(height: Value) -> Result<i32, RPCError> {
        let err_msg = "Invalid height".to_string();
        match height {
            Value::Number(h) => match h.as_i64() {
                Some(h) if h >= 0 => i32::try_from(h).map_err(|_| RPCError::CustomError(1, err_msg)),
                _ => Err(RPCError::CustomError(1, err_msg)),
            },
            _ => Err(RPCError::CustomError(1, err_msg)),
        }
    }
    

    As cafce25 points out in their answer, Value has a dedicated as_i64 method that does what you want, so this would become:

    fn json_to_block_height(height: Value) -> Result<i32, RPCError> {
        let err_msg = "Invalid height".to_string();
        match height.as_i64() {
            Some(h) if h >= 0 => i32::try_from(h).map_err(|_| RPCError::CustomError(1, err_msg)),
            _ => Err(RPCError::CustomError(1, err_msg)),
        }
    }
    

    From this, it is only one more small step to cafce25's excellent answer using and_then, which is really what you should use instead, once you wrap your head around all the useful methods on Option and Result.