Skip to content
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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion src/libcore/option.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@
#![stable(feature = "rust1", since = "1.0.0")]

use iter::{FromIterator, FusedIterator, TrustedLen};
use mem;
use {mem, ops};

// Note that this is not a lang item per se, but it has a hidden dependency on
// `Iterator`, which is one. The compiler assumes that the `next` method of
Expand Down Expand Up @@ -1123,3 +1123,29 @@ impl<A, V: FromIterator<A>> FromIterator<Option<A>> for Option<V> {
}
}
}

/// 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.
#[unstable(feature = "try_trait", issue = "42327")]
#[derive(Clone, Copy, PartialEq, PartialOrd, Eq, Ord, Debug, Hash)]
pub struct NoneError;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we implement std::error::Error for this type?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this be exposed insta-stably as <Option<()> as Try>::Error? Would that imply it should be #[stable]?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existence of a such a type is basically exposed, but not the precise name of it, I think. So I think it's ok to mark it unstable still.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, and, I think the Try trait is still marked as unstable, so actually that name is not stable either.


#[unstable(feature = "try_trait", issue = "42327")]
impl<T> ops::Try for Option<T> {
type Ok = T;
type Error = NoneError;

fn into_result(self) -> Result<T, NoneError> {
self.ok_or(NoneError)
}

fn from_ok(v: T) -> Self {
Some(v)
}

fn from_error(_: NoneError) -> Self {
None
}
}
1 change: 1 addition & 0 deletions src/libcore/tests/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
#![feature(test)]
#![feature(trusted_len)]
#![feature(try_from)]
#![feature(try_trait)]
#![feature(unique)]

#![feature(const_atomic_bool_new)]
Expand Down
27 changes: 27 additions & 0 deletions src/libcore/tests/option.rs
Original file line number Diff line number Diff line change
Expand Up @@ -270,3 +270,30 @@ fn test_cloned() {
assert_eq!(opt_ref_ref.clone().cloned(), Some(&val));
assert_eq!(opt_ref_ref.cloned().cloned(), Some(1));
}

#[test]
fn test_try() {
fn try_option_some() -> Option<u8> {
let val = Some(1)?;
Some(val)
}
assert_eq!(try_option_some(), Some(1));

fn try_option_none() -> Option<u8> {
let val = None?;
Some(val)
}
assert_eq!(try_option_none(), None);

fn try_option_ok() -> Result<u8, NoneError> {
let val = Some(1)?;
Ok(val)
}
assert_eq!(try_option_ok(), Ok(1));

fn try_option_err() -> Result<u8, NoneError> {
let val = None?;
Ok(val)
}
assert_eq!(try_option_err(), Err(NoneError));
}
29 changes: 29 additions & 0 deletions src/libcore/tests/result.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

use core::option::*;

fn op1() -> Result<isize, &'static str> { Ok(666) }
fn op2() -> Result<isize, &'static str> { Err("sadface") }

Expand Down Expand Up @@ -202,3 +204,30 @@ pub fn test_unwrap_or_default() {
assert_eq!(op1().unwrap_or_default(), 666);
assert_eq!(op2().unwrap_or_default(), 0);
}

#[test]
fn test_try() {
fn try_result_some() -> Option<u8> {
let val = Ok(1)?;
Some(val)
}
assert_eq!(try_result_some(), Some(1));

fn try_result_none() -> Option<u8> {
let val = Err(NoneError)?;
Some(val)
}
assert_eq!(try_result_none(), None);

fn try_result_ok() -> Result<u8, u8> {
let val = Ok(1)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Ok(val)
}
assert_eq!(try_result_ok(), Ok(1));
Copy link
Contributor

@nikomatsakis nikomatsakis Sep 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that the major concern was the error messages, I agree with @oli-obk that it would be good to add some src/test/ui tests covering incorrect usage of ? when applied to Option. E.g., given x: Option<u32>:

Here is a guide to adding ui tests: https://github.com/rust-lang/rust/blob/master/src/test/COMPILER_TESTS.md#guide-to-the-ui-tests


fn try_result_err() -> Result<u8, u8> {
let val = Err(1)?;
Ok(val)
}
assert_eq!(try_result_err(), Err(1));
}
25 changes: 25 additions & 0 deletions src/test/ui/suggestions/try-on-option.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![feature(try_trait)]

fn main() {}

fn foo() -> Result<u32, ()> {
let x: Option<u32> = None;
x?;
Ok(22)
}

fn bar() -> u32 {
let x: Option<u32> = None;
x?;
22
}
22 changes: 22 additions & 0 deletions src/test/ui/suggestions/try-on-option.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
error[E0277]: the trait bound `(): std::convert::From<std::option::NoneError>` is not satisfied
--> $DIR/try-on-option.rs:17:5
|
17 | x?;
| ^^ the trait `std::convert::From<std::option::NoneError>` is not implemented for `()`
|
= note: required by `std::convert::From::from`

error[E0277]: the `?` operator can only be used in a function that returns `Result` (or another type that implements `std::ops::Try`)
--> $DIR/try-on-option.rs:23:5
|
23 | x?;
| --
| |
| cannot use the `?` operator in a function that returns `u32`
| in this macro invocation
|
= help: the trait `std::ops::Try` is not implemented for `u32`
= note: required by `std::ops::Try::from_error`

error: aborting due to 2 previous errors