Change bounds on `TryFrom` blanket impl to use `Into` instead of `From` #56796

RustyYato · 2018-12-13T21:57:19Z

This is from this comment I made.

This will expand the impls available for TryFrom and TryInto, without losing anything in the process.

rust-highfive · 2018-12-13T21:57:23Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @shepmaster (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.

rust-highfive · 2018-12-13T22:46:54Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_time:end:20e6936e:start=1544738342305069551,finish=1544738344148611549,duration=1843541998
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---
[00:45:08] .................................................................................................... 500/5170
[00:45:11] ..............................i..................................................................... 600/5170
[00:45:14] .................................................................................................... 700/5170
[00:45:20] .................................................................................................... 800/5170
[00:45:24] .i...............i.................................................................................. 900/5170
[00:45:27] ........................iiiii...........................................................F........... 1000/5170
[00:45:32] .................................................................................................... 1200/5170
[00:45:34] .................................................................................................... 1300/5170
[00:45:36] .................................................................................................... 1400/5170
[00:45:38] .................................................................................................... 1500/5170
---
[00:47:37] ---- [ui] ui/e0119/conflict-with-std.rs stdout ----
[00:47:37] diff of stderr:
[00:47:37] 
[00:47:37] 25    |
[00:47:37] 26    = note: conflicting implementation in crate `core`:
[00:47:37] 27            - impl<T, U> std::convert::TryFrom<U> for T
[00:47:37] -              where T: std::convert::From<U>;
[00:47:37] +              where U: std::convert::Into<T>;
[00:47:37] 30 error: aborting due to 3 previous errors
[00:47:37] 31 
[00:47:37] 
[00:47:37] 
[00:47:37] 
[00:47:37] The actual stderr differed from the expected stderr.
[00:47:37] Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/e0119/conflict-with-std/conflict-with-std.stderr
[00:47:37] To update references, rerun the tests and pass the `--bboth the `impl<T> MyTrait for T` where T is all types and the `impl\nMyTrait for Foo`. Since a trait cannot be implemented multiple times,\nthis is an error. So, when you write:\n\n```\ntrait MyTrait {\n    fn get(&self) -> usize;\n}\n\nimpl<T> MyTrait for T {\n    fn get(&self) -> usize { 0 }\n}\n```\n\nThis makes the trait implemented on all types in the scope. So if you\ntry to implement it on another one after that, the implementations will\nconflict. Example:\n\n```\ntrait MyTrait {\n    fn get(&self) -> usize;\n}\n\nimpl<T> MyTrait for T {\n    fn get(&self) -> usize { 0 }\n}\n\nstruct Foo;\n\nfn main() {\n    let f = Foo;\n\n    f.get(); // the trait is implemented so we can use it\n}\n```\n"},"level":"error","spans":[{"file_name":"/checkout/src/test/ui/e0119/conflict-with-std.rs","byte_start":567,"byte_end":591,"line_start":17,"line_end":17,"column_start":1,"column_end":25,"is_primary":true,"text":[{"text":"impl AsRef<Q> for Box<Q> { //~ ERROR conflicting implementations","highlight_start":1,"highlight_end":25}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[{"message":"conflicting implementation in crate `alloc`:\n- impl<T> std::convert::AsRef<T> for std::boxed::Box<T>\n  where T: ?Sized;","code":null,"level":"note","spans":[],"children":[],"rendered":null}],"rendered":"error[E0119]: conflicting implementations of trait `std::convert::AsRef<Q>` for type `std::boxed::Box<Q>`:\n  --> /checkout/src/test/ui/e0119/conflict-with-std.rs:17:1\n   |\nLL | impl AsRef<Q> for Box<Q> { //~ ERROR conflicting implementations\n   | ^^^^^^^^^^^^^^^^^^^^^^^^\n   |\n   = note: ct":24,"line_end":24,"column_start":1,"column_end":19,"is_primary":true,"text":[{"text":"impl From<S> for S { //~ ERROR conflicting implementations","highlight_start":1,"highlight_end":19}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[{"message":"conflicting implementation in crate `core`:\n- impl<T> std::convert::From<T> for T;","code":null,"level":"note","spans":[],"children":[],"rendered":null}],"rendered":"error[E0119]: conflicting implementations of trait `std::convert::From<S>` for type `S`:\n  --> /checkout/src/test/ui/e0119/conflict-with-std.rs:24:1\n   |\nLL | impl From<S> for S { //~ ERROR conflicting implementations\n   | ^^^^^^^^^^^^^^^^^^\n   |\n   = note: conflicting implementation in crate `core`:\n           - impl<T> std::convert::From<T> for T;\n\n"}
[00:47:37] {"message":"conflicting implementations of trait `std::convert::TryFrom<X>` for type `X`:","code":{"code":"E0119","explanation":"\nThere are conflicting trait implementations for the same type.\nExample of erroneous code:\n\n```compile_fail,E0119\ntrait MyTrait {\n    fn get(&self) -> usize;\n}\n\nimpl<T> MyTrait for T {\n    fn get(&self) -> usize { 0 }\n}\n\nstruct Foo {\n    value: usize\n}\n\nimpl MyTrait for Foo { // error: conflicting implementations of trait\n                       //        `MyTrait` for type `Foo`\n    fn get(&self) -> usize { self.value }\n}\n```\n\nWhen looking for the implementation for the trait, the compiler finds\nboth the `impl<T> MyTrait for T` where T is all types and the `impl\nMyTrait for Foo`. Since a trait cannot be implemented multiple times,\nthis is aages
24092 ./src/tools/lldb/packages/Python/lldbsuite
23856 ./src/tools/lldb/packages/Python/lldbsuite/test
23704 ./src/llvm-emscripten/test/tools
23012 ./.git/modules/src/tools/cargo
---
travis_time:end:0242df68:start=1544741210943534273,finish=1544741210947715258,duration=4180985
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:160c8b10
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:c

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

RustyYato · 2018-12-13T23:12:22Z

@TimNN @shepmaster I can't figure out where it thinks there is an impl conflict, could you please help me find it?

shepmaster · 2018-12-16T02:10:01Z

thinks there is an impl conflict

There's a test that's asserting on an error message and your changes have changed the error message:

[00:47:37] ---- [ui] ui/e0119/conflict-with-std.rs stdout ----
[00:47:37] diff of stderr:

Reason: Due to changes in bounds of `TryFrom` from `From` to `Into`

fix to last commit

shepmaster · 2018-12-16T19:25:03Z

/cc @SimonSapin any objections?

SimonSapin · 2018-12-16T23:49:45Z

I think this is fine, especially since TryFrom is unstable. @rust-lang/libs, any other opinion?

sfackler · 2018-12-16T23:53:57Z

This seems weird to me, honestly.

RustyYato · 2018-12-17T00:52:59Z

@sfackler Why?

sfackler · 2018-12-17T01:31:10Z

Because these are direct promotions of the related trait, not the opposite version of that trait.

RustyYato · 2018-12-17T02:17:59Z

@sfackler I understand that, but if we want the impl TryFrom to imply TryInto, which I think we do, and we want to keep the impl From to imply Into, then the only way to get Into to imply TryInto is by going through TryFrom. Otherwise we could get conflicting impls of TryInto. This is simply a way to sidestep that. It does look a bit weird, but it is necessary to have the obvious things happen.

We can think of it like this
From implies Into (and these two traits are semantically equivalent)
Into implies TryFrom (this impl stitches together fallible and infallible conversions)
TryFrom implies TryInto (similar to how From implies Into, TryFrom and TryInto are semantically equivalent, so this impl must exist in one form or another)

Another way to do it would be to do
From implies Into
Into implies TryInto
TryInto implies TryFrom

This would also work, and may be easier to think about.

If we don't want to change the TryFrom implies TryInto impl, then the only way to get all infallible conversions to have fallible equivalents (where the Err is !), is to have Into imply TryFrom.
Or we could chave Into imply TryInto and TryInto imply TryFrom, and that would pipeline all conversions.

Overall, having a tree structure for this would be bad, as it would punish some impls for reasons beyond their control. So I want to have a linear structure.

Tree:
From -> TryFrom -> TryInto
L> Into

Linear:
From -> Into -> TryFrom -> TryInto
or
From -> Into -> TryInto -> TryFrom

RustyYato · 2018-12-17T17:31:17Z

Are there any more objections?

RustyYato · 2018-12-18T19:33:48Z

@shepmaster

I think all objections have been dealt with, can this be merged now?

shepmaster · 2018-12-19T03:01:31Z

@bors r+ rollup

I think, since it's unstable, it's worth giving this a shot.

@KrishnaSannasi is it possible to write a test that shows the benefit of this ordering that would not be possible with the current setup?

bors · 2018-12-19T03:01:33Z

📌 Commit a1be813 has been approved by shepmaster

RustyYato · 2018-12-19T03:40:29Z

@shepmaster I'm not sure how to write tests, as this is my first go at hacking into the compiler, could you please give an example of how to do it?

edit:
I found the docs for adding tests, should I just add the test to src/test/rust-pass/ in a new file, or should I put it in another folder?

rust-highfive · 2018-12-19T05:12:39Z

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_time:end:00ef4094:start=1545193215939943394,finish=1545193284925386171,duration=68985442777
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
[00:49:48] ....................i............................................................................... 2400/2928
[00:49:58] .................................................................................................... 2500/2928
[00:50:29] .................................................................................................... 2600/2928
[00:50:37] .................................................................................................... 2700/2928
[00:50:47] ..............F..................................................................................... 2800/2928
primary":true,"text":[{"text":"impl<T> Into<Box<T>> for Foo<T> {","highlight_start":1,"highlight_end":32}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[{"message":"#[deny(incoherent_fundamental_impls)] on by default","code":null,"level":"note","spans":[],"children":[],"rendered":null},{"message":"this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!","code":null,"level":"warning","spans":[],"children":[],"rendered":null},{"message":"for more information, see issue #46205 <https://github.com/rust-lang/rust/issues/46205>","code":null,"level":"note","spans":[],"children":[],"rendered":null},{"message":"conflicting implementation in crate `core`:\n- impl<T, U> std::convert::Into<U> for T\n  where U: std::convert::From<T>;","code":null,"level":"note","spans":[],"children":[],"rendered":null},{"message":"downstream crates may implement trait `std::convert::From<Foo<_>>` for type `std::boxed::Box<_>`","code":null,"level":"note","spans":[],"children":[],"rendered":null}],"rendered":"error: conflicting implementations of trait `std::convert::Into<std::boxed::Box<_>>` for type `Foo<_>`: (E0119)\n  --> /checkout/src/test/run-pass/try_from.rs:35:1\n   |\nLL | impl<T> Into<Box<T>> for Foo<T> {\n   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n   |\n   = note: #[deny(incoherent_fundamental_impls)] on by default\n   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!\n   = note: for more information, see issue #46205 <https://github.com/rust-lang/rust/" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-6.0/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "6.0.0\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[00:51:01] 
[00:51:01] 
[00:51:01] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[00:51:01] Build completed unsuccessfully in 0:09:28
[00:51:01] Build completed unsuccessfully in 0:09:28
[00:51:01] Makefile:58: recipe for target 'check' failed
[00:51:01] make: *** [check] Error 1

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

i.e. `Box`, instead it now uses `Vec`

RustyYato · 2018-12-19T14:45:37Z

@shepmaster I added the test, will that be sufficient or should it be different?

RustyYato · 2018-12-20T23:29:39Z

@shepmaster It looks like TryFrom and TryInto are going to be merged soon, can this pull request be merged before that?

Centril · 2018-12-24T17:14:38Z

Assigning over to make sure that
r? @sfackler
agrees with merging this.

src/test/run-pass/try_from.rs

shepmaster · 2019-01-06T01:36:23Z

@sfackler any further objections?

RustyYato · 2019-01-19T23:23:11Z

@sfackler @SimonSapin @shepmaster
Since it has been two weeks since the last objection, and no new objections have been made, I don't think there are any objections to merging this. Can this be merged?

shepmaster · 2019-01-20T00:43:48Z

@bors r+ rollup

bors · 2019-01-20T00:43:48Z

📌 Commit ea68b3f has been approved by shepmaster

…, r=shepmaster Change bounds on `TryFrom` blanket impl to use `Into` instead of `From` This is from this [comment](rust-lang#33417 (comment)) I made. This will expand the impls available for `TryFrom` and `TryInto`, without losing anything in the process.

@ghost

Rollup of 5 pull requests Successful merges: - #56796 (Change bounds on `TryFrom` blanket impl to use `Into` instead of `From`) - #57768 (Continue parsing after parent type args and suggest using angle brackets) - #57769 (Suggest correct cast for struct fields with shorthand syntax) - #57783 (Add "dereference boxed value" suggestion.) - #57784 (Add span for bad doc comment) Failed merges: r? @ghost

Change bounds on TryFrom blanket impl to use Into instead of From

f8bd80a

rust-highfive assigned shepmaster Dec 13, 2018

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

scottmcm added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Dec 15, 2018

RustyYato added 2 commits December 16, 2018 13:01

Changed expected error message for TryFrom

7a25a7c

Reason: Due to changes in bounds of `TryFrom` from `From` to `Into`

Fixed Into bound on TryFrom error.

a1be813

fix to last commit

RustyYato mentioned this pull request Dec 17, 2018

Tracking issue for TryFrom/TryInto traits #33417

Closed

3 tasks

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 19, 2018

added test to show motivation for modified TryFrom impl

db9fe1c

fixed test, now it doesn't use a fundemental type

7dd078f

i.e. `Box`, instead it now uses `Vec`

rust-highfive assigned sfackler and unassigned shepmaster Dec 24, 2018

shepmaster reviewed Jan 2, 2019

View reviewed changes

update to reflect changes recommended by @shepmaster his review

ea68b3f

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 3, 2019

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 20, 2019

Centril mentioned this pull request Jan 21, 2019

Rollup of 5 pull requests #57792

Merged

bors merged commit ea68b3f into rust-lang:master Jan 21, 2019

RustyYato deleted the try_from_impl_change branch February 21, 2019 21:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change bounds on `TryFrom` blanket impl to use `Into` instead of `From` #56796

Change bounds on `TryFrom` blanket impl to use `Into` instead of `From` #56796

RustyYato commented Dec 13, 2018

rust-highfive commented Dec 13, 2018

rust-highfive commented Dec 13, 2018

RustyYato commented Dec 13, 2018 •

edited

Loading

shepmaster commented Dec 16, 2018

shepmaster commented Dec 16, 2018

SimonSapin commented Dec 16, 2018

sfackler commented Dec 16, 2018

RustyYato commented Dec 17, 2018

sfackler commented Dec 17, 2018

RustyYato commented Dec 17, 2018 •

edited

Loading

RustyYato commented Dec 17, 2018

RustyYato commented Dec 18, 2018

shepmaster commented Dec 19, 2018

bors commented Dec 19, 2018

RustyYato commented Dec 19, 2018 •

edited

Loading

rust-highfive commented Dec 19, 2018

RustyYato commented Dec 19, 2018

RustyYato commented Dec 20, 2018

Centril commented Dec 24, 2018

shepmaster commented Jan 6, 2019

RustyYato commented Jan 19, 2019

shepmaster commented Jan 20, 2019

bors commented Jan 20, 2019

Change bounds on TryFrom blanket impl to use Into instead of From #56796

Change bounds on TryFrom blanket impl to use Into instead of From #56796

Conversation

RustyYato commented Dec 13, 2018

rust-highfive commented Dec 13, 2018

rust-highfive commented Dec 13, 2018

RustyYato commented Dec 13, 2018 • edited Loading

shepmaster commented Dec 16, 2018

shepmaster commented Dec 16, 2018

SimonSapin commented Dec 16, 2018

sfackler commented Dec 16, 2018

RustyYato commented Dec 17, 2018

sfackler commented Dec 17, 2018

RustyYato commented Dec 17, 2018 • edited Loading

RustyYato commented Dec 17, 2018

RustyYato commented Dec 18, 2018

shepmaster commented Dec 19, 2018

bors commented Dec 19, 2018

RustyYato commented Dec 19, 2018 • edited Loading

rust-highfive commented Dec 19, 2018

RustyYato commented Dec 19, 2018

RustyYato commented Dec 20, 2018

Centril commented Dec 24, 2018

shepmaster commented Jan 6, 2019

RustyYato commented Jan 19, 2019

shepmaster commented Jan 20, 2019

bors commented Jan 20, 2019

Change bounds on `TryFrom` blanket impl to use `Into` instead of `From` #56796

Change bounds on `TryFrom` blanket impl to use `Into` instead of `From` #56796

RustyYato commented Dec 13, 2018 •

edited

Loading

RustyYato commented Dec 17, 2018 •

edited

Loading

RustyYato commented Dec 19, 2018 •

edited

Loading