New lint: Use Into/TryInto in bounds as opposed to From/TryFrom

[Kixunil]

When writing generic bounds such as:

fn foo<T>(a: T) where u32: From<T>;

Into should be preferred, like this:

fn foo<T>(a: T) where T: Into<u32>;

Why the former is bad: Into is a superset of From. In some cases coherence rules prevent implementing From but do allow implementing Into. As a result Into is more generic bound than From.

Category: Style, I guess?

[blasrodri]

@Kixunil Why is it that Into is a superset of From? I thought that they were linked 1:1.
Quoting The Book:

The From and Into traits are inherently linked, and this is actually part of its implementation. If you are able to convert type A from type B, then it should be easy to believe that we should be able to convert type B to type A.
Can you please elaborate further? Thanks :)

[Kixunil]

It's possible to implement Into without implementing From. From implies Into, but Into doesn't imply From Of course normally, people should implement From, not Into but that is not always possible due to coherence issues. (Can't remember specific example right now, but I'm sure I did encounter it once.)

[blasrodri]

Thanks, @Kixunil .
So the idea is that whenever it's possible to implement From, and Into was chosen instead, it should be linted?

[MikailBag]

As far as I remember, several months ago coherence checking rules were improved, and now there is no case when impl Into works, but impl From doesn't.

[blasrodri]

@MikailBag

I think there's still the case where you want to implement a From for a type that isn't defined in your crate. In that case, you can always implement Into and it will work.

Source: https://doc.rust-lang.org/std/convert/trait.Into.html

[sinkuu]

re_balance_coherence feature will be stabilized in 1.41. Beta version of doc says "Prior to Rust 1.40, if the destination type was not part of the current crate...".

[Kixunil]

@blasrodri The point of this issue is to lint bounds, not impls. I guess another lint could be created for impls. (Basically lint whether the code is written according to what the 1.41 docs says.)

[xiongmao86]

If no one is working on this, I'll take a stab.

[xiongmao86]

The history of rust-clippy on the component history pages shows that the cilippy is successfully build from 05-30 to 06-02 today. But I have failed to compile the master of 5-31 with nightly-2020-05-31, a commit which pass ci test at that day. I have also tried 5-30's nightly and also failed. This is really strange, since both ci test show passed when I look at the pr of the commit that I tried to compile. And the compile of my fork master update to that remote master is also failed. Does anybody have idea about what's going on, what makes compile failed? I am really curios why this would happen.

Right now I am trying to rustup to the 06-02 to see if I can build the master. But I suspect that would be failing too.

[flip1995]

@xiongmao86 Clippy is now a subtree in the rust repo, which effectively means, that Clippy will never show up as "missing" on this page again. It may be the case, that some commits from the rust repo have to be copied/synced to the Clippy repo. See https://github.com/rust-lang/rust-clippy/blob/master/CONTRIBUTING.md#fixing-build-failures-caused-by-rust for the new process. We're currently looking in pinning a nightly, but aren't quite there yet: #5561

[xiongmao86]

@flip1995, Thanks for replying, I understand subtree is said to be the new way recommended against submodule. I vaguely grabbed the idea. But I couldn't find articles discussing this in detail. I am interesting in why clippy is decided to transit to subtree way. But I didn't find discussion by searching on this repo.

And what is the impact on the way of development: after pinning to the specific version of nightly, I am very happy that I won't need proxy to download rustc, rust-std etc. and the mirror in my country can really speed up my download. But I not sure do we still need to make "rustup" pr, do we still have to change clippy to catch up the change happen in rustc-dev? Or do we shift the responsibility to rustc developers, and here we only focus on lints, and if failed to build, we sync the change?

[flip1995]

@xiongmao86 This is the issue about moving to subtrees: rust-lang/rust#70651

TL;DR: with subtrees rustc contributors have to deal with fixing the Clippy build in the rustc repo, while folks who just want to contribute to Clippy and not clone/compile rustc, can still do this in this repo.

