Check if trait items with defaults may need special handling to avoid false-positives

Oooh interesting, this is a false-positive — we wanted this to not get flagged.

Perhaps because the N constant isn't defined in the impl block and is instead a default item on the trait? 🤔

Add a new test case, similar to the trait+struct one we have already, but where the const doesn't have a default value and is instead given a value in the impl block for the trait.

Add comments on both the old and new test cases, to explain what they test, how they are different, and why it's important to have both of them and not just one or the other.

Depending on how this new test case interacts with the query (whether it gets flagged or not), we'd continue tweaking the query -- for example, by checking two separate cases explicitly: "inherent impl must not have the constant" and "impl of a trait, where that trait must not have a const by that name either."

_Originally posted by @obi1kenobi in https://github.com/obi1kenobi/cargo-semver-checks/pull/714#discussion_r1545470986_

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.

Hi! I started to look at inherent_method_missing.

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.

Isn't this still some kind of semver violation? For example, given a struct Cat that impls trait MakeNoise and method make_noise, consider this code:

use lib::Cat;

fn my_func() {
   let cat = Cat;
   cat.make_noise();
}

If we move Cat::make_noise to a (default) impl in MakeNoise::make_noise, this code will error on the client because they have to import the trait MakeNoise.

Should lints like inherent_xyz_missing be okay with this and it be caught in another lint?

This is an excellent example of why building a tool like cargo-semver-checks is hard — there are so many edge cases to think through!

You ask a great question, and your intuition is correct 👇

Should lints like inherent_xyz_missing be okay with this and it be caught in another lint?

Yes, for two reasons.

Imagine a hypothetical help: note for the lint, just like the ones rustc gives when it knows how to fix the problem in the code. The advice in that note is different between the case where the item is completely gone ("either don't use it or add it back") versus where it's disappeared to a trait item ("import this trait"). This suggests we want a separate lint, so we can give separate advice. But even that separate lint is trickier to write than it seems! 👇

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. pyo3's:

For example, if pyo3 moves an inherent method to a trait in its prelude, we'd like to report that case as a hazard not necessarily as a semver-major change. The former tells maintainers "there's a bit of risk here so at least call this out in your release notes" whereas the latter is overstating our case a bit and could be perceived as crying wolf.
It's of course possible (if a bit rare) that an inherent method may shadow an identical trait method in the built-in Rust prelude. This is a bit subtle! You may ask, why might a maintainer add an inherent method instead of using the built-in one? The answer is that the Rust standard library is actively developed too, and it's possible that the inherent method was added before std and the Rust prelude gained their version. E.g. the itertools crate has many candidates that are being considered for inclusion into the Iterator trait which is in the prelude. If a maintainer added an inherent impl for a useful pub helper method for their own iterator type, and later the Iterator trait gained an identical helper, removing that pub helper method is not breaking in practice because the Iterator trait is imported by the prelude.

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 // REMOVE lines are removed?

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 cargo-semver-checks.

Short answer

Ideally, we want multiple lints here. One is "on by default, error level" and it allows Deref/DerefMut as a backstop. Another is "on by default, warning level," does not allow Deref/DerefMut as backstops, and never fires if the error-level lint has fired already. This is because the first lint catches code that is definitely wrong, whereas the second lint catches code that is probably wrong. This is analogous to clippy::correctness vs clippy::suspicious.

Long answer

We 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.

Side note: In the case of #763, we actually strengthened the lint -- we made it lint more often, not less. The previous implementation was overly conservative. It had observed that it would be a false-positive if we flagged an inherent next() method backstopped by an implemented Iterator::next() on the same type, so it discarded all cases where a matching method existed on a trait. #763 made it a bit less conservative: now the impl where that method lives must not be #[doc(hidden)].
- The lint's analysis is still overly conservative. For example, it would consider a method that takes a different number of arguments a valid backstop, even though it clearly isn't. Room for future improvements!

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 Deref example, for example, continues to compile after all the removals! So we can't produce a witness, but this code change is still suspicious and worth a look by the maintainer. Hence, clippy::suspicious -- equivalent.

Opened #766 for this. Thanks again for the awesome example -- we can continue the discussion in that dedicated issue.

obi1kenobi / cargo-semver-checks

Check if trait items with defaults may need special handling to avoid false-positives #727

Short answer

Long answer