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

Implement From<T> for Option<T> #1402

Closed
abonander opened this issue Dec 10, 2015 · 23 comments
Closed

Implement From<T> for Option<T> #1402

abonander opened this issue Dec 10, 2015 · 23 comments
Labels
T-libs-api Relevant to the library API team, which will review and decide on the RFC.

Comments

@abonander
Copy link

impl From<T> for Option<T> {
    fn from(val: T) -> Self {
        Option::Some(val)
    }
}

This is a trivial impl and I think it would help ergonomics in certain areas of API design. As an example, consider a method on a fictional I/O wrapper for setting a timeout:

struct Socket;

impl Socket {
    // Set the timeout, in seconds. Removes timeout if `None`.
    fn set_timeout<T: Into<Option<u64>>(&mut self, timeout: T) { /* .. */ }
}

This can be called in both of the following forms without needing separate methods for u64 and Option<u64>:

let mut socket = Socket;

socket.set_timeout(3600);
socket.set_timeout(None);

Currently, if this API wanted to take an Option<u64>, it would have to be invariant over it:

fn set_timeout(&mut self, timeout: Option<u64>);

Requiring Some everywhere:

socket.set_timeout(Some(3600));

I think the above is much cleaner.

There are existing APIs such as the following:

http://doc.rust-lang.org/nightly/std/net/struct.UdpSocket.html#method.set_read_timeout

which I think could be migrated relatively seamlessly. I think the only breakage would be if someone was referring to them by UFCS, so the addition of a type parameter would break resolution (maybe). A Crater run would probably help here.

Otherwise, I don't see much as far as downsides go, except maybe some brief confusion for newbies. However, I don't think it would take them long to realize, "Oh! It can take u64 or Option<u64>. Cool!" Then it's just another neat little feature of Rust. I don't think accidental conversions would be a problem, either.

@ticki
Copy link
Contributor

ticki commented Dec 10, 2015

The main objection, I can see, is that you'll now have multiple ways of converting T to Some(T). But generally, I'm in favor of using From more.

@Stebalien
Copy link
Contributor

👍 This is actually pretty cool.

@killercup
Copy link
Member

This looks pretty convenient!

The only issue I can see is that APIs will be harder to read, just compare

fn set_timeout(&mut self, timeout: Option<u64>);

and

fn set_timeout<T: Into<Option<u64>>(&mut self, timeout: T);

