cargo-semver-checks
cargo-semver-checks copied to clipboard
obi1kenobi
Fix false-positive in lint trait_method_unsafe_removed
If a trait sealed, it should be okay to make its methods safe, since it's impossible for an external type to implement it.
Nice catch!
It might be worth splitting this PR into two: one for adding unsafe and one for removing it. This is because we'll be able to merge the "unsafe added" one immediately, whereas the other one is blocked on checking whether the trait is sealed or not. This is something that's on the "to do" list but isn't implemented yet -- if you're interested in giving it a shot, I'd be happy to point you in the right direction!
I'll give it a try. If you give me an overview of the changes that you think will need to be made, I can take it from there.
On second thought, implementing sealed trait detection is quite hard, so I suggested to the rustdoc maintainers that they consider implementing it in rustdoc itself. We'll see what they say 🤞 You can find the details here: https://github.com/rust-lang/rust/issues/119280
If you're open to implementing other functionality, feel free to grab any E-mentor issue that looks interesting. For example, this one is a fairly easy one to get started with: https://github.com/obi1kenobi/cargo-semver-checks/issues/454
👋 Hi! Just wanted to double-check on this PR.
I came up with a clever way to eliminate the vast majority of the false-positives targeted here, without having to implement the complex check for whether traits are sealed or not. It's fairly simple: in the lint, check if the trait in question has any supertraits that have no importable paths.
The overwhelming majority of traits are sealed by inheriting a pub-in-priv supertrait, which have no importable paths. If the lint only reports traits that don't have any supertraits without importable paths, that will eliminate almost all false-positives. This is 0.1% the work of the full solution (trait sealing analysis) and produces 99.9% of the benefit.
Are you still interested in pursuing this? No rush, and no worries if you aren't able to at all — I don't mind taking it over.
No pressure if you're busy! I appreciate your help and I just wanted to make sure you still get first dibs on this if you're still interested in it :)
I don't have a lot of experience with Trustfall (or even GraphQL) yet, so if you think it could be written more idiomatically, I'd be interested in learning how.
I don't have a lot of experience with Trustfall (or even GraphQL) yet, so if you think it could be written more idiomatically, I'd be interested in learning how.
No, you did exactly the right thing with the query. At some point in the future we'll add syntax sugar (something like @nonexistent) to simplify writing the count = 0 clause, but right now this is the only available way to write this query.