CODEWITHSUNDEEP
rustmove-semantics
PiRK
PiRK

Reputation: 1075

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?

Upvotes: 1

Views: 78

Answers (3)

hkBst
hkBst

Reputation: 3370

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.

Upvotes: 0

cafce25
cafce25

Reputation: 27550

This cleans up really nicely using the functional combination methods (Option::and_then), you can also use Value::as_i64 directly instead of going through an intermediary &Number:

fn json_to_block_height(height: Value) -> Result<i32, RPCError> {
    height
        .as_i64()
        .and_then(|height| (height >= 0).then_some(height))
        .and_then(|height| i32::try_from(height).ok())
        .ok_or_else(|| RPCError::CustomError(1, "Invalid height".to_string()))
}

Now there is only one place where you construct the error still and thus no problem with cloning or moving.

Because I use ok_or_else at the end this code also doesn't have to allocate the String in case everything works out ok, which avoids another unnecessary allocation.

Upvotes: 3

fdan
fdan

Reputation: 1814

In addition to the other response (which shows how to move instead of cloning), you should consider not creating a String at all before the error occurs. Right now, your function always allocates memory for the error message, even if the function succeeds. Using a string slice first would solve your problem as well.

Besides that, the idiomatic way is to seperate the error and message entirely using the Display trait. Check Rust By Example for an example.

Generation of an error is completely separate from how it is displayed. There's no need to be concerned about cluttering complex logic with the display style.

Upvotes: 1

Related Questions