(But that's probably something that rustdoc could be optimised for.)

@abonander
Copy link
Author

The ergonomics could be improved with an alias of some sort, like IntoOpt<T>, to drop one pair of angle-brackets and a few letters. Something like:

type IntoOpt<T> = Into<Option<T>>;

@killercup
Copy link
Member

That reads a bit nicer, @cybergeek94, but doesn't really solve the problem that the significant type information is no longer part in the function's argument as the T is defined either before or after (with where) the argument list.

(IntoOpt reads weird. I don't have a better suggestions, though. Except, if you want to troll people coming from Haskell: Call it MaybeOption<T>! 😉)

@abonander
Copy link
Author

I don't know if it's that big of an issue, at least for most intermediate to advanced users. Whenever I see a type parameter in documentation or code, my eyes instantly jump to the type parameter list to find its definition, and then to the where clause to see if there's anything more.

@ticki
Copy link
Contributor

ticki commented Dec 14, 2015

I, too, don't think that's a problem. You quickly get used to it.

@killercup
Copy link
Member

I didn't mean to imply it's a big issue, just something to be aware of. Beginners are often confused by the notion of Option itself, so Into<Option<T>> can be quite confusing. If we were to add this and use it in more APIs, I'd expect it be mentioned in the docs in a few places.

In general, I'm in favour of this addition.

@abonander
Copy link
Author

@killercup If we did go with the trait alias, we could have additional documentation on it saying what it is for, so they could click the link in Rustdoc output and instantly find out how it's supposed to work.

@BlacklightShining
Copy link

Not an objection or anything…but, in case no one's noticed, this basically gives Rust (opt-in) nullable types.

@Stebalien
Copy link
Contributor

@BlacklightShining shh.... 😶

@BlacklightShining
Copy link

@Stebalien I'm conflicted, actually. On the one hand, set_timeout(3600) and set_timeout(None) read really well. On the other, I liked the explicitness that Some() provides…

The decrease in readability is also something to consider. Did we already decide against allowing traits there? fn set_timeout(&mut self, timeout: Into<Option<u64>>) seems better to me than the alternative using currently-available generics syntax.

@killercup
Copy link
Member

Coming back to this after two weeks, I'm still in favour of this. While it makes designing the API a bit more complicated for library authors, it makes the code using the API pretty nice to read and write. I think it works pretty good for people coming from languages with null-able types: You can pass None only to certain functions (as defined by their argument types) and the compiler/type system ensures that you deal with the nullability.

Other than that, I've been searching for APIs where this would be useful (i.e., functions that actually take Option<T>), but after looking in std, time, and hyper, I could only find read_timeout, which is already @cybergeek94's example above. (It seems to me that most APIs that work on structs with optional fields tend to use the builder pattern.)

What are some other APIs where this would be useful?

@kamalmarhubi
Copy link
Contributor

kamalmarhubi commented Apr 24, 2016

@killercup

What are some other APIs where this would be useful?

There are a few APIs in nix that could benefit. We aim to take Option<&T> in the wrapper for any system API parameter that is a nullable pointer. Some examples:

kamalmarhubi added a commit to kamalmarhubi/rust that referenced this issue Apr 24, 2016
This allows improved ergonomics for functions that have optional
parameters: instead of taking `Option<T>`, they can have a type
parameter bounded by `Into<Option<T>>`. That way, a value of type `T`
can be passed directly without being wrapped by `Some`.

As an example, a function

    fn foo<T>(required: i32, optional: T) -> i32
        where T: Into<Option<i32>>
    {
        required + optional.into().unwrap_or(0)
    }

can be called as `foo(2, None)` or as `foo(2, 3)`.

Refs rust-lang/rfcs#1402
@bluss
Copy link
Member

bluss commented Apr 24, 2016

@kamalmarhubi Does it need to be the Into trait? You could use another trait for the same effect.

@codyps
Copy link

codyps commented Apr 24, 2016

@bluss is there a reason you're concerned about the use of the Into/From trait?

@bluss
Copy link
Member

bluss commented Apr 24, 2016

Changes to From/Into impact everyone's API design (also preexisting APIs) so it needs close scrutiny.

Using the same From/Into traits for everything just means we risk having conflicts down the line when independently convenient conversions clash. A specific trait seems more in line with Rust's explicitness and may even be an easier to understand API.

fn ping<T>(host: &str, timeout: T) where T: Optional<i32> {  }
// where both i32 and Option<i32> implement Optional<i32>.

@kamalmarhubi
Copy link
Contributor

@bluss there's no strong reason, and I'd be open to something like Optional. It was mostly in keeping with the conversion traits RFC's motivation:

The motivation is to remove the need for ad hoc conversion traits (like FromStr, AsSlice, ToSocketAddr, FromError) whose sole role is for generics bounds.

@notriddle
Copy link
Contributor

Not an objection or anything…but, in case no one's noticed, this basically gives Rust (opt-in) nullable types.

The problem with nullable types is handling them once you have them. Needing to convert a type into its optional version explicitly doesn't really help anything, except that Into<Option<Option<T>>> is kinda confusing. How should calling a function with that has that type (probably because of generics or macros assembling it) behave?

@BlacklightShining
Copy link

@notriddle You can pass a T, an Option<T>, or an Option<Option<T>>. It's up to whoever wrote|generated the function to decide (and then document) how that argument is interpreted.

@kamalmarhubi
Copy link
Contributor

@BlacklightShining

You can pass a T, an Option<T>, or an Option<Option<T>>. It's up to whoever wrote|generated the function to decide (and then document) how that argument is interpreted.

I don't think you'd be able to pass T to something accepting Into<Option<Option<T>>. There's the blanket impl From<T> for T, and the inverting impl<T, U> Into<U> for T where U: From<T>, but I don't see anything transitive like impl<T, U: From<T>, V: From<U>> From<T> for V which would be necessary for it to work.

@tshepang
Copy link
Member

Can this now be closed?

@Centril Centril added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Feb 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

No branches or pull requests