Warn when a user-defined trait is used on an std library type without full path

[gnzlbg]

I'd like rustc to warn me when I use a trait method on a std (or liballoc, core, ...) type without using the full trait path.

That is, if I implement my own trait for a std library type:

trait Foo { fn bar(&self); }

impl Foo for Vec<u32> {
  fn bar(&self) { }
}

fn main() {
    let v = Vec::new();
    v.bar(); // Warning
    Foo::bar(&v); // OK
}

The motivation is that:

• if the std library adds a new trait with a method called foo and implements it for Vec<u32> my code stops compiling due to an ambiguity error
• if the std library adds an inherent method to Vec<u32> called foothat has the same signature as Foo::foo, my code continues to compile but might silently change behavior

---

I don't know whether this warning should apply exclusively to the std library or to types defined in external crates as well.

[oli-obk]

That seems like it will produce a lot of lints on a lot of projects. Maybe it should start out as an allow-by-default clippy lint?

[gnzlbg]

That seems like it will produce a lot of lints on a lot of projects. Maybe it should start out as an allow-by-default clippy lint?

The std library reserves the right to add new methods to types, new trait implementations for existing types, new trait methods to existing traits, new blanket impls, etc. and has exercised this right many times in the past. It does not consider doing any of this a breaking change.

That is, users that do any of the above are breaking one of the std library backward-comnpatibility pre-conditions, and their code is allowed to break with a Rust upgrade. Therefore, I think that rustc should warn about it for the std library.

However, this contract is crate specific, and there are many crates that guarantee that minor semver bumps won't break it - the std library just isn't one of them, but the num crate guarantees this to some extent IIRC. If clippy wants to offer a lint for this, it must be a lint that crates can enable for their APIs, such that when an user wants to check this in their crates, the lint is only enabled for those third-party crates that do not guarantee this.

[sfackler]

num treats the addition of a method as a breaking change? Really?

[gnzlbg]

@sfackler IIRC there were a bunch of PRs "ready" waiting for a long time because they added new trait methods (with default impls) that were only merged just before the 0.2.

[est31]

+1 to adding this to the compiler and making it warn by default in the new epoch.

This particular pattern is a major cause for regressions between Rust compiler versions (see e.g. #43239, #43657, #43665, #41793). If we added the lint to the compiler, we could make it warn-by-default in the 2018 epoch. This would be a breaking change but make make Rust more stable in the long run.

[est31]

There are various alternatives to the lint addition:

• allow overloading and overriding of implementations... IMO a very bad idea
• considering all additions of methods as breaking changes, like num.
• keep it as it is, maintaining the state of constant breakage between versions. The likelihood that your particular project breaks on a new rust release increases with the number of crates you are depending on directly and transitively. And usually, Rust projects have large dependency graphs.

While PR #48552 was an improvement, it didn't solve the actual issue itself.

[gnzlbg]

considering all additions of methods as breaking changes, like num.

The semver RFC defines these as Schrödinger changes:

For example, changes that may require occasional type annotations or use of UFCS to disambiguate are not automatically "major" changes. (But in such cases, one must evaluate how widespread these "minor" changes are).

which might or might not be major changes depending on how many people complain. For users, these changes can break their code, so they are major changes in practice.

Not having a clear policy on this topic, and not having a way to warn users about code that might be breaking the std library pre-conditions, is actually causing some parts of std to stagnate. For example, there is an aversion to moving methods from Itertools to Iterator, because of the amount of code that would break. Then we had what happened with clamp...

[scottmcm]

without using the full trait path.

Needing to use UFCS for everything sounds miserable -- no autoref, no chains, etc -- so if this happens I think it needs to be coupled with the ability to do something like v.Foo::bar().

Also, I think it's a substantial enough idiom change that it should take an RFC.

[gnzlbg]

@scottmcm

Needing to use UFCS for everything sounds miserable

One only needs it for trait method calls of non-std library traits when implemented on std library types. That is not "everything".

Also, I think it's a substantial enough idiom change that it should take an RFC.

First, it is just a warning in this edition, so I don't think we need an RFC for that. Second, for turning this into an error in the next edition we might not need an RFC since we already have a merged one since 1.0 (The semver for the std-library RFC, which @aturon wrote).

@aturon 's RFC says:

For example, changes that may require occasional type annotations or use of UFCS to disambiguate are not automatically "major" changes.

But I cannot find information about this in the Rust reference. How would one write that in the reference at all?

The most straight-forward way I can think of is to just state that "if you do this, the behavior is unspecified".

Where some Rust toolchains might specify it in such a way that your code compiles and your trait method gets called, and other toolchains specify it in a different way, e.g., such that your code either does not compile, or a different function is called instead, or... that your code compiles, your trait method gets called, but you get a warning about it, which is what I am suggesting here.

So from this point-of-view, this is just requesting a warning in this edition to warn about the usage of unspecified behavior. Until it becomes more clear what to do with respect to it here, it is a bit moot to think about what we will do in the next epoch.

[Ixrec]

API Guidelines issue on this same topic: rust-lang/api-guidelines#138