Is it considered bad design to not accept borrow references, when possible?

help
1

Sometimes I come across situations where I'd expect a borrow reference to be supported by a function call, but it's not. I'm trying to determine if this is necessary, or if it's just an overlooked design decision.

Case in point: I've been working with the inquire crate recently. I like this crate, because it provides an easy mechanism to accept user input from terminal applications. Implementing the prompts is incredibly straightforward, and doesn't require any specialized knowledge.

One of the things I noticed about the API design is that I can't pass in a reference to a Vec<String> as a list of selectable options. See the second argument to new(..., &option_list).

let option_list = vec![ "option1", "option2", ];
let prompt_message = "Please select one of the following options";
let result = inquire::Select::new(&prompt_message, &option_list).prompt();

This results in the following error:

mismatched types
expected struct Vec<_>
found reference &Vec<&str>

To fix this, I have to remove the borrow & from it.

let result = inquire::Select::new(&prompt_message, option_list).prompt();

Now, you might think that this doesn't matter so much. Who cares? The second example compiles, right?

Well, the reason it concerns me is because I need to reuse these options in a loop. Because I can't reuse the options, I am forced to clone them each time, which seems inefficient to me.

This fails to compile, with the error:

use of moved value: option_list value moved here, in previous iteration of loop

let option_list = vec![
    "option1",
    "option2",
];
let prompt_message = "Please select one of the following options";
loop {
    let result = inquire::Select::new(&prompt_message, option_list).prompt();
}

To fix this, I have to clone the Vec<&str> each time.

let option_list = vec![
    "option1",
    "option2",
];
let prompt_message = "Please select one of the following options";
loop {
    let result = inquire::Select::new(&prompt_message, option_list.clone()).prompt();
}

Is this a result of a less than ideal design decision? Or is there a better way of writing this program than what I'm currently aware of?

2

User inputs are provided by human, and humans are pretty slow. It's unrealistic for any human to type few million words in a second. Which means often you don't need peak efficiency on user interaction. The main logic of the program may need to be lightening fast, but the interaction itself doesn't have to. User may not recognize it if it happens within 0.1s anyway.

2 Likes
3

I’m not sure how it’s possible, anyway. The API takes a Vec<T>, but also returns a choice of type T. That wouldn’t be possible either, if it worked via borrows.

This is in some ways the most ‘generic’ version, too. To support &Vec<T> (or &[T]), it would need to return &T choices, too, or involve T: Clone, either way not supporting types with actual ownership. (Arguably, the use cases for this are quite limited, given the API also drops all other list entries, so usually is used for types where you don’t care about ownership too much, anyways.) Whereas as it stands, you can just use a Vec<&T> if you want references anyways, at the small cost of still needing the vec to be owned, but looking through the code, there is so much more happening on each prompt anyways that it doesn’t matter.

Finally, not designing this around &[T] also adds simplicity. Less lifetimes and borrows usually help usability (though to be fair, the API already contains lifetimes anyway? On the other hand, those are arguably different because they all end after the prompt, so you wouldn’t want to couple them to the &T you get back, necessarily).

My main point being, the whole API is affected by the design choice of using a Vec<T> field here. It’s not possible to just “accept a borrow” here, unless multiple other parts of the API also change – and in case you’re thinking “writing .clone() worked fine as a caller, why doesn’t the API just do this internally?”: that’s usually considered bad API design. An API that receives some borrowed value that isn’t trivially cheap to clone and for the first thing it does always unconditionally clones that value should usually accept the owned value instead, so that the caller can check whether or not the clone is avoidable on their end instead, as is e.g. the case if they already have an owned value and don’t need it anymore afterwards (think: the non-loop use-case).

As for your code, one thing I’m noticing is that Select itself supports Clone. So one possible alternative approach is to just set up the Select once, and then clone the whole thing, sorta like:

let option_list = vec![
    "option1",
    "option2",
];
let prompt_message = "Please select one of the following options";
let select = inquire::Select::new(prompt_message, option_list);

loop {
    let result = select.clone().prompt();
}
4 Likes
4

when it comes to performance reasoning, always use measurements, not "in theory it could have ...".

cloning a Vec of strings isn't big deal really. realistically, user prompts should be at most several hundreds or maybe thousands of bytes. besides, it will not be in the critical path anyway.

the way Select is defined, surely it can use borrowed value instead of owned, but what is an "ideal" or "less than ideal" design anyway? when it comes to software design, it always a trade off.

the Select api is designed with custom options in mind, not just strings (also it doesn't return an index, but the selected option itself). in many cases, if you change the design a bit, it is NOT necessary to loop through the options. example:

enum Fruit {
    Apple,
    Banana,
    Cherry,
}
impl Display for Fruit {
    //...
}
use Fruit::*;
let favorite = Select::new(
        "what is your favorite fruit?",
         vec![Apple, Banana, Cherry]
    ).prompt()?;
match favorite {
    //...
}
1 Like
5

forgot to mention, if you find the API of inquire not suitable for your use case, maybe you would like dialoguer. example using dialoguer::Select looks like:

let options = ["Apple", "Banana", "Cherry"];
let choice = Select::new()
        .with_prompt("what is your favorite fruit?")
        .items(&options)
        .default(0)
        .interact()?:
println!("your favorite is {}", options[choice]);

see documentation:

1 Like
6

The reason I'm looping is for user experience, not because of the presented options. The fact that I'm looping though, is what's forcing me to clone the Vec. Otherwise, it gets consumed on the first iteration and can't be used again. I was hoping to find a way to avoid doing that, but it seems like the general consensus is that it doesn't matter that much.

I was trying to use this as a learning experience to find more efficient ways of writing Rust code.

I'm not worried about the API so much as I am trying to learn if there's a more efficient way of building such an API if I were the one developing it. Just trying to understand what is considered good practice in these scenarios.

Thanks very much for the responses, everyone!

1 Like
7

I would be tempted to add to the inquire API a method to prompt and return a borrow of the selected option (and/or) its index.

It's more complicated but provides the power to avoid cloning to the user if they wanted to.

However it's possible the author of the crate considered this but chose not to, to discourage premature optimisation and over complicating.

E.g. so the user could do

let option_list = vec![
    "option1",
    "option2",
];
let prompt_message = "Please select one of the following options";
let select = inquire::Select::new(&prompt_message, option_list)

loop {
    let result: &str = select.prompt_ref();
}

Possibly the naming should be prompt (, prompt_mut) and into_prompt to follow the convention used by iter, iter_mut, and into_iter.

But that's a breaking change and makes the common case (into_prompt) more verbose/less obvious. Though it does clarify that the Select is dropped when used.

4 Likes
8

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.