Why does refactoring by extracting a method trigge

Question

The architecture of my networking application can be stripped down to the following:

use std::collections::HashMap;

/// Represents remote user. Usually has fields,
/// but we omit them for the sake of example.
struct User;

impl User {
    /// Send data to remote user.
    fn send(&mut self, data: &str) {
        println!("Sending data to user: \"{}\"", data);
    }
}

/// A service that handles user data.
/// Usually has non-trivial internal state, but we omit it here.
struct UserHandler {
    users: HashMap<i32, User>,  // Maps user id to User objects.
    counter: i32  // Represents internal state
}

impl UserHandler {
    fn handle_data(&mut self, user_id: i32, data: &str) {
        if let Some(user) = self.users.get_mut(&user_id) {
            user.send("Message received!");
            self.counter += 1;
        }
    }
}

fn main() {
    // Initialize UserHandler:
    let mut users = HashMap::new();
    users.insert(1, User{});
    let mut handler = UserHandler{users, counter: 0};

    // Pretend we got message from network:
    let user_id = 1;
    let user_message = "Hello, world!";
    handler.handle_data(user_id, &user_message);
}

Playground

This works OK. I would like to make a separate method in UserHandler that handles user input when we have already established that user with given id exists. So it becomes:

impl UserHandler {
    fn handle_data(&mut self, user_id: i32, data: &str) {
        if let Some(user) = self.users.get_mut(&user_id) {
            self.handle_user_data(user, data);
        }
    }

    fn handle_user_data(&mut self, user: &mut User, data: &str) {
        user.send("Message received!");
        self.counter += 1;
    }
}

Playground

All of a sudden, it doesn't compile!

error[E0499]: cannot borrow `*self` as mutable more than once at a time
  --> src/main.rs:24:13
   |
23 |         if let Some(user) = self.users.get_mut(&user_id) {
   |                             ---------- first mutable borrow occurs here
24 |             self.handle_user_data(user, data);
   |             ^^^^                  ---- first borrow later used here
   |             |
   |             second mutable borrow occurs here

At first glance, the error is pretty obvious: you can not have a mutable reference to self and to an attribute of self - it is like having two mutable references to self. But then, what the heck, I do have two mutable references like this in the original code!

1. Why does this simple refactoring trigger borrow checker error?
2. How do I work around it and decompose UserHandler::handle_data method like this?

If you wonder why I want such refactoring, consider a case when there are multiple types of messages that user could send, all need to be handled differently, but there is a common part: having to know which User object sent this message.

Answer 1

When calling self.handle_user_data, you're taking the whole self mutably while still having a mutable borrowed user object, which Borrow Checker doesn't like. You can't have two mutable borrows as the same time.

One method to circumvent this is not take whole self mutably but take the counter mutably:

impl UserHandler {
    fn handle_data(&mut self, user_id: i32, data: &str) {
        if let Some(user) = self.users.get_mut(&user_id) {
            handle_user_data(user, data, &mut self.counter);
        }
    }
}

fn handle_user_data(user: &mut User, data: &str, counter: &mut i32) {
    user.send(data);
    *counter += 1;
}

Answer 2

The compiler is right to prevent you from borrowing the HashMap twice. Suppose in handle_user_data() you also tried to borrow self.users. You would break the rules of borrowing in Rust because you already have a mutable borrow on it and you can only have one.

Since you can't borrow self twice for your handle_user_data(), I will propose a solution. I don't know if it's the best, but it works without unsafe and without overhead (I think).

The idea is to use an intermediate struct that will borrow the other fields of self:

impl UserHandler {
    fn handle_data(&mut self, user_id: i32, data: &str) {
        if let Some(user) = self.users.get_mut(&user_id) {
            Middle::new(&mut self.counter).handle_user_data(user, data);
        }
    }
}

struct Middle<'a> {
    counter: &'a mut i32,
}

impl<'a> Middle<'a> {
    fn new(counter: &'a mut i32) -> Self {
        Self {
            counter
        }
    }

    fn handle_user_data(&mut self, user: &mut User, data: &str) {
        user.send("Message received!");
        *self.counter += 1;
    }
}

This way, the compiler knows we can't borrow users twice.

If you have only one or two things to borrow, a quick solution is to have an associated function that takes them as parameters:

impl UserHandler {
    fn handle_user_data(user: &mut User, data: &str, counter: &mut i32) {
        // ...
    }
}

We can improve this design:

struct UserHandler {
    users: HashMap<i32, User>, // Maps user id to User objects.
    middle: Middle,              // Represents internal state
}

impl UserHandler {
    fn handle_data(&mut self, user_id: i32, data: &str) {
        if let Some(user) = self.users.get_mut(&user_id) {
            self.middle.handle_user_data(user, data);
        }
    }
}

struct Middle {
    counter: i32,
}

impl Middle {
    fn new(counter: i32) -> Self {
        Self {
            counter
        }
    }

    fn handle_user_data(&mut self, user: &mut User, data: &str) {
        user.send("Message received!");
        self.counter += 1;
    }
}

Now we are sure we don't have overhead and the syntax is much cleaner.