Cleanest way to unwrap layers of Option/Result?

help
1

Hey there, apologies if this is a super common question, I just wanted to run a specific bit of code past a more experienced rustacean. I'm trying to work out if it's an idiomatic approach or not.

pub enum Request {
    Exit,
    Connect {
        addr: net::SocketAddr,
        expr: regex::Regex,
    },
    Error(String),
}

fn parse_index<T: std::str::FromStr>(args: &Vec<neovim::Value>, index: usize) -> Option<T> {
    args.get(index)?.as_str()?.parse().ok()
}

impl Request {
    fn from(name: &str, args: Vec<neovim::Value>) -> Request {
        match name {
            "exit" => Request::Exit,
            "connect" => {
                if let (Some(addr), Some(expr)) = (parse_index(&args, 0), parse_index(&args, 1)) {
                    Request::Connect { addr, expr }
                } else {
                    Request::Error("connect expects an address and expression".to_owned())
                }
            }
            _ => Request::Error(format!("unknown request name `{}`", name)),
        }
    }
}

So I have this enum that takes some values from Neovim over RPC, it converts those values to my own enum that I match on and handle later. As you can see in the middle, my connect request takes a SocketAddress and Regex.

Since I have to extract those values from a vector and unwrap a few layers I thought I'd wrap that functionality up inside a function using the ? syntax. So is this approach of "move the messy part to a fn using ?" and tuple destructuring a neat approach to this problem or a horrible approach I'll regret later?

Also if you notice anything else gross with this, please let me know! I've read through The Book but I'm still very much getting to grips with the basics, I am still yet to have my first full borrow checker burn.

Thanks a lot!

2

Another Rust + neovim user, howdy! If you can share your code, I'd certainly like to have a closer look :slight_smile:

As for your example, wrapping the argument parsing in another function is a good approach imho, it's a weak spot of neovim_lib. You can see how I did it here, and in particular look at this parsing function, which shows how fine grained the error messages can easily become (I mention this below).

The overall handling seems off though, you've subsumed the error case in your Request struct, which I think is a bit of a mistake. Imho, remove the Error case from the struct, and use Rusts Result as the return type of your functions. That will streamline your error handling, right now you can't give a good feedback about what went wrong with parsing the args, which would be valuable for yourself in particular. If you need more help with this, let me know :slight_smile:

3

There's literally dozens of us! My project is Olical/conjure (and this is the commit at the time of writing), it's going to be some Clojure REPL tooling to replace my current kinda brittle and bloated stack.

I'm just worried I'm not approaching things in an idiomatic way and I'll regret it later? Also I'm only looking at main.rs and server.rs right now, client.rs can be ignored.

Your comments on the error are noted, makes a lot of sense and I'll change it to that. Thank you! I know I can go much finer grained but I guess I'm keeping it vague out of habit at the moment. I'm not used to a language that's looking out for me and actually helping me handle these things :grimacing:

I'll take a look at your code too, I remember seeing it on my "examples of neovim_lib" Googles a while ago.

Edit: I'm already learning about file structure from looking at your repository. I knew I should probably use lib.rs and split my binary code out from they "library" code but thought I'd do that later when I have more things working. I didn't realise I could make a file called bin/cargo.rs, I thought it had to be bin/main.rs? Seems really neat.

4

I'll have a look at your repo later, but for now let me just throw a bit of experience writing my plugin at you. You're bound to have errors during development, and quite some will be in the gray area of communication between your plugin and neovim, which of course goes through neovim-lib and rmpv, and you will only be able to see those error during runtime, and they will be obnoxious to track down (note that your plugin can't really use its stdin/stdout, because that's how it communicates with neovim, so println won't be your choice tool). So set up good error messages + logging early on and save yourself some of your hair :smiley:

1 Like
5

First, you should consider removing Error variant from the Request enum. Error handling is usually handled using Result type, and you'll find using a custom Error variant much less ergonomic.

The if let (Some(addr), Some(expr)) = ... pattern will become very inconvenient if you have 5 arguments instead of 2.

Here is an example of another possible approach. Note that here we not only want to unwrap all the layers but also supply a meaningful error message for any possible failure.

use std::fmt::Display;
use std::str::FromStr;

fn parse_index<T: FromStr>(args: &[neovim::Value], index: usize) -> Result<T, String>
where
    T::Err: Display,
{
    args.get(index)
        .ok_or_else(|| format!("expected argument at position {}", index + 1))?
        .as_str()
        .ok_or_else(|| String::from("expr must be a string"))?
        .parse()
        .map_err(|e| format!("expr parse error: {}", e))
}

impl Request {
    fn from(name: &str, args: Vec<neovim::Value>) -> Result<Request, String> {
        let request = match name {
            "exit" => Request::Exit,
            "connect" => {
                let addr = parse_index(&args, 0).map_err(|e| format!("invalid addr: {}", e))?;
                let expr = parse_index(&args, 0).map_err(|e| format!("invalid expr: {}", e))?;

                Request::Connect { addr, expr }
            }
            _ => return Err(format!("unknown request name `{}`", name)),
        };
        Ok(request)
    }
}

This scales better when you have many arguments.

Using String as the error type is not ideal, so you may want to take a look at the failure crate for a more powerful error handling approach.

Note that &Vec<T> is considered non-idiomatic, since &[T] provides the same functionality and is more general.

6

Oh this is great, thank you. It makes a lot more sense and my errors are so much more useful. I think I'll set up some logging to a file so I can debug a little better as well but this really helps. I think for that I need simplelog and log.

Thanks a lot for your help, I feel a lot better about the current code now!

7