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

core: Implement From<T> for Option<T> #33170

Closed
wants to merge 2 commits into from

Conversation

kamalmarhubi
Copy link
Contributor

@kamalmarhubi kamalmarhubi commented Apr 23, 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

@rust-highfive
Copy link
Collaborator

r? @aturon

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

@kamalmarhubi
Copy link
Contributor Author

I posted about this on internals a couple of months ago, but forgot to submit a PR to see what the maintainers think.

@kamalmarhubi
Copy link
Contributor Author

kamalmarhubi commented Apr 23, 2016

I added a commit changing the set_{read,write}_timeout methods in std::net to use this setup as an example.

This allows writing code like

stream.set_read_timeout(Duration::from_millis(200));

instead of requiring an additional Some wrapping the duration as is
currently the case:

stream.set_read_timeout(Some(Duration::from_millis(200)));

(Thanks to @sfackler for mentioning those APIs on the internals discussion!)

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Apr 24, 2016
@killercup
Copy link
Member

This was also discussed here: rust-lang/rfcs#1402

@kamalmarhubi
Copy link
Contributor Author

@killercup oh thanks for that link! I'm glad I'm not the only person thinking down these lines.

I wondered if this requires a full-blown RFC. It falls under small library additions, but is possibly a bit controversial and so could warrant the discussion period.

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
This allows writing code like

    stream.set_read_timeout(Duration::from_millis(200));

instead of requiring an additional `Some` wrapping the duration as is
currently the case:

    stream.set_read_timeout(Some(Duration::from_millis(200)));
@tbu-
Copy link
Contributor

tbu- commented May 6, 2016

This behaves badly with nested options: Suppose you have a function

fn foobar<I, T: Into<Option<T>>>(t: T) -> Option<T> {
    t.into()
}

and you call it with

let x: Option<Option<u32>> = foobar(None);

then it is unclear whether you mean Some(None) or None with your None parameter.

Not sure how relevant this is, but I could imagine this popping up in some generic context.

@alexcrichton
Copy link
Member

The libs team discussed this during triage the other day and the conclusion was that we don't want to do this at this time. The lang team has also been discussing the possibility of optional arguments recently (e.g. they are welcoming RFCs), and we wouldn't want the library side to conflict with the language side of these APIs.

Thanks though for the PR @kamalmarhubi!

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 PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants