Lint against using generic conversion traits when concrete methods are available

[sfackler]

The standard library's backwards compatibility rules allow the addition of new trait implementations even though this can cause previously working code to hit type inference failures in some contexts. Generic conversion traits (From, AsRef, Borrow, etc) frequently come up in regression reports since they're commonly implemented multiple times for a given type.

Because of this, the standard library commonly provides concrete methods with identical functionality which can be used to avoid these kinds of issues. For example, the String::into_bytes method has an equivalent signature to the Into::<Vec<u8>>::into method on String.

For example, the code in #36352 passed the result of the Borrow::borrow method into the From::from method, which failed to type infer after a second From implementation was added to String. This could have been avoided if the concrete .as_bytes() method was used instead of .borrow().

We should consider adding a lint, probably backed by annotations on the impls, which warn against calling the generic conversion trait methods in concrete contexts when a concrete method is available.

cc @rust-lang/libs

[brson]

Makes sense to me.

[cramertj]

This sounds like a good idea, but I'm not sure what an implementation of this would look like. is the idea just to annotate things like String::into_bytes with #[concrete_typed(Into::<Vec<u8>>::into)]? That seems rather clumsy and will require a lot of annotations. Why not suggest adding concrete types where inference could fail in the future? For example, code like

pub fn new<S: AsRef<str>>(somedata: S) -> String {
    String::from(somedata.as_ref())
}

could be changed to

pub fn new<S: AsRef<str>>(somedata: S) -> String {
    String::from(AsRef::<str>::as_ref(&somedata))
}

This requires no annotations, but I believe it provides the same functionality. One drawback of this approach is that it guides users towards uglier (more verbose) code, but I think this is a worthwhile trade-off, especially considering that veteran users will probably still reach for .as_bytes() or similar after seeing this warning.

Edit: Changed the second code block slightly to make it actually compile.

[bluss]

@cramertj That code is unproblematic and does not need further annotations -- the explicit trait bound AsStr<str> is enough.

The issue is code more like this: fn foo(somedata: &String) { println!("{}", somedata.as_ref()); } where the .as_ref() may or may not resolve uniquely.

[cramertj]

@bluss The code I used in the example above was reported as causing type annotations required: cannot resolve std::string::String: std::convert::From<&_> in #36352. Is that not the issue we're discussing?

Either way, a similar recommendation could be made for the example you provided. fn foo(somedata: &String) { println!("{}", somedata.as_ref()); } could be replaced with fn foo(somedata: &String) { println!("{}", AsRef::<str>::as_ref(somedata)); }

[Mark-Simulacrum]

This is not E-easy.

[steveklabnik]

New lints require an RFC these days; additionally, there's been no comments in two years, so there doesn't seem to be a ton of interest at this time. Closing!