Clippy and &Vec<String>

help
1

Two questions abut clippy:
Context: I got the program to clean compiler, passing unit tests, and running successfullly. I then asked Clippy what cleanups I should make.
As I built the program, I used a lot of &Vec as a parameter type. Clippy told me that I should use &[String] to avoid extra string copies.

Q1: Why does &Vec result in an extra copy?

Okay, whatever the reason, I took Cliipy's advice, and made the change. Inmost cases, that sufficed. In a few cases, the compiler then complained and asked me to add a lifetime marker. Okay, I can do that. A bit surprised that Clippy's advice resulted in a compiler error, but since the advice was easily taken, good enough.

However, in exactly one case, when I replaced &Vec with &'a [String], I got a borrow error on the caller. And it was not at all obvious how to fix it.

Q2: Is this sort of cascading errors from the compiler after clippy's advice a known behavior or I have I stumbled into an obscure corner case?

Thank you,
Joel

2

are you sure that's what clippy says?

the intention of this lint is not to avoid string copies, but to avoid the necessity to create a Vec<>.

in most situations, you actually care about the content of the Vec, not the container itself, in which case a slice reference is more desirable then a reference to the Vec, because it makes your function usable in more context.

however, it is very rare to cause a compilation error when you apply the suggestion. it would be great if you could show the type signature of the actual function that caused the error, it could well be a false positive of clippy, but it can't be said for sure without more details.

1 Like
3

To add to the previous response, while &mut Vec<T> or Vec<T> are substantially more powerful than slices, &Vec<T> only adds the ability to read the capacity of the Vec. Most of the time, the capacity is irrelevant, in which case &[T] suffices.

3 Likes
4

function signature:

fn pers_assoc_picker<'a>(
    assoc_choices: &Vec<String>,
    assoc_pick: &Option<String>,
    pchoices: &'a combo_box::State<PersonIDName>,
    person_choice: &Option<PersonIDName>,
    edit: bool,
) -> Element<'a, Message>

The clippy message is a little vague, and could be expressing concern about creating a Vec. It says:

warning: writing `&Vec` instead of `&[_]` involves a new object where a slice will do

And then if I do that, the compiler then tells me I need a lifetime reference. And when I do that, the caller gets an error. The caller is:

            pers_assoc_picker(
                &guistate.rolodex.as_ref().unwrap().assoc_label_list(),
                &guistate.people.person.assoc_pick,
                &guistate.people.person.assoc_person_picker,
                &guistate.people.person.assoc_person,
                guistate.people.edit,
            ),

And the error is on the assoc_label_list() call, which works with the original &Vec.
For completeness, assoc_label_list() is:

    pub fn assoc_label_list(&self) -> Vec<String> {
        self.assoclabels.clone()
    }

where assoclabels is a \

Vec<String>.

Yorus,
Joel

5

Your code looks like it should work if you just make exactly this change:

-    assoc_choices: &Vec<String>,
+    assoc_choices: &[String],

