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

Rename TryFrom's associated type and implement str::parse using TryFrom. #40281

Merged
merged 1 commit into from
Mar 20, 2017
Merged

Rename TryFrom's associated type and implement str::parse using TryFrom. #40281

merged 1 commit into from
Mar 20, 2017

Conversation

jimmycuadra
Copy link
Contributor

Per discussion on the tracking issue, naming TryFrom's associated type Error is generally more consistent with similar traits in the Rust ecosystem, and what people seem to assume it should be called. It also helps disambiguate from Result::Err, the most common "Err".

See #33417 (comment).

TryFrom<&str> and FromStr are equivalent, so have the latter provide the former to ensure that. Using TryFrom in the implementation of str::parse means types that implement either trait can use it. When we're ready to stabilize TryFrom, we should update FromStr to
suggest implementing TryFrom<&str> instead for new code.

See #33417 (comment)
and #33417 (comment).

Refs #33417.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@sfackler
Copy link
Member

sfackler commented Mar 5, 2017

This seems reasonable to me, though I'm not sure if that blanket impl will conflict with any other's we may want to add (e.g. impl<T, U> TryFrom<U> for T where T: From<U>).

@alexcrichton
Copy link
Member

The renaming also sounds fine to me, yeah, but I agree that I'm less sure of the coherence impact of the blanket impl here. It definitely seems desirable but I'd want to make sure it doesn't otherwise forbid useful TryFrom impls.

