Implement and_modify on Entry

[mchlrhw]

Motivation

Entrys are useful for allowing access to existing values in a map while also allowing default values to be inserted for absent keys. The existing API is similar to that of Option, where or and or_with can be used if the option variant is None.

The Entry API is, however, missing an equivalent of Option's and_then method. If it were present it would be possible to modify an existing entry before calling or_insert without resorting to matching on the entry variant.

Tracking issue: #44733.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @BurntSushi (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[kennytm]

What about BTreeMap?

[mchlrhw]

Sorry I had overlooked BTreeMap, but yes you're right, its Entry deserves this too 😄.
I think in general I would expect both Entry APIs to be as similar to Option as possible, but since I'm new to contributing I didn't want to suggest too many changes at once.

[BurntSushi]

@mchlrhw Thanks for submitting this! I'd like to get @rust-lang/libs's opinion on adding methods like this to Entry before merging. Are we content with loading up the Entry type with various combinators? I kind of feel like that's been our traditional precedent, but I'd like to double check.

Also, I'm not exactly sure if the name and_then is quite right for this? The type of the closure on Option::and_then for example is fn(T) -> Option<U>, but the signature used here is basically fn(&mut T). It doesn't feel quite the same to me.

[mchlrhw]

I would like to match Option as much as possible, but the ownership of the map itself gets in the way a bit. I am happy to take suggestions, but the best I could come up with was implementing it as a closure that receives and returns an OccupiedEntry, which is not as ergonomic for the user. As for the name, maybe and_mut or and_then_mut?

[kennytm]

🚲🏠 .and_modify() / .and_update() / .and_change()?

map.entry("poneyland").and_modify(|e| *e.new = false).or_insert(Foo{new:true});

[mchlrhw]

.and_modify() gets my vote

[alexcrichton]

@mchlrhw mind elaborating on the rationale here as well a bit? The doc examples here aren't super convincing because that should work today as or_insert returns a mutable reference, so I'm curious in what situations you'd want to mutate-if-present and then also take a different action on a vacancy

[mchlrhw]

Ah yes, most of my rationale was in a pre-RFC, which I haven't linked to yet, so let me fix that: https://internals.rust-lang.org/t/pre-rfc-and-then-method-on-std-collections-hash-map-entry/5918/2.

[mchlrhw]

The main purpose is to be able to modify existing entries before you attempt an insert. If your use case allows insert first and then modify then sure, you can use the current API, but if you need to track new state then *map.entry("poneyland").or_insert(Foo { new: true }).new = false; will never allow you to track new entries because they'll always be set to false.

[alexcrichton]

Ok thanks! I'm not 100% sold on the API but don't mind landing it as unstable, the name and_modify seems reasonable to me

[mchlrhw]

Cool, thanks! I'll change the name now and wait for more feedback on the API. I was worried it was a bit of a niche use case, but I found it useful for something I'm working on and hopefully someone else will find it useful too 😄. Do you think it's worth changing the doctests to use the struct Foo { new: bool } examples, or shall I leave it for now?

[BurntSushi]

@mchlrhw All righty, this seems fine to me then. Should this get an entry in the unstable book before merging?

[mchlrhw]

That's great, thanks! That question probably wasn't aimed at me, but if it turns out you do need an entry I'd be happy to write one.

[kennytm]

@mchlrhw Could you change the title of this PR and the tracking issue to mention and_modify instead of and_then?

[BurntSushi]

@mchlrhw I think we're trying to make sure there's an entry in the unstable book for every feature, and I think this PR introduces a new feature, so I think it would be good to add it in this PR! cc @steveklabnik

[kennytm]

@mchlrhw Ping. Could you address @BurntSushi's comment? The unstable book page can be written to src/doc/unstable-book/src/library-features/entry-and-modify.md.

[mchlrhw]

@kennytm Sorry for the delay. I was waiting to see what @steveklabnik's thoughts were. I've had a go at documenting it, but I'm not sure what sort of tone to use, or tense for that matter 😄. Also, I could do with some help with regards to the code snippets and the use of rust,ignore, etc. I wasn't quite sure how best to write them.

[kennytm]

@mchlrhw Instead of ```rust,ignore, you could use # to hide irrelevant code:

```rust
#![feature(entry_and_modify)]
# fn main(){use std::collections::HashMap;structFoo{new:bool,}letmut map:HashMap<&str,Foo> = HashMap::new();
map.entry("quux").and_modify(|e| *e.new = false).or_insert(Foo{new:true});
# }
``` 

[mchlrhw]

Ah, ok. I'll try and fix that

[mchlrhw]

Should I squash fix-up commits, or is that considered bad practice once a PR review has started?

[BurntSushi]

@mchlrhw This PR is small enough that I'm fine with just squashing this down!

[BurntSushi]

@mchlrhw The docs look great. Thank you!

[mchlrhw]

Thanks! Do I squash, or is that something that bors does as part of merging?

[kennytm]

@mchlrhw bors won't squash.

[BurntSushi]

@bors r+

[bors]

📌 Commit 9e36111 has been approved by BurntSushi

[bors]

⌛ Testing commit 9e36111 with merge a8feaee...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: BurntSushi
Pushing a8feaee to master...

[mchlrhw]

Thanks everyone!