Implement get_disjoint_mut (previously get_many_mut) #238

NiklasJonsson · 2022-07-14T19:51:11Z

std::collections::HashMap is adding get_many_mut in rust-lang/rust#97601 and I'm working on replacing some uses of that hash map with this one where some of the code uses get_many_mut (currently only a single location in rustc) so I would appreciate it if indexmap::IndexMap could provide the same interface.

This code uses a decent amount of unsafe compared to the rest of the file and I saw the comment in lib.rs on having almost all unsafe code in raw.rs but I couldn't figure out how to move parts of my implementation there as it is mostly dealing with initializing the array. I tried to use array::map to avoid all unsafe but couldn't quite get it to work with the lifetimes and the lambda.

If you want this change, I'll also write some docs.

cuviper · 2022-07-15T00:40:51Z

I think ideally we would want two things for this, especially the first:

Stabilizing HashMap::get_many_mut so we can be sure to match that API as much as possible.
Stabilizing <[T]>::get_many_mut (still new in Add slice methods for indexing via an array of indices. rust-lang/rust#83608) to deal with all the unsafe aspects. We might also want to wrap that into our own get_many_index_mut, just like we have get_index and get_index_mut for slice-like indexing.

I tried to use array::map to avoid all unsafe but couldn't quite get it to work with the lifetimes and the lambda.

I suspect part of this is because you should be using a raw pointer to offset/index each of the elements, otherwise you have the main &mut [T] aliasing each of the &mut T you pull out while you're working. And of course, using raw pointers will still need unsafe to form the final references.

NiklasJonsson · 2022-07-15T12:29:42Z

Stabilizing HashMap::get_many_mut so we can be sure to match that API as much as possible.

Yeah, this makes a lot of sense. Is it okay to just leave the PR open until then?

Stabilizing <[T]>::get_many_mut

Haven't seen this before but it would indeed fit pretty well. It seems to have been open for quite a while without resolution though. Also, if they opt for a result-based return value, that'd not match the hashmap api (as it is now) so we'd have to do conversions in the implementation.

I suspect part of this is because you should be using a raw pointer to offset/index each of the elements, otherwise you have the main &mut [T] aliasing each of the &mut T you pull out while you're working. And of course, using raw pointers will still need unsafe to form the final references.

Yeah, exactly, the lambda captures the whole array lifetime mutably and can't be invoked multiple times. I did not consider pointer arithmetic though, I'll experiment a bit and see how it looks. Probably similar to the <[T]>::get_many_mut examples 🙂

cuviper · 2022-07-15T16:44:21Z

src/map.rs

+            // SAFETY: OK to discard mutable borrow lifetime as each index is unique
+            #[allow(unsafe_code)]
+            unsafe {
+                &mut *(&mut entries[i].value as *mut V)


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...

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?

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.

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?

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.

cuviper · 2022-07-15T16:45:39Z

Is it okay to just leave the PR open until then?

Yeah, that's fine. I should probably make a label for this...

src/map.rs

jyn514 · 2022-07-16T13:18:59Z

I think ideally we would want two things for this, especially the first: Stabilizing HashMap::get_many_mut so we can be sure to match that API as much as possible.

Can IndexMap add it under a nightly or unstable opt-in cargo feature? Then people will have to opt in to breaking changes, and you can document that changes to this API won't result in a new major version.

(Or if you don't feel strongly, you can just make a new major release if the HashMap API changes for some reason.)

cuviper · 2022-07-19T00:14:57Z

I don't like using nightly/unstable features, in part because it's an extra maintenance burden, but also because the feature may be opted-in on your behalf, arbitrarily deep in your dependency tree.

I'd rather not set us up for needing a semver bump either. If this is something that folks really need, let's apply that pressure on std stabilization. :)

cuviper · 2025-02-25T01:28:47Z

Rust 1.86 (now in beta) will stabilize this as get_disjoint_mut with two different styles:

For [T]: https://doc.rust-lang.org/beta/std/primitive.slice.html#method.get_disjoint_mut

pub fn get_disjoint_mut<I, const N: usize>(
    &mut self,
    indices: [I; N],
) -> Result<[&mut <I as SliceIndex<[T]>>::Output; N], GetDisjointMutError>
where
    I: GetDisjointMutIndex + SliceIndex<[T]>,

For HashMap<K, V>: https://doc.rust-lang.org/beta/std/collections/struct.HashMap.html#method.get_disjoint_mut

pub fn get_disjoint_mut<Q, const N: usize>(
    &mut self,
    ks: [&Q; N],
) -> [Option<&mut V>; N]
where
    K: Borrow<Q>,
    Q: Hash + Eq + ?Sized,

We can decide for ourselves whether it makes sense to build on the standard library, or (unsafely) implement our own. GetDisjointMutIndex won't be stable though, nor the old SliceIndex, so if we want to replicate that support for both usize and ranges, then I think we'll be on our own anyway.

@NiklasJonsson it's been some time -- are you still interested in working on this?

cuviper · 2025-02-25T01:36:57Z

We can decide for ourselves whether it makes sense to build on the standard library, or (unsafely) implement our own. GetDisjointMutIndex won't be stable though, nor the old SliceIndex, so if we want to replicate that support for both usize and ranges, then I think we'll be on our own anyway.

Either way, I'd be fine with supporting just usize to start with, as long as there's a possibility to add ranges later. But even that could be separate, like how we have get_index_mut and get_ranges_mut covering the same as slice get_mut (and our get_mut matches HashMap). In other words, we could leave that extension to a future get_disjoint_ranges_mut.

NiklasJonsson · 2025-02-25T06:02:39Z

Yeah I've been following the stabilization and I'd like to finish this up. I'll have some time this weekend to have a deeper look but I think I'll favor supporting less to start out and then I'll see if I have time to do more.

…

On Tue, Feb 25, 2025, 02:29 Josh Stone ***@***.***> wrote: Rust 1.86 (now in beta) will stabilize this as get_disjoint_mut with two different styles: For [T]: https://doc.rust-lang.org/beta/std/primitive.slice.html#method.get_disjoint_mut pub fn get_disjoint_mut<I, const N: usize>( &mut self, indices: [I; N],) -> Result<[&mut <I as SliceIndex<[T]>>::Output; N], GetDisjointMutError>where I: GetDisjointMutIndex + SliceIndex<[T]>, For HashMap<K, V>: https://doc.rust-lang.org/beta/std/collections/struct.HashMap.html#method.get_disjoint_mut pub fn get_disjoint_mut<Q, const N: usize>( &mut self, ks: [&Q; N],) -> [Option<&mut V>; N]where K: Borrow<Q>, Q: Hash + Eq + ?Sized, We can decide for ourselves whether it makes sense to build on the standard library, or (unsafely) implement our own. GetDisjointMutIndex won't be stable though, nor the old SliceIndex, so if we want to replicate that support for both usize and ranges, then I think we'll be on our own anyway. @NiklasJonsson <https://github.com/NiklasJonsson> it's been some time -- are you still interested in working on this? — Reply to this email directly, view it on GitHub <#238 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADNRXP6VM3HKI4SGOGBRNOD2RPBONAVCNFSM6AAAAABXZNCFMWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMOBQGEZDQMRTGI> . You are receiving this because you were mentioned.Message ID: ***@***.***> [image: cuviper]*cuviper* left a comment (indexmap-rs/indexmap#238) <#238 (comment)> Rust 1.86 (now in beta) will stabilize this as get_disjoint_mut with two different styles: For [T]: https://doc.rust-lang.org/beta/std/primitive.slice.html#method.get_disjoint_mut pub fn get_disjoint_mut<I, const N: usize>( &mut self, indices: [I; N],) -> Result<[&mut <I as SliceIndex<[T]>>::Output; N], GetDisjointMutError>where I: GetDisjointMutIndex + SliceIndex<[T]>, For HashMap<K, V>: https://doc.rust-lang.org/beta/std/collections/struct.HashMap.html#method.get_disjoint_mut pub fn get_disjoint_mut<Q, const N: usize>( &mut self, ks: [&Q; N],) -> [Option<&mut V>; N]where K: Borrow<Q>, Q: Hash + Eq + ?Sized, We can decide for ourselves whether it makes sense to build on the standard library, or (unsafely) implement our own. GetDisjointMutIndex won't be stable though, nor the old SliceIndex, so if we want to replicate that support for both usize and ranges, then I think we'll be on our own anyway. @NiklasJonsson <https://github.com/NiklasJonsson> it's been some time -- are you still interested in working on this? — Reply to this email directly, view it on GitHub <#238 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADNRXP6VM3HKI4SGOGBRNOD2RPBONAVCNFSM6AAAAABXZNCFMWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMOBQGEZDQMRTGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

cuviper · 2025-02-25T18:13:36Z

Sounds great, thanks! And please don't take that as any pressure to do this quickly -- we can take our time and get it right.

cuviper · 2025-02-25T19:16:40Z

2. We might also want to wrap that into our own get_many_index_mut,

Correcting myself -- this should be plural, so now it would be get_disjoint_indices_mut.

NiklasJonsson · 2025-03-01T16:58:20Z

@cuviper Just to verify that I understand what you mean. An alright initial implementation would be:

The key-based get_disjoint_mut:

pub fn get_disjoint_mut<Q, const N: usize>(
    &mut self,
    ks: [&Q; N],
) -> [Option<&mut V>; N]
where
    K: Borrow<Q>,
    Q: Hash + Eq + ?Sized,

A get_disjoint_indices_mut that takes a usize array (pretty much the current get_many_index_mut)

and then we can leave the implementation of get_disjoint_mut with ranges/for a future PR? If so, then it seems to be mostly about renaming the functions.

NiklasJonsson · 2025-03-01T17:47:31Z

Just to verify that I understand what you mean. An alright initial implementation would be:
...

I went ahead and implemented what I understood you to mean 🙂 Also added some extra tests.

cuviper · 2025-03-01T18:47:35Z

src/map.rs

+    /// let mut map = indexmap::IndexMap::from([(1, 'a'), (3, 'b'), (2, 'c')]);
+    /// assert_eq!(map.get_disjoint_mut([&2, &1]), Some([&mut 'c', &mut 'a']));
+    /// ```
+    pub fn get_disjoint_mut<Q, const N: usize>(&mut self, keys: [&Q; N]) -> Option<[&mut V; N]>


To match HashMap, this should move the Option inward, returning [Option<&mut V>; N]. Unfortunately, the slice version is all-or-nothing though, so we can't directly rely on that, but we can probably have some internal method doing the unsafe part for either way.

Also, HashMap panics if there's any overlap. The bounds-check panic you have is fine too, since that shouldn't happen normally, but possibly could if we have a logic error somewhere.

Aah right, I totally missed this part of the signature. I'll study the API in more detail.

I noticed now that indexmap uses the Equivalent<K> bound rather than Eq + Borrow like the stdlib hashmap. I assume that I should keep using Equivalent for this function as well? As far as I understand, that only makes indexmap more flexible than the stdlib hashmap API. Everything the Eq + Borrow combo supports should also be supported with Equivalence. Am I correct?

Yeah, that's right, keep using the Equivalent bound.

so we can't directly rely on that, but we can probably have some internal method doing the unsafe part for either way.

The internal helper ended up like this:

pub(crate) fn get_disjoint_opt_mut<const N: usize>( &mut self, indices: [Option<usize>; N], ) -> Result<[Option<(&K, &mut V)>; N], GetDisjointMutError> {

and does most of the heavy-lifting for both of the methods. Does it seem alright to you? I don't feel great about wrapping indices in Some and then unwrapping after calling the helper in Slice::get_disjoint_mut but it does make the outer functions share most of the tricky code.

cuviper · 2025-03-01T18:54:31Z

src/map.rs

+    pub fn get_disjoint_indices_mut<const N: usize>(
+        &mut self,
+        indices: [usize; N],
+    ) -> Option<[(&K, &mut V); N]> {


To mimic the slice version, but without the generic usize/range output, this should return Result<[(&K, &mut V); N], GetDisjointMutError>. It will have to be our own version of that error for now, but maybe in the future we'll have indexmap v3 where we can switch to std's.

It would be nice to have this method on map::Slice too -- perhaps primarily implemented there and then forwarded from here with as_mut_slice().

cuviper · 2025-03-01T19:00:28Z

src/map.rs

+        let out = indices.map(|i| {
+            // SAFETY: The base pointer is valid as it comes from a slice and the deref is always
+            // in-bounds as we've already checked the indices above.
+            #[allow(unsafe_code)]


Nit: I think this allow should go around the entire method. While the actual unsafe operation is narrow, the SAFETY requirements need care in the whole thing, and the allowed lint is a good signal for that.

NiklasJonsson force-pushed the get_many_mut branch 2 times, most recently from 0ef0e90 to d3f1968 Compare July 14, 2022 19:53

cuviper reviewed Jul 15, 2022

View reviewed changes

cuviper added the waiting-for-std Changes waiting for stabilization in the standard library, so we can match the API. label Jul 15, 2022

NiklasJonsson commented Jul 15, 2022

View reviewed changes

src/map.rs Outdated Show resolved Hide resolved

NiklasJonsson mentioned this pull request Jul 16, 2022

[WIP] Remove allow(rustc::potential_query_instability) in rustc_session rust-lang/rust#99065

Closed

NiklasJonsson mentioned this pull request Sep 10, 2022

Audit and handle remaining cases of rustc::potential_query_instability lint (iterating HashMaps) rust-lang/rust#84447

Open

cuviper mentioned this pull request Mar 21, 2024

implement get_many_mut #324

Open

Implement get_disjoint_mut for arrays of keys

c6f35fd

NiklasJonsson force-pushed the get_many_mut branch from 9f800a0 to c6f35fd Compare March 1, 2025 17:44

NiklasJonsson changed the title ~~Implement get_many_mut~~ Implement get_disjoint_mut (previously get_many_mut) Mar 1, 2025

cuviper requested changes Mar 1, 2025

View reviewed changes

Address review feedback

11017c9

NiklasJonsson force-pushed the get_many_mut branch from 3ec4a1b to 11017c9 Compare March 2, 2025 14:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement get_disjoint_mut (previously get_many_mut) #238

Implement get_disjoint_mut (previously get_many_mut) #238

NiklasJonsson commented Jul 14, 2022

cuviper commented Jul 15, 2022 •

edited

Loading

NiklasJonsson commented Jul 15, 2022

cuviper Jul 15, 2022

NiklasJonsson Jul 15, 2022

NiklasJonsson Jul 15, 2022

NiklasJonsson Jul 15, 2022

cuviper Jul 15, 2022

cuviper commented Jul 15, 2022

jyn514 commented Jul 16, 2022

cuviper commented Jul 19, 2022

cuviper commented Feb 25, 2025

cuviper commented Feb 25, 2025

NiklasJonsson commented Feb 25, 2025 via email

cuviper commented Feb 25, 2025

cuviper commented Feb 25, 2025

NiklasJonsson commented Mar 1, 2025 •

edited

Loading

NiklasJonsson commented Mar 1, 2025 •

edited

Loading

cuviper Mar 1, 2025

cuviper Mar 1, 2025

NiklasJonsson Mar 1, 2025

NiklasJonsson Mar 1, 2025 •

edited

Loading

cuviper Mar 1, 2025

NiklasJonsson Mar 2, 2025

cuviper Mar 1, 2025

cuviper Mar 1, 2025

Implement get_disjoint_mut (previously get_many_mut) #238

Are you sure you want to change the base?

Implement get_disjoint_mut (previously get_many_mut) #238

Conversation

NiklasJonsson commented Jul 14, 2022

cuviper commented Jul 15, 2022 • edited Loading

NiklasJonsson commented Jul 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cuviper commented Jul 15, 2022

jyn514 commented Jul 16, 2022

cuviper commented Jul 19, 2022

cuviper commented Feb 25, 2025

cuviper commented Feb 25, 2025

NiklasJonsson commented Feb 25, 2025 via email

cuviper commented Feb 25, 2025

cuviper commented Feb 25, 2025

NiklasJonsson commented Mar 1, 2025 • edited Loading

NiklasJonsson commented Mar 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NiklasJonsson Mar 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cuviper commented Jul 15, 2022 •

edited

Loading

NiklasJonsson commented Mar 1, 2025 •

edited

Loading

NiklasJonsson commented Mar 1, 2025 •

edited

Loading

NiklasJonsson Mar 1, 2025 •

edited

Loading