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: impl From<T> for Option<T> #34828

Merged
merged 1 commit into from Jul 21, 2016
Merged

Conversation

seanmonstar
Copy link
Contributor

First, the semantics of this impl seem spot on. If I have a value T, and I wish to make a Option<T>, then Option::from(val) should always give Some(val).

Second, this allows improvement for several APIs that currently take Option<T> as arguments. Consider:

fn set_read_timeout(&mut self, timeout: Option<u32>) {
    // ...
}

x.set_read_timeout(Some(30));
x.set_read_timeout(Some(10));
x.set_read_timeout(None);

With this impl:

fn set_read_timeout<T: Into<Option<u32>>>(&mut self, timeout: T) {
    let timeout = timeout.into();
    // ...
}

x.set_read_timeout(30);
x.set_read_timeout(10);
x.set_read_timeout(Some(10)); // backwards compatible
x.set_read_timeout(None);

The change to those methods aren't included, but could be modified later.

r? @sfackler

@sfackler sfackler added I-needs-decision Issues in need of decision. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jul 14, 2016
@jonas-schievink
Copy link
Member

Also see rust-lang/rfcs#1402 and #33170

@seanmonstar
Copy link
Contributor Author

I know it's been discussed a bunch before. Having the impl exist by itself doesn't mean the APIs need to use it.

@alexcrichton
Copy link
Member

Indeed, thanks for the cc @jonas-schievink! @seanmonstar this has been discussed and turned down before, do you have new information which has changed since the decision was last made? If not we can probably just close this.

@seanmonstar
Copy link
Contributor Author

@alexcrichton a difference to the previous PR is that this only adds the From impl for Option, which is a legitimate implementation currently missing.

The choice to alter APIs to use that is not made in this PR, and so the standard library does not need to worry about stabilizing "optional arguments" in itself that could conflict with lang features.

@seanmonstar
Copy link
Contributor Author

Here's a longer rationale:

I don't see these as "optional arguments" in the usual sense. I have several arguments that are definitely not optional, they must have an answer, but whether that answer has a value, or a null, is the reason to use Option<T>.

Optional arguments look like this:

fn action(&mut self, action: Action, dur: Option<Duration> = None) {

}

thing.action(Action::Read); // same as thing.action(Action::Read, None)

Instead, I have methods that do not make sense to have this form of "optional arguments":

fn timeout(&mut self, dur: Option<Duration>) {

}

thing.timeout(); // this would not make sense
thing.timeout(None); // you must give an answer
thing.timeout(Some(10));

The point with this method is to specify if there should be a timeout, and if so, for how long. It does not make sense for this to have a signature of fn timeout(&mut self, dur: Option<Duration> = None).

So, what to do. Seems there's 4 options.

  1. Accept a T in the argument.

    fn timeout(&mut self, dur: Duration);
    
    // elsewhere
    if let Some(dur) = config.action_timeout {
      thing.timeout(dur)
    }

    Passes the Option noise on to the user. This also means that if the timeout cannot be default to Some, because there is no way to pass the intent "please no timeout (None)".

  2. Accept an Option<T> in the argument.

    fn timeout(&mut self, dur: Option<Duration>);
    
    thing.timeout(Some(10)); // thing.timeout(10) would be less noisy

    This removes the Option matching, and now allows thing to have a default of Some(1000), because you can pass None. However, requires that all instances wrap the value in Some, which becomes noisy.

  3. Accept Into<Option<T>> in the argument.

    fn timeout<T: Into<Option<Duration>>>(&mut self, dur: T);
    
    thing.timeout(10);
    thing.timeout(None);

    Requires a patch to libstd, such as this PR.

  4. Create Maybe<T>, with a bunch of Into impls, and now I have the API ergonomics I need.

    enum Maybe<T> { Yes(T), No }
    impl<T> From<T> for Maybe<T> { ... }
    impl From<Option<T>> for Maybe<T> { ... }
    impl From<Maybe<T>> for Option<T> { ... }
    
    fn timeout<T: Into<Maybe<Duration>>>(&mut self, dur: T) {
      let opt_dur = dur.into().into(); // -> Maybe<T> -> Option<T>
    }
    
    thing.timeout(10);
    thing.timeout(Some(5));
    thing.timeout(None);

    Downside is I now have this public Maybe enum, whose sole purpose is so that I can present an ergonomic API to users.

