New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce the `Result::into_inner` method #54219

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@Kerollmops
Contributor

Kerollmops commented Sep 14, 2018

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

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Sep 14, 2018

Collaborator

r? @joshtriplett

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

Collaborator

rust-highfive commented Sep 14, 2018

r? @joshtriplett

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

@Kerollmops Kerollmops changed the title from Introduce the into_inner Result method to Introduce the `Result::into_inner` method Sep 14, 2018

@joshtriplett

This comment has been minimized.

Show comment
Hide comment
@joshtriplett

joshtriplett Sep 14, 2018

Member

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.

Member

joshtriplett commented Sep 14, 2018

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 Centril added the T-libs label Sep 14, 2018

@Centril

This comment has been minimized.

Show comment
Hide comment
@Centril

Centril Sep 14, 2018

Contributor

@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.

Contributor

Centril commented Sep 14, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@joshtriplett

joshtriplett Sep 14, 2018

Member

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).

Member

joshtriplett commented Sep 14, 2018

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

This comment has been minimized.

Show comment
Hide comment
@Kerollmops

Kerollmops Sep 15, 2018

Contributor

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,
Contributor

Kerollmops commented Sep 15, 2018

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

This comment has been minimized.

Show comment
Hide comment
@Centril

Centril Sep 15, 2018

Contributor

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.

Contributor

Centril commented Sep 15, 2018

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

This comment has been minimized.

Show comment
Hide comment
@joshtriplett

joshtriplett Sep 15, 2018

Member

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().

Member

joshtriplett commented Sep 15, 2018

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().

@Aaronepower

This comment has been minimized.

Show comment
Hide comment
@Aaronepower

Aaronepower Sep 26, 2018

Contributor

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

Contributor

Aaronepower commented Sep 26, 2018

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

@Centril

This comment has been minimized.

Show comment
Hide comment
@Centril
Contributor

Centril commented Sep 26, 2018

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Sep 26, 2018

Contributor

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

Contributor

nikomatsakis commented Sep 26, 2018

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

@joshtriplett

This comment has been minimized.

Show comment
Hide comment
@joshtriplett

joshtriplett Sep 26, 2018

Member

@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.

Member

joshtriplett commented Sep 26, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@Centril

Centril Sep 26, 2018

Contributor

"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.

Contributor

Centril commented Sep 26, 2018

"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

This comment has been minimized.

Show comment
Hide comment
@joshtriplett

joshtriplett Sep 26, 2018

Member

@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.

Member

joshtriplett commented Sep 26, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@Centril

Centril Sep 26, 2018

Contributor

@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.

Contributor

Centril commented Sep 26, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@joshtriplett

joshtriplett Sep 26, 2018

Member

@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).

Member

joshtriplett commented Sep 26, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@Centril

Centril Sep 26, 2018

Contributor

@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.

Contributor

Centril commented Sep 26, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@joshtriplett

joshtriplett Sep 26, 2018

Member

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

Member

joshtriplett commented Sep 26, 2018

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

@Centril

This comment has been minimized.

Show comment
Hide comment
@Centril

Centril Sep 26, 2018

Contributor

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

Contributor

Centril commented Sep 26, 2018

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

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Sep 27, 2018

Member

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)

Member

alexcrichton commented Sep 27, 2018

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

This comment has been minimized.

Show comment
Hide comment
@Kerollmops

Kerollmops Sep 28, 2018

Contributor

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

let x = res.unwrap_or_else(identity);
Contributor

Kerollmops commented Sep 28, 2018

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

let x = res.unwrap_or_else(identity);
@Kerollmops

This comment has been minimized.

Show comment
Hide comment
@Kerollmops

Kerollmops Sep 29, 2018

Contributor

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 :)

Contributor

Kerollmops commented Sep 29, 2018

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

This comment has been minimized.

Show comment
Hide comment
@Centril

Centril Oct 6, 2018

Contributor

r? @alexcrichton

Some decision by T-libs would be nice :)

Contributor

Centril commented Oct 6, 2018

r? @alexcrichton

Some decision by T-libs would be nice :)

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Oct 8, 2018

Member

@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)

Member

alexcrichton commented Oct 8, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Oct 8, 2018

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

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 commented Oct 8, 2018

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

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 comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Oct 9, 2018

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

rfcbot commented Oct 9, 2018

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

@kazcw

This comment has been minimized.

Show comment
Hide comment
@kazcw

kazcw Oct 10, 2018

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

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

kazcw commented Oct 10, 2018

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

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

This comment has been minimized.

Show comment
Hide comment
@jswrenn

jswrenn Oct 15, 2018

(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).

jswrenn commented Oct 15, 2018

(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

This comment has been minimized.

Show comment
Hide comment
@Amanieu

Amanieu Oct 18, 2018

Contributor

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.

Contributor

Amanieu commented Oct 18, 2018

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

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Oct 19, 2018

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

rfcbot commented Oct 19, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment