From 1d03e12a5fc555d66d40d24c58404365939cf373 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Tue, 7 Dec 2021 16:24:20 -0500 Subject: [PATCH 1/9] Add some failing `spread_allocate` tests --- crates/storage/src/collections/vec/tests.rs | 79 +++++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/crates/storage/src/collections/vec/tests.rs b/crates/storage/src/collections/vec/tests.rs index 8f3bd7805a2..1f9a08bc49e 100644 --- a/crates/storage/src/collections/vec/tests.rs +++ b/crates/storage/src/collections/vec/tests.rs @@ -632,3 +632,82 @@ fn drop_works() { }) .unwrap() } + +#[test] +fn spread_allocate_drop_works() -> ink_env::Result<()> { + use crate::traits::{ + KeyPtr, + SpreadAllocate, + }; + + ink_env::test::run_test::(|_| { + let setup_result = std::panic::catch_unwind(|| { + let root_key = Key::from([0x42; 32]); + let mut key_ptr = KeyPtr::from(root_key); + let instance = + as SpreadAllocate>::allocate_spread(&mut key_ptr); + drop(instance) + }); + + assert!(setup_result.is_ok()); + + Ok(()) + }) +} + +#[test] +fn spread_allocate_push_works() -> ink_env::Result<()> { + use crate::traits::{ + KeyPtr, + SpreadAllocate, + }; + + ink_env::test::run_test::(|_| { + let setup_result = std::panic::catch_unwind(|| { + let root_key = Key::from([0x42; 32]); + let mut key_ptr = KeyPtr::from(root_key); + let mut instance = + as SpreadAllocate>::allocate_spread(&mut key_ptr); + instance.push(1u8); + dbg!(&instance); + }); + + assert!(setup_result.is_ok()); + + Ok(()) + }) +} + +#[test] +fn spread_allocate_vector_works() -> ink_env::Result<()> { + use crate::traits::{ + KeyPtr, + SpreadAllocate, + }; + + ink_env::test::run_test::(|_| { + let root_key = Key::from([0x42; 32]); + let mut key_ptr = KeyPtr::from(root_key); + let mut instance = + as SpreadAllocate>::allocate_spread(&mut key_ptr); + + instance.push(1); + assert_eq!(instance.pop(), Some(1)); + + instance.push(2); + assert!(instance.pop_drop().is_some()); + + instance.push(3); + assert!(instance.swap_remove_drop(0).is_some()); + + instance.push(4); + assert!(instance.set(0, 5).is_ok()); + + instance.clear(); + assert!(instance.is_empty()); + + dbg!(&instance); + + Ok(()) + }) +} From e3927d2844d35f4ca7343d2e607d8d69f4fcee58 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Tue, 7 Dec 2021 17:05:10 -0500 Subject: [PATCH 2/9] Add `Lazy::try_get` method --- crates/storage/src/lazy/mod.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/crates/storage/src/lazy/mod.rs b/crates/storage/src/lazy/mod.rs index b2076ccb251..e055829f3ac 100644 --- a/crates/storage/src/lazy/mod.rs +++ b/crates/storage/src/lazy/mod.rs @@ -157,6 +157,17 @@ where lazy.cell.get().expect("encountered empty storage cell") } + /// Attempts to load a shared reference to a value in storage. + /// + /// If there is no value to load `None` is returned. + /// + /// This method may be useful in situations where a data structure has been + /// initialized with `SpreadAllocate`, but nothing has been written to its + /// storage yet. + pub fn try_get(lazy: &Self) -> Option<&T> { + lazy.cell.get() + } + /// Returns an exclusive reference to the lazily loaded value. /// /// # Note From c1d0a793fd725475ea2f68c0e8a3a4a4a3b1d60b Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Tue, 7 Dec 2021 17:06:48 -0500 Subject: [PATCH 3/9] Return `0` for vector's length if no length is in storage --- crates/storage/src/collections/vec/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/storage/src/collections/vec/mod.rs b/crates/storage/src/collections/vec/mod.rs index 1063ad3fde9..071975bdb0b 100644 --- a/crates/storage/src/collections/vec/mod.rs +++ b/crates/storage/src/collections/vec/mod.rs @@ -91,7 +91,7 @@ where /// Returns the number of elements in the vector, also referred to as its length. pub fn len(&self) -> u32 { - *self.len + *Lazy::try_get(&self.len).unwrap_or(&0) } /// Returns `true` if the vector contains no elements. From c25d7f41cbf6a881ec109bd7bffea7a87536c515 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Tue, 7 Dec 2021 17:07:36 -0500 Subject: [PATCH 4/9] Use `Lazy::set` instead of `Lazy::get_mut` This method doesn't panic if there are no values in storage when trying to perform the `get_mut` operation. --- crates/storage/src/collections/vec/mod.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/storage/src/collections/vec/mod.rs b/crates/storage/src/collections/vec/mod.rs index 071975bdb0b..fcf1dc14989 100644 --- a/crates/storage/src/collections/vec/mod.rs +++ b/crates/storage/src/collections/vec/mod.rs @@ -197,7 +197,7 @@ where "cannot push more elements into the storage vector" ); let last_index = self.len(); - *self.len += 1; + Lazy::set(&mut self.len, last_index + 1); self.elems.put(last_index, Some(value)); } @@ -387,7 +387,7 @@ where return None } let last_index = self.len() - 1; - *self.len = last_index; + Lazy::set(&mut self.len, last_index); self.elems.put_get(last_index, None) } @@ -404,7 +404,7 @@ where return None } let last_index = self.len() - 1; - *self.len = last_index; + Lazy::set(&mut self.len, last_index); self.elems.put(last_index, None); Some(()) } @@ -481,7 +481,7 @@ where let last_index = self.len() - 1; let last = self.elems.put_get(last_index, None); self.elems.put(n, last); - *self.len = last_index; + Lazy::set(&mut self.len, last_index); Some(()) } @@ -513,6 +513,6 @@ where for index in 0..self.len() { self.elems.put(index, None); } - *self.len = 0; + Lazy::set(&mut self.len, 0); } } From d5b5aa601e7ee5fdc48ab47e4fbb54c3f4c339b5 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Tue, 7 Dec 2021 17:10:06 -0500 Subject: [PATCH 5/9] Update some vector tests to not expect panics --- .../storage/src/collections/bitvec/tests.rs | 12 ++++++++- crates/storage/src/collections/vec/tests.rs | 27 ++++++++++++++----- 2 files changed, 32 insertions(+), 7 deletions(-) diff --git a/crates/storage/src/collections/bitvec/tests.rs b/crates/storage/src/collections/bitvec/tests.rs index 091c10e39d8..3932f26ca95 100644 --- a/crates/storage/src/collections/bitvec/tests.rs +++ b/crates/storage/src/collections/bitvec/tests.rs @@ -205,7 +205,17 @@ fn spread_layout_clear_works() { // loading another instance from this storage will panic since the // vector's length property cannot read a value: SpreadLayout::clear_spread(&bv1, &mut KeyPtr::from(root_key)); - let _ = ::pull_spread(&mut KeyPtr::from(root_key)); + + let len_result = std::panic::catch_unwind(|| { + let instance = ::pull_spread(&mut KeyPtr::from(root_key)); + let _ = crate::Lazy::get(&instance.len); + }); + assert!(len_result.is_err(), "Length shouldn't be in storage at this point."); + + // In practice we wouldn't get the raw `len` field though, so no panic occurs + let instance = ::pull_spread(&mut KeyPtr::from(root_key)); + assert!(instance.is_empty()); + Ok(()) }) .unwrap() diff --git a/crates/storage/src/collections/vec/tests.rs b/crates/storage/src/collections/vec/tests.rs index 1f9a08bc49e..188d1ae1922 100644 --- a/crates/storage/src/collections/vec/tests.rs +++ b/crates/storage/src/collections/vec/tests.rs @@ -375,7 +375,6 @@ fn spread_layout_push_pull_works() -> ink_env::Result<()> { } #[test] -#[should_panic(expected = "encountered empty storage cell")] fn spread_layout_clear_works() { ink_env::test::run_test::(|_| { let vec1 = vec_from_slice(&[b'a', b'b', b'c', b'd']); @@ -388,8 +387,17 @@ fn spread_layout_clear_works() { // loading another instance from this storage will panic since the // vector's length property cannot read a value: SpreadLayout::clear_spread(&vec1, &mut KeyPtr::from(root_key)); - let _ = - as SpreadLayout>::pull_spread(&mut KeyPtr::from(root_key)); + + let len_result = std::panic::catch_unwind(|| { + let instance = as SpreadLayout>::pull_spread(&mut KeyPtr::from(root_key)); + let _ = Lazy::get(&instance.len); + }); + assert!(len_result.is_err(), "Length shouldn't be in storage at this point."); + + // In practice we wouldn't get the raw `len` field though, so no panic occurs + let instance = as SpreadLayout>::pull_spread(&mut KeyPtr::from(root_key)); + assert!(instance.is_empty()); + Ok(()) }) .unwrap() @@ -534,7 +542,6 @@ fn storage_is_cleared_completely_after_pull_lazy() { } #[test] -#[should_panic(expected = "encountered empty storage cell")] #[cfg(not(feature = "ink-experimental-engine"))] fn drop_works() { ink_env::test::run_test::(|_| { @@ -561,8 +568,16 @@ fn drop_works() { .expect("used cells must be returned"); assert_eq!(used_cells, 0); - let _ = - as SpreadLayout>::pull_spread(&mut KeyPtr::from(root_key)); + let len_result = std::panic::catch_unwind(|| { + let instance = as SpreadLayout>::pull_spread(&mut KeyPtr::from(root_key)); + let _ = Lazy::get(&instance.len); + }); + assert!(len_result.is_err(), "Length shouldn't be in storage at this point."); + + // In practice we wouldn't get the raw `len` field though, so no panic occurs + let instance = as SpreadLayout>::pull_spread(&mut KeyPtr::from(root_key)); + assert!(instance.is_empty()); + Ok(()) }) .unwrap() From 94302ac4305de2f97fd62257648b27e5c887d78a Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Tue, 7 Dec 2021 17:10:59 -0500 Subject: [PATCH 6/9] RustFmt --- .../storage/src/collections/bitvec/tests.rs | 11 +++++--- crates/storage/src/collections/vec/tests.rs | 26 ++++++++++++++----- 2 files changed, 27 insertions(+), 10 deletions(-) diff --git a/crates/storage/src/collections/bitvec/tests.rs b/crates/storage/src/collections/bitvec/tests.rs index 3932f26ca95..6507af8cda7 100644 --- a/crates/storage/src/collections/bitvec/tests.rs +++ b/crates/storage/src/collections/bitvec/tests.rs @@ -207,13 +207,18 @@ fn spread_layout_clear_works() { SpreadLayout::clear_spread(&bv1, &mut KeyPtr::from(root_key)); let len_result = std::panic::catch_unwind(|| { - let instance = ::pull_spread(&mut KeyPtr::from(root_key)); + let instance = + ::pull_spread(&mut KeyPtr::from(root_key)); let _ = crate::Lazy::get(&instance.len); }); - assert!(len_result.is_err(), "Length shouldn't be in storage at this point."); + assert!( + len_result.is_err(), + "Length shouldn't be in storage at this point." + ); // In practice we wouldn't get the raw `len` field though, so no panic occurs - let instance = ::pull_spread(&mut KeyPtr::from(root_key)); + let instance = + ::pull_spread(&mut KeyPtr::from(root_key)); assert!(instance.is_empty()); Ok(()) diff --git a/crates/storage/src/collections/vec/tests.rs b/crates/storage/src/collections/vec/tests.rs index 188d1ae1922..6b7ed8073a8 100644 --- a/crates/storage/src/collections/vec/tests.rs +++ b/crates/storage/src/collections/vec/tests.rs @@ -389,13 +389,19 @@ fn spread_layout_clear_works() { SpreadLayout::clear_spread(&vec1, &mut KeyPtr::from(root_key)); let len_result = std::panic::catch_unwind(|| { - let instance = as SpreadLayout>::pull_spread(&mut KeyPtr::from(root_key)); + let instance = as SpreadLayout>::pull_spread( + &mut KeyPtr::from(root_key), + ); let _ = Lazy::get(&instance.len); }); - assert!(len_result.is_err(), "Length shouldn't be in storage at this point."); + assert!( + len_result.is_err(), + "Length shouldn't be in storage at this point." + ); // In practice we wouldn't get the raw `len` field though, so no panic occurs - let instance = as SpreadLayout>::pull_spread(&mut KeyPtr::from(root_key)); + let instance = + as SpreadLayout>::pull_spread(&mut KeyPtr::from(root_key)); assert!(instance.is_empty()); Ok(()) @@ -569,13 +575,19 @@ fn drop_works() { assert_eq!(used_cells, 0); let len_result = std::panic::catch_unwind(|| { - let instance = as SpreadLayout>::pull_spread(&mut KeyPtr::from(root_key)); + let instance = as SpreadLayout>::pull_spread( + &mut KeyPtr::from(root_key), + ); let _ = Lazy::get(&instance.len); }); - assert!(len_result.is_err(), "Length shouldn't be in storage at this point."); + assert!( + len_result.is_err(), + "Length shouldn't be in storage at this point." + ); // In practice we wouldn't get the raw `len` field though, so no panic occurs - let instance = as SpreadLayout>::pull_spread(&mut KeyPtr::from(root_key)); + let instance = + as SpreadLayout>::pull_spread(&mut KeyPtr::from(root_key)); assert!(instance.is_empty()); Ok(()) @@ -704,7 +716,7 @@ fn spread_allocate_vector_works() -> ink_env::Result<()> { let root_key = Key::from([0x42; 32]); let mut key_ptr = KeyPtr::from(root_key); let mut instance = - as SpreadAllocate>::allocate_spread(&mut key_ptr); + as SpreadAllocate>::allocate_spread(&mut key_ptr); instance.push(1); assert_eq!(instance.pop(), Some(1)); From dc0fed3f391ba4954277d25bb8ce77aaf88ae36e Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Tue, 7 Dec 2021 17:52:30 -0500 Subject: [PATCH 7/9] Fix vector tests when using `ink-experimental-engine` feature --- crates/storage/src/collections/vec/tests.rs | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/crates/storage/src/collections/vec/tests.rs b/crates/storage/src/collections/vec/tests.rs index 6b7ed8073a8..03a68ceb277 100644 --- a/crates/storage/src/collections/vec/tests.rs +++ b/crates/storage/src/collections/vec/tests.rs @@ -629,7 +629,6 @@ fn storage_is_cleared_completely_after_pull_lazy() { } #[test] -#[should_panic(expected = "encountered empty storage cell")] #[cfg(feature = "ink-experimental-engine")] fn drop_works() { ink_env::test::run_test::(|_| { @@ -653,8 +652,21 @@ fn drop_works() { .expect("used cells must be returned"); assert_eq!(used_cells, 0); - let _ = + let len_result = std::panic::catch_unwind(|| { + let instance = as SpreadLayout>::pull_spread( + &mut KeyPtr::from(root_key), + ); + let _ = Lazy::get(&instance.len); + }); + assert!( + len_result.is_err(), + "Length shouldn't be in storage at this point." + ); + + // In practice we wouldn't get the raw `len` field though, so no panic occurs + let instance = as SpreadLayout>::pull_spread(&mut KeyPtr::from(root_key)); + assert!(instance.is_empty()); Ok(()) }) .unwrap() From b3c6d15dad7f63019ff0a20fe34e00f455331f3c Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Thu, 9 Dec 2021 16:36:40 -0500 Subject: [PATCH 8/9] Fix `alloc`, `binary_heap`, and `bitstash` tests --- crates/storage/src/alloc/tests.rs | 2 +- .../storage/src/collections/binary_heap/tests.rs | 16 +++++++++++++--- crates/storage/src/collections/bitstash/tests.rs | 6 +++--- 3 files changed, 17 insertions(+), 7 deletions(-) diff --git a/crates/storage/src/alloc/tests.rs b/crates/storage/src/alloc/tests.rs index ce467313007..1c7d89bf984 100644 --- a/crates/storage/src/alloc/tests.rs +++ b/crates/storage/src/alloc/tests.rs @@ -158,7 +158,7 @@ fn spread_pull_push_works() { } #[test] -#[should_panic(expected = "encountered empty storage cell")] +#[should_panic(expected = "index is out of bounds")] fn spread_clear_works() { run_default_test(|| { let alloc = spread_layout_alloc_setup(); diff --git a/crates/storage/src/collections/binary_heap/tests.rs b/crates/storage/src/collections/binary_heap/tests.rs index 4280d657fd8..77f47c0c7f1 100644 --- a/crates/storage/src/collections/binary_heap/tests.rs +++ b/crates/storage/src/collections/binary_heap/tests.rs @@ -229,8 +229,10 @@ fn spread_layout_clear_works() { // loading another instance from this storage will panic since the // heap's length property cannot read a value: SpreadLayout::clear_spread(&heap1, &mut KeyPtr::from(root_key)); - let _ = + let instance = as SpreadLayout>::pull_spread(&mut KeyPtr::from(root_key)); + instance.len(); + Ok(()) }) .unwrap() @@ -266,8 +268,12 @@ fn drop_works() { .expect("Used cells must be returned"); assert_eq!(used_cells, 0); - let _ = + // We try and grab the length from storage, but it doesn't exist anymore + // so this will panic + let instance = as SpreadLayout>::pull_spread(&mut KeyPtr::from(root_key)); + instance.len(); + Ok(()) }) .unwrap() @@ -300,8 +306,12 @@ fn drop_works() { .expect("Used cells must be returned"); assert_eq!(used_cells, 0); - let _ = + // We try and grab the length from storage, but it doesn't exist anymore + // so this will panic + let instance = as SpreadLayout>::pull_spread(&mut KeyPtr::from(root_key)); + instance.len(); + Ok(()) }) .unwrap() diff --git a/crates/storage/src/collections/bitstash/tests.rs b/crates/storage/src/collections/bitstash/tests.rs index 7efd9747c8f..e7800aca0a0 100644 --- a/crates/storage/src/collections/bitstash/tests.rs +++ b/crates/storage/src/collections/bitstash/tests.rs @@ -138,7 +138,7 @@ fn spread_layout_push_pull_works() { } #[test] -#[should_panic(expected = "encountered empty storage cell")] +#[should_panic(expected = "index is out of bounds")] fn spread_layout_clear_works() { ink_env::test::run_test::(|_| { let default = filled_bitstash(); @@ -155,9 +155,9 @@ fn spread_layout_clear_works() { ::pull_spread(&mut KeyPtr::from(root_key)); // We have to prevent calling its destructor since that would also panic but // in an uncontrollable way. - let mut invalid = core::mem::ManuallyDrop::new(invalid); + let invalid = core::mem::ManuallyDrop::new(invalid); // Now interact with invalid instance. - let _ = invalid.put(); + invalid.get(0); Ok(()) }) .unwrap() From d9b4382e8b571f7f3b7c180aa97fd66e443ea1bc Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Thu, 9 Dec 2021 16:40:56 -0500 Subject: [PATCH 9/9] Remove `spread_allocate_push_works` test This is kinda rendundant since this is already covered by the `spread_allocate_vector_works` test. --- crates/storage/src/collections/vec/tests.rs | 28 +++------------------ 1 file changed, 3 insertions(+), 25 deletions(-) diff --git a/crates/storage/src/collections/vec/tests.rs b/crates/storage/src/collections/vec/tests.rs index 03a68ceb277..37152f58ab1 100644 --- a/crates/storage/src/collections/vec/tests.rs +++ b/crates/storage/src/collections/vec/tests.rs @@ -672,6 +672,9 @@ fn drop_works() { .unwrap() } +// We explicitly test that `Drop` works here since there was a bug in which a `StorageVec` +// initialized with `SpreadAllocate` would panic on `Drop` due to an empty storage cell +// for its length. #[test] fn spread_allocate_drop_works() -> ink_env::Result<()> { use crate::traits::{ @@ -694,29 +697,6 @@ fn spread_allocate_drop_works() -> ink_env::Result<()> { }) } -#[test] -fn spread_allocate_push_works() -> ink_env::Result<()> { - use crate::traits::{ - KeyPtr, - SpreadAllocate, - }; - - ink_env::test::run_test::(|_| { - let setup_result = std::panic::catch_unwind(|| { - let root_key = Key::from([0x42; 32]); - let mut key_ptr = KeyPtr::from(root_key); - let mut instance = - as SpreadAllocate>::allocate_spread(&mut key_ptr); - instance.push(1u8); - dbg!(&instance); - }); - - assert!(setup_result.is_ok()); - - Ok(()) - }) -} - #[test] fn spread_allocate_vector_works() -> ink_env::Result<()> { use crate::traits::{ @@ -745,8 +725,6 @@ fn spread_allocate_vector_works() -> ink_env::Result<()> { instance.clear(); assert!(instance.is_empty()); - dbg!(&instance); - Ok(()) }) }