Stabilize Result::map_or_else

[tesuji]

Stabilized this API:

impl<T, E> Result<T, E> {
    pub fn map_or_else<U, D: FnOnce(E) -> U, F: FnOnce(T) -> U>(self, default: D, f: F) -> U {
        match self {
            Ok(t) => f(t),
            Err(e) => default(e),
        }
    }
}

Closes #53268
r? @SimonSapin

[SimonSapin]

This matches the signature of already-stable Option::map_or_else (except that the default callback gets the error value as a parameter)

@rfcbot fcp merge

[rfcbot]

Team member @SimonSapin has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @Kimundi
• @KodrAus
• @SimonSapin
• @alexcrichton
• @dtolnay
• @sfackler
• @withoutboats

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[timvermeulen]

What was wrong with the previous implementation?

[tesuji]

The new implementation is consistent with Option::map_or_else: https://doc.rust-lang.org/nightly/src/core/option.rs.html#491-496

So we are following Principle of Least Surprise.

[timvermeulen]

Even less Surprising would be to not change an implementation in a stabilization PR.

Also, IMO the original implementation was better because it more clearly shows the relation between map_or_else, map, and unwrap_or_else. The whole point of these methods is that we don't have to write matches everywhere.

[tesuji]

If you like it, we could send a PR later to fix it in both Option and Result.

[KatsuoRyuu]

@timvermeulen That might be true, but following the paradigm of keeping things simple and consistent is also needed. The original idea was to create map_or_else and unwrap_or_else the same as on Option, and then things get switched around. That is just absurd, and makes it harder to deal with because you to constantly ask yourself, "which way arround was this command?!" - and honestly i dont have time for that in a progeramming language.

[KatsuoRyuu]

@tomwhoiscontrary I think that question has been asked before, or a derivative thereof. The conclusion was that Options map_or_else is already in stable and would therefore not be changed, as that would be breaking much more.
Part of the discussion is in here #53268

[tomwhoiscontrary]

@KatsuoRyuu My question is the one i asked - why is Option::map_or_else the way it is. I am not suggesting changing it. I want to understand why it is the way it is.

[KatsuoRyuu]

@tomwhoiscontrary sorry for the misunderstanding that, will see if I can find the documentation for that for you, I think I saw it somewhere ;)

[KatsuoRyuu]

@tomwhoiscontrary this is the thread I remembered seeing, rust-lang/rfcs#1025. So the reason for the initially is to follow the convention of map_or, so all parameters are always prepended.

[tomwhoiscontrary]

Thanks! The explanation makes sense. If we had keyword arguments, this problem would disappear!

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[NilsIrl]

Shouldn't the arguments be the other way round to match those of Option?

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

[tesuji]

r? @dtolnay

[dtolnay]

Thanks!

Relatedly, I wish that we had more realistic examples for these type of combinator methods. The current example code is:

let k = 21;

let x : Result<_, &str> = Ok("foo");
assert_eq!(x.map_or_else(|e| k * 2, |v| v.len()), 3);

which is so contrived that it doesn't help you understand how the method is intended to be used.

It would be better to find real uses in the wild and distill them into example code. Something like this is where I would expect to encounter map_or_else:

    ...
    .map_or_else(Response::DatabaseError, |id| {
        if id.is_empty() {
            Response::Empty
        } else {
            Response::User(id.to_string())
        }
    })

[dtolnay]

@bors r+

[bors]

📌 Commit c06a8ea has been approved by dtolnay

[bors]

⌛ Testing commit c06a8ea with merge 7d761fe...

[bors]

☀️ Test successful - checks-azure
Approved by: dtolnay
Pushing 7d761fe to master...