(regardless due to a change in signature we'll likely want to do a crater run just to make sure)

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Mar 6, 2017
@aturon
Copy link
Member

aturon commented Mar 14, 2017

We discussed this PR in the libs team triage, and are happy to go in this directions. Thanks @jimmycuadra!

@bors: r+

@bors
Copy link
Contributor

bors commented Mar 14, 2017

📌 Commit 7fe2e7e has been approved by aturon

@bors
Copy link
Contributor

bors commented Mar 15, 2017

🔒 Merge conflict

Per discussion on the tracking issue, naming `TryFrom`'s associated type
`Error` is generally more consistent with similar traits in the Rust
ecosystem, and what people seem to assume it should be called. It
also helps disambiguate from `Result::Err`, the most common "Err".

See
#33417 (comment).

TryFrom<&str> and FromStr are equivalent, so have the latter provide the
former to ensure that. Using TryFrom in the implementation of
`str::parse` means types that implement either trait can use it.
When we're ready to stabilize `TryFrom`, we should update `FromStr` to
suggest implementing `TryFrom<&str>` instead for new code.

See
#33417 (comment)
and
#33417 (comment).

Refs #33417.
@alexcrichton
Copy link
Member

@bors: r=aturon

@bors
Copy link
Contributor

bors commented Mar 15, 2017

📌 Commit 2561dcd has been approved by aturon

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 17, 2017
…uron

Rename TryFrom's associated type and implement str::parse using TryFrom.

Per discussion on the tracking issue, naming `TryFrom`'s associated type `Error` is generally more consistent with similar traits in the Rust ecosystem, and what people seem to assume it should be called. It also helps disambiguate from `Result::Err`, the most common "Err".

See rust-lang#33417 (comment).

`TryFrom<&str>` and `FromStr` are equivalent, so have the latter provide the former to ensure that. Using `TryFrom` in the implementation of `str::parse` means types that implement either trait can use it. When we're ready to stabilize `TryFrom`, we should update `FromStr` to
suggest implementing `TryFrom<&str>` instead for new code.

See rust-lang#33417 (comment)
and rust-lang#33417 (comment).

Refs rust-lang#33417.
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 17, 2017
…uron

Rename TryFrom's associated type and implement str::parse using TryFrom.

Per discussion on the tracking issue, naming `TryFrom`'s associated type `Error` is generally more consistent with similar traits in the Rust ecosystem, and what people seem to assume it should be called. It also helps disambiguate from `Result::Err`, the most common "Err".

See rust-lang#33417 (comment).

`TryFrom<&str>` and `FromStr` are equivalent, so have the latter provide the former to ensure that. Using `TryFrom` in the implementation of `str::parse` means types that implement either trait can use it. When we're ready to stabilize `TryFrom`, we should update `FromStr` to
suggest implementing `TryFrom<&str>` instead for new code.

See rust-lang#33417 (comment)
and rust-lang#33417 (comment).

Refs rust-lang#33417.
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 17, 2017
…uron

Rename TryFrom's associated type and implement str::parse using TryFrom.

Per discussion on the tracking issue, naming `TryFrom`'s associated type `Error` is generally more consistent with similar traits in the Rust ecosystem, and what people seem to assume it should be called. It also helps disambiguate from `Result::Err`, the most common "Err".

See rust-lang#33417 (comment).

`TryFrom<&str>` and `FromStr` are equivalent, so have the latter provide the former to ensure that. Using `TryFrom` in the implementation of `str::parse` means types that implement either trait can use it. When we're ready to stabilize `TryFrom`, we should update `FromStr` to
suggest implementing `TryFrom<&str>` instead for new code.

See rust-lang#33417 (comment)
and rust-lang#33417 (comment).

Refs rust-lang#33417.
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 18, 2017
…uron

Rename TryFrom's associated type and implement str::parse using TryFrom.

Per discussion on the tracking issue, naming `TryFrom`'s associated type `Error` is generally more consistent with similar traits in the Rust ecosystem, and what people seem to assume it should be called. It also helps disambiguate from `Result::Err`, the most common "Err".

See rust-lang#33417 (comment).

`TryFrom<&str>` and `FromStr` are equivalent, so have the latter provide the former to ensure that. Using `TryFrom` in the implementation of `str::parse` means types that implement either trait can use it. When we're ready to stabilize `TryFrom`, we should update `FromStr` to
suggest implementing `TryFrom<&str>` instead for new code.

See rust-lang#33417 (comment)
and rust-lang#33417 (comment).

Refs rust-lang#33417.
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 18, 2017
…uron

Rename TryFrom's associated type and implement str::parse using TryFrom.

Per discussion on the tracking issue, naming `TryFrom`'s associated type `Error` is generally more consistent with similar traits in the Rust ecosystem, and what people seem to assume it should be called. It also helps disambiguate from `Result::Err`, the most common "Err".

See rust-lang#33417 (comment).

`TryFrom<&str>` and `FromStr` are equivalent, so have the latter provide the former to ensure that. Using `TryFrom` in the implementation of `str::parse` means types that implement either trait can use it. When we're ready to stabilize `TryFrom`, we should update `FromStr` to
suggest implementing `TryFrom<&str>` instead for new code.

See rust-lang#33417 (comment)
and rust-lang#33417 (comment).

Refs rust-lang#33417.
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 18, 2017
…uron

Rename TryFrom's associated type and implement str::parse using TryFrom.

Per discussion on the tracking issue, naming `TryFrom`'s associated type `Error` is generally more consistent with similar traits in the Rust ecosystem, and what people seem to assume it should be called. It also helps disambiguate from `Result::Err`, the most common "Err".

See rust-lang#33417 (comment).

`TryFrom<&str>` and `FromStr` are equivalent, so have the latter provide the former to ensure that. Using `TryFrom` in the implementation of `str::parse` means types that implement either trait can use it. When we're ready to stabilize `TryFrom`, we should update `FromStr` to
suggest implementing `TryFrom<&str>` instead for new code.

See rust-lang#33417 (comment)
and rust-lang#33417 (comment).

Refs rust-lang#33417.
@bors
Copy link
Contributor

bors commented Mar 19, 2017

⌛ Testing commit 2561dcd with merge 432843b...

@bors
Copy link
Contributor

bors commented Mar 19, 2017

💔 Test failed - status-travis

@arielb1
Copy link
Contributor

arielb1 commented Mar 19, 2017

sccache broken pipe:

DEBUG:sccache::compiler::compiler: parse_arguments: `-DGTEST_HAS_RTTI=0 -D_DEBUG -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/Users/travis/build/rust-lang/rust/build/i686-apple-darwin/llvm/build/lib/MC -I/Users/travis/build/rust-lang/rust/src/llvm/lib/MC -I/Users/travis/build/rust-lang/rust/build/i686-apple-darwin/llvm/build/include -I/Users/travis/build/rust-lang/rust/src/llvm/include -ffunction-sections -fdata-sections -fPIC -m32 -stdlib=libc++ -fPIC -fvisibility-inlines-hidden -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Werror=date-time -std=c++11 -O3 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk -mmacosx-version-min=10.8 -UNDEBUG -fno-exceptions -fno-rtti -o CMakeFiles/LLVMMC.dir/MCCodeEmitter.cpp.o -c /Users/travis/build/rust-lang/rust/src/llvm/lib/MC/MCCodeEmitter.cpp`

DEBUG:sccache::compiler::compiler: parse_arguments: Ok

DEBUG:sccache::compiler::compiler: [MCCodeEmitter.cpp.o]: get_cached_or_compile: -DGTEST_HAS_RTTI=0 -D_DEBUG -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/Users/travis/build/rust-lang/rust/build/i686-apple-darwin/llvm/build/lib/MC -I/Users/travis/build/rust-lang/rust/src/llvm/lib/MC -I/Users/travis/build/rust-lang/rust/build/i686-apple-darwin/llvm/build/include -I/Users/travis/build/rust-lang/rust/src/llvm/include -ffunction-sections -fdata-sections -fPIC -m32 -stdlib=libc++ -fPIC -fvisibility-inlines-hidden -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Werror=date-time -std=c++11 -O3 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk -mmacosx-version-min=10.8 -UNDEBUG -fno-exceptions -fno-rtti -o CMakeFiles/LLVMMC.dir/MCCodeEmitter.cpp.o -c /Users/travis/build/rust-lang/rust/src/llvm/lib/MC/MCCodeEmitter.cpp

DEBUG:sccache::compiler::compiler: [MCCodeEmitter.cpp.o]: preprocessor failed: Error(Msg("failed to spawn child"), State { next_error: Some(Error { repr: Os { code: 32, message: "Broken pipe" } }) })

@bors retry

@arielb1
Copy link
Contributor

arielb1 commented Mar 19, 2017

@alexcrichton - why is spawn of all things returning EPIPE? It does not seem to do anything that could cause that error.

@bors
Copy link
Contributor

bors commented Mar 19, 2017

⌛ Testing commit 2561dcd with merge 37e4c14...

@arielb1
Copy link
Contributor

arielb1 commented Mar 19, 2017

Mac builders not starting. Someone call travis.

@bors retry

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 20, 2017
…uron

Rename TryFrom's associated type and implement str::parse using TryFrom.

Per discussion on the tracking issue, naming `TryFrom`'s associated type `Error` is generally more consistent with similar traits in the Rust ecosystem, and what people seem to assume it should be called. It also helps disambiguate from `Result::Err`, the most common "Err".

See rust-lang#33417 (comment).

`TryFrom<&str>` and `FromStr` are equivalent, so have the latter provide the former to ensure that. Using `TryFrom` in the implementation of `str::parse` means types that implement either trait can use it. When we're ready to stabilize `TryFrom`, we should update `FromStr` to
suggest implementing `TryFrom<&str>` instead for new code.

See rust-lang#33417 (comment)
and rust-lang#33417 (comment).

Refs rust-lang#33417.
@bors
Copy link
Contributor

bors commented Mar 20, 2017

⌛ Testing commit 2561dcd with merge 6738cd4...

bors added a commit that referenced this pull request Mar 20, 2017
Rename TryFrom's associated type and implement str::parse using TryFrom.

Per discussion on the tracking issue, naming `TryFrom`'s associated type `Error` is generally more consistent with similar traits in the Rust ecosystem, and what people seem to assume it should be called. It also helps disambiguate from `Result::Err`, the most common "Err".

See #33417 (comment).

`TryFrom<&str>` and `FromStr` are equivalent, so have the latter provide the former to ensure that. Using `TryFrom` in the implementation of `str::parse` means types that implement either trait can use it. When we're ready to stabilize `TryFrom`, we should update `FromStr` to
suggest implementing `TryFrom<&str>` instead for new code.

See #33417 (comment)
and #33417 (comment).

Refs #33417.
@bors
Copy link
Contributor

bors commented Mar 20, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: aturon
Pushing 6738cd4 to master...

bors added a commit that referenced this pull request Mar 20, 2017
Rollup of 9 pull requests

- Successful merges: #40241, #40281, #40398, #40521, #40532, #40554, #40566, #40581, #40587
- Failed merges:
@bors bors merged commit 2561dcd into rust-lang:master Mar 20, 2017
@jimmycuadra jimmycuadra deleted the try-from-from-str branch March 20, 2017 08:35
mcginty added a commit to mcginty/snow that referenced this pull request Mar 21, 2017
Emilgardis added a commit to Emilgardis/ATM-machine that referenced this pull request Mar 26, 2017
TedDriggs added a commit to TedDriggs/rust-derive-builder that referenced this pull request Apr 11, 2017
Lukazoid added a commit to Lukazoid/lz_quic that referenced this pull request Apr 20, 2017
bors added a commit that referenced this pull request Sep 29, 2017
Add blanket TryFrom impl when From is implemented.

Adds `impl<T, U> TryFrom<T> for U where U: From<T>`.

Removes `impl<'a, T> TryFrom<&'a str> for T where T: FromStr` (originally added in #40281) due to overlapping impls caused by the new blanket impl. This removal is to be discussed further on the tracking issue for TryFrom.

Refs #33417.

/cc @sfackler, @scottmcm (thank you for the help!), and @aturon
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.

7 participants