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

Into implementations should be changed to From #37561

Closed
clarfonthey opened this issue Nov 3, 2016 · 7 comments
Closed

Into implementations should be changed to From #37561

clarfonthey opened this issue Nov 3, 2016 · 7 comments
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@clarfonthey
Copy link
Contributor

clarfonthey commented Nov 3, 2016

Currently, libstd has:

impl Into<Vec<u8>> for String
impl Into<OsString> for PathBuf

And this seems inconsistent with the other collection implementations which all use From instead. It makes sense that these would be changed to From to allow use of from, and it doesn't seem like it'd be a breaking change at all.

@steveklabnik
Copy link
Member

it doesn't seem like it'd be a breaking change at all.

Given that all From impls yield an Into impl, probably not, but I wonder what @rust-lang/libs has to say about why this was chosen.

@steveklabnik steveklabnik added A-libs T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Nov 3, 2016
@alexcrichton
Copy link
Member

Seems reasonable to me! I believe Into doesn't imply From (but the other way around does). We should switch these for consistency.

@retep998
Copy link
Member

retep998 commented Nov 3, 2016

From for the Vec<u8> options is suspicious because that operation can fail. From for OsString to PathBuf seems fine though.

@caipre
Copy link
Contributor

caipre commented Nov 3, 2016

@clarcharr : Did you mean to include a distinct third case? The first and third are both Into<Vec<u8>> for String.

@cuviper
Copy link
Member

cuviper commented Nov 4, 2016

@retep998 I think you're reading those backwards. String can become Vec<u8> without fail (but not vice versa), so impl Into<Vec<u8>> for String would be changed to impl From<String> for Vec<u8>.

@clarfonthey
Copy link
Contributor Author

@caipre I actually completely missed that. I just copied this from the docs, which seem to have duplicated the impl on the list? Not sure if that's a bug.

@retep998
Copy link
Member

retep998 commented Nov 4, 2016

@cuviper Oh right, From<T> for U is still omni directional, it just allows you to do U::from() in addition to T::into(). In that case all those impls are fine by me.

alexcrichton added a commit to alexcrichton/rust that referenced this issue Nov 4, 2016
…alexcrichton

Change `Into<Vec<u8>> for String` and `Into<OsString> for PathBuf` to From

Fixes rust-lang#37561. First contribution, happy with any and all feedback!
sophiajt pushed a commit to sophiajt/rust that referenced this issue Nov 5, 2016
…alexcrichton

Change `Into<Vec<u8>> for String` and `Into<OsString> for PathBuf` to From

Fixes rust-lang#37561. First contribution, happy with any and all feedback!
alexcrichton added a commit to alexcrichton/rust that referenced this issue Nov 5, 2016
…alexcrichton

Change `Into<Vec<u8>> for String` and `Into<OsString> for PathBuf` to From

Fixes rust-lang#37561. First contribution, happy with any and all feedback!
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

No branches or pull requests

6 participants