Better way to avoid double mutable borrow?

code review
1

Data structure:

   GuiState {
   ...
       menu_group: Option<MenuGroup>,
   }

This, in a member function of GuiState, is a prohibited double mutable borrow, and won't compile, of course:

    if let Some(menu_group) = &mut self.menu_group_opt {
        menu_group.draw(self)
    }

This works, but it's ugly.

    // take ownership of menu group temporarily to avoid double mutable borrow
    let taken_menu_group = self.menu_group_opt.take(); 
    if let Some(mut menu_group) = taken_menu_group {
        menu_group.draw(self);
        self.menu_group_opt = Some(menu_group); // put it back
    }

Is this bad form? Would it be better to use an Rc to allow interior mutability? Something else?

2

It’s not always possible, but the ideal solution is to separate out the parts of GuiState that MenuGroup::draw needs, and pass a reference to that instead of the entire state:

struct GuiState {
    canvas: Canvas,
    menu_group: Option<MenuGroup>,
    …
}

impl GuiState {
    fn draw(&mut self) {
        …
        if let Some(menu_group) = &mut self.menu_group_opt {
            menu_group.draw(&mut self.canvas)
        }
        …
    }
}
3

The Rc solution works, but it's ugly:

pub type MenuGroupLink = Rc<RefCell<Box<dyn MenuGroup>>>;
struct GuiState {
    menu_group: MenuGroupLink,
}

  ....
if self.menu_group_opt.is_some() {
    let menu_group = Rc::clone(self.menu_group_opt.as_ref().unwrap());
    menu_group.borrow_mut().draw(self);
}

Having to use is_some() and unwrap() is painful. But I have to let go of self before calling draw with it, or I get a double mutable borrow. So if let doesn't work here. Suggestions?

4

Would putting the interior mutability inside GuiState work better for you?

struct GuiState {
    menu_group: Option<RefCell<MenuGroup>>,
    … // other cell-based fields 
}


impl GuiState {
    fn draw(&self) {
        …
        if let Some(menu_group) = &self.menu_group {
            menu_group.borrow_mut().draw(self);
        }
        …
    }
}
5

Can't do that. self is double-borrowed.

error[E0502]: cannot borrow `*self` as mutable because it is also borrowed as immutable
   --> src/guistate.rs:174:13
    |
173 |         if let Some(menu_group) = &self.menu_group_opt {
    |                                   -------------------- immutable borrow occurs here
174 |             menu_group.borrow_mut().draw(self);
    |             -----------------------^^^^^^^^^^^- ... and the immutable borrow might be used here, when that temporary is dropped and runs the destructor for type `RefMut<'_, dyn MenuGroup>`
    |             |
    |             mutable borrow occurs here
    |             a temporary with access to the immutable borrow is created here ...
6

MenuGroup.draw needs to be modified to take &GuiState, and then borrow the internal cells that it needs access to.

The general idea is that, per your architecture, you can’t have exclusive access to GuiState, so instead of &mut, you can use & in most places and let RefCell verify your field accesses at runtime.

7

Right. Got that. Current question is, can I write

    if self.menu_group_opt.is_some() {
        let menu_group = Rc::clone(self.menu_group_opt.as_ref().unwrap());
        inuse |= menu_group.borrow_mut().draw(self);
    }

without is_some() and unwrap()? The first line borrows self and lets it go. The second line borrows self and lets it go. So, the third line can pass self. Trying to do this with if let or match results in a double borrow, because of the way the scopes nest. Is there a way to express exactly this in a more tidy manner?

8

If self is a shared reference &GuiState, then the double borrow shouldn’t be a problem: You can have as many shared borrows as you like to the same object without issue.

9

Yes, I know I can redesign it that way. The question is, is there a way to write the above three lines without is_some() and unwrap(). That code works fine; it's just ugly.

10

You could try the Cow type.

11

Why? I think it's perfectly fine. You can also avoid relying on pattern binding modes ("match ergonomics") by explicitly asking for an optional-over-the-inner-ref using as_mut():

if let Some(menu_group) = self.menu_group_opt.as_mut() {
    menu_group.draw(self)
}
12

What I finally ended up doing was eliminating the Option. I created a menu group that doesn't do anything to put in when "None" was previously used. So, the code now reads.

    let menu_group = Rc::clone(&self.menu_group);
    menu_group.borrow_mut().draw(self);
13

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.