(Note that I did not add any 'a lifetime; I changed only the type of the referent, not the lifetime of the reference.) Does it work when you do this?

If not, what is the full error message from cargo check? (Make sure you include everything starting from the error: — sometimes people fail to notice that that line even exists, but it’s actually the most important line of them all.)

1 Like
6

Fair ask. I have missed important parts of the error message in the past.

error[E0621]: explicit lifetime required in the type of `assoc_choices`
   --> src\rolo_window\person_gui.rs:879:5
    |
879 | /     row![
880 | |         pick_list(assoc_choices.clone(), assoc_pick.clone(), |instr| {
881 | |             Message::PeopleMessages(PeopleMessage::ChooseAssoc(instr))
882 | |         })
...   |
889 | |     .into()
    | |___________^ lifetime `'a` required
    |
help: add explicit lifetime `'a` to the type of `assoc_choices`
    |
862 -     assoc_choices: &[String],
862 +     assoc_choices: &'a [std::string::String],
    |

For more information about this error, try `rustc --explain E0621`.

(Does the difference between String and std::string::String matter, or is that just the compiler being verbose?)
Yours,
Joel

7

That error is weird.

  • It shouldn’t happen for a function signature. Are you sure that line 862 is in fact part of fn poers_assoc_picker’s signature, and not a struct or something else?
  • If it is going to happen at all, it should happen for both &Vec<_> and &[_].

Does the function have an attribute macro applied to it, perhaps? That macro might be imposing additional requirements.

8

Now I am more puzzled than I was. Line 862 is the line declaring the first parameter of the function. There are no attribute macros on the function. The lines before the function declaration are the end of the previous function, a blank line, and a comment line. (I am still trying to learn enough github to be able to post the whole thing.).
I suppose there could be some odd effect of the fact that is is an iced application view manipulation function. But I have no idea what that would be.
Thanks for your attention,
Joel

9

the error must be caused by code changes somewhere else. try to rollback to a previous clean state, then apply the clippy changes without changing other part of the code base.

as @kpreid said, as for lifetimes in the type signature, there's no difference wether it's &Vec<String> or &[String]: changing from one to the other should not generate a lifetime error [1].

  1. it might cause errors related to inherent methods on Vec, but not a lifetime error like in this case ↩︎

10

I would have thought it was other code changes, except. I compiled the code cleanly. Ran clippy. t confirm the first message. Then made the clippy change, recompiled, and got the reported error.
Yours,
Joel

11

In fact, the other 'a annotation in the parameters was to fix similar compiler complaints when I made the clippy change to the other &Vec<...> parameter. The compiler asked for the lifetime on all of them. Only the first one seems to have produced errors from the caller.
Yours,
Joel

12

I don't know the details of the error being reported, but I do know that when I used pick_list, it has a funny signature and it was pretty picky about whether I was passing a vec, slice or array to it. It's also typical to get a complaint about the lifetime 'a, because ultimately you're returning something that impls (at least Into) Element<a>`, which is borrowing from your app state.

(Take this with a pinch of salt, I don't fully understand how this all might interact)

13

thanks for pointing it out.

now looking at pick_list(), I think I have (possibly?) figured out why it made a difference in this case changing from &Vec<String> to &[String].

in the error message, OP calls .clone() on assoc_choices, and this actually behaves differently for &Vec<String> and &[String]. Vec<String> is clonable, while the slice [String] is DST so the code only cloned the references.

I overlooked this difference between Vec and slices. when I need ownership of something, I would usually take the argument by value, instead of taking it by reference then cloning it internally. so I didn't realise OP's code actually required Vec<String>.

in this case, &Vec<String> cannot be replaced with &[String] directly without changing other code, and if you do, at least you need to change .clone() to .to_vec() in order to get an owned Vec from a slice.

a better solution would be to pass the slice directly to pick_list, eliminating the need to make a clone of the Vec, which requires the data to be borrowed from the app's gui state, but I don't know whether this is suitable for OP's case.

14

You have a bigger "problem" here than passing &Vec<T>. You are expecting as an argument &Option<T>, however you should (almost) never do this. Instead functions should accept as parameters and return Option<&T>.

There is very good youtube video by Logan Smith on this matter, but TL;DR is that in order to use reference to the option you must map it to the option to the reference anyway (either using Option::as_ref, or doing this implicitly using pattern matching). And on your callers it puts a burden of actually storing Option somewhere. It is just over-constraining API design.

5 Likes
15

After the clippy suggestion is working, then that allows simplifying this getter to not clone the Vec:

-pub fn assoc_label_list(&self) -> Vec<String> {
+pub fn assoc_label_list(&self) -> &[String] {
-    self.assoclabels.clone()
+    &self.assoclabels
 }
1 Like
16

The combination of using to_vec in the pick_list call and replacing the return with an &[String} (and fixign the one call that mediates between the two) does seem to have fixed the problem. And had the positive side-effect of removing some redundant clone()s that Clippy couldn't detect.
I do find the interaction between clippy and the compiler in this corner case to be unfortuante. But such things happen.
Thank for all the help.
Yours,
Joel

17

The reason I am passing an &Option is that the state stores an Option. (The update method changes it, as per iced structure.) I need to pass it around, and eventually pass a copy of the value in to the pick_list itself. So at the end I have to clone the Option. Rather than passing it by value all the way around, I just pass a reference to it (&Option), and then clone the reference at the pick_lsit parameter. Clippy and the compiler do not complain about this. Is there some drawback I am missing?
Thanks,
Joel

18

Did you look at the youtube video?

Instead of returning &Option, return Option<&T>. You can do it easily with:

fn whatever(&self) -> Option<&T> {
   self.var.as_ref()
}
1 Like
19

I hate learning from videos. So no, I did not look at the video.
The Option Is stored in the iced state structure, which I have access to in the caller of this method. What benefit is there in calling as_ref instead of the clear passing of a reference to the element I need (the Option), and having a returned type that does not match what I need to end up with? Sure, I can pass self.statevar.as_ref() instead of &self.statevar. But why? If there is a substantive reason I am missing, I am happy to change it. (And more, happy to learn for the future.)
Yours,
Joel

20

Copying an Option<&T> is exactly as cheap as copying a &Option<T>, so your "just pass a reference" doesn't describe a cheaper operation.

Let's say you have two functions:

fn f1(a: Option<&String>) {}
fn f2(a: &Option<String>) {}

This works:

    let x = String::from("hello");
    f1(Some(&x));
    println!("{x}");

This doesn't compile:

    let x = String::from("hello");
    f2(&Some(x));
    println!("{x}");

error[E0382]: borrow of moved value: `x`

So f1 is a better API.

3 Likes