Handling multiple mutable borrows (but to different struct fields)

Hello folks --

I have a simplified tree node data structure with a few fields and a few methods such as:

use std::collections::hash_map;

pub struct Node {
  metadata : Option<String>,
  value : Option<String>,
  edges: HashMap<i32, Node>,
}

impl Node {
    pub(crate) fn value_mut(&mut self) -> Option<&mut String> {
        self.value.as_deref_mut()
    }

    pub(crate) fn edges_values_mut(&mut self) -> ValuesMut<'a, i32, Node> {
        self.edges.values_mut()
    }
}

and I want to create an IterMut iterator to cycle through values in the tree

pub struct IterMut<'a> {
    stack: Vec<&'a mut Node>
}

impl Iterator for IterMut {
    type Item = &'a mut String;
    
    fn next(&mut self) -> Option<Self::Item> {
      match self.stack.pop() {
         None => return None;
         Some(n) => {
           self.stack.extend(n.edges_values_mut())
           -------------------------
           |
           first mutable borrow occurs here

           return Some(n.value_mut());
                      ^^^^^^^^^^^^^ second mutable borrow occurs here
         }
      }
    }
}

I'm unable to do this without getting compiler errors about multiple mutable borrows which, understandably, follows the exclusive writer rule.

error[E0499]: cannot borrow *n as mutable more than once at a time

One of the ways around this that I've used is changing the pub struct Node's visibility access specifiers for fields edges and value to pub(crate). Since Rust knows that struct fields are separate and that potentially writing to one won't affect another field, it allows mutably accessing these as shown below:

pub struct Node {
  pub(crate) value : Option<String>,
  pub(crate) edges: HashMap<i32, Node>,
...
}

...
            Some(n) => {
              //updated lines
              let edges = &mut n.edges;
              let v = &mut n.value;

              self.stack.extend(edges.values_mut())
                 //updated statement
                 return Some(v.as_deref_mut());
               }
            }

This works BUT it feels a bit like a hack and it compromises the encapsulation of Node by making some of its fields pub(crate)

Is there a better way to do this? I had also considered moving the Iter module underneath Node to be able to freely access the Node struct fields without having to change the access modifier fields.

Would appreciate any idiomatic suggestions on how to get around this concern properly

Cleaned up problem case.

And with the fix (effectively):

             Some(n) => {
-                self.stack.extend(n.edges_values_mut());
+                self.stack.extend(n.edges.values_mut());
-                n.value_mut()
+                n.value.as_mut()
             }

This problematic pattern does come up more generally; see this article on the topic. However, for this particular use case, I think the change is the correct fix (with perhaps some more tuning to avoid pub(crate)). That is, I think it's reasonable that the "built-in" iterator of a type has access to the fields of the type, in Rust.

More generally, while getters and setters have their place in Rust (for encapsulation / to provide flexibility of implementation), that place is usually not within a structs own implementation, due to interprocedural conflicts and other borrow-check/ownership related barriers. I would argue that the built-in iterator also falls into that category.

As such, I would put the iterator definition in the same module as the Node definition. I feel this is pretty common (e.g. HashMap, though I didn't check to see how much internal information it makes use of).

2 Likes

I think that encapsulation is already broken at the point where you decide you want value_mut and edges_values_mut to be callable concurrently. That is the decision that means you cannot treat Node as a unit, anymore; you have to consider it as (at least) two distinct and separable things. In other words: fields.

That doesn't mean you have to expose the fields fully; you could, for instance, wrap value and edges in opaque newtypes and only expose a very minimal API for each of them. But you can't pretend to not expose their existence at all; what you want to do is exactly breaking encapsulation.

1 Like

I would agree, but the issue is the real iter module is long hence the use of modules, as is HashMap with its Iter functionality combined within source...

I think you raise a fair point but I think there should be a reasonable pattern to handle a pretty generic case without over-fragmenting general fields. So would edges and value be in a sub struct within Node? Specifics as applied to this code perhaps?

Enjoying reading the replies, looking for some sort of middle ground...

You could have submodules with pub(super) types, re-exported to the parent mod.

Did you read the linked article? It covers various workarounds like having a substruct. It also covers view types, which could be used in a multivalued accessor fashion.

Some sort of ad-hoc view type / place expression is a (far) future possibility.

The baby steps article was informative. It's just not clear how to apply it in this simplified code setup as its a little different than the event signal refactoring.. Even if it was a sub struct it would still have a pub crate access specifier on it.

The view approach doesn't fully make sense to me and feels like it adds a lot more complexity.

Hence the request for a code example(s) specifics given the sample code...

If the iter functionality was included within node then there's no discussion left.

I will probably stick to the sub module idea as originally presented. Was just looking for other potential solutions. (Using my phone now with limited screen awesome ness)

Ah, sure.

This is sick!

Thank you immensely for going out of your way to unpack these ideas!! :pray::pray::pray:

Seeing the code has explained everything and cleared up my original confusion.

I'm smitten with the view approach as it keeps the original Node struct intact (avoiding any extra fragmentation concerns) while providing an auxiliary structure, when needed, to assist with the mutable iteration while avoiding the pub crate hack because we have safe mutable reborrows of those fields!

How beautiful :heart_eyes: