Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
gnzlbg opened this issue May 7, 2018 · 10 comments
Open
Labels
A-trait-system Area: Trait system C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@gnzlbg
Copy link
Contributor

gnzlbg commented May 7, 2018

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
Copy link
Contributor

oli-obk commented May 7, 2018

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
Copy link
Contributor Author

gnzlbg commented May 7, 2018

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
Copy link
Member

sfackler commented May 7, 2018

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

@gnzlbg
Copy link
Contributor Author

gnzlbg commented May 7, 2018

@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
Copy link
Member

est31 commented May 7, 2018

+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
Copy link
Member

est31 commented May 7, 2018

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
Copy link
Contributor Author

gnzlbg commented May 7, 2018

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
Copy link
Member

scottmcm commented May 9, 2018

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
Copy link
Contributor Author

gnzlbg commented May 9, 2018

@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
Copy link
Contributor

Ixrec commented Jun 25, 2018

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

@XAMPPRocky XAMPPRocky added C-enhancement Category: An issue proposing an enhancement or a PR with one. A-trait-system Area: Trait system T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-trait-system Area: Trait system C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants