Skip to content

Conversation

@WaffleLapkin
Copy link
Member

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

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 Change bounds on TryFrom blanket impl to use Into instead of From #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?
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 13, 2025
@rustbot
Copy link
Collaborator

rustbot commented Nov 13, 2025

r? @davidtwco

rustbot has assigned @davidtwco.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Copy link
Contributor

@jdonszelmann jdonszelmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've looked through the tests, and I can't find one (not even doc test) that tests the fact that try_from has a blanket impl for T: Into, etc. So maybe having some library test for that would be nice? This test on its own seems indeed useless though, since 1.41 definitely.

View changes since this review

@rustbot rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants