Introduce the Result::into_inner method

[Kerollmops]

This method extract the value of either the Ok or the Err Result variant in the case where the variant types are the same.

// It is too boring to write this
let x = match Ok(16) {
    Ok(x) => x,
    Err(x) => x,
};

// this is much more convenient to write that
let x: Result<i32, i32> = Ok(16);
assert_eq!(x.into_inner(), 16);

let x: Result<i32, i32> = Err(42);
assert_eq!(x.into_inner(), 42);

It is inspired by the Either::into_inner method.

I found use cases, in particular when using slice::binary_search like functions which returns Result<usize, usize>. Note that it is always possible to apply logic on one or the other variant like so:

let res: Result<usize, usize> = unimplemented!();

// this maps the `Ok` part
let x = res.map(|i| i + 1).into_inner();

// this maps the `Err` part
let x = res.map_err(|i| i - 2).into_inner();

// returns a &usize
let x = res.as_ref().into_inner();

[rust-highfive]

r? @joshtriplett

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

[joshtriplett]

I don't think calling map_err followed by into_inner makes sense. res.map_err(|x| y).into_inner() is just res.unwrap_or_else(|x| y).

Beyond that, how often does this use case come up? You can always write res.unwrap_or_else(|x| x) for this case. And I don't know any common case other than binary_search that returns a Result<T, T>.

I think this method needs an RFC proposal, or at the very least a discussion on internals.

[Centril]

@joshtriplett we have merged larger bits without an RFC into the standard library so I don't think this needs an RFC; T-libs can just make a decision here.

[joshtriplett]

Fair enough. But I do feel that the use case needs more detailed justification, and more examples than just binary_search (which has been noted as an unusual case in the past).

[Kerollmops]

Just in the codebase of rust there is 5 possible candidates according to the result of this command:

rg -USP 'match\s+.+\s+{\s+Ok\((\w+)\)\s+=>\s+\1.*\s+Err\((\w+)\)\s+=>\s+\2'

# This command searches this pattern:
# match xxx {
#   Ok(a) => a,
#   Err(b) => b

Note that I removed unrelated results. I totaly agree that this is a much better candidate for the Either enum.

Another usage that I found is in my own library but it is related to something like binary search.

collapsed ripgrep output

