Calling a `&mut` object's method in a loop - Mutable borrow starts here in previous iteration of loop

Hello!

With the following code (rust playground link):

use std::collections::HashMap;

struct Foo {
    d: HashMap<String, f32>
}

impl Foo {
    fn get(&mut self, s: &str) -> Option<&f32> {
        if !self.d.contains_key(s) {
            self.d.insert(s.to_string(), 0.0);
        }
        self.d.get(s)
    }
}

fn main() {
    let mut foo = Foo {
        d: HashMap::new() 
    };
    
    let mut saved = Vec::new();
    
    for _i in 1..3 {
        let l = foo.get("a");
        saved.push(l);
    }
}

I get the following error:

error[E0499]: cannot borrow `foo` as mutable more than once at a time
  --> src/main.rs:24:17
   |
24 |         let l = foo.get("a");
   |                 ^^^ mutable borrow starts here in previous iteration of loop

What am I missing? Why is the borrow checker flagging the mutable borrow of foo across loop iterations?

Because you are returning an Option<&f32> so the reference is persisted across calls to Foo::get().

Think about what is happening during the second iteration of the loop. You would have a reference to something inside Foo as well as a mutable reference inside the get() call, which is what Rust is here to prevent us from doing.

f32 implements copy though, so you don't have to return a reference, eg:

Rust Playground

3 Likes

In case you have to handle something, that is not Copy, you should wrap your HashMap in a std::cell::RefCell and have the get-method take &self instead of &mut self. The documentation of std::cell specifically mentions this use-case (see the bold text):

When to choose interior mutability

The more common inherited mutability, where one must have unique access to mutate a value, is one of the key language elements that enables Rust to reason strongly about pointer aliasing, statically preventing crash bugs. Because of that, inherited mutability is preferred, and interior mutability is something of a last resort. Since cell types enable mutation where it would otherwise be disallowed though, there are occasions when interior mutability might be appropriate, or even must be used, e.g.

  • Introducing mutability 'inside' of something immutable
  • Implementation details of logically-immutable methods.
  • Mutating implementations of Clone .
1 Like

This works:

The reason why the borrow lives past one iteration of the loop is because your getter means this:

fn get<'a>(&'a mut self, s: unrelated) -> Option<&'a f32> {

i.e. the returned element belongs to the hashmap, which belongs to self. So the lifetime of the &f32 is the same as lifetime of &mut self. They have to be connected. If there were independent lifetimes, the borrow checker wouldn't be able to stop you from calling self.d.clear() somewhere and destroying all the references this method has given out. So as long as there exists any &f32 reference out there (and it exist in saved that survives loop iteration), you're not allowed to call &mut self anymore. It wouldn't be safe to allow that.

Note that the borrow checker intentionally verifies interfaces, not implementations. It doesn't matter if the implementation doesn't currently do anything breaking. If the interface allows bugs, it's not legal.

3 Likes

hi @scarfone, thanks for the reply. In my actual application I need to return a reference, but the data that is referred to is owned by what would be Foo in this example. It would be something like this:

(Rust playground link here)

use std::collections::HashMap;

struct MyData {
    d: f32
}

struct Foo {
    d: HashMap<String, MyData>
}

impl Foo {
    fn get(&mut self, s: &str) -> Option<&MyData> {
        if !self.d.contains_key(s) {
            self.d.insert(s.to_string(), MyData{ d: 0.0 });
        }
        self.d.get(s)
    }
}

fn main() {
    let mut foo = Foo {
        d: HashMap::new() 
    };
    
    let mut saved = Vec::new();
    
    for _i in 1..3 {
        let l = foo.get("a");
        saved.push(l);
    }
}

@Phlopsi, thank you for the link! This is intriguing but I would like this to work without resorting to RefCells. Do you think there's a fundamental problem with the design of the get(&mut self, ...) -> Option<&T> method if self in this case also owns the reference returned?

thanks for that explanation @kornel, that was very helpful. Also it is similar to what @scarfone was referring (I think). This now sounds to me like there's a fundamental problem in the design of this interface. Perhapse the get() method returns a copy/clone(?) of the object rather than a reference like it has been suggested.

HashMap::get() returns a reference, because it's a generic container that has to work with copyable and non-copyable types. But in your code you can call get().copied() to change &f32 into f32, and this will break the dependence between values and the hashmap, and make the borrow checker happy. It's also marginally faster, because on 64-bit architectures references are larger than 32-bit floats.

1 Like

A theoretical fundamental design problem I can see is, that you require exclusive access to the instance in order to call a logically immutable operation (from an outside view). However, there are valid reasons to keep it as it is. You have to know what you need the API to perform before you can decide on how to design it. Using Option::copied, as suggested above is definitely the simplest solution to your problem and if it works for you, then don't use RefCell. I'd also not necessarily return Option<T> instead of Option<&T>, as it is an optimization and you shouldn't optimize too early.

1 Like

I don't believe RefCell works here, anyway. You can't hold a reference to the interior of the HashMap and add something to the map at the same time; if you try with RefCell, it will just panic. It's fundamentally unsound because insert may reallocate the table backing the map, which would invalidate all interior references.

3 Likes

You're right about that. I totally forgot about the loop when I wrote what I did. It'd still error at the second iteration, but at runtime. It'd complain about the RefCell already having handed out a Ref<...> when it tries to get a MutRef<...> for manipulating the HashMap.

In that case, these are all the options I could think of, that'd work:

  • Implement your own hash map with a fixed capacity, that can add new keys without requiring a mutable reference; Foo::get doesn't require &mut self in that case, anymore (not recommended)
  • Instead of storing a reference to the result in the Vec, only store the key in the Vec and let the program lookup the result dynamically whenever needed; you're cashing the result, i.e. calling Foo::get multiple times may be acceptable (recommended, if cloning the result is not an option)
  • Do it like suggested above: Clone the result (recommended, if cloning is acceptable; for Copy types, it's always fine to clone them)
2 Likes

Sorry for the delay in response, however if you are still on this issue here is the soloution. You would want a HashMap<String, Rc<Data>>, for example:

See the docs on Rc:

Getting mutability if you need too will also be fine, that is what std::sync::Arc is for. you will just have to ensure only one Arc is "active" at a time. (meaning only one mutable refrence from the arc is alive at any given point). This is done at runtime other than compile time. Rc and Arc are fantastic tools of rust. Good luck!

Joel

Edit: As one more follow up, the reason this works is because you are cloning the Rc within the function instead of returning a reference to the HashMap. You then have two Rc's pointing to the same thing, with shared, immutable access without affecting the ability to modify (eg. get a &mut) to the original HashMap.

1 Like

I'd like to say thanks to everyone who replied with such helpful messages!

Ultimately I made design change that involved Foo no longer owning the struct (MyData in this post). The result was that the get() method now returns the MyData object (as opposed to returning a &MyData) and ownership moved to the callee. My design was flexible enough to allow that. Thanks again!

1 Like

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.