feat: add trait_added_supertrait

[mrnossiom]

Lint for "non-sealed trait added new supertraits"

See #870
Closes #441

I blessed the tests, but it makes duplicate reports for trait_newly_sealed (as sealing a trait requires adding a supertrait) which may be annoying.

[obi1kenobi]

This looks good, and don't worry about the matches in other crates. That's very common — we even actively encourage it in cases where it helps us ensure that precisely one lint fires per discovered problem.

For example, if a pub struct gets deleted, we don't also want to complain that each of its public fields and methods got deleted too. The test for "what triggers the struct fields deleted" lint then contains instances where the entire struct is deleted, and we use the tests to ensure we don't match it.

Assuming you feel the reference link I suggested is good, or you can find a better one, this is good to merge!

[obi1kenobi]

Sorry, I just realized something else as well. In #5 we listed a real world instance of this breakage where a trait gained Debug as a supertrait (the built-in one): https://github.com/smithy-lang/smithy-rs/pull/2577/files#diff-82252c5a4ad7e9d34be9254db1afe172d0f01c067b121de20ff7f7f29d7cf223L24

Would you mind adding a couple of test cases that add supertraits like Debug, Sync, and PartialEq to make sure those get detected correctly too?

[mrnossiom]

Added tests with std supertraits, but the current query doesn't catch them. Do you have any idea why?

[suaviloquence]

Another test case to add, maybe:

given:

pub mod one {
    /// Trait #1
    pub trait MyTrait {}
}

pub mod two {
    /// trait #2
    pub trait MyTrait {}
}

old:

use one::MyTrait;

pub trait Subtrait: MyTrait {}

new:

use two::MyTrait;

pub trait Subtrait: MyTrait {}

where one::MyTrait and two::MyTrait are different traits, even though they have the same name.

[suaviloquence]

Added tests with std supertraits, but the current query doesn't catch them. Do you have any idea why?

To me, it looks like a bug in the trustfall rustdoc adapter: it doesn't seem to show supertraits from other crates.

Querying the itertools crate for traits with supertraits, it only returns HomogenousTuple: itertools::tuple_impl::TupleCollect. However, this is certainly not all of them, take the Itertools trait which is : core::iter::Iterator, but it doesn't come up in the query.

[obi1kenobi]

Yes, in general we can't see traits from other crates. Recall that rustdoc JSON only contains data for the crate being analyzed, so other crates' items (including traits) are not present in the JSON. This unfortunately includes Rust's built-in crates std and core.

To work around this limitation until we have the ability to connect multiple rustdoc JSONs, we have a hardcoded list of traits from Rust's builtins that we pretend are part of the JSON and should be able to lint.

The code to pretend those traits exist when they are implemented by a type is here. Unfortunately, it appears that the implementation of traits' supertrait edge has neglected to add an equivalent block. This is a bug.

Thanks for catching it! It should be an easy fix, if you'd like to try your hand at writing some Trustfall adapter code @mrnossiom! We'd want to update this test and its corresponding test crate to contain some std bounds, and add a small block of code to look them up like in the other place.

[obi1kenobi]

@mrnossiom no pressure, just checking in to see if you're interested in putting together the fix for trustfall-rustdoc-adapter to unblock this PR, or if I or someone else should take over either just that component or this PR as a whole. It's no trouble either way, and it's totally fine if you're busy and would like to step away completely. I just wanted to get your preferences on how you'd like this PR to proceed, since you did a lot of work on it already.

[mrnossiom]

Hi Predrag, I'm afraid I don't currently have the time to make the fix myself. But I'll be happy to finish the work of this PR's scope once the fix is merged.

[obi1kenobi]

No worries at all, I can take care of it and unblock this PR.

[obi1kenobi]

The fix is in and I've updated the adapter library versions here, so if you pull the issue should go away.

While working on the fix, I thought of another interesting edge case we want to ensure isn't going to cause a false-positive:

pub trait Example: std::fmt::Debug {}
// becomes
pub trait Example: core::fmt::Debug {}

This shouldn't get flagged since the name change is essentially only cosmetic — we don't currently track std requirements, especially not within any given lint. But in principle, some possible implementations of the supertrait name property might have considered this a name change, and we want to protect against a bad implementation there causing us a false-positive here.

[obi1kenobi]

Just checking in to see if you're still able to and interested in working on this PR. It's no trouble at all if you had other stuff come up, and you can always contribute more on other things if/when you have the time and energy for it.

Heads-up that I'll be travelling quite a bit in October as well as for a part of the rest of September, so it would be a bit easier for me to review changes and support you before the end of the month as opposed to later on.

[mrnossiom]

The current lint doesn't flag the edge case you mentioned, but I feel like it should. If you have any idea why tell me else I'll look further into it this weekend.

[obi1kenobi]

The current lint doesn't flag the edge case you mentioned, but I feel like it should.

This message was referencing this edge case, right?

pub trait Example: std::fmt::Debug {}
// becomes
pub trait Example: core::fmt::Debug {}

If so, the correct behavior is to not flag this — we want the test case as a guard against false-positives here. The reason is that std::fmt::Debug is just a pub re-export of core:fmt::Debug, so those two bounds are equivalent from a supertraits perspective. The only difference is that one of them is usable in a no_std context and the other is not, but that's not the subject of this lint.

So I believe everything is in order with this lint and I think it's safe to merge. Do you agree? Happy to merge it if so, just wanted to confirm with you in case I've missed something 👍

[mrnossiom]

It's just that there is no special handling for std to core changes. This means that if std::fmt::Debug to core::fmt::Debug doesn't trigger, it also means that switching lib_foo::Trait to lib_bar::Trait would not trigger either.

I don't know if that's acceptable, else I have no other concern for this PR.

[obi1kenobi]

You're right, there's definitely an issue there, though not with std::fmt::Debug and core::fmt::Debug specifically (the std one is a re-export of the core one) but with lib_foo::Trait to lib_bar::Trait. This is recorded in issue #922.

However, because of #638, bounds like lib_foo::Trait currently won't show up at all, which massively diminishes the surface area of the problem. Technically #638 prevents us from resolving even common built-in Rust traits like Debug and Clone, but we've special-cased a number of them in particular to work around this temporarily.

There's another (quite remote) false-positive issue here, around blanket impls: adding a supertrait bound can be "non-binding" (essentially, irrelevant and non-breaking) if there's already a blanket impl for that supertrait over all types that could impl the trait. This is #923.

We will resolve all three of these eventually, and the name-matching (#922) is a much bigger concern than blanket impls (#923). But given that #638 prevents the vast majority of the cases where the name-matching would be a problem, I think we'll help vastly more people by shipping this lint as-is and improving it later.

Thanks for bringing this up — I appreciate it! You have an excellent eye for detail, and I really enjoy working with you as a result 🚀