Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix stacked-borrows violations in Vector #83

Merged
merged 1 commit into from
Jan 2, 2025
Merged

Fix stacked-borrows violations in Vector #83

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/ser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,26 +246,31 @@ mod test {
use serde_json::{from_str, to_string};

proptest! {
#[cfg_attr(miri, ignore)]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain in a comment somewhere the criterion for excluding a test from miri? Also, if you want to exclude a whole mod then maybe just #[cfg(not(miri))] at the top is easier instead of annotating every test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic behind not using cfg(not(miri)) is that it causes a bunch of 'unused import' warnings, and with ignore it's possible to use --ignored to run it anyways, if you really want to try waiting for it to complete.

I'll add some documentation when I get a minute. For posterity, it's basically just 'takes more than ~30 seconds to run', and some tests may actually prefer running but with reduced iteration values. I just didn't want to take the time to track down why the specific iteration numbers were chosen for many of the issue tests, to find out whether they could be reduced and still have the test be meaningful.

#[test]
fn ser_ordset(ref v in ord_set(i32::ANY, 0..100)) {
assert_eq!(v, &from_str::<OrdSet<i32>>(&to_string(&v).unwrap()).unwrap());
}

#[cfg_attr(miri, ignore)]
#[test]
fn ser_ordmap(ref v in ord_map(i32::ANY, i32::ANY, 0..100)) {
assert_eq!(v, &from_str::<OrdMap<i32, i32>>(&to_string(&v).unwrap()).unwrap());
}

#[cfg_attr(miri, ignore)]
#[test]
fn ser_hashmap(ref v in hash_map(i32::ANY, i32::ANY, 0..100)) {
assert_eq!(v, &from_str::<HashMap<i32, i32>>(&to_string(&v).unwrap()).unwrap());
}

#[cfg_attr(miri, ignore)]
#[test]
fn ser_hashset(ref v in hash_set(i32::ANY, 0..100)) {
assert_eq!(v, &from_str::<HashSet<i32>>(&to_string(&v).unwrap()).unwrap());
}

#[cfg_attr(miri, ignore)]
#[test]
fn ser_vector(ref v in vector(i32::ANY, 0..100)) {
assert_eq!(v, &from_str::<Vector<i32>>(&to_string(&v).unwrap()).unwrap());
Expand Down
3 changes: 2 additions & 1 deletion src/tests/vector.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ fn cap_index(len: usize, index: usize) -> usize {
}

proptest! {
#[cfg_attr(miri, ignore)]
#[test]
fn comprehensive(actions: Actions<u8>) {
let mut vec = Vector::new();
Expand Down Expand Up @@ -220,7 +221,7 @@ proptest! {

#[test]
fn test_inserts() {
const N: usize = 2000;
const N: usize = if cfg!(miri) { 100 } else { 2000 };
let mut v = Vector::new();
for i in 0..N {
v.insert(v.len() / 2, i);
Expand Down
73 changes: 64 additions & 9 deletions src/vector/focus.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,11 @@
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at http://mozilla.org/MPL/2.0/.

use std::mem::{replace, swap};
use std::mem::{replace, swap, MaybeUninit};
use std::ops::{Range, RangeBounds};
use std::ptr::null;
use std::sync::atomic::{AtomicPtr, Ordering};
use std::{mem, ptr};

use crate::nodes::chunk::Chunk;
use crate::sync::Lock;
Expand All @@ -16,6 +17,18 @@ use crate::vector::{
RRB,
};

fn check_indices<const N: usize>(len: usize, indices: &[usize; N]) -> Option<()> {
let mut seen = [None; N];
for idx in indices {
if *idx > len || seen.contains(&Some(*idx)) {
return None;
}
let empty = seen.iter_mut().find(|a| a.is_none()).unwrap();
*empty = Some(*idx);
}
Some(())
}

/// Focused indexing over a [`Vector`][Vector].
///
/// By remembering the last tree node accessed through an index lookup and the
Expand Down Expand Up @@ -642,6 +655,22 @@ where
}
}

fn get_many_mut<const N: usize>(&mut self, indices: [usize; N]) -> Option<[&mut A; N]> {
check_indices(self.len(), &indices)?;
match self {
FocusMut::Single(_, chunk) => {
// FIXME: Stable polyfill for std `get_many_mut`
let chunk: *mut A = ptr::from_mut(*chunk).cast();
let mut arr = [const { MaybeUninit::uninit() }; N];
for idx in 0..N {
arr[idx].write(unsafe { &mut *chunk.add(indices[idx]) });
}
unsafe { mem::transmute_copy(&arr) }
}
FocusMut::Full(pool, tree) => tree.get_many(pool, indices),
}
}

/// Get a reference to the value at a given index.
///
/// Panics if the index is out of bounds.
Expand All @@ -657,6 +686,11 @@ where
self.get_mut(index).expect("index out of bounds")
}

pub fn index_many_mut<const N: usize>(&mut self, indices: [usize; N]) -> [&mut A; N] {
self.get_many_mut(indices)
.expect("index out of bounds or overlapping")
}

/// Update the value at a given index.
///
/// Returns `None` if the index is out of bounds, or the replaced value
Expand Down Expand Up @@ -701,9 +735,8 @@ where
if a == b {
panic!("vector::FocusMut::pair: indices cannot be equal!");
}
let pa: *mut A = self.index_mut(a);
let pb: *mut A = self.index_mut(b);
unsafe { f(&mut *pa, &mut *pb) }
let [pa, pb] = self.index_many_mut([a, b]);
f(pa, pb)
}

/// Lookup three indices simultaneously and run a function over them.
Expand All @@ -730,10 +763,8 @@ where
if a == b || b == c || a == c {
panic!("vector::FocusMut::triplet: indices cannot be equal!");
}
let pa: *mut A = self.index_mut(a);
let pb: *mut A = self.index_mut(b);
let pc: *mut A = self.index_mut(c);
unsafe { f(&mut *pa, &mut *pb, &mut *pc) }
let [pa, pb, pc] = self.index_many_mut([a, b, c]);
f(pa, pb, pc)
}

/// Get the chunk for the given index.
Expand Down Expand Up @@ -870,6 +901,10 @@ where
fn get_focus(&mut self) -> &mut Chunk<A> {
unsafe { &mut *self.target_ptr.load(Ordering::Relaxed) }
}

fn get_focus_ptr(&mut self) -> *mut Chunk<A> {
self.target_ptr.load(Ordering::Relaxed)
}
}

impl<'a, A> TreeFocusMut<'a, A>
Expand Down Expand Up @@ -936,6 +971,26 @@ where
Some(&mut self.get_focus()[target_phys_index])
}

pub fn get_many<const N: usize>(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe call this get_many_mut to match the other one? Also, does it need to be pub? If so, I think it needs to be documented. If not, maybe you could even make all the internal functions unsafe and skip the index-checking. Because internally we're already checking indices in, e.g. triplet

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I skipped the mut because the above get method also skips it, despite returning a mutable reference. In terms of needing to be pub, it doesn't really need to be, though it may be useful to users. To match the std pattern of such APIs, if it stays public, there should likely be an unsafe variant that skips the checks, then triplet can use it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks. I don't have a strong opinion about whether it should be pub or not, just that if it's pub then its documentation should be up to par with the documentation on other pub methods.

&mut self,
pool: &RRBPool<A>,
indices: [usize; N],
) -> Option<[&mut A; N]> {
check_indices(self.len(), &indices)?;
let mut arr = [const { MaybeUninit::uninit() }; N];
for idx in 0..N {
let phys_idx = self.physical_index(indices[idx]);
if !contains(&self.target_range, &phys_idx) {
self.set_focus(pool, phys_idx);
}
let target_idx = phys_idx - self.target_range.start;
let chunk = self.get_focus_ptr();
let ptr = unsafe { Chunk::as_mut_slice_ptr(chunk) };
arr[idx].write(unsafe { &mut *ptr.cast::<A>().add(target_idx) });
}
unsafe { mem::transmute_copy(&arr) }
}

/// Gets the chunk for an index as a slice and its corresponding range within the TreeFocusMut.
pub fn get_chunk(&mut self, pool: &RRBPool<A>, index: usize) -> (Range<usize>, &mut [A]) {
let phys_index = self.physical_index(index);
Expand All @@ -953,7 +1008,7 @@ where
let phys_range = (self.target_range.start + left)..(self.target_range.end - right);
let log_range = self.logical_range(&phys_range);
let slice_len = self.get_focus().len();
let slice = &mut (self.get_focus().as_mut_slice())[left..(slice_len - right)];
let slice = &mut self.get_focus().as_mut_slice()[left..(slice_len - right)];
(log_range, slice)
}
}
33 changes: 32 additions & 1 deletion src/vector/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2412,6 +2412,7 @@ mod test {
assert_eq!(0, vec[0]);
}

#[cfg_attr(miri, ignore)]
#[test]
fn large_vector_focus() {
let input = Vector::from_iter(0..100_000);
Expand All @@ -2425,6 +2426,7 @@ mod test {
assert_eq!(expected, sum);
}

#[cfg_attr(miri, ignore)]
#[test]
fn large_vector_focus_mut() {
let input = Vector::from_iter(0..100_000);
Expand All @@ -2440,6 +2442,7 @@ mod test {
assert_eq!(expected, vec);
}

#[cfg_attr(miri, ignore)]
#[test]
fn issue_55_fwd() {
let mut l = Vector::new();
Expand All @@ -2451,6 +2454,7 @@ mod test {
assert_eq!(Some(&4096), l.get(4096));
}

#[cfg_attr(miri, ignore)]
#[test]
fn issue_55_back() {
let mut l = Vector::unit(0);
Expand Down Expand Up @@ -2515,6 +2519,7 @@ mod test {
for _ in x.iter() {}
}

#[cfg_attr(miri, ignore)]
#[test]
fn issue_67() {
let mut l = Vector::unit(4100);
Expand Down Expand Up @@ -2595,6 +2600,7 @@ mod test {
x.insert(514, 0);
}

#[cfg_attr(miri, ignore)]
#[test]
fn issue_105() {
let mut v = Vector::new();
Expand All @@ -2608,6 +2614,7 @@ mod test {
}
}

#[cfg_attr(miri, ignore)]
#[test]
fn issue_107_split_off_causes_overflow() {
let mut vec = Vector::from_iter(0..4289);
Expand All @@ -2622,6 +2629,7 @@ mod test {
}
}

#[cfg_attr(miri, ignore)]
#[test]
fn collect_crash() {
let _vector: Vector<i32> = (0..5953).collect();
Expand Down Expand Up @@ -2652,7 +2660,8 @@ mod test {

#[test]
fn ptr_eq() {
for len in 32..256 {
const MAX: usize = if cfg!(miri) { 64 } else { 256 };
for len in 32..MAX {
let input = std::iter::repeat(42).take(len).collect::<Vector<_>>();
let mut inp2 = input.clone();
assert!(input.ptr_eq(&inp2));
Expand All @@ -2662,7 +2671,18 @@ mod test {
}
}

#[test]
fn full_retain() {
let mut a = Vector::from_iter(0..128);
let b = Vector::from_iter(128..256);
a.append(b);
assert!(matches!(a.vector, Full(_, _)));
a.retain(|i| *i % 2 == 0);
assert_eq!(a.len(), 128);
}

proptest! {
#[cfg_attr(miri, ignore)]
#[test]
fn iter(ref vec in vec(i32::ANY, 0..1000)) {
let seq: Vector<i32> = Vector::from_iter(vec.iter().cloned());
Expand All @@ -2672,6 +2692,7 @@ mod test {
assert_eq!(vec.len(), seq.len());
}

#[cfg_attr(miri, ignore)]
#[test]
fn push_front_mut(ref input in vec(i32::ANY, 0..1000)) {
let mut vector = Vector::new();
Expand All @@ -2684,6 +2705,7 @@ mod test {
assert_eq!(input2, Vec::from_iter(vector.iter().cloned()));
}

#[cfg_attr(miri, ignore)]
#[test]
fn push_back_mut(ref input in vec(i32::ANY, 0..1000)) {
let mut vector = Vector::new();
Expand All @@ -2695,6 +2717,7 @@ mod test {
assert_eq!(input, &Vec::from_iter(vector.iter().cloned()));
}

#[cfg_attr(miri, ignore)]
#[test]
fn pop_back_mut(ref input in vec(i32::ANY, 0..1000)) {
let mut vector = Vector::from_iter(input.iter().cloned());
Expand All @@ -2711,6 +2734,7 @@ mod test {
assert_eq!(0, vector.len());
}

#[cfg_attr(miri, ignore)]
#[test]
fn pop_front_mut(ref input in vec(i32::ANY, 0..1000)) {
let mut vector = Vector::from_iter(input.iter().cloned());
Expand Down Expand Up @@ -2747,6 +2771,7 @@ mod test {
// assert_eq!(true, vector.is_empty());
// }

#[cfg_attr(miri, ignore)]
#[test]
fn split(ref vec in vec(i32::ANY, 1..2000), split_pos in usize::ANY) {
let split_index = split_pos % (vec.len() + 1);
Expand All @@ -2762,6 +2787,7 @@ mod test {
}
}

#[cfg_attr(miri, ignore)]
#[test]
fn append(ref vec1 in vec(i32::ANY, 0..1000), ref vec2 in vec(i32::ANY, 0..1000)) {
let mut seq1 = Vector::from_iter(vec1.iter().cloned());
Expand All @@ -2777,6 +2803,7 @@ mod test {
}
}

#[cfg_attr(miri, ignore)]
#[test]
fn iter_mut(ref input in vector(i32::ANY, 0..10000)) {
let mut vec = input.clone();
Expand All @@ -2789,6 +2816,7 @@ mod test {
assert_eq!(expected, vec);
}

#[cfg_attr(miri, ignore)]
#[test]
fn focus(ref input in vector(i32::ANY, 0..10000)) {
let mut vec = input.clone();
Expand All @@ -2803,6 +2831,7 @@ mod test {
assert_eq!(expected, vec);
}

#[cfg_attr(miri, ignore)]
#[test]
fn focus_mut_split(ref input in vector(i32::ANY, 0..10000)) {
let mut vec = input.clone();
Expand All @@ -2826,6 +2855,7 @@ mod test {
assert_eq!(expected, vec);
}

#[cfg_attr(miri, ignore)]
#[test]
fn chunks(ref input in vector(i32::ANY, 0..10000)) {
let output: Vector<_> = input.leaves().flatten().cloned().collect();
Expand All @@ -2835,6 +2865,7 @@ mod test {
assert_eq!(rev_in, rev_out);
}

#[cfg_attr(miri, ignore)]
#[test]
fn chunks_mut(ref mut input_src in vector(i32::ANY, 0..10000)) {
let mut input = input_src.clone();
Expand Down
2 changes: 2 additions & 0 deletions src/vector/rayon.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,11 +193,13 @@ mod test {
use ::rayon::iter::{IntoParallelRefIterator, IntoParallelRefMutIterator, ParallelIterator};

proptest! {
#[cfg_attr(miri, ignore)]
#[test]
fn par_iter(ref mut input in vector(i32::ANY, 0..10000)) {
assert_eq!(input.iter().max(), input.par_iter().max())
}

#[cfg_attr(miri, ignore)]
#[test]
fn par_mut_iter(ref mut input in vector(i32::ANY, 0..10000)) {
let mut vec = input.clone();
Expand Down
Loading