src/libsyntax_pos/lib.rs
1332:    match lines.binary_search(&pos) {
1333:        Ok(line) => line as isize,
1334:        Err(line) => line as isize - 1

src/librustc_data_structures/sorted_map.rs
225:                match self.lookup_index_for(k) {
226:                    Ok(index) => index + 1,
227:                    Err(index) => index,
235:                match self.lookup_index_for(k) {
236:                    Ok(index) => index + 1,
237:                    Err(index) => index,

src/libcore/sync/atomic.rs
456:        match self.compare_exchange(current, new, order, strongest_failure_ordering(order)) {
457:            Ok(x) => x,
458:            Err(x) => x,
943:        match self.compare_exchange(current, new, order, strongest_failure_ordering(order)) {
944:            Ok(x) => x,
945:            Err(x) => x,

src/tools/clippy/clippy_lints/src/doc.rs
218:                    let index = match spans.binary_search_by(|c| c.0.cmp(&offset)) {
219:                        Ok(o) => o,
220:                        Err(e) => e - 1,

[Centril]

Hmm... one advantage of res.into_inner() is that, compared to using match and unwrap_or_else(|x| y), the intention is clearer as to both variants having the same type. 5 possible candidates sounds sorta enough here.

[joshtriplett]

It looks like all but one of those cases are effectively searches similar to binary_search. And on top of that, all but one of them also need to change one of the two cases before they could call into_inner. Personally, I'd consider .unwrap_or_else(|x| x - 1) clearer than .map_err(|x| x - 1).into_inner().

[XAMPPRocky]

Tirage; @joshtriplett What's the current status of the review of this PR?

[Centril]

cc @alexcrichton

[nikomatsakis]

I've wanted this method from time to time, though it's fairly unusual and I can't even recall the specifics.

[joshtriplett]

@nikomatsakis If the only cases where you've needed this is for binary_search, I'd be tempted to fix binary_search (e.g. introduce a binsearch) instead.

[Centril]

"Fixing" binary_search by introducing a new method doesn't seem the better and more general solution to me; Here .into_inner() seems more general and descriptive.

[joshtriplett]

@Centril What I'm suggesting here is that so far almost all the use cases for .into_inner() have been because binary_search (ab)uses Result to represent a case that isn't an error. We could fix that.

[Centril]

@joshtriplett Maybe what we lack is Either in the standard library; but I don't see how we could change binary_search here without breakage. As for (ab)use, it doesn't seem wildly inappropriate to me to use it as a sum-type since the type constructor Result has so many niceties.

[joshtriplett]

@Centril We can't change binary_search, but we could introduce a binsearch that uses a separate type, for instance.

enum BinSearchResult {
    Found(usize),
    NotFound(usize),
}

Or, alternatively, return (bool, usize).

[Centril]

@joshtriplett (bool, usize) feels strictly worse due to boolean blindness but introducing BinSearchResult also seems less worth it compared to .into_inner() which can be used in more contexts.

[joshtriplett]

@Centril That's the question at hand: what other contexts or use cases?

[Centril]

@joshtriplett #54219 (comment) did note some instances that were not binary_search no?

[alexcrichton]

I would personally think we probably don't want to add this in the sense that this is vanishingly rare to come up in practice. Almost all idiomatic usages of Result have a custom error type to make .unwrap() have a better error message which means that this method wouldn't apply. It's true binary_search is an odd one out, but it may not justify the existence of into_inner() over unwrap_or_else(|e| e)

[Kerollmops]

You are probably right, with the new identity function it will be even clearer:

let x = res.unwrap_or_else(identity);

[Kerollmops]

Something that I forget to match is the unwrap_or_else function called with an identity function.
This command rg -P 'unwrap_or_else\(\|(\w+)\|\s+\1\)' matches:

src/libsyntax/source_map.rs
342:                        .unwrap_or_else(|x| x);
346:                        .unwrap_or_else(|x| x);
373:                        .unwrap_or_else(|x| x);

src/librustc_codegen_llvm/back/write.rs
1861:                            .unwrap_or_else(|e| e);

All of these matches are associated with binary_search :)

[Centril]

r? @alexcrichton

Some decision by T-libs would be nice :)

[alexcrichton]

@rfcbot fcp close

I personally thing this is not worth adding to the standard library in that it won't necessarily get enough usage to justify having it nor is it necessarily an idiomatic patten we'd like to encourage (ok and err having the same type)

[rfcbot]

Team member @alexcrichton has proposed to close this. The next step is review by the rest of the tagged teams:

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

No concerns currently listed.

Once a majority of reviewers approve (and none object), 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.

[rfcbot]

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

[kazcw]

rust-lang/rfcs#2535 has been approved, and includes irrefutable Or patterns:

let Ok(x) | Err(x) = x;

[jswrenn]

(Commenting because I've encountered a context where this would be useful that isn't a binary search.)

I've repeatedly encountered instances where this method would be useful in the past year. My data analysis pipeline for some research about automated assessment of students' implementations and test suites is written in Rust. Implementations and test suites are either Ok because of Reasons, or Err because of Reasons. (In both cases, Reasons is just a list of other implementations or test suites that lead to the particular conclusion of whether the artifact being evaluated was Ok or not.)

Each time I've "needed" into_inner, I've just picked any one of the alternatives listed above. It's not too much of a hassle, but this is a case where I was surprised there wasn't already a standard library function for this (and my source code is just a little less clear because of its absence).

[Amanieu]

Result<T, T> is used by compare_exchange and compare_exchange_weak. In particular, in both the success and error cases it returns the previous value of the atomic variable.

[rfcbot]

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

[alexcrichton]

Ok I'm going to close this as a result of the FCP. @jswrenn it sounds like you've found a suitable enough replacement when this comes up (hopefully meaning this isn't super pressing) and we've got an RFC which may make this a bit nicer in some situations.

@Amanieu while those are definitely an example of this pattern (thanks for the reminder!) I don't think that it's an argument in favor because *_weak semantically requires the Ok and Err and into_inner would almost surely be wrong, whereas compare_exchange was explicitly changed from not returning a usize to return a Result because it was a more ergonomic method of checking the result.

[SoniEx2]

we have a bunch of logical operations for Option and Result, it would seem to make sense to also have the "coalesce" logical operation for Result<T, T>