Uh oh!
There was an error while loading. Please reload this page.
Change bounds on TryFrom blanket impl to use Into instead of From - #56796
Conversation
rust-highfive
commented
Dec 13, 2018
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
commented
Dec 13, 2018
The job Click to expand the log.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@shepmaster I can't figure out where it thinks there is an impl conflict, could you please help me find it? |
shepmaster
commented
Dec 16, 2018
There's a test that's asserting on an error message and your changes have changed the error message:
|
Reason: Due to changes in bounds of `TryFrom` from `From` to `Into`
fix to last commit
shepmaster
commented
Dec 16, 2018
/cc @SimonSapin any objections? |
SimonSapin
commented
Dec 16, 2018
I think this is fine, especially since |
sfackler
commented
Dec 16, 2018
This seems weird to me, honestly. |
RustyYato
commented
Dec 17, 2018
@sfackler Why? |
sfackler
commented
Dec 17, 2018
Because these are direct promotions of the related trait, not the opposite version of that trait. |
@sfackler I understand that, but if we want the impl We can think of it like this Another way to do it would be to do This would also work, and may be easier to think about. If we don't want to change the 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: Linear: |
RustyYato
commented
Dec 17, 2018
Are there any more objections? |
RustyYato
commented
Dec 18, 2018
I think all objections have been dealt with, can this be merged now? |
shepmaster
commented
Dec 19, 2018
@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
commented
Dec 19, 2018
📌 Commit a1be813 has been approved by |
@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: |
rust-highfive
commented
Dec 19, 2018
The job Click to expand the log.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 |
i.e. `Box`, instead it now uses `Vec`
RustyYato
commented
Dec 19, 2018
@shepmaster I added the test, will that be sufficient or should it be different? |
RustyYato
commented
Dec 20, 2018
@shepmaster It looks like |
Centril
commented
Dec 24, 2018
Assigning over to make sure that |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
shepmaster
commented
Jan 6, 2019
@sfackler any further objections? |
RustyYato
commented
Jan 19, 2019
@sfackler@SimonSapin@shepmaster |
shepmaster
commented
Jan 20, 2019
@bors r+ rollup |
bors
commented
Jan 20, 2019
📌 Commit ea68b3f has been approved by |
…, 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.
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
This... is a weird test. It has two impls: - `impl<T> From<Foo<T>> for Box<T>` (commented out, more on that later), and - `impl<T> Into<Vec<T>> for Foo<T>` The idea of that test is to show that the first impl doesn't compile, but the second does, thus `TryFrom` should be using `Into` and not `From` ( because `Into` is more general, since the `From` impl doesn't compile). However: 1. The types are different -- `Box` vs `Vec`, which is significant b/c `Box` is fundamental 2. The commented out impl actually compiles! (which wasn't detected b/c it's commented out :\ ) Here is a table for compilation of the impls: | | `Vec` | `Box` | |--------|--------------|----------------| | `From` | since 1.41.0 | never | | `Into` | always | not since 1.28 | [godbolt used to test this](https://godbolt.org/z/T38E3jGKa) Order of events: 1. in `1.28` the `incoherent_fundamental_impls` lint becomes deny by default (this is *not* mentioned in the changelog yay) 2. `1.32` changed absolutely nothing, even though this version is credited in the test 3. the test was added (I'm not exactly sure when) (see rust-lang#56796) 4. in `1.41` coherence was relaxed to allow `From`+`Vec` to compile To conclude: since `1.41` this test does nothing (and before that it was written in a way which did not detect this change). It looks to me like today (since `1.41`) we *could* bound `TryFrom` impl with `From` (but now it'd be a useless breaking change of course). Am I missing anything? Is there a useful version of this test that could be written?
This... is a weird test. It has two impls: - `impl<T> From<Foo<T>> for Box<T>` (commented out, more on that later), and - `impl<T> Into<Vec<T>> for Foo<T>` The idea of that test is to show that the first impl doesn't compile, but the second does, thus `TryFrom` should be using `Into` and not `From` (because `Into` is more general, since the `From` impl doesn't compile). However: 1. The types are different -- `Box` vs `Vec`, which is significant b/c `Box` is fundamental 2. The commented out impl actually compiles! (which wasn't detected b/c it's commented out :\ ) Here is a table for compilation of the impls: | | `Vec` | `Box` | |--------|--------------|----------------| | `From` | since 1.41.0 | never | | `Into` | always | not since 1.28 | [godbolt used to test this](https://godbolt.org/z/T38E3jGKa) Order of events: 1. in `1.28` the `incoherent_fundamental_impls` lint becomes deny by default (this is *not* mentioned in the changelog yay) 2. `1.32` changed absolutely nothing, even though this version is credited in the test 3. the test was added (I'm not exactly sure when) (see rust-lang#56796) 4. in `1.41` coherence was relaxed to allow `From`+`Vec` to compile To conclude: since `1.41` this test does nothing (and before that it was written in a way which did not detect this change). It looks to me like today (since `1.41`) we *could* bound `TryFrom` impl with `From` (but now it'd be a useless breaking change of course). Am I missing anything? Is there a useful version of this test that could be written?
This... is a weird test. It has two impls: - `impl<T> From<Foo<T>> for Box<T>` (commented out, more on that later), and - `impl<T> Into<Vec<T>> for Foo<T>` The idea of that test is to show that the first impl doesn't compile, but the second does, thus `TryFrom` should be using `Into` and not `From` (because `Into` is more general, since the `From` impl doesn't compile). However: 1. The types are different -- `Box` vs `Vec`, which is significant b/c `Box` is fundamental 2. The commented out impl actually compiles! (which wasn't detected b/c it's commented out :\ ) Here is a table for compilation of the impls: | | `Vec` | `Box` | |--------|--------------|----------------| | `From` | since 1.41.0 | never | | `Into` | always | not since 1.28 | [godbolt used to test this](https://godbolt.org/z/T38E3jGKa) Order of events: 1. in `1.28` the `incoherent_fundamental_impls` lint becomes deny by default (this is *not* mentioned in the changelog yay) 2. `1.32` changed absolutely nothing, even though this version is credited in the test 3. the test was added (I'm not exactly sure when) (see rust-lang#56796) 4. in `1.41` coherence was relaxed to allow `From`+`Vec` to compile To conclude: since `1.41` this test does nothing (and before that it was written in a way which did not detect this change). It looks to me like today (since `1.41`) we *could* bound `TryFrom` impl with `From` (but now it'd be a useless breaking change of course). Am I missing anything? Is there a useful version of this test that could be written?
…onszelmann Remove an outdated test This... is a weird test. It has two impls: - `impl<T> From<Foo<T>> for Box<T>` (commented out, more on that later), and - `impl<T> Into<Vec<T>> for Foo<T>` The idea of that test is to show that the first impl doesn't compile, but the second does, thus `TryFrom` should be using `Into` and not `From` (because `Into` is more general, since the `From` impl doesn't compile). However: 1. The types are different -- `Box` vs `Vec`, which is significant b/c `Box` is fundamental 2. The commented out impl actually compiles! (which wasn't detected b/c it's commented out :\ ) Here is a table for compilation of the impls: | | `Vec` | `Box` | |--------|--------------|----------------| | `From` | since 1.41.0 | never | | `Into` | always | not since 1.28 | [godbolt used to test this](https://godbolt.org/z/T38E3jGKa) Order of events: 1. in `1.28` the `incoherent_fundamental_impls` lint becomes deny by default (this is *not* mentioned in the changelog yay) 2. `1.32` changed absolutely nothing, even though this version is credited in the test 3. the test was added (I'm not exactly sure when) (see rust-lang#56796) 4. in `1.41` coherence was relaxed to allow `From`+`Vec` to compile To conclude: since `1.41` this test does nothing (and before that it was written in a way which did not detect this change). It looks to me like today (since `1.41`) we *could* bound `TryFrom` impl with `From` (but now it'd be a useless breaking change of course). Am I missing anything? Is there a useful version of this test that could be written?
Rollup merge of #148918 - WaffleLapkin:tryfromwhattttt, r=jdonszelmann Remove an outdated test This... is a weird test. It has two impls: - `impl<T> From<Foo<T>> for Box<T>` (commented out, more on that later), and - `impl<T> Into<Vec<T>> for Foo<T>` The idea of that test is to show that the first impl doesn't compile, but the second does, thus `TryFrom` should be using `Into` and not `From` (because `Into` is more general, since the `From` impl doesn't compile). However: 1. The types are different -- `Box` vs `Vec`, which is significant b/c `Box` is fundamental 2. The commented out impl actually compiles! (which wasn't detected b/c it's commented out :\ ) Here is a table for compilation of the impls: | | `Vec` | `Box` | |--------|--------------|----------------| | `From` | since 1.41.0 | never | | `Into` | always | not since 1.28 | [godbolt used to test this](https://godbolt.org/z/T38E3jGKa) Order of events: 1. in `1.28` the `incoherent_fundamental_impls` lint becomes deny by default (this is *not* mentioned in the changelog yay) 2. `1.32` changed absolutely nothing, even though this version is credited in the test 3. the test was added (I'm not exactly sure when) (see #56796) 4. in `1.41` coherence was relaxed to allow `From`+`Vec` to compile To conclude: since `1.41` this test does nothing (and before that it was written in a way which did not detect this change). It looks to me like today (since `1.41`) we *could* bound `TryFrom` impl with `From` (but now it'd be a useless breaking change of course). Am I missing anything? Is there a useful version of this test that could be written?
This is from this comment I made.
This will expand the impls available for
TryFromandTryInto, without losing anything in the process.