cargo-semver-checks icon indicating copy to clipboard operation
cargo-semver-checks copied to clipboard

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

Open obi1kenobi opened this issue 10 months ago • 5 comments

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.

obi1kenobi avatar Mar 30 '24 18:03 obi1kenobi