Retrieving a mutable reference from a HashMap<Rc<RefCell<T>>>

help
1

Hi there,

I am in the process of learning Rust and started using it for a personal project.

I have implemented a struct that has a couple of data structures referencing the same Lists:

        pub struct Store {
            lists: Vec<Rc<RefCell<List>>>,
            list_by_id_map: HashMap<Uuid, Rc<RefCell<List>>>
        }

        #[derive(Default)]
        pub struct List {
            pub id: Uuid,
            pub name: String,
            pub lists: Option<Vec<List>>,
            ...
        }

I am working in one of the methods the requires a mutable reference for a List retrieved from the HashMap. After a lot of "playing" with rustc, the best I could come with was:

            pub fn process(&mut self, action: actions::Action) -> Result<(), String> {
                match action {
                    actions::Action::AddList{parent_id, list, index} => 
                            ...
                            let mut parent = (self.list_by_id_map.get(&parent_id)
                                                                 .ok_or("Parent not found!")? as &RefCell<List>)
                                                                 .borrow_mut();
                            if parent.lists.is_none() {
                                parent.lists = Some(vec![*list])
                            }
                            ...
                        },
                }
                Ok(())
            }

My question is that if there is a better/more idiomatic way of retrieving a mutable reference from the HashMap when using a Rc<RefCell<T>>. I had to force the casting to &RefCell<List> and I couldn't find other examples that required doing it.

Thanks!

2

Are you sure you need Rc in the first place? One way around that is to use indicies, that refer into the Vec.

pub struct Store {
    lists: Vec<List>,
    list_by_id_map: HashMap<Uuid, usize>
}

Of course if you remove items from the Vec, these indices could be invalidated, in that case you have a few options

  1. use slab
    • This allows you to remove values at indices without invalidating other indices
  2. use generic-arena
    • This allows you to remove values at indices without invalidating other indices, and guarantees that old indices can't be reused.

And you can just swap the Vec out for one of these.

3

Hi @RustyYato, thanks for your reply.

The thing is that List is a recursive type that contains other (sub)Lists. The list_by_id_map will contain references for all Lists, not only for the "root" ones that are directly contained in the Vec<List> (Store.lists). Storing only the indices wouldn't work for this scenario.

I will look into slab and generic-arena. Thanks for these suggestions.

4

Is the cast required? It seems like you can remove it at least in the code you posted: Rust Playground

Note that parent has type RefMut<'a, List>, not &mut List. Here 'a is tied to an immutable borrow of list_by_id_map, so you won't be able to modify the map as long as parent is around. To fix that you could clone the Rc first.

5

Thanks, @branpk. I wasn't aware of the Rust Playground.

The code snippet I added to my post was a simplified version and you are correct, it does work without the cast. I added the full version to the playground and I am get the same error I am getting in my dev machine when trying to remove the cast:

error[E0609]: no field `lists` on type `&mut &Rc<RefCell<List>>`
  --> src/lib.rs:34:39
   |
34 | ...                   if parent.lists.is_none() {
   |                                 ^^^^^ unknown field

It seems that instead of having a reference to List I am still getting a reference to Rc<RefCell<List>>. I don't know why this is the case.

The full version can be found here.

Thanks again for your help.

6

That's because you have a superfluous import of BorrowMut.

Since HashMap::<K, V>::get() returns an Option<&T>, the result will have type &Rc<RefCell<List>>. Now, every value has a BorrowMut blanket implementation for itself, so you are getting back a &mut &Rc<RefCell<List>>. Since the method is found, the compiler stops here, and you are calling the trait method BorrowMut::borrow_mut() instead of the inherent borrow_mut() of RefCell.

If, however, you didn't import BorrowMut, the compiler wouldn't find that trait's borrow_mut() method and it would keep looking. It would attempt to dereference once, still nothing. It would dereference once again, and then it would find the inherent method RefCell::borrow_mut(), which is what you are looking for.

Please, look at the documentation before making blind assumptions about the APIs you are using. It is very clear from the documentation of RefCell::borrow_mut() that it is an inherent method, and not a trait impl.

There are several other, stylistic issues with the code you posted:

  1.  if option.is_none() { … } else { option.unwrap() }
    is unnecessarily risking a panic and is therefore non-idiomatic. Use
    if let Some(inner) = option { … }
    instead.
  2. There's no need to borrow Uuid values – they are small (16 bytes) and trivially copiable (they don't need explicit clone()ing). The Action enum could greatly benefit from having its lifetime pollution removed, too.
  3. The boxing of Action::AddList::list is also unnecessary.
  4. Store::new() and List::new() are unnecessarily verbose. They both could just defer to the Default impl of their respective types.
  5. In general the spacing, indentation, and formatting of the code should be improved.

An improved Playground can be found here.

7

You can still do that with indicies, but it isn't very ergonomic, each List could store the id of the sublists like so:

struct List {
    pub id: Uuid,
    pub name: String,
    pub sub_lists: Vec<usize>,
}

(note: you could represent no sub-lists with an empty Vec)

Then you can store all of the lists inside Store.lists and refer to them by index. But this does require sub-lists to be inserted before parents, or you could add the children after insertion. So this may be less ergonomic than Rc. But once you have your data stored in this way, you can freely access or mutate any of the items without bothering with RefCell, and which is an ergonomic win, and it allows you to return references to the List directly, wheras with Rc you can't. You'll have to return Ref or RefMut, which is a leaky abstraction.

8

Not sure "very clear" is the right descriptor here, given that the docs don't explicitly mention the distinction at all. I think this is a pretty understandable mistake and could come up if BorrowMut were imported for unrelated reasons. Maybe the docs should mention it.

9

This sort of conflict between trait methods and inherent methods can be tricky to decipher. (It's bitten me a few times and I've had a hard time spotting it, despite having literal years of full-time Rust experience.) Only after you realize what's going on is it clear what to look for in the docs. Sometimes an “obvious” problem still requires a second pair of eyes to find, and we shouldn’t scold people for seeking them here.

10

Hi @H2CO3. Many thanks for the detailed explanation. It does make sense but personally, perhaps due to my inexperience with Rust, I don't think it's obvious and I don't think the referred documentation would really help me in this case. What I was unaware is that the compiler would use BorrowMut::borrow_mut() before trying to deference the values. Now that you explained it makes sense though.

I also appreciate the other code suggestions you offered, they are super helpful to me for learning how to write better Rust.

@RustyYato, this is a good suggestion but I will have to think about the performance impact given that retrieving a sub-list tree will require multiple lookups in the HashMap. But it might simplify other operations so I will consider it. Thanks for it.

@branpk and @mbrubeck, thanks for your replies. As I mentioned above, as someone starting with Rust I find that compiler behaviour hard to figure out by myself. Perhaps improvements to the documentation would be helpful. IMO ideally the compiler error message could also be improved to account for multiple method implementations that may be competing for priority from the compiler.