obi1kenobi / cargo-semver-checks

Scan your Rust crate for semver violations.
Apache License 2.0
1.14k stars 72 forks source link

New lints: Breaking changes in non-sealed trait items #870

Open obi1kenobi opened 1 month ago

obi1kenobi commented 1 month ago

We can now query whether lints are sealed or not: https://github.com/obi1kenobi/trustfall-rustdoc-adapter/pull/343

{
    Crate {
        item {
            ... on Trait {
                name @output
                sealed @output
            }
        }
    }
}

Many changes in non-sealed traits are breaking downstream, since any downstream implementations of that trait will need to be adjusted to match the trait item changes.

Querying the sealed status of traits was a prerequisite for a variety of SemVer major lints, which can now be implemented:

For guidance on how to write these lints, look at existing lints that check trait-related breakage. For example, here's the lint for "a non-sealed trait's method stopped being unsafe, so implementations must remove the unsafe keyword in their declarations": https://github.com/obi1kenobi/cargo-semver-checks/blob/661fcedaf8b1700959a61306433807f08edf457f/src/lints/trait_method_unsafe_removed.ron

Also make sure to check out our contributing guide, and take a look at what prior lints' merged PRs looked like so you know what to expect. It's easier than you think!

dmatos2012 commented 1 month ago

Hi! I have checked the linked PRs and followed along and I would like start working on the item:

Thanks for the extremely well written out explanation, it really helps for newer contributors to get context of why/hows.

mrnossiom commented 1 month ago

I would like to claim the following to start off

Is there a written convention on lints name? non_sealed_trait_added_assoc_const_without_default seems right to me yet quite lengthy.

obi1kenobi commented 1 month ago

Let's go with simpler names like trait_associated_const_added, and mention the sealed-ness of the trait in the description field and in the error message as needed. We wouldn't lint this on sealed traits, or if there were a default value, so I don't see a reason to disambiguate from those cases.

There's no written convention, we're just aiming for consistency and relative brevity for now. Eventually I'd like to do a pass where we improve all the lint names, but that's not that high-priority right now.

dmatos2012 commented 1 month ago

From the PR it looks like the has_body: Boolean! property on Method was merged recently, so if I am not mistaken that means we can also add the lints that need it like:

If thats the case, Id like to claim this one :)

obi1kenobi commented 4 weeks ago

Updated the list, go for it!

PedroTurik commented 4 weeks ago

I can claim this one