Closed parasyte closed 1 year ago
Hey, thanks for checking this out! MIRI is serious business, we should fix these issues and add it to our CI pipeline.
Similar to https://github.com/orlp/slotmap/issues/92#issuecomment-1487426290, using MIRIFLAGS='-Zmiri-tree-borrows'
does not report any issues!
Noticed this because of the linked issue
It looks like this crate is very similar to slab
which implements .get2_mut()
by using .split_at_mut()
on the underlying Vec
to get two mutable slices that each contain one of the desired elements. Then you can call .get_mut()
on each of the slices to get a mutable reference
https://docs.rs/slab/latest/src/slab/lib.rs.html#751-773
Do you think the same technique would work here? It allows you to avoid using unsafe
Looks like you can. I opened #42
Thanks for the investigation and PR!
In general, I'm happy to reduce unsafety in crates when there's an easy replacement. In this case though, I'm a little concerned — this safe alternative has a lot more code! It's hard to make sure that all of these branches are tested and work correctly when compared with the original code.
Based on https://github.com/orlp/slotmap/issues/92#issuecomment-1366857727, I'm not sure we should change the implementation of this method. It seems like there might be a compelling argument that MIRI is raising a false alarm here. I am not familiar enough with the current status of UB in Rust to know for sure.
The main part that has a lot of explicit branches is when the two indices occupy the same slot. I was mostly trying to avoid duplicate checks, but they could probably just use .get_mut()
and the compiler should be smart enough to remove the duplicate checks. That should reduce a lot of the near-duplicate logic
I was mostly just making this change to reduce unsafe usage, not because I'm worried about aliasing issues here (which the compiler doesn't have a firm stance on yet). slab
seems to be largely the same as this library aside from the generational indices and manages to avoid any unsafe aside from a couple of unsafe methods they expose for comparison
Went ahead and pushed changes to consolidate a lot of the branches
Hi! Evaluating some crates and decided to check out
thunderdome
. It looked pretty good until I tried running the test suite under miri, which reports Undefined Behavior:FWIW, asan does not report any issues when running the test suite.