Mutable borrow of structs inside BTreeMap

help
1

Hello,

I'm pretty new to Rust, mainly used to OOP development in higher-level languages like PHP, Python, etc, so I haven't had to deal with ownership and borrows before. My project is a controller for some Neopixel LEDs driven by an RP2040 microcontroller that knows where each LED is in a 2D grid to display effects, but I think this question is fundamental to how Rust works, not specifically about embedded systems. Here, I'm trying to simulate spotlights by lighting up all the LEDs in a given circle. The spotlights randomly appear, fade in, then move around, and eventually stop moving and fade out.

Here is one version of my code:

pub fn tick(&mut self, draw_helper: &mut Draw, tick_count: usize) {
        for (id, light) in self.spotlights.iter_mut() {
            // Move the light and update its status
            if matches!(light.mode, SpotlightMode::FADE_IN) {
                // Do stuff
            } else if matches!(light.mode, SpotlightMode::ACTIVE) {
                // Do stuff
            } else if matches!(light.mode, SpotlightMode::STOP) {
                // Do stuff
            }
            
            // Draw the light
        }

self.spotlights is a BTreeMap of random IDs and a struct that holds the light's state, location, colour, etc. I'd use a HashSet, but it looks like this isn't available in embedded-alloc, and I can't guarantee that every light is comparable to put them in a BTreeSet, so I used the map instead.

This code compiles and runs, but there is a bug: the spotlights seem to disappear when the state changes from FADE_IN to ACTIVE, and I want to write unit tests to test this code on my PC. So I tried to refactor this tick method to split out the draw function and each of the blocks I've marked as // Do stuff. But this means I need a mutable borrow of the SpotlightStatus (light inside the for loop). The compiler is happy with me modifying the values inside that struct, but not having a second mutable borrow of the struct itself while inside that iterator:

error[E0499]: cannot borrow `*self` as mutable more than once at a time
   --> src/scene/spotlights.rs:172:13
    |
171 |         for (id, light) in self.spotlights.iter_mut() {
    |                            --------------------------
    |                            |
    |                            first mutable borrow occurs here
    |                            first borrow later used here
172 |             self.handle_light(light);
    |             ^^^^ second mutable borrow occurs here

error[E0502]: cannot borrow `*self` as immutable because it is also borrowed as mutable
   --> src/scene/spotlights.rs:173:13
    |
171 |         for (id, light) in self.spotlights.iter_mut() {
    |                            --------------------------
    |                            |
    |                            mutable borrow occurs here
    |                            mutable borrow later used here
172 |             self.handle_light(light);
173 |             self.draw_light(*id, light, draw_helper);
    |             ^^^^ immutable borrow occurs here

I tried this both by defining methods on the controller (as here), and defining methods on the SpotlightStatus struct (a more OOP style, but not great here because only the controller has an instance of tinyrand available).

So, in either a general sense or in this specific case, is there a way to pass a mutable borrow of structs in a collection when iterating through that collection? I understand the problems you can create when doing this, e.g. by deleting the object, or changing it so that the collection is affected, like changing ordering of a tree. Is there a workaround to make it safe, or do I need a fundamentally different approach? I really do want to avoid having everything in a single function, though, so I can unit test it.

Suggestions of better collections are also welcome. I'm expecting only around 0-9 objects active at once, I don't care about order, and they need to be created and destroyed randomly.

Thanks

2

It's hard to say what is best without seeing a complete example of both your data structures and the code that accesses them, but as a general principle here, the usual beginner mistake is to make more things methods than should be. Write a function which takes the borrows it needs and not the ones it doesn't. Then if one of those borrows can be the function's &self or &mut self, make it so, but don't add an over-broad borrow you don't actually need. Think of borrows first and methods second.

In particular, you say "only the controller has an instance of tinyrand available" - so pass a borrow of it, and one of SpotlightStatus, and nothing else until you know what else you need.

If that doesn't help, then try to condense your problem to an example we can run on the Rust Playground. It doesn't have to have any actual IO but it has to have the borrows that you need for that IO.

2 Likes
3

First of all, have you heard of match?

match light.mode {
    SpotlightMode::FADE_IN => {
        // Do stuff
    }
    SpotlightMode::ACTIVE => {
        // Do stuff
    }
    SpotlightMode::STOP => {
        // Do stuff
    }
}

For the actual question, there's mainly two ways to solve this. Either copy the data out of self so you can pass self without problems:

let ids: Vec<_> = self.spotlights.keys().cloned().collect();
for id in ids {
    self.handle_light(id); // this is now fine
}

Or don't use a method on self, but instead pass only the fields you need.

for (id, light) in self.spotlights.iter_mut() {
    Self::handle_light(&mut self.otherfield, id);
}

Also you should be able to use HashSet, but you need to use a different hasher.

4

Don't feel bad at all about this, it's a fundamentally tough issue that Rust throws in your face a lot more than it might theoretically be required to, but for a generally good reason. It helps to consider what Rust is actually upset about here: for all it knows, handle_light might call self.spotlights.clear() and invalidate the for loop, leading to, if you're lucky, a crash! Nearly all the time, there's a clearly bad thing that you can translate these errors to like that.

The simplest way to avoid issues is that if everything is a shared borrow &, instead of exclusive &mut then you're good.

In this case, you presumably want to update the lights in handle_light which is why you have the exclusive borrow, the easy answer is to put the updated state inside a Cell or RefCell which lets you mutate through a shared borrow by restricting the actions you can take or checking for conflicts at runtime, respectively.

Alternative answers need a bit more work, but they generally look like separating the thing that's getting changed from the information it needs. This can look like splitting out the spotlights from context information, then the container can just pass one to the other; it can look like splitting computing the update to apply from actually applying it, it can look like double buffering, where you read from the current state and write to the next state, then swap.

3 Likes
5

Thanks, I think RefCell worked - at least, it compiles and runs with the same bug as before. Now I can get into the unit testing and try to fix that. I'll mark the answer as the solution once I've managed to check that out properly. I remember RefCell from the Rust Book, but I think when a feature doesn't have an equivalent in languages I've used before, it's easy to forget it.

Thanks for the explanation, too. I understand exactly why the compiler has to be strict, and in this case I needed something to tell it "no, this particular thing is OK".

6

Ha, good point on match. I think I went from "switch doesn't exist, use multiple ifs", to "How do I check the type of an Enum? Oh, I use matches!" and forgot about the match structure. Although, I had been getting into my head that it's only used for returning a single value, more like C's ternary operator, so thanks for the correction on that, too.

1 Like