Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
8 tasks done
obi1kenobi opened this issue Aug 18, 2024 · 6 comments
Closed
8 tasks done

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

obi1kenobi opened this issue Aug 18, 2024 · 6 comments
Labels
A-lint Area: new or existing lint E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: Mentorship is available for this issue.

Comments

@obi1kenobi
Copy link
Owner

obi1kenobi commented Aug 18, 2024

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:

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!

@obi1kenobi obi1kenobi added A-lint Area: new or existing lint E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: Mentorship is available for this issue. labels Aug 18, 2024
@dmatos2012
Copy link
Contributor

dmatos2012 commented Aug 19, 2024

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.

@mrnossiom
Copy link
Contributor

mrnossiom commented Aug 19, 2024

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.

@obi1kenobi
Copy link
Owner Author

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
Copy link
Contributor

dmatos2012 commented Aug 21, 2024

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 :)

@obi1kenobi
Copy link
Owner Author

Updated the list, go for it!

@PedroTurik
Copy link
Contributor

PedroTurik commented Aug 22, 2024

I can claim this one

  • non-sealed trait added associated type without default value

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: new or existing lint E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: Mentorship is available for this issue.
Projects
None yet
Development

No branches or pull requests

4 participants