More idiomatic way to write this code

Hi, could somebody could please explain (and show) why I cannot use ? operator when using parse? And how to do it so this code it is more rust idiomatic?
Thanks

fn collect_primary(&mut self, a_String: &String) -> Result<i128, &str> {
        let primary: i128;
        if let Some(sign_found) = a_String.find(&['+', '-', '*'][..]) {
            let a_slice = &a_String[..=sign_found - 1];
            primary = match a_slice.parse::<i128>() {//I CANNOT use ? here, don't understand why...
                Ok(primary) => primary,
                Err(s) => return Err("Cannot parse"),
            };
            Ok(primary)
        } else {
            return Err("Cannot convert");
        }
    }

This is because the Error variant of a .parse::<i128>() operation failing (impl) does not implement an automatic conversion to your error type (&str).

You thus need to "manually" convert it, the idiomatic way being to use the .map_err() adapter:

primary = a_slice.parse::<i128>().map_err(|_| "Cannot parse")?;

Addendum

Note that using &str in your signature like you are doing will tie the lifetime of the string slice to that of one of the input parameters, as dictated by the rules of lifetime elision. In your case, you are actually writing:

fn collect_primary<'slf>(&'slf mut self, a_String: &String)
  -> Result<i128, &'slf str>

which means when you call that function, Rust will think that even the error variant borrows from *self, and prevent you from further passing it along the call chain.

To fix this, be explicit about the 'static lifetime from your static string literals:

-  ... -> Result<i128, &str>
+  ... -> Result<i128, &'static str>

You can then even alleviate a bit the notation, by setting "a default error type":

/// If not specified, the error type shall default to `&'static str`:
type Result<Ok, Err = &'static str> = ::core::result::Result<Ok, Err>;

// Now you can write:
... -> Result<i128>

(Also, taking a &String in input position is a bit too restrictive, since a String represents a necessarily heap-allocated str slice, and thus &String represents a shared reference to such a thing. This means you won't be able to call your function on references to str slices that are not heap-allocated, such as string literals: "...". To fix that, replace &String with &str).

This "mistake" is linted by clippy, so it can be a good trick to learn how to write more idiomatic and thus more ergonomic Rust code to use it on your codebase :wink:. You can also use it on the playground: there is a button on the drop-down menu at the top right corner.

Screenshot 2020-11-09 at 17.37.48

Warnings emitted on your code
warning: unused variable: `s`
 --> src/lib.rs:9:25
  |
9 |                     Err(s) => return Err("Cannot parse"),
  |                         ^ help: if this is intentional, prefix it with an underscore: `_s`
  |
  = note: `#[warn(unused_variables)]` on by default

warning: associated function is never used: `collect_primary`
 --> src/lib.rs:3:8
  |
3 |     fn collect_primary(&mut self, a_String: &String) -> Result<i128, &str> {
  |        ^^^^^^^^^^^^^^^
  |
  = note: `#[warn(dead_code)]` on by default

warning: writing `&String` instead of `&str` involves a new object where a slice will do.
 --> src/lib.rs:3:45
  |
3 |     fn collect_primary(&mut self, a_String: &String) -> Result<i128, &str> {
  |                                             ^^^^^^^ help: change this to: `&str`
  |
  = note: `#[warn(clippy::ptr_arg)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#ptr_arg

warning: unneeded `return` statement
  --> src/lib.rs:13:17
   |
13 |                 return Err("Cannot convert");
   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove `return`: `Err("Cannot convert")`
   |
   = note: `#[warn(clippy::needless_return)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_return

warning: variable `a_String` should have a snake case name
 --> src/lib.rs:3:35
  |
3 |     fn collect_primary(&mut self, a_String: &String) -> Result<i128, &str> {
  |                                   ^^^^^^^^ help: convert the identifier to snake case (notice the capitalization): `a_string`
  |
  = note: `#[warn(non_snake_case)]` on by default

warning: 5 warnings emitted
2 Likes

Thanks

1 Like

An alternative is to define your own error type to replace &str in the function signature. Crates like anyhow and failure are designed to make that easier, but a quick hack is also possible:

pub struct MiscError(pub String);

impl<T: std::fmt::Debug> From<T> for MiscError {
    fn from(e: T) -> Self {
        MiscError(format!("{:?}", e))
    }
}

fn collect_primary(a_String: &String) -> Result<i128, MiscError> {
    let primary: i128;
    if let Some(sign_found) = a_String.find(&['+', '-', '*'][..]) {
        let a_slice = &a_String[..=sign_found - 1];
        primary = a_slice.parse::<i128>()?;
        Ok(primary)
    } else {
        Err(MiscError::from("Cannot convert"))
    }
}

(Playground)

1 Like