After completing a Rust exercise (the "React" exercise on the Exercism site, in case it's relevant) the code passed all of the unit tests, but relied on large methods, each responsible for several things.
A much (much) simplified version of one such working method is this:
pub fn working_set_value(&'a mut self, id: InputCellID) {
let original_values: HashMap<&'a ComputeCellID, Option<T>> = HashMap::new();
let end_values: HashMap<&'a ComputeCellID, Option<T>> = HashMap::new();
for (cell_id, original_value) in original_values.iter() {
let end_value = end_values.get(cell_id).unwrap();
if end_value != original_value && end_value.is_some() {
// Fire callbacks for current cell_id.
}
}
}
This method is a lot of code (in its unsimplified form), and is responsible for two things: firstly updating cell values and storing the before/after values into two maps, and secondly iterating those maps and firing cell callbacks for all cells which have changed.
My attempt to refactor, so that this large method is split into smaller methods (each responsible for just the one thing) looks like this (again, this is a greatly simplified version):
pub fn broken_set_value(&'a mut self, id: InputCellID) {
let (original_value_map, end_value_map) = self.update_listeners(&id);
self.fire_callbacks(original_value_map, end_value_map);
}
fn update_listeners(
&'a mut self,
_id: &InputCellID,
) -> (
HashMap<&'a ComputeCellID, Option<T>>,
HashMap<&'a ComputeCellID, Option<T>>,
) {
let original_values = HashMap::new();
let end_values = HashMap::new();
(original_values, end_values)
}
fn fire_callbacks(
&self,
original_value_map: HashMap<&'a ComputeCellID, Option<T>>,
end_value_map: HashMap<&'a ComputeCellID, Option<T>>,
) {
for (cell_id, original_value) in original_value_map.iter() {
let end_value = end_value_map.get(cell_id).unwrap();
if end_value != original_value && end_value.is_some() {
// Fire callbacks for current cell_id.
}
}
}
This causes the compiler to give the error:
error[E0502]: cannot borrow `*self` as immutable because it is also borrowed as mutable
--> src/lib.rs:109:9
|
79 | impl<'a, T: Copy + PartialEq + Debug> Reactor<'a, T> {
| -- lifetime `'a` defined here
...
108 | let (original_value_map, end_value_map) = self.update_listeners(&id);
| --------------------------
| |
| mutable borrow occurs here
| argument requires that `*self` is borrowed for `'a`
109 | self.fire_callbacks(original_value_map, end_value_map);
| ^^^^ immutable borrow occurs here
Note that this error does not occur if the return type of update_listeners is changed so that the references &'a ComputeCellID (with or without lifetime specifiers) are replaced with owned instances: ComputeCellID. And also note that the error occurs whether or not the references are then passed to the fire_callbacks method, and even calling drop to discard the references will not stop the error being reported.
The explanation?
Is the following a correct summary of what is upsetting the Rust compiler: because the update_listeners method creates references which rely on the mutable borrowing of self, the scope (not sure that's the correct word) of the mutable borrow (made by the call to update_listeners) must be extended to the end of the set_value method. And with the scope of the mutable borrow now covering the call to fire_callbacks, it's not permitted to make any further borrowing of self.
And if owned instances were used (instead of references reliant on self), then the "scope" of the mutable borrow would end immediately after the call to update_listeners, and so we'd be free to allow fire_callbacks to make another borrow of self.
The workaround?
If that understanding is correct, then how can a big method like this be refactored into small, focused methods, each of which is responsible for just one thing? I've been impressed by Rust so far, and I suspect there must be an elegant way to do this successfully.
