Borrow checker is rejecting a function that looks like it doesn't violate ownership rules

help
1

I wrote this method that checks if a BTreeMap has an entry, inserts a default value if it isn't there, and then returns it:

    fn select_storage(&mut self, stream_config: &StreamConfig) -> &mut Box<dyn storage::Storage> {
        let sk = StreamKey::from(&stream_config);
        if let Some(result) = self.sorted_storages.get_mut(&sk) {
            result
        } else {
            self.sorted_storages.insert(sk.clone(), self.create_storage_for_stream(&stream_config));
            self.sorted_storages.get_mut(&sk).unwrap()
        }
    }

The compilation fails with this error:

error[E0499]: cannot borrow `self.sorted_storages` as mutable more than once at a time
   --> uplink/src/base/serializer/mod.rs:309:13
    |
299 |       fn select_storage(&mut self, stream_config: &StreamConfig) -> &mut Box<dyn storage::Storage> {
    |                         - let's call the lifetime of this reference `'1`
...
306 |           if let Some(result) = self.sorted_storages.get_mut(&sk) {
    |           -                     -------------------- first mutable borrow occurs here
    |  _________|
    | |
307 | |             result
308 | |         } else {
309 | |             self.sorted_storages.insert(sk.clone(), self.create_storage_for_stream(&stream_config));
    | |             ^^^^^^^^^^^^^^^^^^^^ second mutable borrow occurs here
310 | |             self.sorted_storages.get_mut(&sk).unwrap()
311 | |         }
    | |_________- returning this value requires that `self.sorted_storages` is borrowed for `'1`

Even though I'm not using the result of the get_mut call in the else branch. Shouldn't the lifetime of that borrow be limited to the if let branch? How do I make it work?

2

This false conflict occurs because the borrow checker is bad at reasoning about conditionally returned borrows.

To avoid this problem and make your function more efficient too, use BTreeMap::entry() to replace both get_mut() and insert().

4 Likes
3

Unfortunately they're using this to avoid cloning the key when the entry is occupied, so entry isn't optimal either. And it doesn't look like BTreeMap has any public interface to get this done in one lookup and fewest clones.

1 Like
4

RUSTFLAGS='-Zpolonius' accepts the code as is. [1]

  1. I think a lot of us are eager for the next evolution of the borrow checker. It's been cooking for quite a while. On the other hand, I don't know what kinds of bugs it has. Using it could potentially lead to accepting unsound borrows or other problems. Looks like a few things could go wrong: Tag: NLL-polonius ↩︎

2 Likes
5

You could also try to refactor this using polonius-the-crab as a stopgap until the new borrow checker is ready.

6

I tried it, but could not get it working (maybe I'm just dumb). Using a macro works, though [1]:

macro_rules! select_storage {
    ($self:ident, $stream_config:expr) => {{
        let sk = $crate::StreamKey::from($stream_config);
        if let Some(result) = $self.sorted_storages.get_mut(&sk) {
            result
        } else {
            $self
                .sorted_storages
                .insert(sk.clone(), $self.create_storage_for_stream($stream_config));
            $self.sorted_storages.get_mut(&sk).unwrap()
        }
    }};
}

Replace instances of foo.select_storage(&stream_config) with select_storage!(foo, &stream_config)

  1. This is called out as an alternative in polonius-the-crab docs. ↩︎

7

How safe is "-Zpolonius" to use in production and mission critical systems?

8

Probably "don't use it", but I linked to the list of bugs in the tracker tagged NLL-polonius. You can use those reports to judge whether it meets your criteria.

9

Still not one lookup though.

10

That can be done with the raw entry API (nightly or hashbrown only).

1 Like
11

No, the key is being constructed on the first line of the function, and the clone is needed for following insert() by get_mut().