Tracking Issue for Result::into_ok_or_err / feature(result_into_ok_or_err)

[thomcc]

Feature gate: #![feature(result_into_ok_or_err)]

This is a tracking issue for Result::into_ok_or_err, a method to get the T out of Result<T, T> regardless of which variant is active.

Public API

impl<T> Result<T, T> {
    pub const fn into_ok_or_err(self) -> T;
}

Steps / History

• Implementation: Add a Result::into_ok_or_err method to extract a T from Result<T, T> #80572
• Final commenting period (FCP)
• Stabilization PR

Unresolved Questions

• What color should the bikeshed be What name should it have?
  Some options that have been suggested:
  • Result::into_ok_or_err
  • Result::ok_or_err
  • Result::into_either
  • Result::into_inner
  • Result::either_value
  • Result::unwrap_either
  • Several more suggested options are listed in the issue.
• Do we want a reference version as well, as add Result::{value, into_value} #79315 proposed?

[thomcc]

Do we want a reference version as well, as #79315 proposed?

I don't feel that strongly, but considered this (it was suggested to me when implementing the PR) and didn't do it because it can be achieved using Result::as_ref, e.g: r.as_ref().into_ok_or_err().

The same is true for a hypothetical &mut version (with Result::as_mut), etc.

[camsteffen]

I would narrow down the names to ones that are into_* which I understand to mean "transform infallibly". unwrap_* means "transform or panic" and ok_or_err to me looks like an Option-returning function like ok and err.

[leonardo-m]

The API surface of Result/Option is quite large already. The bar for adding even more methods for them should be raised because it's getting harder for me to remember and use all of them.

[thomcc]

The API surface of Result/Option is quite large already. The bar for adding even more methods for them should be raised because it's getting harder for me to remember and use all of them.

There are multiple cases in the stdlib (e.g. ignoring external crates) where something like this is beneficial. The alternatives are less clear (r.unwrap_or_else(core::convert::identity)) and/or more verbose (explicit match).

There were also multiple PRs proposing this, showing it's not just me that considers it to meet the bar for addition.

I also think that broadly the current design of these types (and many other APIs in libcore, Iterator as an example, or the functions on the numeric types) favor functionality over minimalism in the API, but that's more of a philosophical question. You also don't have to use them if you fail to remember them.

[leonardo-m]

You also don't have to use them if you fail to remember them.

Please don't say this, it's a fallacy :-)

[faern]

This method became more relevant since deprecating Atomic*::compare_and_swap. Since now people should use compare_exchange which returns Result<T, T>.

[thomcc]

Yeah, that actually motivated this.

Edit: FWIW in the mean time I think r.unwrap_or_else(core::convert::identity) can be used, although it's not as clear.

[memoryruins]

With or_patterns stabilized in #79278, the following is accepted without a feature gate (not yet on a stable release)

let (Ok(x) | Err(x)) = slice.binary_search(&42);

The method this issue tracks could still be useful for chaining, but I thought I may as well mention the above ^^

[TheDan64]

Calling it into_inner seems like it makes the most sense; you don't really care if it was Ok or Err, just that you take ownership to get the inner value out of it

[photino]

It will be more useful if we can impl it for Result<T, E> where E: Into<T>.

[m-ou-se]

It will be more useful if we can impl it for Result<T, E> where E: Into<T>.

I think it's best to keep this method symmetric. It's not always clear that E should be converted to T rather than the other way around.

[m-ou-se]

@rfcbot merge

This is Result<T, T>::into_ok_or_err(), which consumes self and returns the T that was either in Ok or Err. It is useful for things like Atomic*::fetch_update where there are many situations where you simply don't care whether an update happened, but are only interested in the (new) value.

impl<T> Result<T, T> {
    pub const fn into_ok_or_err(self) -> T;
}

There are a lot of possible names for this function, but I think into_ok_or_err is the clearest description of what it does, although maybe a tiny bit verbose. It also matches nicely with the (unstable) into_ok and into_err functions.

[rfcbot]

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

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se
• @yaahc

Concerns:

• rather not (Tracking Issue for Result::into_ok_or_err / feature(result_into_ok_or_err) #82223 (comment))

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.

[scottmcm]

I think the name here being a bit verbose is fine, because it's really only for slightly-weird situations, as normally one would have an error as E and a non-error as T, in which case this is method doesn't apply. Thus a slightly-longer name to help emphasize it's in the unusual T == E case makes sense.

[dtolnay]

Of the above comments, I think my perspective most closely matches #82223 (comment) and #82223 (comment). The bar for whether a method goes on Result isn't about how simple the transformation it's expressing is (which this one obviously is). In other words our job isn't to assign a name to every "sufficiently simple" transformation that people want. Rather we look at how ubiquitous the need for that transformation is (things like unwrap_or) and how significantly giving that a name helps readability of code over the alternative way of expressing (like as_ref or flatten), as a comparison against the cost of users of the type all familiarizing themselves with a larger API. In the case of this method, my feeling is that it doesn't deserve a spot.

compare_exchange and binary_search were mentioned above as examples of core APIs that create Result<T, T>. I tried skimming ".compare_exchange(" language:rust and ".binary_search(" language:rust and in both cases my impression is that it's only about 15% of call sites where into_ok_or_err would even be applicable, and the significant majority of those would be equally clear with let Ok(i) | Err(i) = ... as suggested above. Or-pattern syntax was not available yet at the time that into_ok_or_err was added, but has since been stabilized in Rust 1.53.

So we're starting with a niche set of methods (a few percent of crates ever encounter a Result<T, T>), and a fraction of the call sites of those methods, and not really offering a substantial improvement to even those. From the perspective of looking at how ubiquitously a transformation is needed and how much giving it a name can facilitate understandability of the code, I would prefer Result not to get this one added.

I think that broadly the current design of these types (and many other APIs in libcore, Iterator as an example, or the functions on the numeric types) favor functionality over minimalism in the API

This is not a convincing justification for into_ok_or_err unless functionality always takes precedence over minimalism in the standard library, which it doesn't. The library designers negotiate an optimum on that continuum, and we regularly conclude that some things are over the line. Minimalism of the number of patterns one must learn and combine (e.g. the */*_or/*_or_else or */*_by/*_by_key or as_/to_/into_) and the number of never-relevant-to-you methods in rustdoc one needs to scroll past remain important considerations.

@rfcbot concern rather not

[yaahc]

I agree with @dtolnay on this one, though I think the second comment he linked is the much more persuasive justification for me. The fact that we already have stable or-patterns that let you write almost the exact same thing, and which is useful in far more situations makes me strongly prefer we focus on pushing users towards using or-patterns rather than stabilizing this API. In general I prefer a smaller API surface area with more expressive and composable features to one with many useful helper methods, since it's much easier to remember a smaller set of features.

The only difference I can see between this API and or-patterns is that this API can be used in the expression position, but or-patterns are exclusively used in let statements or match expressions and require an extra subsequent expression to refer to the bound variable. This will probably be pretty annoying if you're forced to break up a long chain of method calls but I would prefer to resolve this by finding a small additional language syntax (some sort of postfix match expressions maybe? result.match { Ok(a) | Err(a) => a }) to bridge the gap to use or-patterns as expressions in method chains rather than stabilizing this API, especially since I expect this to rarely be an issue in practice with Result, but this is almost certainly a common issue with many other enums.

As a follow up though I imagine that many users don't know about or-patterns, so I would love to see a PR where we add some additional docs to the relevant APIs in std that return Result<T, T> (e.g. binary_search and compare_exchange) to show examples of using or-patterns.

For binary search it looks like we already have a relevant example which we could update.

let mut s = vec![0, 1, 1, 1, 1, 2, 3, 5, 8, 13, 21, 34, 55];
let num = 42;
let (Ok(idx) | Err(idx) = s.binary_search(&num);
s.insert(idx, num);
assert_eq!(s, [0, 1, 1, 1, 1, 2, 3, 5, 8, 13, 21, 34, 42, 55]);

[m-ou-se]

I tried skimming ".compare_exchange(" language:rust and ".binary_search(" language:rust and in both cases my impression is that it's only about 15% of call sites where into_ok_or_err would even be applicable, and the significant majority of those would be equally clear with let Ok(i) | Err(i) = ... as suggested above.

Interesting, I was expecting that number to be higher. We should also check out fetch_update and compare_and_swap, though. Especially compare_and_swap is interesting, as that one is deprecated and its drop-in replacement is basically compare_exchange followed by into_ok_or_err. Once I have some time I'll try to analyze usage of these on GitHub and crates.io as well.

[m-ou-se]

(I should also note that binary_search isn't really a great example of a use case for into_ok_or_err, because if you don't care about the difference between Ok and Err you should probably use partition_point instead.)

[synek317]

Recently, I've read the interface design rules in the "Rust for Rustaceans" book and it mentions "The Principle of Least Surprise".
I have encountered Result<T, T> today (binary_search) and I must admit it was very surprising that I can't just .unwrap_whatever() it.

Personally, I dislike using let pattern for that - it looks noisy and less clear.

Btw. @m-ou-se thanks for mentioning partition_point, it's indeed more meaningful for this usecase!

[yaahc]

Recently, I've read the interface design rules in the "Rust for Rustaceans" book and it mentions "The Principle of Least Surprise". I have encountered Result<T, T> today (binary_search) and I must admit it was very surprising that I can't just .unwrap_whatever() it.

Personally, I dislike using let pattern for that - it looks noisy and less clear.

Btw. @m-ou-se thanks for mentioning partition_point, it's indeed more meaningful for this usecase!

I went ahead and submitted a PR to update the docs for binary_search to instead push people towards partition_point, hopefully that will significantly reduce the pressure to add Result::into_ok_or_err.

[Stargateur]

@jplatte I also think we need a Either structure in std, we already have ControlFlow where I proposed a similar feature for it, #84277 (comment)

I also believe such feature on Result is error prone for user, the need to be very explicit on the name show it.

[Lokathor]

I wanted to get a reader on an RrLock and also put a type on the binding so that it was clear what was going on, to do that using or-patterns you have to write... this entire thing:

  let db: &RwLockHashSetStaticStr = get_str_db_rw_lock_ref();
  let (Ok(read_guard) | Err(read_guard)): Result<
    RwLockReadGuard<_>,
    RwLockReadGuard<_>,
  > = db.read().map_err(|e| e.into_inner());

So I don't really think that or-patterns are as good a solution as having a method, because to make them work nicely you end up being nearly forced to not write down as many types in the code.

[dtolnay]

@Lokathor if I understand correctly, you want to write this?

let read_guard: RwLockReadGuard<_> =
    db.read().map_err(|e| e.into_inner()).into_ok_or_err();

I think into_ok_or_err immediately following map_err is especially poorly motivated when unwrap_or_else already exists.

let read_guard: RwLockReadGuard<_> =
    db.read().unwrap_or_else(PoisonError::into_inner);

[dtolnay]

@rfcbot fcp close

#82223 (comment)

Nothing compelling has been brought forth since then. I would like to remove this method.

That's right; I don't understand how this would possibly be controversial.

[rfcbot]

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

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se
• @yaahc

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.

[dtolnay]

• I think we've seen that this method existing (even unstable) misleads people into thinking they need it. I consider it likely that this negative effect dwarfs the rare instances of anyone legitimately benefiting from this method.

• We've also seen confirmation of @leonardo-m's comment: "the API surface of Result/Option is quite large already […] it's getting harder for me to remember and use all of them." Continuing to grow the API with ultra-niche methods makes it even less likely that folks learn and consistently remember the fundamentals like unwrap_or_else.

[scottmcm]

👍 to close. To be useful, this needs either an Err that's not an error or an Ok that is an error, both of which aren't something worth de-facto encouraging by the existence of this method. The pattern or .unwrap_or_else(identity) or whatever are fine if really needed.

And @Stargateur's comment above (#82223 (comment)) reinforces that for me. The problem here is not the isomorphic structure of the enum, but the semantic meaning of the variants here. A method like this is totally fine on Either because it doesn't have this strongly different intent between the variants, and indeed is often used when not caring about which variant, but where two types representing conceptually the same thing can be needed (like with different iterators).

[Lokathor]

That's right; I don't understand how this would possibly be controversial.

This is certainly not the place to have that extended discussion, which I think you already know, so I don't know why you brought it up.

[m-ou-se]

I agree we should close this. We have very few cases where we use a Result<T, T>.

For binary_search, one should probably use partition_point instead when the difference between Ok and Err doesn't matter.

The equivalent of the now deprecated compare_swap involves a compare_exchange followed by a into_ok_or_err, but the vast majority of cases just checks if the returned value equals the expected value, making .is_ok() or if let Err(e) = the right way to go.

There are not many usages of compare_exchange where one doesn't care about the difference between Ok and Err.

Similar for fetch_update, although there are quite a few use cases where one doesn't care about the return value at all. That doesn't fit the Result return type which is #[must_use]. :( Perhaps that's another indicator that maybe fetch_update shouldn't have used Result, but another enum instead. (ControlFlow<T, T> makes more sense than a Result<T, T>, although the variant names Continue and Break don't really match here either.)

[rfcbot]

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

[rfcbot]

The final comment period, with a disposition to close, 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.

[JohnTitor]

FCP is now complete and #100604 has been merged, closing.

[AngelicosPhosphoros]

Btw, if you find yourself in a need for such code, you can write this: let (Ok(my_var)|Err(my_var)) = my_result;

[superatomic]

For those who want the method syntax, an extension trait can be used:

pub trait ResultExt<T> {
    fn into_inner(self) -> T;
}

impl<T> ResultExt<T> for Result<T, T> {
    fn into_inner(self) -> T {
        let (Ok(i) | Err(i)) = self;
        i
    }
}