diff --git a/concordium-std/CHANGELOG.md b/concordium-std/CHANGELOG.md index ec4dd937..e4397537 100644 --- a/concordium-std/CHANGELOG.md +++ b/concordium-std/CHANGELOG.md @@ -3,6 +3,8 @@ ## Unreleased changes - Set minimum Rust version to 1.66. +- Fix bug in `StateMap::get_mut`, which allowed multiple mutable references to the state to coexist. + - The signature has changed using `&self` to using `&mut self`. ## concordium-std 8.0.0 (2023-08-21) diff --git a/concordium-std/src/impls.rs b/concordium-std/src/impls.rs index 56cfb9f1..469c0ae2 100644 --- a/concordium-std/src/impls.rs +++ b/concordium-std/src/impls.rs @@ -908,7 +908,8 @@ where /// Lookup a mutable reference to the value with the given key. Return /// [None] if there is no value with the given key. - pub fn get_mut(&self, key: &K) -> Option> { + + pub fn get_mut(&mut self, key: &K) -> Option> { let k = self.key_with_map_prefix(key); let entry = self.state_api.lookup_entry(&k)?; Some(StateRefMut::new(entry, self.state_api.clone())) @@ -2992,3 +2993,21 @@ impl Deserial for MetadataUrl { }) } } + +#[cfg(test)] +mod tests { + + /// Check that you cannot have multiple active entries from a statemap at + /// the same time. See the test file for details. + #[test] + fn statemap_multiple_entries_not_allowed() { + let t = trybuild::TestCases::new(); + t.compile_fail("tests/state/map-multiple-entries.rs"); + } + + #[test] + fn statemap_multiple_state_ref_mut_not_allowed() { + let t = trybuild::TestCases::new(); + t.compile_fail("tests/state/map-multiple-state-ref-mut.rs"); + } +} diff --git a/concordium-std/src/test_infrastructure.rs b/concordium-std/src/test_infrastructure.rs index b04c4dc2..60ed6d2b 100644 --- a/concordium-std/src/test_infrastructure.rs +++ b/concordium-std/src/test_infrastructure.rs @@ -2431,17 +2431,6 @@ mod test { assert!(iter.nth(1).is_none()); } - #[test] - fn multiple_entries_not_allowed() { - let mut state_builder = TestStateBuilder::new(); - let mut map = state_builder.new_map(); - map.insert(0u8, 1u8); - let e1 = map.entry(0u8); - // Uncommenting this line should give a borrow-check error. - // let e2 = map.entry(1u8); - e1.and_modify(|v| *v += 1); - } - #[test] fn occupied_entry_truncates_leftover_data() { let mut state_builder = TestStateBuilder::new(); diff --git a/concordium-std/tests/state/map-multiple-entries.rs b/concordium-std/tests/state/map-multiple-entries.rs new file mode 100644 index 00000000..35f678fe --- /dev/null +++ b/concordium-std/tests/state/map-multiple-entries.rs @@ -0,0 +1,16 @@ +//! This test checks that you cannot have multiple entries from a `StateMap` +//! alive at the same time. +//! +//! When compiling it, the borrow-checker is supposed to throw an error. +use concordium_std::*; + +pub fn main() { + let mut state_builder = StateBuilder::open(ExternStateApi::open()); + let mut map: StateMap = state_builder.new_map(); + // Get two entries. + let e1 = map.entry(0u8); + let e2 = map.entry(1u8); + // Use them, so we are certain that their lifetimes overlap. + e1.or_insert(1); + e2.or_insert(2); +} diff --git a/concordium-std/tests/state/map-multiple-entries.stderr b/concordium-std/tests/state/map-multiple-entries.stderr new file mode 100644 index 00000000..9fa1d65a --- /dev/null +++ b/concordium-std/tests/state/map-multiple-entries.stderr @@ -0,0 +1,10 @@ +error[E0499]: cannot borrow `map` as mutable more than once at a time + --> tests/state/map-multiple-entries.rs:12:14 + | +11 | let e1 = map.entry(0u8); + | -------------- first mutable borrow occurs here +12 | let e2 = map.entry(1u8); + | ^^^^^^^^^^^^^^ second mutable borrow occurs here +13 | // Use them, so we are certain that their lifetimes overlap. +14 | e1.or_insert(1); + | -- first borrow later used here diff --git a/concordium-std/tests/state/map-multiple-state-ref-mut.rs b/concordium-std/tests/state/map-multiple-state-ref-mut.rs new file mode 100644 index 00000000..4cd2b920 --- /dev/null +++ b/concordium-std/tests/state/map-multiple-state-ref-mut.rs @@ -0,0 +1,17 @@ +//! This test checks that you cannot have multiple `StateRefMut` from a +//! `StateMap` alive at the same time. +//! +//! When compiling it, the borrow-checker is supposed to throw an error. +use concordium_std::*; + +pub fn main() { + let mut state_builder = StateBuilder::open(ExternStateApi::open()); + let mut map: StateMap = state_builder.new_map(); + map.insert(0, 1); + map.insert(1, 2); + // Get two mutable references and unwrap the options. + let e1 = map.get_mut(&0u8).unwrap(); + let e2 = map.get_mut(&1u8).unwrap(); + // Use them, so we are certain that their lifetimes overlap. + assert_eq!(*e1, *e2); +} diff --git a/concordium-std/tests/state/map-multiple-state-ref-mut.stderr b/concordium-std/tests/state/map-multiple-state-ref-mut.stderr new file mode 100644 index 00000000..444cfbf3 --- /dev/null +++ b/concordium-std/tests/state/map-multiple-state-ref-mut.stderr @@ -0,0 +1,10 @@ +error[E0499]: cannot borrow `map` as mutable more than once at a time + --> tests/state/map-multiple-state-ref-mut.rs:14:14 + | +13 | let e1 = map.get_mut(&0u8).unwrap(); + | ----------------- first mutable borrow occurs here +14 | let e2 = map.get_mut(&1u8).unwrap(); + | ^^^^^^^^^^^^^^^^^ second mutable borrow occurs here +15 | // Use them, so we are certain that their lifetimes overlap. +16 | assert_eq!(*e1, *e2); + | -- first borrow later used here