Implement new method HashMap.insert_or_update_with()

[lilyball]

std::hashmap::HashMap.insert_or_update_with() is basically the opposite
of find_or_insert_with(). It inserts a given key-value pair if the key
does not already exist, or replaces the existing value with the output
of the passed function if it does.

This is useful because replicating this with existing functionality is awkward, especially with the current borrow-checker. In my own project I have code that looks like

if match map.find_mut(&key) {
    None => { true }
    Some(x) => { *x += 1; false }
} {
    map.insert(key, 0);
}

and it took several iterations to make it look this good. The new function turns this into

map.insert_or_update_with(key, 0, |_,x| *x += 1);

[erickt]

Another implementation of this could be:

map.insert(key, map.pop(&key).map_default(0, |x| *x + 1))

But your way is much more elegant.

[erickt]

That said, why do you return the value instead of a bool like HashMap.insert? Is there a precedent to that style? If so, does that precedent also hold for HashMap.insert? Or if there is none, should insert_or_modify_with return bool instead?

Other than this question, this request looks good to me.

[lilyball]

I actually don't care about the return value at all, but I returned it for parity with find_or_insert_with(). Returning the value seems slightly more useful than returning bool; if I want a bool value I can always modify a captured variable in the passed lambda.

[lilyball]

Of course, find_or_insert() is a "find" function, where returning the value is part of its core functionality. insert_or_modify_with() isn't, so the return value choice here is much less obvious. The more I think about it, the more a bool return value actually seems sensible, for parity with insert().

[erickt]

@kballard and I talked things through on IRC, but we didn't come up with a good conclusion. Haskell's Data.Map has a couple similar functions, namely insertWith and insertLookupWithKey. A slight variation on this function could cover all the use cases of Haskell's functions:

fn insert_with(&mut self, key: K, value: V, f: &fn(&V, &V) -> V) -> Option<V>
fn insert_with_key(&mut self, key: K, value: V, f: &fn(&K, &V, &V) -> V) -> Option<V>

where the first argument of the insert_with closure is the old value, the second is the new value, and the return value is the old value if existed. insert_with_key also includes the key if necessary. The downside to this is that it makes @kballard's example program:

map.insert_with(key, 0, |x, _| *x + 1);

Which is a little uglier. I don't think this is that bad. What does everyone else think?

[lilyball]

After thinking about this some more, one worry I have with the new functions is it won't let me modify the existing value, instead I have to create a brand new one. So I can't say something like

map.insert_with(key, ~[5], |x, _| { x.push(5); x })

[nikomatsakis]

I think the most general pattern is this:

fn mangle(
  &mut self,
  k: K,
  not_found: &fn(&K) -> V,
  found: &fn(&K, &mut V)) -> bool

and we should add that, and then we can build find_or_insert and find_or_insert_with on top of it.

[nikomatsakis]

This code, incidentally, becomes:

map.mangle(key, |_| 0, |_, x| *x += 1)

[nikomatsakis]

I guess I have no strong opinion about what mangle should return, perhaps &mut V would be best, since this function is all about maximum flexibility.

[nikomatsakis]

and the proper name is probably insert_or_update

[lilyball]

@nikomatsakis: Building find_or_insert on top of mangle is actually non-trivial, since it needs to return the found/inserted value. I don't see any easy way to do that without performing a second find after the mangle, which is rather non-optimal.

Two alternatives I can see are either to have mangle return the bucket index, which would require it to be a private function (did you intend for it to be private or public?), or to have it return &'a V.

[lilyball]

Also, presumably it should be insert_or_update_with, since the other functions that take a closure have the _with suffix.

[lilyball]

AFAIK I can't actually build find_or_insert and friends on top of mangle without working once functions.

self.mangle(k, |_| v, |_,_| ())

complains (rightly) that |_| v is trying to move a value from its environment.

[lilyball]

Looks like I can do the job using Option and swap_unwrap, but I assume this is now producing suboptimal code.

[nikomatsakis]

A couple of comments:

1. Make mangle return &mut V, not a hashtable index. It's supposed to be something an outside user would use so it should not expose an internal implementation detail, I don't think.
2. While you're modifying this stuff, make find_or_insert_with return &mut V, not &V (issue find_or_insert_with should return a &mut pointer #6394).
3. This is a good example where once fns would be useful, which is nice. For the time being, you can address the inefficiency by modifying the signature of mangle to take an extra "user-supplied argument" as follows:

fn mangle<A>(
  &mut self,
  k: K,
  a: A,
  not_found: &fn(&K, A) -> V,
  found: &fn(&K, &mut V, A)) -> bool

in which case you can write find_or_insert as follows:

self.mangle(k, v, |_, v| v, |_,_,_| ())

[lilyball]

I wasn't sure if you intended for it to be public or private. I'm happy making it public.

[lilyball]

The latest commits make the suggested changes.

[erickt]

@kballard: Awesome, thanks for doing this!