New lint: trait removed supertrait #470
Conversation
There was a problem hiding this comment.
This looks really good so far. The lint query is correct and the new test crate is nicely set up. We're 90% of the way there and I'm excited to merge this very soon!
Two remaining things to consider: user ergonomics when this lint reports a problem, and expanded test coverage to prevent future debugging.
Users of cargo-semver-checks might not know that removing a supertrait is a breaking change. Let's channel the helpfulness of rustc and tailor the error message and description in the lint to make them maximally helpful. This is one of the major challenges of writing software aimed at a very large audience like "everyone writing Rust" 😅 But that also makes it really rewarding to get it right!
Consider expanding the test suite a bit with more true-positive cases as well as guard test cases against possible false-positives.
- Example true-positive cases: removing one of multiple supertraits, removing supertrait when the subtrait is generic, or removing a generic supertrait.
- Example false-positive guard cases: removing a lifetime trait bound (
pub trait Foo<'a> : Bar + 'athen lose the'a), changing the generic trait bound in a breaking way (to a different type, to a different lifetime, etc.) or in a non-breaking way (e.g. fromi64toInto<i64>).
The guard cases are as important as the true-positive test cases, since false-positives are very harmful: they make users have a bad experience and make them not trust the tool, and they cause support load for us when issues get opened and we have to triage and fix them. An ounce of prevention is worth a pound of cure :)
To come up with good guard cases, consider:
- similar code changes that are non-breaking, and shouldn't be caught
- similar changes that are breaking but should be caught by another lint — we have a policy that any breakage should be reported by one lint and one lint only, for the sake of user ergonomics.
Ping me with any questions! I'm super excited by all the awesome work you've been doing, and I'm very happy to support you along the way! 🚀
Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
| human_readable_name: "supertrait removed or renamed", | ||
| description: "It is a breaking change to loosen generic bounds on a type since this can break users expecting the tightened bounds.", | ||
| required_update: Major, | ||
| reference_link: None, |
There was a problem hiding this comment.
Any chance we can find something to cite here?
There was a problem hiding this comment.
I should have asked about this earlier, on the semver reference does not seem to be an entry for this lint.
There is this that is somewhat similar but it's definitely not the exact case nor does the explanation help (IMHO).
The documentation about supertraits helps to understand what is breaking indirectly.
Is it worth adding any of those links?
There was a problem hiding this comment.
Yes, the docs link for supertraits is the best option. It's not perfect, but it's certainly better than nothing.
There was a problem hiding this comment.
Thank you, I will add it :)
| "public": "public", | ||
| "zero": 0, | ||
| }, | ||
| error_message: "A supertrait was removed from a trait.", |
There was a problem hiding this comment.
Perhaps tweak this to include a hint of why this might be a problem. Feel free to use some of the language from the description field.
Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
| human_readable_name: "supertrait removed or renamed", | ||
| description: "It is a breaking change to loosen generic bounds on a type since this can break users expecting the tightened bounds.", | ||
| required_update: Major, | ||
| reference_link: None, |
There was a problem hiding this comment.
Yes, the docs link for supertraits is the best option. It's not perfect, but it's certainly better than nothing.
Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
|
Thank you again for all the mentoring during the PR, I'm super grateful for all the time you invested in it and all the comments you add. Super thank you! |
|
No worries, I'm happy I could help. Thank you for all the hard work you've been putting in to make Now that you've seen a few different sides of How can I best support you and make sure you're set up for success? |
|
No preferences :), I liked the process of both. I will review the list you gave me and pick something interesting from there. And at the moment I don't need anything specific, I think now I have a better macro picture of the whole components. Thank you again for all the reviews! |
Resolves #232
Besides the new lint, this PR also includes a newer version of trustfall-rustdoc-adapter because we need the
supertraitfield.