Implementing find_by_name: cannot move out of borrowed content

I'm trying to implement a simple collection-with-elements type program. Here is a code example with a Zoo containing Animals:

The issue is in find_by_name which looks like this:

 fn find_by_name(&self, name: &str) -> Result<Animal, &'static str> {
        for animal in self.animals {
            if animal.name == name {
                return Ok(animal);
            }
        }
        return Err("Could not find animal")
    }

I get an error saying cannot move out of borrowed content.

Is there a way to return the Animal without copying it?

1 Like

Borrow vector, don't own it.

for animal in &self.animals {
1 Like

That is not quite enough, because then animal is an animal pointer &Animal instead of an owned animal. But if you just borrow out your animal you won't have that problem: (Note that the function now returns Result<&Animal, &'static str>).

fn find_by_name(&self, name: &str) -> Result<&Animal, &'static str> {
        for animal in &self.animals {
            if animal.name == name {
                return Ok(animal);
            }
        }
        return Err("Could not find animal")
    }

This also makes some intuitive sense: If you only borrow the zoo, you can't give away ownership of the animals. They aren't your animals.

5 Likes

In most scenarios, you will also want a second method to go along with find_by_name:

fn find_by_name_mut(&mut self, name: &str) -> Result<&mut Animal, &'static str> {
    for animal in &mut self.animals {
        if animal.name == name {
            return Ok(animal);
        }
    }
    return Err("Could not find animal")
}

Otherwise you can only look at the animals and not feed, caress or bath them.

The std does this similar, I have been relying on Vec::get and Vec::get_mut a lot recently.

Not sure if this is right, but how I worked it out after a little learning. (This is the most complex thing I've thought through in Rust so far, so thanks for that.)

    fn find_by_name(&self, name: &str) -> Result<&Animal, &'static str> {
        match self.animals.iter().position(|ref r| r.name == name) {
            None => return Err("Could not find animal"),
            Some(index) => return Ok(&self.animals[index])
        }
    }

...

    let ref rosie = zoo.find_by_name("Rosie").unwrap();

I'd also be interested in the more experienced as to if this is good or bad. :slight_smile: I'm worried that I'm doing bad things with ownership or something.

1 Like

Introducing some iterators is nice, but I would do it differently.

fn find_by_name(&self, name: &str) -> Result<&Animal, &'static str> {
    self.animals
        .iter()
        .filter(|animal| animal.name == name)
        .nth(0)
        .ok_or("Could not find animal")
}

This avoids the detour via the index you do. Otherwise I am perfectly fine with your code. Here you can also define a mutable borrow by swapping .iter() out for .iter_mut().

Edit: Removed ref, thanks to @xfix for pointing that out.

.find(|r| r.name == name)

Also, no need to introduce unnecessary references, filter already provides a reference.

1 Like

The .nth(0) is because filter could return more than one in some instances?

This type of efficiency and clean code is what I really like coming from Python.

Because it's still an iterator (filter returns an iterator that only has element that fit a given predicate). .nth(0) probably can be written in more clear way as .next(), as it returns the first item of iterator.

2 Likes

Is there a cleaner way for the mut method?

    fn find_by_name(&self, name: &str) -> Result<&Animal, &'static str> {
        self.animals
            .iter()
            .filter(|r| r.name == name)
            .next()
            .ok_or("Animal not found")
    }

    fn find_by_name_mut(&mut self, name: &str) -> Result<&mut Animal, &'static str> {
        let ref mut animals = self.animals;
        animals
            .iter_mut()
            .filter(|r| r.name == name)
            .next()
            .ok_or("Animal not found")
    }

And to make sure I'm understand everything. This is not semicolon'ed to make it take place of having return in front, right?

Using &self.animals and returning a &Animal make sense! That's what I wanted to figure out. I didn't know I had to use &self.animals (I assumed I already had the reference since I used &self in the function arguments).

I've also seen this _mut pattern — it makes sense to apply this here as well.

