rust - How not to capture or move a String in a match branch or ok_or argument - Stack Overflow

admin管理员组

文章数量:1126094

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?

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?

• rust
• move-semantics

Share Improve this question Follow edited 2 days ago cafce25 27k55 gold badges4141 silver badges5252 bronze badges asked 2 days ago PiRKPiRK 1,05533 gold badges1313 silver badges3333 bronze badges 1

• Because ok_or is just a function, the compiler can't reason about the scenarios in which it will or won't return early. if-else lets the compiler reason about reachability. – BallpointBen Commented 2 days ago

Add a comment  | 

3 Answers 3

Sorted by: Reset to default 3

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.

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.

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.

本文标签： rustHow not to capture or move a String in a match branch or okor argumentStack Overflow