obi1kenobi/cargo-semver-checks

New lints: Breaking changes in non-sealed trait items

obi1kenobi opened this issue ยท 6 comments

We can now query whether lints are sealed or not: obi1kenobi/trustfall-rustdoc-adapter#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:

  • non-sealed trait added associated constant without default value: #875
  • non-sealed trait added associated type without default value: #890
  • non-sealed trait removed the default value for an associated constant: #888
  • non-sealed trait removed the default value for an associated type: #889
  • trait newly became sealed, breaking any external implementors: #876
  • non-sealed trait added new supertraits (also tracked by #441): #892
  • non-sealed trait added method without default impl: #891
  • non-sealed trait removed a default impl for a trait method (also tracked by #294, claimed by @dmatos2012): #880

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!

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

  • trait newly became sealed (breaking any external implementors)

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

I would like to claim the following to start off

  • non-sealed trait added associated constant without default value

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

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.

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:

  • non-sealed trait removed a default impl for a trait method

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

Updated the list, go for it!

I can claim this one

  • non-sealed trait added associated type without default value