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

Impl Try for Option #42526

Merged
merged 5 commits into from Sep 30, 2017

Conversation

@huntiep
Contributor

huntiep commented Jun 8, 2017

This is part of #31436.

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Jun 8, 2017

Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Collaborator

rust-highfive commented Jun 8, 2017

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Show outdated Hide outdated src/libcore/option.rs Outdated
@scottmcm

This comment has been minimized.

Show comment
Hide comment
@scottmcm

scottmcm Jun 8, 2017

Member

This is exciting, and applying right after branching for release is probably the best option we have for insta-stable things like this. (This will allow ? on Option without a feature gate, right?)

Something I didn't want to push in the RFC discussion, but thought interesting: what about a None type (instead of Missing) for <Option<T> as Try>::Error? Then at the type level it's also "an option is either a T or None". Sample implementation: https://doc.rust-lang.org/nightly/unstable-book/library-features/try-trait.html

That's intentionally non-constructible for now, since what I think it ought to be is type None = Option<!>; so that let _: None = None; would be valid, and more importantly that a throw sugar (in auto-wrapping) would allow throw None; and return 4; (paralleling return None; and return Some(4);).

Though I guess the type will stay unstable for a while, so it doesn't matter for now...

Member

scottmcm commented Jun 8, 2017

This is exciting, and applying right after branching for release is probably the best option we have for insta-stable things like this. (This will allow ? on Option without a feature gate, right?)

Something I didn't want to push in the RFC discussion, but thought interesting: what about a None type (instead of Missing) for <Option<T> as Try>::Error? Then at the type level it's also "an option is either a T or None". Sample implementation: https://doc.rust-lang.org/nightly/unstable-book/library-features/try-trait.html

That's intentionally non-constructible for now, since what I think it ought to be is type None = Option<!>; so that let _: None = None; would be valid, and more importantly that a throw sugar (in auto-wrapping) would allow throw None; and return 4; (paralleling return None; and return Some(4);).

Though I guess the type will stay unstable for a while, so it doesn't matter for now...

