-
-
Notifications
You must be signed in to change notification settings - Fork 62
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
Check if trait items with defaults may need special handling to avoid false-positives #727
Comments
Hi! I started to look at
Isn't this still some kind of semver violation? For example, given a use lib::Cat;
fn my_func() {
let cat = Cat;
cat.make_noise();
} If we move Should lints like |
This is an excellent example of why building a tool like You ask a great question, and your intuition is correct π
Yes, for two reasons. Imagine a hypothetical There's a case where this is in practice not actually breaking: if the trait in question is part of a prelude β either a built-in one or a crate's own prelude like e.g.
|
This issue about trait associated consts and functions acting as a backstop for removed inherent items got me thinking, what about Deref? Is the following breaking if the pub struct Foo {
pub field: (), // REMOVE
inner: Inner,
}
impl Foo {
pub fn method(&self) {} // REMOVE
pub fn new() -> Self {
Self {
field: (), // REMOVE
inner: Inner { field: () },
}
}
}
pub struct Inner {
pub field: (),
}
impl Inner {
pub fn method(&self) {}
}
impl core::ops::Deref for Foo {
type Target = Inner;
fn deref(&self) -> &Self::Target {
&self.inner
}
}
impl core::ops::DerefMut for Foo {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.inner
}
}
fn test() {
let mut foo = Foo::new();
foo.field = ();
foo.method();
} |
It's a great question, and not something I've considered before so I gave it some thought. Tiny answer: It's complicated. It's breaking-ish, and will require careful UX work in Short answerIdeally, we want multiple lints here. One is "on by default, error level" and it allows Long answerWe have to zoom out a bit for context. We want our "on by default, error level" lints to be fairly bulletproof. In an ideal case, they fire exactly when needed and only then: no false-positives, no false-negatives. This is often extremely hard to do! When we must err on one side or the other, we choose to be conservative -- to avoid false-positives at the expense of false-negatives. In other words, better to not raise a lint when we should have, than to raise a lint when we shouldn't have.
An "on by default, error level" lint should in principle have enough information to be able to produce a "witness" -- a concrete piece of Rust code that is affected by the problem it's pointing out. By "is affected" I mean "machine-checkably prove the problem exists." For example, by having its example code trigger a compile error, UB, linker error, or use of a specific non-public API item. This is how we tested the top 1000 Rust crates in our study last year -- we generated witness programs for every instance of breakage we reported! Our (still conceptual, until #58 is done) warning lints have a lower bar. Your excellent |
Opened #766 for this. Thanks again for the awesome example -- we can continue the discussion in that dedicated issue. |
Originally posted by @obi1kenobi in #714 (comment)
It's plausible that other lints that look for inherent items, such as
inherent_method_missing
, may also have the same subtle logic error that fails to prevent a false positive in a "move the inherent item to a defaulted item on an impl'd trait" scenario.More test cases are needed, and I expect multiple lints will need updates.
The text was updated successfully, but these errors were encountered: