Consider this code. coeff_state is a static mut item and this is a body of unsafe function:
let previous_set_item = &mut coeff_state.set[prev as usize];
let next_set_item = &mut coeff_state.set[next as usize];
This code compiles, even though it takes two mutable references to the same field (set is an array).
This code came out of c2rust and ton of refactoring and I've been slowly migrating it to safe rust, so I tried to extract the coeff_state into a variable like this:
let state = &mut coeff_state;
let previous_set_item = &mut state.set[prev as usize];
let next_set_item = &mut state.set[next as usize];
This code suddendly doesn't compile, giving you a classic borrow checker error:
error[E0499]: cannot borrow `state.set[_]` as mutable more than once at a time
--> engine/src/getcoeff.rs:421:29
|
420 | let previous_set_item = &mut state.set[prev as usize];
| ----------------------------- first mutable borrow occurs here
421 | let next_set_item = &mut state.set[next as usize];
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ second mutable borrow occurs here
422 | set_item.constant = ((weight1 * previous_set_item.constant as i32 +
| -------------------------- first borrow later used here
I understand the error, it's obviously a problem, but I don't understand why the first version compiles fine. Does rust allow taking two mutable references to static mut item?
It does, but it can only do so because taking a reference to a static mut is unsafe in the first place. Actually using those references is often unsound, as you'd expect.
To be clear, I don't think this specific instance is a result from c2rust, I already rewrote it a lot, so it's probably my fault. c2rust gets around these issues by always using raw pointers everywhere.
In the meantime, using the actual replacement of static mut here lets us choose the level of control we want:
mod lib {
pub
struct StaticMut<T> /* = */ (
Cell<T>,
);
impl<T> StaticMut<T> {
pub
const
fn new (value: T)
-> StaticMut<T>
{ ... }
/// # Safety
/// There must not be any parallel accesses during `'uniq`.
pub
unsafe
fn assume_unique_thread<'uniq> (self: &'uniq StaticMut<T>)
-> &'uniq Cell<T>
{
&self.0
}
}
unsafe // Safety: No non-unsafe API
impl<T : Sync> Sync for StaticMut<T>
{}
}
use lib::StaticMut;
struct ThingWithSet {
set: [i32; 42],
// ...
}
static coeff_state: StaticMut<ThingWithSet> = { ... };
// ...
use ::cell_project::cell_project as cp; // renamed for ergonomics
let state: &Cell< ThingWithSet > = unsafe { coeff_state.assume_unique_thread() };
let at_set: &Cell<[i32]> = cp!(ThingWithSet, state.set);
let at_set: &[Cell<i32>] = at_set.as_slice_of_cells();
let previous_set_item = &at_set[prev as usize];
let next_set_item = &at_set[next as usize];
and you then use .get() and .set() to mutate those fields.
You still get references and checks (compared to using raw pointers), but you no longer have to worry about &mut-aliasing. This is of course only safe if operating from within a single thread each time (which obviously was also a requirement of static mut + raw pointers).
If you know there is no multi-threading in your code whatsoever, you can go full non-unsafe by using a thread_local! { static ... }.
Thanks for the detailed answer, but in my case I think the best thing to do is just get rid of the globals completely, which I've been trying to do for some time already (I got from 300 to 16 globals in last few weeks). There's no reason for these to be globals, it's just very oldschool C code.
let previous_set_item = &mut state.set[prev as usize];
let next_set_item = &mut state.set[next as usize];
This is OK if prev != next (and in case of static mut, if there are no other threads touching it), but the borrow checker is unable to prove that. It works based on interfaces, not values or symbolic execution, so it sees only calls to std::ops::IndexMut::index_mut that takes &mut self, which borrows all of state.set exclusively, twice.
If you need to borrow multiple elements from the same vec/array, there's split_at_mut (or just use unsafe).
What I find weird is that while this syntax is clearly not enough to satisfy borrow checker on its own, somehow just state being static mut enables it to compile. Compare that with split_at_mut which has to go through raw pointers to compile. It seems like a unexpected footgun to me.
let previous_set_item = &mut state.set[prev as usize];
let next_set_item = &mut state.set[next as usize];
Well technically no. To perform the indexing you must first create a mutable reference to the entire array, which would overlap with the &mut you already have to a specific element.
No, I expect what it's doing is treating each mention of state as a different place.
So I suspect this would also compile:
let previous_set_item = &mut state.set[0];
let next_set_item = &mut state.set[0]; // UB because overlapping `&mut`s
use(previous_set_item, next_set_item); // keep the borrows alive
True. Still feels like something that should be caught, because it would be if it were a local. But maybe it'd be better as a "we looked and the liveness range of the references and they overlap" deny-by-default lint than as a different MIR desugaring that could hard-error.
I'd definitely expect this to be the case - ie. treat references to globals the same way as references to locals but only across single function scope.
Still, I am not sure if I should open an issue about this. It seems like it's intentional choice - even very obvious &mut aliasing violations still compile eg. function calls like these foo(&mut state, &mut state.set).
One consideration is that a function-local lint might mislead people into thinking the cross-function version is okay, because the lint stops firing.
Once you've reached for static mut and unsafe, you should already be thinking hard about aliasing and reentrancy- and the lint would only catch the easy cases.
Further, the cross-function version often is okay in C and C++, and is okay in even more cases when you consider "UB but happens to work because the optimizer doesn't see it because it's cross-function" cases. So the lint might wind up flagging exactly the cases a C compiler would miscompile, and ignoring exactly the cases a C compiler would not break but that Rust might miscompile.
(I don't think this is actually an argument against adding the lint, just something to consider in its design.)
