Add `Option::owned` #98061

WaffleLapkin · 2022-06-13T15:23:33Z

This PR adds the following public library APIs:

// alloc::option
impl<T: ?Sized> Option<&T> {
    pub fn owned(self) -> Option<T::Owned>
    where
        T: ToOwned;
}

impl<T: ?Sized> Option<&mut T> {
    pub fn owned(self) -> Option<T::Owned>
    where
        T: ToOwned;
}

// alloc::result
impl<T: ?Sized, E> Result<&T, E> {
    pub fn owned(self) -> Result<T::Owned, E>
    where
        T: ToOwned;
}

impl<T: ?Sized, E> Result<&mut T, E> {
    pub fn owned(self) -> Result<T::Owned, E>
    where
        T: ToOwned;
}

Option::owned is similar to Option::cloned and Option::copied, but uses the ToOwned trait.

I thought that would be easier, but since ToOwned is defined in the alloc crate this becomes incoherent :')

rust-highfive · 2022-06-13T15:23:35Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

rust-highfive · 2022-06-13T15:23:37Z

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

WaffleLapkin · 2022-06-13T15:25:29Z

@rustbot label +T-libs-api -T-libs

RReverser · 2022-06-16T14:57:23Z

It should probably add same methods to Result as well, since it usually mirrors such Option utils (e.g. it also has copied & cloned).

WaffleLapkin · 2022-06-16T15:48:36Z

@RReverser Good catch! I'll add owned method to Result too.

joshtriplett · 2022-06-16T20:57:00Z

I like the concept of this, but as a nit on the name, I think this should be to_owned.

RReverser · 2022-06-17T00:15:16Z

I like the concept of this, but as a nit on the name, I think this should be to_owned.

That wouldn't be possible - .to_owned() can already be called on arbitrary T: Clone refs, including on &Option, and does something different:

let a: &Option<i32> = &Some(1);
let b: Option<i32> = a.to_owned();

camsteffen · 2022-06-23T16:11:05Z

What about borrowed?

let x = Some(String::new());
let y: Option<&str> = x.borrowed();

WaffleLapkin · 2022-06-23T16:14:33Z

@camsteffen your snippet can use .as_deref instead, are there any reasons/use cases to use Borrow instead of Deref?

camsteffen · 2022-06-23T16:16:27Z

Yeah I just realized that. Probably not many such use cases.

zesterer · 2022-08-18T10:41:32Z

Could an implementation for Option<Cow<T>> be added too?

Edit: On second thoughts, that might not be possible given that this would need to be an inherent method in core that mentions a type in std (unless std has magic privileges that allows it to circumvent that problem?)

WaffleLapkin · 2022-08-19T00:42:00Z

@zesterer we already need magic (#[rustc_allow_incoherent_impl]) to define T: ToOwned methods, since ToOwned is defined in alloc :)

Still, I don't think implementation for Option<Cow<T>> is possible -- Cow<T> can implement ToOwned which will overlap with the methods being added.

bors · 2022-09-21T20:29:03Z

☔ The latest upstream changes (presumably #102097) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot · 2022-10-02T22:46:02Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

joshtriplett · 2022-10-03T13:09:28Z

library/alloc/src/option.rs

+    /// to convert borrowed types to their owned variants. For example `Option<&[T]>` to
+    /// `Option<Vec<T>>`


Suggested change

/// to convert borrowed types to their owned variants. For example `Option<&[T]>` to

/// `Option<Vec<T>>`

/// to convert borrowed types to their owned variants. For example, `Option<&[T]>` to

/// `Option<Vec<T>>`, or `Option<&str>` to `Option<String>`.

joshtriplett · 2022-10-03T13:10:04Z

@bors r+

bors · 2022-10-03T13:10:05Z

📌 Commit 2a6f61c has been approved by joshtriplett

It is now in the queue for this repository.

…triplett Add `Option::owned` This PR adds the following public library APIs: ```rust impl<T: ?Sized> Option<&T> { pub const fn owned(self) -> Option<T::Owned> where T: ~const ToOwned; } impl<T: ?Sized> Option<&mut T> { pub fn owned(self) -> Option<T::Owned> where T: ~const ToOwned; } ``` `Option::owned` is similar to `Option::cloned` and `Option::copied`, but uses the `ToOwned` trait. --- I thought that would be easier, but since `ToOwned` is defined in the alloc crate this becomes incoherent :')

matthiaskrgr · 2022-10-03T18:55:32Z

@bors r- CI is not green

     Finished release [optimized] target(s) in 0.17s
std/option/index.html:274: broken link fragment `#impl-FromIterator%3COption%3CA%3E%3E` pointing to `std/option/enum.Option.html`
std/option/index.html:285: broken link fragment `#impl-Product%3COption%3CU%3E%3E` pointing to `std/option/enum.Option.html`
std/option/index.html:286: broken link fragment `#impl-Sum%3COption%3CU%3E%3E` pointing to `std/option/enum.Option.html`
std/result/index.html:321: broken link fragment `#impl-FromIterator%3CResult%3CA%2C%20E%3E%3E` pointing to `std/result/enum.Result.html`
std/result/index.html:332: broken link fragment `#impl-Product%3CResult%3CU%2C%20E%3E%3E` pointing to `std/result/enum.Result.html`
std/result/index.html:333: broken link fragment `#impl-Sum%3CResult%3CU%2C%20E%3E%3E` pointing to `std/result/enum.Result.html`
alloc/option/index.html:274: broken link fragment `#impl-FromIterator%3COption%3CA%3E%3E` pointing to `alloc/option/enum.Option.html`
alloc/option/index.html:285: broken link fragment `#impl-Product%3COption%3CU%3E%3E` pointing to `alloc/option/enum.Option.html`
alloc/option/index.html:286: broken link fragment `#impl-Sum%3COption%3CU%3E%3E` pointing to `alloc/option/enum.Option.html`
alloc/result/index.html:321: broken link fragment `#impl-FromIterator%3CResult%3CA%2C%20E%3E%3E` pointing to `alloc/result/enum.Result.html`
alloc/result/index.html:332: broken link fragment `#impl-Product%3CResult%3CU%2C%20E%3E%3E` pointing to `alloc/result/enum.Result.html`
alloc/result/index.html:333: broken link fragment `#impl-Sum%3CResult%3CU%2C%20E%3E%3E` pointing to `alloc/result/enum.Result.html`
checked links in: 5.7s
number of HTML files scanned: 33005
number of HTML redirects found: 10114
number of links checked: 2495078
number of links ignored due to external: 118674
number of links ignored due to exceptions: 19
number of intra doc links ignored: 25
errors found: 12
found some broken links
Build completed unsuccessfully in 0:22:26

WaffleLapkin · 2022-10-05T09:52:39Z

CI is green (also filled in tracking issue)

m-ou-se · 2022-10-06T11:56:23Z

I really don't think we should be using #[rustc_has_incoherent_inherent_impls].

There's a good reason we don't normally support incoherent impls. It makes maintenance hard, and can easily result in very surprising behavior. For example, no_std code could suddenly compile differently when some other crate in the dependency tree pulls in std. (We already have this issue with f32::sin(), which is something we should solve by moving that method to core.)

Unless we have a plan towards stabilizing something that allows every crate to do this, we should not be merging new incoherent impls into std. Ideally, std and alloc behave as much as possible like regular crates.

klensy · 2022-10-06T12:04:43Z

Current title and description says about Option, while this PR actually affects Result too.

WaffleLapkin · 2023-01-09T23:03:31Z

I don't think that this is a good idea anymore, @m-ou-se's comment summarizes the problems quite well. On top of that, 1) it can be implemented as a crate (via a trait) 2) these methods are not that commonly needed (my assumption, isn't backed by any data).

Thus, I'm closing this.

rust-highfive assigned joshtriplett Jun 13, 2022

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jun 13, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 13, 2022

rustbot added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 13, 2022

This comment has been minimized.

# to view

WaffleLapkin force-pushed the own_the_option branch from bb8bfb4 to 1ca1c0b Compare June 16, 2022 10:42

WaffleLapkin force-pushed the own_the_option branch from 30a4691 to 753e947 Compare June 17, 2022 17:34

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 24, 2022

WaffleLapkin force-pushed the own_the_option branch from 753e947 to 2a6f61c Compare October 2, 2022 22:46

This comment has been minimized.

# to view

joshtriplett reviewed Oct 3, 2022

View reviewed changes

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 3, 2022

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 3, 2022

matthiaskrgr mentioned this pull request Oct 3, 2022

Rollup of 8 pull requests #102630

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 3, 2022

WaffleLapkin mentioned this pull request Oct 5, 2022

Tracking Issue for {Option,Result}::owned #102696

Closed

3 tasks

WaffleLapkin added 4 commits October 5, 2022 08:25

Add Option::owned

7bef45a

Add Result::owned

433749f

fixup alloc::{option,result} docs

114c2c9

fill-in tracking issue

ac2709c

This comment has been minimized.

# to view

WaffleLapkin force-pushed the own_the_option branch from 17d54aa to ac2709c Compare October 5, 2022 08:36

WaffleLapkin closed this Jan 9, 2023

WaffleLapkin deleted the own_the_option branch January 9, 2023 23:03

		/// to convert borrowed types to their owned variants. For example `Option<&[T]>` to
		/// `Option<Vec<T>>`

Add Option::owned #98061

Add Option::owned #98061

Uh oh!

Conversation

WaffleLapkin commented Jun 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Jun 13, 2022

Uh oh!

rust-highfive commented Jun 13, 2022

Uh oh!

WaffleLapkin commented Jun 13, 2022

Uh oh!

This comment has been minimized.

RReverser commented Jun 16, 2022

Uh oh!

WaffleLapkin commented Jun 16, 2022

Uh oh!

joshtriplett commented Jun 16, 2022

Uh oh!

RReverser commented Jun 17, 2022

Uh oh!

camsteffen commented Jun 23, 2022

Uh oh!

WaffleLapkin commented Jun 23, 2022

Uh oh!

camsteffen commented Jun 23, 2022

Uh oh!

zesterer commented Aug 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WaffleLapkin commented Aug 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Sep 21, 2022

Uh oh!

rustbot commented Oct 2, 2022

Uh oh!

This comment has been minimized.

joshtriplett Oct 3, 2022

Choose a reason for hiding this comment

Uh oh!

joshtriplett commented Oct 3, 2022

Uh oh!

bors commented Oct 3, 2022

Uh oh!

matthiaskrgr commented Oct 3, 2022

Uh oh!

This comment has been minimized.

WaffleLapkin commented Oct 5, 2022

Uh oh!

m-ou-se commented Oct 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

klensy commented Oct 6, 2022

Uh oh!

WaffleLapkin commented Jan 9, 2023

Uh oh!

Uh oh!

Add `Option::owned` #98061

Add `Option::owned` #98061

WaffleLapkin commented Jun 13, 2022 •

edited

Loading

zesterer commented Aug 18, 2022 •

edited

Loading

WaffleLapkin commented Aug 19, 2022 •

edited

Loading

m-ou-se commented Oct 6, 2022 •

edited

Loading