@nagisa
Copy link
Member

nagisa commented Jul 15, 2016

The fact that we have 2 issues and 2 PRs for exactly the same thing (and without any of the optional arguments stuff, although I see it being as a primary motivation), proves that it is a very desired feature, even without some implementation of optional arguments. I know I wanted it for something that’s not optional arguments in some generic code.

@petrochenkov
Copy link
Contributor

@alexcrichton

this has been discussed and turned down before

Turned down without any motivation except for vague concerns about possible future language features.
From<T> for Option<T> meets all possible criterions for From/Into impls, it's faultless, lossless, unambigous, don't change value domain, it doesn't even do extra allocations. It's better than large part of the existing impls in libstd from this point of view.

@sfackler
Copy link
Member

From my recollection, the concerns were over modifications to the timeout APIs in particular. The From impl seems to match with my intuition of what makes sense.

@Stebalien
Copy link
Contributor

I think the concern here is that library authors will start using this feature for optional arguments. Then, if rust ever adds built-in optional/default argument support, there'll be two different ways to do it.

(Note: I'm still in favor of this feature).

@alexcrichton
Copy link
Member

Yes it sounds like this is different than #33170 in that it's not proposing the standard library uses it for optional arguments, but the motivation is still optional arguments in external libraries.

Adding the From impl standalone somewhat makes sense to me, but I disagree with the motivation to add it for optional arguments externally.

@alexcrichton
Copy link
Member

We discussed this during libs triage today and the decision was to merge given that this PR is only adding a From impl and it's the most reasonable impl that could be done. The standard library won't be using this for optional arguments and conventionally it won't "be a thing", but it's up to library authors externally if they'd like to do this themselves.

@alexcrichton
Copy link
Member

@bors: r+

Thanks again for the PR @seanmonstar!

@bors
Copy link
Contributor

bors commented Jul 19, 2016

📌 Commit 501c615 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Jul 19, 2016

Testing commit 501c615 with merge 18e4090...

bors added a commit that referenced this pull request Jul 19, 2016
core: impl From<T> for Option<T>

First, the semantics of this `impl` seem spot on. If I have a value `T`, and I wish to make a `Option<T>`, then `Option::from(val)` should always give `Some(val)`.

Second, this allows improvement for several APIs that currently take `Option<T>` as arguments. Consider:

```rust
fn set_read_timeout(&mut self, timeout: Option<u32>) {
    // ...
}

x.set_read_timeout(Some(30));
x.set_read_timeout(Some(10));
x.set_read_timeout(None);
```

With this `impl`:

```rust
fn set_read_timeout<T: Into<Option<u32>>>(&mut self, timeout: T) {
    let timeout = timeout.into();
    // ...
}

x.set_read_timeout(30);
x.set_read_timeout(10);
x.set_read_timeout(Some(10)); // backwards compatible
x.set_read_timeout(None);
```

The change to those methods aren't included, but could be modified later.

r? @sfackler
@bors
Copy link
Contributor

bors commented Jul 19, 2016

💔 Test failed - auto-linux-64-opt

@alexcrichton
Copy link
Member

@bors: retry

On Tue, Jul 19, 2016 at 12:50 PM, bors notifications@github.com wrote:

💔 Test failed - auto-linux-64-opt
https://buildbot.rust-lang.org/builders/auto-linux-64-opt/builds/9812


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34828 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95FRUuNNofOY6YT2cq5YgOuoHF-CKks5qXSp4gaJpZM4JMyh7
.

@bors
Copy link
Contributor

bors commented Jul 20, 2016

Testing commit 501c615 with merge db889f0...

bors added a commit that referenced this pull request Jul 20, 2016
core: impl From<T> for Option<T>

First, the semantics of this `impl` seem spot on. If I have a value `T`, and I wish to make a `Option<T>`, then `Option::from(val)` should always give `Some(val)`.

Second, this allows improvement for several APIs that currently take `Option<T>` as arguments. Consider:

```rust
fn set_read_timeout(&mut self, timeout: Option<u32>) {
    // ...
}

x.set_read_timeout(Some(30));
x.set_read_timeout(Some(10));
x.set_read_timeout(None);
```

With this `impl`:

```rust
fn set_read_timeout<T: Into<Option<u32>>>(&mut self, timeout: T) {
    let timeout = timeout.into();
    // ...
}

x.set_read_timeout(30);
x.set_read_timeout(10);
x.set_read_timeout(Some(10)); // backwards compatible
x.set_read_timeout(None);
```

The change to those methods aren't included, but could be modified later.

r? @sfackler
@bors
Copy link
Contributor

bors commented Jul 20, 2016

💔 Test failed - auto-mac-64-opt-rustbuild

@alexcrichton
Copy link
Member

@bors: retry

On Tue, Jul 19, 2016 at 9:23 PM, bors notifications@github.com wrote:

💔 Test failed - auto-mac-64-opt-rustbuild
https://buildbot.rust-lang.org/builders/auto-mac-64-opt-rustbuild/builds/1808


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34828 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95MXfVvXdw55EkBmKhKZhQ4c-k-xEks5qXaK0gaJpZM4JMyh7
.

@alexcrichton
Copy link
Member

@bors: r-

Looks like travis failure is legitimate

@alexcrichton alexcrichton removed the I-needs-decision Issues in need of decision. label Jul 20, 2016
@seanmonstar
Copy link
Contributor Author

Added use convert::From;.

@alexcrichton
Copy link
Member

@bors: r+ fbfee42 rollup

@bors
Copy link
Contributor

bors commented Jul 21, 2016

Testing commit fbfee42 with merge 13700cf...

bors added a commit that referenced this pull request Jul 21, 2016
core: impl From<T> for Option<T>

First, the semantics of this `impl` seem spot on. If I have a value `T`, and I wish to make a `Option<T>`, then `Option::from(val)` should always give `Some(val)`.

Second, this allows improvement for several APIs that currently take `Option<T>` as arguments. Consider:

```rust
fn set_read_timeout(&mut self, timeout: Option<u32>) {
    // ...
}

x.set_read_timeout(Some(30));
x.set_read_timeout(Some(10));
x.set_read_timeout(None);
```

With this `impl`:

```rust
fn set_read_timeout<T: Into<Option<u32>>>(&mut self, timeout: T) {
    let timeout = timeout.into();
    // ...
}

x.set_read_timeout(30);
x.set_read_timeout(10);
x.set_read_timeout(Some(10)); // backwards compatible
x.set_read_timeout(None);
```

The change to those methods aren't included, but could be modified later.

r? @sfackler
@bors
Copy link
Contributor

bors commented Jul 21, 2016

💔 Test failed - auto-linux-64-cargotest

@alexcrichton
Copy link
Member

@bors: retry

On Wed, Jul 20, 2016 at 6:04 PM, bors notifications@github.com wrote:

💔 Test failed - auto-linux-64-cargotest
https://buildbot.rust-lang.org/builders/auto-linux-64-cargotest/builds/1195


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34828 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95D8uBXtpwXah0YWuyJoxKyBMlvdxks5qXsWPgaJpZM4JMyh7
.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jul 21, 2016
core: impl From<T> for Option<T>

First, the semantics of this `impl` seem spot on. If I have a value `T`, and I wish to make a `Option<T>`, then `Option::from(val)` should always give `Some(val)`.

Second, this allows improvement for several APIs that currently take `Option<T>` as arguments. Consider:

```rust
fn set_read_timeout(&mut self, timeout: Option<u32>) {
    // ...
}

x.set_read_timeout(Some(30));
x.set_read_timeout(Some(10));
x.set_read_timeout(None);
```

With this `impl`:

```rust
fn set_read_timeout<T: Into<Option<u32>>>(&mut self, timeout: T) {
    let timeout = timeout.into();
    // ...
}

x.set_read_timeout(30);
x.set_read_timeout(10);
x.set_read_timeout(Some(10)); // backwards compatible
x.set_read_timeout(None);
```

The change to those methods aren't included, but could be modified later.

r? @sfackler
@bors bors merged commit fbfee42 into rust-lang:master Jul 21, 2016
@apasel422 apasel422 added the relnotes Marks issues that should be documented in the release notes of the next release. label Jul 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. 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.

None yet

9 participants