But I not sure do we still need to make "rustup" pr, do we still have to change clippy to catch up the change happen in rustc-dev?

Yes, currently we have to do a subtree sync for every build failure, since we're still testing with rustc master toolchain. In the future, we want to pin a nightly, which would mean that Clippy development is possible with just rustup (instead of RTIM, which is currently used to install the master toolchain).

Internally, we then also can sync whenever we want, and don't have pressure to sync every time a rustc PR broke Clippy.

[xiongmao86]

Thanks, I'll digest the information.

[xiongmao86]

@flip1995, if I understand correctly, since #6401 pins clippy to a version, I can continue this?

[flip1995]

@xiongmao86 Yes, Clippy now compiles with the nightly specified in rust-toolchain. Make sure you have the latest rustup 1.23.1 (rustup self update)

[xiongmao86]

@flip1995, I am not sure which pass to choose, early lint or late lint?

[xiongmao86]

If I can use late lint pass, this lint would cover wider range of code, by I am not sure it is solvable using late lint.

[flip1995]

You will probably have to use a late pass, to make sure that the bound is actually talking about the From trait from std, not some user defined trait.

[xiongmao86]

@flip1995, do I need to specify a minimum supported rust version? In what case should I specify one?

[flip1995]

An MSRV only has to be added if you produce a suggestion, that includes code, that is not available in early Rust versions. I don't think this is necessary for this lint.

[Kixunil]

Linting bounds should be independent of rustc version, since changing From to Into should work in every version.

Linting impl should depend on version >= 1.41, see https://doc.rust-lang.org/std/convert/trait.Into.html#implementing-into-for-conversions-to-external-types-in-old-versions-of-rust

[xiongmao86]

Thank you for your reply, @Kixunil, so since I am merely concern with bounds, I don't need to use msrv.

@flip1995, @Kixunil, is checking the Path contained in GenericBound against From and TryFrom the right way specify the bound is of From or TryFrom trait?

[flip1995]

You will have to check this with is_{ty_}diagnostic_item.

[Allen-Webb]

I ran into a message generated by this in a case where both Into and TryFrom are implemented for B. When this is the case, it is a good indication that you can go from B -> A, but not always from A -> B and the warning isn't needed.

[flip1995]

@Allen-Webb Thanks for noting that! Do you have a specific code example where you got this message? The lint in this issue isn't in Clippy yet, but WIP. Maybe it's another bug in Clippy? If you can't share the code or a minimal reproducer just the message you got would already help.

[Kixunil]

Sounds like what @Allen-Webb describes is something like this:

struct ValidatedString(String);

impl TryFrom<String> for ValidatedString {
...
}

impl Into<String> or ValidatedString {
...
}

In this case the lint would be correct and the fix is to change last impl to this:

impl From<ValidatedString> for String {
...
}

[Allen-Webb]

Sounds like what @Allen-Webb describes is something like this:

struct ValidatedString(String);

impl TryFrom<String> for ValidatedString {
...
}

impl Into<String> or ValidatedString {
...
}

In this case the lint would be correct and the fix is to change last impl to this:

impl From<ValidatedString> for String {
...
}

This makes sense. My mistake. In that case it might be helpful if the recommendation actually printed the type so it was explicitly obvious (implied). Here is an example error:

warning: an implementation of `From` is preferred since it gives you `Into<_>` for free where the reverse isn't true
   --> /mnt/host/source/src/platform/crosvm/sys_util/src/signal.rs:174:1
    |
174 | impl Into<c_int> for Signal {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: consider to implement `From` instead
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#from_over_into

I would replace impl Into<c_int> for Signal with impl From<Signal> for c_int

I haven't looked at the code that generates the warnings, so I don't know the difficulty involved, but a better message might look something like:

    = help: consider to implement `From<Signal>` instead
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#from_over_into

[Kixunil]

Yeah, maybe even:

  = help: consider to implement `From<Signal> for c_int` instead

[flip1995]

Thanks! This is indeed in another lint. I opened a new issue about this: #7088