From 8972bcb0ddd78c5e3965c4c045736188fcae017b Mon Sep 17 00:00:00 2001
From: Stein Somers <git@steinsomers.be>
Date: Mon, 9 Nov 2020 13:48:45 +0100
Subject: [PATCH] BTreeMap: test chaotic ordering & other bits & bobs
---
.../alloc/src/collections/btree/map/tests.rs | 148 +++++++++++++++---
.../collections/btree/map/tests/ord_chaos.rs | 76 +++++++++
.../alloc/src/collections/btree/node/tests.rs | 5 +-
3 files changed, 202 insertions(+), 27 deletions(-)
create mode 100644 library/alloc/src/collections/btree/map/tests/ord_chaos.rs
diff --git a/library/alloc/src/collections/btree/map/tests.rs b/library/alloc/src/collections/btree/map/tests.rs
index dd3ebcccf76..7bc6a045ad6 100644
--- a/library/alloc/src/collections/btree/map/tests.rs
+++ b/library/alloc/src/collections/btree/map/tests.rs
@@ -14,6 +14,9 @@ use std::ops::RangeBounds;
use std::panic::{catch_unwind, AssertUnwindSafe};
use std::sync::atomic::{AtomicUsize, Ordering};
+mod ord_chaos;
+use ord_chaos::{Cyclic3, Governed, Governor};
+
// Capacity of a tree with a single level,
// i.e., a tree who's root is a leaf node at height 0.
const NODE_CAPACITY: usize = node::CAPACITY;
@@ -28,7 +31,7 @@ const MIN_INSERTS_HEIGHT_1: usize = NODE_CAPACITY + 1;
// It's not the minimum size: removing an element from such a tree does not always reduce height.
const MIN_INSERTS_HEIGHT_2: usize = 89;
-// Gather all references from a mutable iterator and make sure Miri notices if
+// Gathers all references from a mutable iterator and makes sure Miri notices if
// using them is dangerous.
fn test_all_refs<'a, T: 'a>(dummy: &mut T, iter: impl Iterator<Item = &'a mut T>) {
// Gather all those references.
@@ -43,28 +46,43 @@ fn test_all_refs<'a, T: 'a>(dummy: &mut T, iter: impl Iterator<Item = &'a mut T>
}
impl<K, V> BTreeMap<K, V> {
- /// Panics if the map (or the code navigating it) is corrupted.
- fn check(&self)
- where
- K: Copy + Debug + Ord,
- {
+ // Panics if the map (or the code navigating it) is corrupted.
+ fn check_invariants(&self) {
if let Some(root) = &self.root {
let root_node = root.node_as_ref();
+ // Check the back pointers top-down, before we attempt to rely on
+ // more serious navigation code.
assert!(root_node.ascend().is_err());
root_node.assert_back_pointers();
+ // Check consistenty of `length` and some of the navigation.
assert_eq!(self.length, root_node.calc_length());
+ assert_eq!(self.length, self.keys().count());
+ // Lastly, check the invariant causing the least harm.
root_node.assert_min_len(if root_node.height() > 0 { 1 } else { 0 });
} else {
+ // Check consistenty of `length` and some of the navigation.
assert_eq!(self.length, 0);
+ assert_eq!(self.length, self.keys().count());
}
-
- self.assert_ascending();
}
- /// Returns the height of the root, if any.
+ // Panics if the map is corrupted or if the keys are not in strictly
+ // ascending order, in the current opinion of the `Ord` implementation.
+ // If the `Ord` implementation does not honor transitivity, this method
+ // does not guarantee that all the keys are unique, just that adjacent
+ // keys are unique.
+ fn check(&self)
+ where
+ K: Debug + Ord,
+ {
+ self.check_invariants();
+ self.assert_strictly_ascending();
+ }
+
+ // Returns the height of the root, if any.
fn height(&self) -> Option<usize> {
self.root.as_ref().map(node::Root::height)
}
@@ -80,22 +98,18 @@ impl<K, V> BTreeMap<K, V> {
}
}
- /// Asserts that the keys are in strictly ascending order.
- fn assert_ascending(&self)
+ // Panics if the keys are not in strictly ascending order.
+ fn assert_strictly_ascending(&self)
where
- K: Copy + Debug + Ord,
+ K: Debug + Ord,
{
- let mut num_seen = 0;
let mut keys = self.keys();
if let Some(mut previous) = keys.next() {
- num_seen = 1;
for next in keys {
assert!(previous < next, "{:?} >= {:?}", previous, next);
previous = next;
- num_seen += 1;
}
}
- assert_eq!(num_seen, self.len());
}
}
@@ -111,7 +125,7 @@ impl<'a, K: 'a, V: 'a> NodeRef<marker::Immut<'a>, K, V, marker::LeafOrInternal>
}
}
-// Test our value of MIN_INSERTS_HEIGHT_2. It may change according to the
+// Tests our value of MIN_INSERTS_HEIGHT_2. It may change according to the
// implementation of insertion, but it's best to be aware of when it does.
#[test]
fn test_levels() {
@@ -149,6 +163,25 @@ fn test_levels() {
assert_eq!(map.len(), MIN_INSERTS_HEIGHT_2, "{}", map.dump_keys());
}
+// Ensures the testing infrastructure usually notices order violations.
+#[test]
+#[should_panic]
+fn test_check_ord_chaos() {
+ let gov = Governor::new();
+ let map: BTreeMap<_, _> = (0..2).map(|i| (Governed(i, &gov), ())).collect();
+ gov.flip();
+ map.check();
+}
+
+// Ensures the testing infrastructure doesn't always mind order violations.
+#[test]
+fn test_check_invariants_ord_chaos() {
+ let gov = Governor::new();
+ let map: BTreeMap<_, _> = (0..2).map(|i| (Governed(i, &gov), ())).collect();
+ gov.flip();
+ map.check_invariants();
+}
+
#[test]
fn test_basic_large() {
let mut map = BTreeMap::new();
@@ -334,7 +367,7 @@ fn test_iter_rev() {
test(size, map.into_iter().rev());
}
-/// Specifically tests iter_mut's ability to mutate the value of pairs in-line
+// Specifically tests iter_mut's ability to mutate the value of pairs in-line.
fn do_test_iter_mut_mutation<T>(size: usize)
where
T: Copy + Debug + Ord + TryFrom<usize>,
@@ -439,6 +472,8 @@ fn test_iter_entering_root_twice() {
*back.1 = 42;
assert_eq!(front, (&0, &mut 24));
assert_eq!(back, (&1, &mut 42));
+ assert_eq!(it.next(), None);
+ assert_eq!(it.next_back(), None);
map.check();
}
@@ -591,11 +626,12 @@ fn test_range_small() {
#[test]
fn test_range_height_1() {
- // Tests tree with a root and 2 leaves. Depending on details we don't want or need
- // to rely upon, the single key at the root will be 6 or 7.
+ // Tests tree with a root and 2 leaves. The single key in the root node is
+ // close to the middle among the keys.
- let map: BTreeMap<_, _> = (1..=MIN_INSERTS_HEIGHT_1 as i32).map(|i| (i, i)).collect();
- for &root in &[6, 7] {
+ let map: BTreeMap<_, _> = (0..MIN_INSERTS_HEIGHT_1 as i32).map(|i| (i, i)).collect();
+ let middle = MIN_INSERTS_HEIGHT_1 as i32 / 2;
+ for root in middle - 2..=middle + 2 {
assert_eq!(range_keys(&map, (Excluded(root), Excluded(root + 1))), vec![]);
assert_eq!(range_keys(&map, (Excluded(root), Included(root + 1))), vec![root + 1]);
assert_eq!(range_keys(&map, (Included(root), Excluded(root + 1))), vec![root]);
@@ -727,6 +763,19 @@ fn test_range_backwards_4() {
map.range((Excluded(3), Excluded(2)));
}
+#[test]
+#[should_panic]
+fn test_range_backwards_5() {
+ let mut map = BTreeMap::new();
+ map.insert(Cyclic3::B, ());
+ // Lacking static_assert, call `range` conditionally, to emphasise that
+ // we cause a different panic than `test_range_backwards_1` does.
+ // A more refined `should_panic` would be welcome.
+ if Cyclic3::C < Cyclic3::A {
+ map.range(Cyclic3::C..=Cyclic3::A);
+ }
+}
+
#[test]
fn test_range_1000() {
// Miri is too slow
@@ -820,7 +869,7 @@ mod test_drain_filter {
}
#[test]
- fn consuming_nothing() {
+ fn consumed_keeping_all() {
let pairs = (0..3).map(|i| (i, i));
let mut map: BTreeMap<_, _> = pairs.collect();
assert!(map.drain_filter(|_, _| false).eq(iter::empty()));
@@ -828,10 +877,20 @@ mod test_drain_filter {
}
#[test]
- fn consuming_all() {
+ fn consumed_removing_all() {
let pairs = (0..3).map(|i| (i, i));
let mut map: BTreeMap<_, _> = pairs.clone().collect();
assert!(map.drain_filter(|_, _| true).eq(pairs));
+ assert!(map.is_empty());
+ map.check();
+ }
+
+ #[test]
+ fn dropped_removing_all() {
+ let pairs = (0..3).map(|i| (i, i));
+ let mut map: BTreeMap<_, _> = pairs.collect();
+ map.drain_filter(|_, _| true);
+ assert!(map.is_empty());
map.check();
}
@@ -1712,6 +1771,27 @@ fn test_append_drop_leak() {
assert_eq!(DROPS.load(Ordering::SeqCst), 4); // Rust issue #47949 ate one little piggy
}
+#[test]
+fn test_append_ord_chaos() {
+ let mut map1 = BTreeMap::new();
+ map1.insert(Cyclic3::A, ());
+ map1.insert(Cyclic3::B, ());
+ let mut map2 = BTreeMap::new();
+ map2.insert(Cyclic3::A, ());
+ map2.insert(Cyclic3::B, ());
+ map2.insert(Cyclic3::C, ()); // lands first, before A
+ map2.insert(Cyclic3::B, ()); // lands first, before C
+ map1.check();
+ map2.check(); // keys are not unique but still strictly ascending
+ assert_eq!(map1.len(), 2);
+ assert_eq!(map2.len(), 4);
+ map1.append(&mut map2);
+ assert_eq!(map1.len(), 5);
+ assert_eq!(map2.len(), 0);
+ map1.check();
+ map2.check();
+}
+
fn rand_data(len: usize) -> Vec<(u32, u32)> {
assert!(len * 2 <= 70029); // from that point on numbers repeat
let mut rng = DeterministicRng::new();
@@ -1874,11 +1954,27 @@ fn test_insert_remove_intertwined() {
let loops = if cfg!(miri) { 100 } else { 1_000_000 };
let mut map = BTreeMap::new();
let mut i = 1;
+ let offset = 165; // somewhat arbitrarily chosen to cover some code paths
for _ in 0..loops {
- i = (i + 421) & 0xFF;
+ i = (i + offset) & 0xFF;
map.insert(i, i);
map.remove(&(0xFF - i));
}
-
map.check();
}
+
+#[test]
+fn test_insert_remove_intertwined_ord_chaos() {
+ let loops = if cfg!(miri) { 100 } else { 1_000_000 };
+ let gov = Governor::new();
+ let mut map = BTreeMap::new();
+ let mut i = 1;
+ let offset = 165; // more arbitrarily copied from above
+ for _ in 0..loops {
+ i = (i + offset) & 0xFF;
+ map.insert(Governed(i, &gov), ());
+ map.remove(&Governed(0xFF - i, &gov));
+ gov.flip();
+ }
+ map.check_invariants();
+}
diff --git a/library/alloc/src/collections/btree/map/tests/ord_chaos.rs b/library/alloc/src/collections/btree/map/tests/ord_chaos.rs
new file mode 100644
index 00000000000..91d1d6ea9ef
--- /dev/null
+++ b/library/alloc/src/collections/btree/map/tests/ord_chaos.rs
@@ -0,0 +1,76 @@
+use std::cell::Cell;
+use std::cmp::Ordering::{self, *};
+use std::ptr;
+
+#[derive(Debug)]
+pub enum Cyclic3 {
+ A,
+ B,
+ C,
+}
+use Cyclic3::*;
+
+impl PartialOrd for Cyclic3 {
+ fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+ Some(self.cmp(other))
+ }
+}
+
+impl Ord for Cyclic3 {
+ fn cmp(&self, other: &Self) -> Ordering {
+ match (self, other) {
+ (A, A) | (B, B) | (C, C) => Equal,
+ (A, B) | (B, C) | (C, A) => Less,
+ (A, C) | (B, A) | (C, B) => Greater,
+ }
+ }
+}
+
+impl PartialEq for Cyclic3 {
+ fn eq(&self, other: &Self) -> bool {
+ self.cmp(&other) == Equal
+ }
+}
+
+impl Eq for Cyclic3 {}
+
+#[derive(Debug)]
+pub struct Governor {
+ flipped: Cell<bool>,
+}
+
+impl Governor {
+ pub fn new() -> Self {
+ Governor { flipped: Cell::new(false) }
+ }
+
+ pub fn flip(&self) {
+ self.flipped.set(!self.flipped.get());
+ }
+}
+
+#[derive(Debug)]
+pub struct Governed<'a, T>(pub T, pub &'a Governor);
+
+impl<T: Ord> PartialOrd for Governed<'_, T> {
+ fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+ Some(self.cmp(other))
+ }
+}
+
+impl<T: Ord> Ord for Governed<'_, T> {
+ fn cmp(&self, other: &Self) -> Ordering {
+ assert!(ptr::eq(self.1, other.1));
+ let ord = self.0.cmp(&other.0);
+ if self.1.flipped.get() { ord.reverse() } else { ord }
+ }
+}
+
+impl<T: PartialEq> PartialEq for Governed<'_, T> {
+ fn eq(&self, other: &Self) -> bool {
+ assert!(ptr::eq(self.1, other.1));
+ self.0.eq(&other.0)
+ }
+}
+
+impl<T: Eq> Eq for Governed<'_, T> {}
diff --git a/library/alloc/src/collections/btree/node/tests.rs b/library/alloc/src/collections/btree/node/tests.rs
index 38c75de34ee..636b4d03981 100644
--- a/library/alloc/src/collections/btree/node/tests.rs
+++ b/library/alloc/src/collections/btree/node/tests.rs
@@ -5,7 +5,7 @@ use crate::string::String;
use core::cmp::Ordering::*;
impl<'a, K: 'a, V: 'a> NodeRef<marker::Immut<'a>, K, V, marker::LeafOrInternal> {
- /// Asserts that the back pointer in each reachable node points to its parent.
+ // Asserts that the back pointer in each reachable node points to its parent.
pub fn assert_back_pointers(self) {
if let ForceResult::Internal(node) = self.force() {
for idx in 0..=node.len() {
@@ -17,6 +17,9 @@ impl<'a, K: 'a, V: 'a> NodeRef<marker::Immut<'a>, K, V, marker::LeafOrInternal>
}
}
+ // Renders a multi-line display of the keys in order and in tree hierarchy,
+ // picturing the tree growing sideways from its root on the left to its
+ // leaves on the right.
pub fn dump_keys(self) -> String
where
K: Debug,