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

Jump to bottom

Implement get_many_mut #238

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Implement get_many_mut #238

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d3f1968
Implement get_many_mut
NiklasJonsson Jul 14, 2022
66108be
Reduce (unsafe) code with array::map and pointers
NiklasJonsson Jul 15, 2022
72f0146
Implement get_many_index_mut & and add docs
NiklasJonsson Jul 15, 2022
5795fe3
fix comment and add test for many_index oob
NiklasJonsson Jul 15, 2022
dfa303b
change oob panic to debug assert
NiklasJonsson Jul 15, 2022
9f800a0
Avoid using format features not preset on MSRV
NiklasJonsson Jul 16, 2022
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
33 changes: 33 additions & 0 deletions src/map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,39 @@ where
}
}

pub fn get_many_mut<'a, 'b, Q: ?Sized, const N: usize>(
&'a mut self,
keys: [&'b Q; N],
) -> Option<[&'a mut V; N]>
where
Q: Hash + Equivalent<K>,
{
let indices = keys.map(|key| self.get_index_of(key));
if indices.iter().any(Option::is_none) {
return None;
}
let indices = indices.map(Option::unwrap);

// SAFETY: Can't allow duplicate indices as we would return several mutable refs to the same data
for i in 0..N {
let idx = indices[i];
if indices[i + 1..N].contains(&idx) {
return None;
}
}

let entries = self.as_entries_mut();
let out = indices.map(|i| {
// SAFETY: OK to discard mutable borrow lifetime as each index is unique
#[allow(unsafe_code)]
unsafe {
&mut *(&mut entries[i].value as *mut V)
Copy link
Member

Choose a reason for hiding this comment

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

This still creates aliasing &mut between entries and the previous iterations, which is not allowed. I suggest using self.as_entries_mut().as_mut_ptr() to start with a pointer, then &mut (*entries_ptr.add(i)).value for each index.

We probably need a manual bounds-check too, but that can be part of the loop scanning for duplicates. In theory, get_index_of should always be in-bounds, but better safe than sorry...

Copy link
Author

Choose a reason for hiding this comment

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

I don't understand how it can create aliasing &mut for entries. I only get a &mut to a specific element of entries (only the value member of that element) and while that references has a lifetime of entries, it is not mutably borrowing all of entries, no? Maybe I'm misunderstand reference/lifetime semantics and what is allowed. Using the pointer doesn't affect the guarantee of the function: each &mut reference that comes out of this function is unique so what difference will it make?

We probably need a manual bounds-check too...

Yeah that makes sense if pointers are the way to go but it should still panic rather than return None, I assume?

Copy link
Author

Choose a reason for hiding this comment

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

By the way, I did run miri on the tests like:

# rustup +nightly component add miri
cargo clean
cargo +nightly miri test

but I know miri doesn't catch all cases of undefined behaviour.

Copy link
Author

Choose a reason for hiding this comment

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

Actually, maybe I understand. The variable entries is &mut [Bucket<K, V>] so it is a mutable reference the whole slice but the array out also holds mutable references to individual elements, which alias with entries. Is this what you are referring to?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's it. I'm a little surprised that miri doesn't see any problem, but I think it's because slice indexing is an intrinsic operation in MIR, so miri doesn't really see any use of the entire entries. If you force it to go through the IndexMut trait, entries.index_mut(i), then it does complain:

test map::tests::many_mut_multi_success ... error: Undefined Behavior: trying to reborrow <390898> for SharedReadOnly permission at alloc159164[0xc], but that tag does not exist in the borrow stack for this location
    --> /home/jistone/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/cmp.rs:1531:27
     |
1531 |             PartialEq::eq(*self, *other)
     |                           ^^^^^
     |                           |
     |                           trying to reborrow <390898> for SharedReadOnly permission at alloc159164[0xc], but that tag does not exist in the borrow stack for this location
     |                           this error occurs as part of a reborrow at alloc159164[0xc..0x10]
     |
     = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
     = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <390898> was created by a retag at offsets [0xc..0x10]
    --> src/map.rs:502:19
     |
502  |           let out = indices.map(|i| {
     |  ___________________^
503  | |             // SAFETY: OK to discard mutable borrow lifetime as each index is unique
504  | |             #[allow(unsafe_code)]
505  | |             unsafe {
506  | |                 &mut *(&mut entries.index_mut(i).value as *mut V)
507  | |             }
508  | |         });
     | |__________^
help: <390898> was later invalidated at offsets [0x0..0x40]
    --> src/map.rs:506:29
     |
506  |                 &mut *(&mut entries.index_mut(i).value as *mut V)
     |                             ^^^^^^^^^^^^^^^^^^^^
     = note: backtrace: [...]
    --> src/map/tests.rs:449:5
     |
449  |     assert_eq!(map.get_many_mut([&1, &1123]), Some([&mut 10, &mut 100]));
     |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

We probably need a manual bounds-check too...

Yeah that makes sense if pointers are the way to go but it should still panic rather than return None, I assume?

Yeah, if get_index_of returns a bad index, then we have internal errors, so a panic is fine.

}
});

Some(out)
}

/// Remove the key-value pair equivalent to `key` and return
/// its value.
///
Expand Down
61 changes: 61 additions & 0 deletions src/map/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,3 +418,64 @@ fn from_array() {

assert_eq!(map, expected)
}

#[test]
fn many_mut_empty() {
let mut map: IndexMap<u32, u32> = IndexMap::default();
assert!(map.get_many_mut([&0, &1, &2, &3]).is_none());
}

#[test]
fn many_mut_single_fail() {
let mut map: IndexMap<u32, u32> = IndexMap::default();
map.insert(1, 10);
assert!(map.get_many_mut([&0]).is_none());
}

#[test]
fn many_mut_single_success() {
let mut map: IndexMap<u32, u32> = IndexMap::default();
map.insert(1, 10);
assert_eq!(map.get_many_mut([&1]), Some([&mut 10]));
}

#[test]
fn many_mut_multi_success() {
let mut map: IndexMap<u32, u32> = IndexMap::default();
map.insert(1, 10);
map.insert(1123, 100);
map.insert(321, 20);
map.insert(1337, 30);
assert_eq!(map.get_many_mut([&1, &1123]), Some([&mut 10, &mut 100]));
assert_eq!(map.get_many_mut([&1, &1337]), Some([&mut 10, &mut 30]));
assert_eq!(
map.get_many_mut([&1337, &321, &1, &1123]),
Some([&mut 30, &mut 20, &mut 10, &mut 100])
);
}

#[test]
fn many_mut_multi_fail_missing() {
let mut map: IndexMap<u32, u32> = IndexMap::default();
map.insert(1, 10);
map.insert(1123, 100);
map.insert(321, 20);
map.insert(1337, 30);
assert_eq!(map.get_many_mut([&121, &1123]), None);
assert_eq!(map.get_many_mut([&1, &1337, &56]), None);
assert_eq!(map.get_many_mut([&1337, &123, &321, &1, &1123]), None);
}

#[test]
fn many_mut_multi_fail_duplicate() {
let mut map: IndexMap<u32, u32> = IndexMap::default();
map.insert(1, 10);
map.insert(1123, 100);
map.insert(321, 20);
map.insert(1337, 30);
assert_eq!(map.get_many_mut([&1, &1]), None);
assert_eq!(
map.get_many_mut([&1337, &123, &321, &1337, &1, &1123]),
None
);
}