Show outdated Hide outdated src/libcore/tests/option.rs Outdated
Show outdated Hide outdated src/libcore/tests/option.rs Outdated
assert_eq!(try_result_none(), None);
fn try_result_ok() -> Result<u8, u8> {
let val = Ok(1)?;

This comment has been minimized.

@oli-obk

oli-obk Jun 8, 2017

Contributor

This is also nothing new... maybe change it into a compile-fail test by using Some/None and triggering a compilation error

@oli-obk

oli-obk Jun 8, 2017

Contributor

This is also nothing new... maybe change it into a compile-fail test by using Some/None and triggering a compilation error

This comment has been minimized.

@huntiep

huntiep Jun 8, 2017

Contributor

I didn't see any tests for Result's Try implemenation, but if they exist I can remove these tests.

@huntiep

huntiep Jun 8, 2017

Contributor

I didn't see any tests for Result's Try implemenation, but if they exist I can remove these tests.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jun 8, 2017

Member

r? @aturon

Also cc @rust-lang/lang, especially @scottmcm's comment where I think this is insta-stable

Member

alexcrichton commented Jun 8, 2017

r? @aturon

Also cc @rust-lang/lang, especially @scottmcm's comment where I think this is insta-stable

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Jun 9, 2017

Contributor

I think one way to prevent it from being insta-stable would be to make it #[cfg(nightly)], right? Have we rejected this path for some reason?

I am open to bikeshedding the name of Missing -- I agree it doesn't feel right to me. None might be better, though if we ever get types for variants, that might be .. surprising?

Contributor

nikomatsakis commented Jun 9, 2017

I think one way to prevent it from being insta-stable would be to make it #[cfg(nightly)], right? Have we rejected this path for some reason?

I am open to bikeshedding the name of Missing -- I agree it doesn't feel right to me. None might be better, though if we ever get types for variants, that might be .. surprising?

@MoSal

This comment has been minimized.

Show comment
Hide comment
@MoSal

MoSal Jun 10, 2017

Maybe ErrNone or TryNone or TryErrNone.

MoSal commented Jun 10, 2017

Maybe ErrNone or TryNone or TryErrNone.

@kennytm

This comment has been minimized.

Show comment
Hide comment

@frewsxcv frewsxcv added the T-libs label Jun 11, 2017

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Jun 13, 2017

Contributor

@kennytm well that was more about a public "cfg", but yeah apparently we don't have #[cfg(nightly)] even without rustc. For some reason, I thought we did. Regardless, it's not a great suggestion; it wouldn't require any opt-in, which is really what you want.

Contributor

nikomatsakis commented Jun 13, 2017

@kennytm well that was more about a public "cfg", but yeah apparently we don't have #[cfg(nightly)] even without rustc. For some reason, I thought we did. Regardless, it's not a great suggestion; it wouldn't require any opt-in, which is really what you want.

@huntiep

This comment has been minimized.

Show comment
Hide comment
@huntiep

huntiep Jun 13, 2017

Contributor

I also like ErrNone over Missing. I just followed the impl outlined in the RFC.

At this point what do I need to do?

Contributor

huntiep commented Jun 13, 2017

I also like ErrNone over Missing. I just followed the impl outlined in the RFC.

At this point what do I need to do?

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Jun 14, 2017

Contributor

I think we should settle on the name -- I am not 100% sure when we should merge this RFC though. The insta-stable thing bothers me, but I don't have a good idea what else to do about it.

Contributor

nikomatsakis commented Jun 14, 2017

I think we should settle on the name -- I am not 100% sure when we should merge this RFC though. The insta-stable thing bothers me, but I don't have a good idea what else to do about it.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Jun 14, 2017

Contributor

@rust-lang/lang @rust-lang/libs -- anybody care to "weigh in" on the name of the "error type" to use in the Try impl for Option? The RFC said Missing, but ErrNone has been proposed as an alternative. Some newtype is needed, at least.

Contributor

nikomatsakis commented Jun 14, 2017

@rust-lang/lang @rust-lang/libs -- anybody care to "weigh in" on the name of the "error type" to use in the Try impl for Option? The RFC said Missing, but ErrNone has been proposed as an alternative. Some newtype is needed, at least.

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Jun 14, 2017

Member

If we follow the naming practice in libstd, it should be ***Error (e.g. NoneError) instead of ErrNone.

Member

kennytm commented Jun 14, 2017

If we follow the naming practice in libstd, it should be ***Error (e.g. NoneError) instead of ErrNone.

@withoutboats

This comment has been minimized.

Show comment
Hide comment
@withoutboats

withoutboats Jun 14, 2017

Contributor

What's the reason not to use ()?

Contributor

withoutboats commented Jun 14, 2017

What's the reason not to use ()?

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Jun 14, 2017

Member

@withoutboats rust-lang/rfcs#1859 explained why the error must be a dedicated type (to prevent accidentally ?-returning an Option<T> as Result<T, ()> and vice-versa).

Member

kennytm commented Jun 14, 2017

@withoutboats rust-lang/rfcs#1859 explained why the error must be a dedicated type (to prevent accidentally ?-returning an Option<T> as Result<T, ()> and vice-versa).

@huntiep

This comment has been minimized.

Show comment
Hide comment
@huntiep

huntiep Jun 15, 2017

Contributor

I think NoneError is fine, as long as it is well documented. Right now Missing is documented as

/// The equivalent of `Option::None` for a `Result::Err`.

Any thoughts on how to better document this, regardless of what the final name is?

Contributor

huntiep commented Jun 15, 2017

I think NoneError is fine, as long as it is well documented. Right now Missing is documented as

/// The equivalent of `Option::None` for a `Result::Err`.

Any thoughts on how to better document this, regardless of what the final name is?

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jun 15, 2017

Member

Ergonomically at least this seems most likely to arise in something like:

impl From<Missing> for MyErrorType

(if at all). That way you could option? in functions that return Result. In that sense From<Missing> does indeed seem a little ambiguous, but either From<ErrNone> or From<NoneError> are both more clear. Note, though that From<option::Missing> is also pretty clear.

Member

alexcrichton commented Jun 15, 2017

Ergonomically at least this seems most likely to arise in something like:

impl From<Missing> for MyErrorType

(if at all). That way you could option? in functions that return Result. In that sense From<Missing> does indeed seem a little ambiguous, but either From<ErrNone> or From<NoneError> are both more clear. Note, though that From<option::Missing> is also pretty clear.

@sfackler

This comment has been minimized.

Show comment
Hide comment
@sfackler

sfackler Jun 15, 2017

Member

WasNone is another naming option.

Member

sfackler commented Jun 15, 2017

WasNone is another naming option.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Jun 15, 2017

Contributor

@withoutboats

What's the reason not to use ()?

The feeling was that () is not sufficiently precise. In particular, one could readily imagine there being other "option-like" types that might also default to use (), and this would allow interconversion between them when you didn't really want to allow that. For example:

enum List<T> {
    Nil,
    Cons(T, Box<List<T>>),
}

Imagine that I implemented Try for List and made the "error" case be () (because it seemed like a reasonable default). Granted, this is probably not the best example, though it's not so uncommon for "lists" to be a monad, so I could imagine someone doing it. Anyway, this led to the recommendation to use a newtype, so that the intention is more explicit.

Contributor

nikomatsakis commented Jun 15, 2017

@withoutboats

What's the reason not to use ()?

The feeling was that () is not sufficiently precise. In particular, one could readily imagine there being other "option-like" types that might also default to use (), and this would allow interconversion between them when you didn't really want to allow that. For example:

enum List<T> {
    Nil,
    Cons(T, Box<List<T>>),
}

Imagine that I implemented Try for List and made the "error" case be () (because it seemed like a reasonable default). Granted, this is probably not the best example, though it's not so uncommon for "lists" to be a monad, so I could imagine someone doing it. Anyway, this led to the recommendation to use a newtype, so that the intention is more explicit.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Jun 15, 2017

Contributor

👍 for NoneError. I like that. I think the documentation should probably be something like:

"The error type that results from applying the try operator (?) to a None value. If you wish to allow x? (where x is an Option<T>) to be converted into your error type, you can implement impl From<NoneError> for YourErrorType. In that case, x? within a function that returns Result<_, YourErrorType> will translate a None value into an Err result."

Contributor

nikomatsakis commented Jun 15, 2017

👍 for NoneError. I like that. I think the documentation should probably be something like:

"The error type that results from applying the try operator (?) to a None value. If you wish to allow x? (where x is an Option<T>) to be converted into your error type, you can implement impl From<NoneError> for YourErrorType. In that case, x? within a function that returns Result<_, YourErrorType> will translate a None value into an Err result."

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Sep 28, 2017

Contributor

@huntiep I took the liberty of rebasing for you.

Contributor

nikomatsakis commented Sep 28, 2017

@huntiep I took the liberty of rebasing for you.

@huntiep

This comment has been minimized.

Show comment
Hide comment
@huntiep

huntiep Sep 29, 2017

Contributor

@nikomatsakis I'm sorry, I missed that part of your last comment. For future reference, can you briefly explain what you meant by rebase?

Contributor

huntiep commented Sep 29, 2017

@nikomatsakis I'm sorry, I missed that part of your last comment. For future reference, can you briefly explain what you meant by rebase?

@shepmaster

This comment has been minimized.

Show comment
Hide comment
@shepmaster

shepmaster Sep 29, 2017

Member

@huntiep it refers to git rebase. You can do git rebase --help or check out some pages like this one to learn more. Many times it's done to "clean up" a branch before merging by removing "intermediate" work that's not important to track forever, or to resolve any merge conflicts.

Member

shepmaster commented Sep 29, 2017

@huntiep it refers to git rebase. You can do git rebase --help or check out some pages like this one to learn more. Many times it's done to "clean up" a branch before merging by removing "intermediate" work that's not important to track forever, or to resolve any merge conflicts.

@shepmaster

This comment has been minimized.

Show comment
Hide comment
@shepmaster

shepmaster Sep 29, 2017

Member

@nikomatsakis did you mean to review at bors?

Member

shepmaster commented Sep 29, 2017

@nikomatsakis did you mean to review at bors?

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Sep 29, 2017

Member

@bors: r=nikomatsakis

Member

aturon commented Sep 29, 2017

@bors: r=nikomatsakis

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Sep 29, 2017

Contributor

📌 Commit e30d92b has been approved by nikomatsakis

Contributor

bors commented Sep 29, 2017

📌 Commit e30d92b has been approved by nikomatsakis

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Sep 29, 2017

Contributor

⌛️ Testing commit e30d92b with merge 6f87d20...

Contributor

bors commented Sep 29, 2017

⌛️ Testing commit e30d92b with merge 6f87d20...

bors added a commit that referenced this pull request Sep 29, 2017

Auto merge of #42526 - huntiep:try_opt, r=nikomatsakis
Impl Try for Option

This is part of #31436.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Sep 29, 2017

Contributor

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 6f87d20 to master...

Contributor

bors commented Sep 29, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 6f87d20 to master...

@bors bors merged commit e30d92b into rust-lang:master Sep 30, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

bors added a commit that referenced this pull request Nov 8, 2017

Auto merge of #45595 - scottmcm:iter-try-fold, r=dtolnay
Short-circuiting internal iteration with Iterator::try_fold & try_rfold

These are the core methods in terms of which the other methods (`fold`, `all`, `any`, `find`, `position`, `nth`, ...) can be implemented, allowing Iterator implementors to get the full goodness of internal iteration by only overriding one method (per direction).

Based off the `Try` trait, so works with both `Result` and `Option` (🎉 #42526).  The `try_fold` rustdoc examples use `Option` and the `try_rfold` ones use `Result`.

AKA continuing in the vein of PRs #44682 & #44856 for more of `Iterator`.

New bench following the pattern from the latter of those:
```
test iter::bench_take_while_chain_ref_sum          ... bench:   1,130,843 ns/iter (+/- 25,110)
test iter::bench_take_while_chain_sum              ... bench:     362,530 ns/iter (+/- 391)
```

I also ran the benches without the `fold` & `rfold` overrides to test their new default impls, with basically no change.  I left them there, though, to take advantage of existing overrides and because `AlwaysOk` has some sub-optimality due to #43278 (which 45225 should fix).

If you're wondering why there are three type parameters, see issue #45462

Thanks for @bluss for the [original IRLO thread](https://internals.rust-lang.org/t/pre-rfc-fold-ok-is-composable-internal-iteration/4434) and the rfold PR and to @cuviper for adding so many folds, [encouraging me](#45379 (comment)) to make this PR, and finding a catastrophic bug in a pre-review.

bors added a commit that referenced this pull request Nov 17, 2017

Auto merge of #45595 - scottmcm:iter-try-fold, r=dtolnay
Short-circuiting internal iteration with Iterator::try_fold & try_rfold

These are the core methods in terms of which the other methods (`fold`, `all`, `any`, `find`, `position`, `nth`, ...) can be implemented, allowing Iterator implementors to get the full goodness of internal iteration by only overriding one method (per direction).

Based off the `Try` trait, so works with both `Result` and `Option` (🎉 #42526).  The `try_fold` rustdoc examples use `Option` and the `try_rfold` ones use `Result`.

AKA continuing in the vein of PRs #44682 & #44856 for more of `Iterator`.

New bench following the pattern from the latter of those:
```
test iter::bench_take_while_chain_ref_sum          ... bench:   1,130,843 ns/iter (+/- 25,110)
test iter::bench_take_while_chain_sum              ... bench:     362,530 ns/iter (+/- 391)
```

I also ran the benches without the `fold` & `rfold` overrides to test their new default impls, with basically no change.  I left them there, though, to take advantage of existing overrides and because `AlwaysOk` has some sub-optimality due to #43278 (which 45225 should fix).

If you're wondering why there are three type parameters, see issue #45462

Thanks for @bluss for the [original IRLO thread](https://internals.rust-lang.org/t/pre-rfc-fold-ok-is-composable-internal-iteration/4434) and the rfold PR and to @cuviper for adding so many folds, [encouraging me](#45379 (comment)) to make this PR, and finding a catastrophic bug in a pre-review.

@Emilgardis Emilgardis referenced this pull request Nov 28, 2017

Closed

impl Fail for NoneError? #59

@rillian rillian referenced this pull request Jan 12, 2018

Open

Missing crates #23

@Freaky Freaky referenced this pull request Sep 7, 2018

Merged

Tidy Result/Option use #29

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