lun3x
get_mut_ should not be deprecated (and should be safe?)
Hi,
I am trying to use this library, and the modify_by_ methods are sometimes impossible to use (or I may have missed something).
Closures in general are a bit of a pain in Rust. In particular, the modify_by_ methods are not usable with await or Results. For example these two patterns do not seem feasible:
multi.modify_by_id(1, |obj| obj.refresh().await);
multi.modify_by_id(1, |obj| obj.refresh()?);
This leads me to believe that the get_mut methods should not be deprecated as they are the only way to implement this pattern. Also, it brings multi index map closer to a simple HashMap (which is the data structure I was trying to replace in my code).
I would even argue that get_mut methods shouldn't be unsafe, since, as far as I understand it, it would only break some invariants of the multi index. I believe that unsafe should be reserved for code that has the potential to break rust's safety guarantees. For example, BTreeMap's doc says that it is possible modify a key of the tree map, and that the result is unspecified. Still, it does not make BTreeMap unsafe:
It is a logic error for a key to be modified in such a way that the key’s ordering relative to any other key, as determined by the Ord trait, changes while it is in the map. This is normally only possible through Cell, RefCell, global state, I/O, or unsafe code. The behavior resulting from such a logic error is not specified, but will be encapsulated to the BTreeMap that observed the logic error and not result in undefined behavior. This could include panics, incorrect results, aborts, memory leaks, and non-termination
https://doc.rust-lang.org/stable/std/collections/struct.BTreeMap.html
Apologies I had missed your comment until now. I didn't consider async use of this crate, I agree it seems like it would cause issues. You make a good point about the BTreeMap documentation. I will double check that no unsound behaviour could occur, but I think the only potential issues would be the same as a BTreeMap. I will have a think and get back to you here.
Okay so to clarify: get_mut_ methods should only be used to mutate unindexed fields (comparable to using the update_by_ methods). If an indexed field is mutated through get_mut_ the internal indexes will not be updated and panics/incorrect results will follow.
After the element is mutated in the modify_by_ closure we must update the internal indexes. The alternative to a closure here would be to force the user to remember to call a hypothetical "update_indexes" method after a get_mut call where the indexed fields are mutated. I'm not a fan of this approach. Perhaps a (slightly clunky) middle ground is a separate modify_by_async_ method where the closure can be an async closure.
Another alternative would be changing get_mut to take ownership of self and transform the whole MultiIndexMap into a DirtyMultiIndexMap that could be transformed back through a update_indexes(self: DirtyMultiIndexMap) -> MultiIndexMap. Although I'm not sure I like this either.