Thanks for the alternative implementations with .iter() and .filter(), although I don't see how this would improve readability or performance.

You should take note that add_animal can add more than one of the same exact animal, and you only search for the first animal found, by name. It probably makes a bit more sense to use a HashMap and either a Vec of animals with the same type, or make them unique-- then you can directly expose the get and get_mut methods of HashMap.

Like this: Rust Playground

1 Like

I would agree that it makes more sense to use a HashMap keyed on the name. This is a slightly contrived example that doesn't really represent the real project.

In the actual project I also need to have control over the ordering of Animals. So I should probably use something like Java's LinkedHashMap, which guarantees insertion ordering.

You may want to use a crate for ordered hash maps.

https://crates.io/crates/ordermap

(if you are worried about performance, OrderMap is actually faster than builtin HashMap type, as it has better cache locality)

1 Like

@sacherjj: Yes, for the mut method you only need to change .iter() to .iter_mut() and adapt the function signature.

fn find_by_name_mut(&mut self, name: &str) -> Result<&mut Animal, &'static str> {
    self.animals
        .iter_mut()
        .filter(|animal| animal.name == name)
        .nth(0)
        .ok_or("Could not find animal")
}

@fdb: You were asking about "how this would improve readability or performance". To be honest I think both styles are equally clearer for this example and you should feel free to keep using for loops. For loops are definitely more beginner friendly. But some people, me included, just fancy this iterator style. It feels clean, functional and when you have more complex iterators it is nice to compose them from simple blocks.

Performance wise, the great thing about Rust is that the iterator style isn't slower. The compiler is smart enough to reduce it to a your loop.

And about ordermap, I haven't used it myself but I believe it only guarantees the order when you add elements to it. Removing elements will mess up the order. (Someone please correct me on this if I am wrong)

3 Likes

Even using the _mut pattern, I can't change the value.

I made a scope so the borrowed animals only live inside. But I think I'm doing something wrong with strings. How would you set the value for .name. Or is this an issue with the default object property not being mutable.

Getting: cannot assign to data in a & reference

    {
        let ref rosie = zoo.find_by_name_mut("Rosie").unwrap();
        println!("In my zoo, {} is a {}", rosie.name, rosie.species);
        rosie.name = String::from("Bob");
    }
    let ref bob = zoo.find_by_name("Bob").unwrap();
    println!("In my zoo, {} is a {}", bob.name, bob.species);

Wouldn't it be more idiomatic to return an Option for functions like these ?

The _mut function is fine, but you throw mutability away by using let ref rosie instead of let ref mut rosie. Actually, you don't even need ref mut it works fine without.

    {
        let rosie = zoo.find_by_name_mut("Rosie").unwrap();
        println!("In my zoo, {} is a {}", rosie.name, rosie.species);
        rosie.name = String::from("Bob");
    }
    let bob = zoo.find_by_name("Bob").unwrap();
    println!("In my zoo, {} is a {}", bob.name, bob.species);

And I agree with @ndusart, an option would be more in line with how Vec and friends do it.

1 Like

Thanks; I was trying this out and it seems the innocuously placed brackets around the "let rosie" block are quite important, otherwise I get this error:

error[E0502]: cannot borrow `zoo` as mutable because it is also borrowed as immutable
  --> <anon>:57:17
   |
53 |         let rosie = zoo.find_by_name("Rosie").unwrap();
   |                     --- immutable borrow occurs here
...
57 |     let koala = zoo.find_by_name_mut("Cynthia").unwrap();
   |                 ^^^ mutable borrow occurs here
...
63 | }
   | - immutable borrow ends here

Here's the updated example:

I'm not sure I understand why I can't have a mutable and immutable borrow at the same time. Is the Rust book the best place to learn about this?

I guess the rust book is a great place to start. If you have any questions you can still ask them in the forum.

https://doc.rust-lang.org/book/references-and-borrowing.html

In particular

There is a ‘data race’ when two or more pointers access the same memory location at the same time, where at least one of them is writing, and the operations are not synchronized.

1 Like