Add or_default to Entry APIs

[jonhoo]

As argued for in #44324, this PR adds a new or_default method to the various Entry APIs (currently just for BTreeMap and HashMap) when V: Default. This method is effectively a shorthand for or_insert_with(Default::default).

[rust-highfive]

r? @BurntSushi

(rust_highfive has picked a reviewer for you, use r? to override)

[kennytm]

The link label is wrong.

[kennytm]

You need to add #![feature(entry_or_default)] to the doc test, otherwise it won't pass.

/// ```
/// #![feature(entry_or_default)]
/// # fn main() {                                   // <-- note that the `fn main` is required
/// use std::collections::BTreeMap;
/// // etc.
/// assert_eq!(map["poneyland"], None);
/// # }
/// ```

[kennytm]

What's the point of let s = "hoho".to_string()?

[kennytm]

(ditto for everything)

[jonhoo]

@kennytm the let s = "hoho" part was copied from the or_insert_with doctest. I'd be happy to change it, but figured I'd try to keep the code examples as similar as possible?

[kennytm]

@jonhoo or_insert doesn't have the "hoho" either.

[jonhoo]

I guess or_insert_with should probably also be changed then. Though I feel like that shouldn't be done in this PR?

[kennytm]

@jonhoo or_insert_with actually uses the variable s so the "hoho" is valid there, although the current example makes no sense 😄. Yes I think it is out of scope of this PR.

[jonhoo]

@arielb1 why S-waiting-on-author? I have removed s = "hoho", and addressed the other comments. Currently just waiting on Travis to give the green light.

[vitalyd]

'or_default' doesn't immediately convey that an insertion happens. In fact one can misconstrue this as returning a default value without inserting anything.

Also, meta question - what's the bar for adding purely convenience APIs? I'd imagine it has to be fairly high to warrant putting oneself on the hook for documenting, testing, and otherwise maintaining these convenience methods. Users are also forced to see a larger API surface and the "important" bits may get buried amidst convenience functions.

[jonhoo]

@vitalyd we could make it or_insert_default instead if you think that'd be better?

[vitalyd]

@jonhoo, I'm just sideline commenting :). However, if I did have a horse in the race, I think I'd want the insert part to be prominent in the name (especially since the other methods have that, so symmetry, if nothing else, is nice).

I think the bigger question is the bar for convenience methods, as mentioned. My own $.02 is this doesn't meet that due to how trivial, albeit slightly more wordy, the existing usage is today. But I don't know what core libs team's threshold is for that.

[kennytm]

I'm slightly against calling it .or_insert_default() because it is too long, I think .or_default() is fine 😆.

@vitalyd There are {Option,Result}::unwrap_or_default and Vec::resize_default (#41758), so there's precedent for these convenient methods.

[vitalyd]

@kennytm, fair enough. The Vec::resize_all may allow further optimizations though based on that issue (didn't read beyond that so don't know if that's actually true or not). So something that's convenient but can also exploit internals to optimize better is almost definitely a win.

Option/Result unwrap_or_default is probably more common than Entry usage, but I don't have stats to back that up.

[jonhoo]

I guess this should be changed to S-waiting-on-review then? /cc @arielb1 @Mark-Simulacrum

[H2CO3]

@jonhoo In the same style, one could add map_or_default() to Option<T: Default>, and maybe Result<T: Default, E> as well. I happen to have needed this functionality literally yesterday, and I think it would be nice to have too.

[jonhoo]

@H2CO3: I really like that idea! Though isn't that what unwrap_or_default does? Or am I missing something?

[H2CO3]

@jonhoo unwrap_or_default() doesn't accept a transformation. What I wanted to do is essentially equivalent with:

let option = Some("foo");                                        
let len_or_zero = option.map(str::len).unwrap_or_default();      

Using map_or_default(), this could be simplified as:

let option = Some("foo");                                        
let len_or_zero = option.map_or_default(str::len);

An implementation could be literally:

impl<T> Option<T> {
    …
    pub fn map_or_default<F, U>(self, f: F) -> U
        where F: FnOnce(T) -> U,
              U: Default {

        self.map(f).unwrap_or_default()
    }
    …
}

[jonhoo]

@H2CO3 I suspect that map_or_default would be considered too close to map + unwrap_or_default for it to be worth adding, but @steveklabnik might have thoughts on that?

[H2CO3]

@jonhoo I understand; but in my opinion the point is not that it's hard to implement. It's as simple to implement as map() and similar utility methods themselves. It's just that it adds elegance to the code and removes a bit of mental pressure which can be beneficial for reading, understanding, and correctly writing code.

Option<T> already has such small – and not always orthogonal – methods which would be not hard to write – or even just decompose into a match inline –, so I think this has reasonable precedent. I also feel that this is a relatively common use case based on my personal experience.

[carols10cents]

ping for review @BurntSushi !

[BurntSushi]

My own personal opinion is that there is a cost to the number of methods we add to these APIs, but we've traditionally been pretty loose with adding convenience methods (like this one) as long as one can articulate a reasonable use case for them. I do think there is room for us to get more principled on this (for example, a tool that recognizes code that can be replaced with a hypothetical convenience method in the ecosystem could tell us how common something really is), but I don't think we need to litigate that on this PR.

I also feel like the name or_default is reasonable in this context, particularly given the type signature. But since this is unstable, we can always bikeshed the name during stabilization!

@bors r+

[bors]

📌 Commit 9389d26 has been approved by BurntSushi

[bors]

⌛ Testing commit 9389d26 with merge 817e1b8...

[bors]

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