-
-
Notifications
You must be signed in to change notification settings - Fork 78
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
Add lint for "deprecated" attribute on structs and enums #365
Conversation
This was mostly done copying and adjusting code from the "must_use" attribute lint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks awesome — excellently done, thank you so much!
I'm happy to merge this as-is, but I wanted to bring up an interesting option: with a few tiny changes, you can make the same query work not just for structs but also for enums and unions as well.
If you'd like to do that, feel free to take as long as you need and ask any questions that might come up.
If you're happy with this PR as-is, I'm happy to merge it right now too, just let me know if that's your preference.
Thanks again, and welcome to the project!
CrateDiff { | ||
current { | ||
item { | ||
... on Struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As is, this query is perfect and does what it sets out completely correctly.
There's an opportunity to make this same query apply not just for structs, but also for enums and (in the future, when we expose them in the query API) unions as well. To do that, instead of only selecting structs here, you could use ImplOwner
, which is a common supertype of struct/enum/union
, as the items that are allowed to have an inherent impl
block.
Then we'd just need to make sure that the "current" and the "baseline" arms of the query are also talking about the same kind of item in addition to having the same importable_path
value.
You can see an example here: https://github.com/obi1kenobi/cargo-semver-checks/blob/main/src/lints/inherent_method_must_use_added.ron#L15-L18
Now the same query also checks enums and unions.
Unions are not supported yet by the query engine, so the output file doesn't change.
Thank you very much for the warm welcome. Glad to help! I took your suggestion and implemented it more generally with If these changes are to your liking, you can merge. I'm looking forward to add the lints for the other items in the issue (functions, fields etc.). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good! The new query will work perfectly and the tests are exceptionally thorough. Well done!
I made a couple of tiny suggestions on the naming of the new lint, and the organization of the test code. Should take just a couple of minutes to apply, if you agree with them.
I'm excited to merge this very soon!
id: "enum_struct_union_deprecated_added", | ||
human_readable_name: "enum/struct/union #[deprecated] added", | ||
description: "Either an enum, a struct or a union has been newly marked with #[deprecated].", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enums, structs, and unions are all kinds of types, so let's simplify the terminology.
id: "enum_struct_union_deprecated_added", | |
human_readable_name: "enum/struct/union #[deprecated] added", | |
description: "Either an enum, a struct or a union has been newly marked with #[deprecated].", | |
id: "type_marked_deprecated", | |
human_readable_name: "#[deprecated] added on type", | |
description: "A type has been newly marked with #[deprecated].", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you agree with the suggested rename, please remember to change the name of the file, update src/query.rs
with the new name, and also change the name of the test crates and test outputs.
"deprecated": "deprecated", | ||
"zero": 0, | ||
}, | ||
error_message: "Either an enum, a struct or a union is now #[deprecated]. Downstream crates will get a compiler warning.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error_message: "Either an enum, a struct or a union is now #[deprecated]. Downstream crates will get a compiler warning.", | |
error_message: "A type is now #[deprecated]. Downstream crates will get a compiler warning when using this type.", |
@@ -0,0 +1,179 @@ | |||
// STRUCTS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely love the thorough test cases here!
For easier readability and maintainability, consider splitting up the contents here across three files: structs.rs, enums.rs, unions.rs
. Then lib.rs
can just do:
pub mod structs;
pub mod enums;
pub mod unions;
And everything should work correctly.
This will make the spans a bit easier to work with, since we won't have to scroll deep into the file to find the reported line, and it's less likely that a test fails just because an item moved up or down a line and changed the reported span.
Also, feel free to get started on the other new lints without waiting for this to merge! You clearly have gotten the hang of this, and if you're still interested, I'm confident you can implement those new lints with no trouble at all. |
I love that you were able to suggest good solutions to the things that irked me the most. I'll gladly implement those suggestions. |
Nicely done! I added two tiny formatting fixes, and have configured the branch to be merged as soon as the CI run completes. This new lint will be part of the next release, tentatively planned for sometime tomorrow. Thanks for contributing, and welcome to the project! If you're interested, you're welcome to add more lints, and I'm also happy to suggest other good starting issues as well. |
Awesome! Thanks for your guidance and I'll gladly help more in the future. |
I'm new to Rust and Open Source, so please review carefully and give feedback liberally.
Implements a small part of issue #57.
This was mostly done copying and adjusting code from the "must_use" attribute lint.
Also I would like to mention, that there is a lot of code duplication between the attribute queries. E.g. the query for must_use attributes on functions looks almost identical to the one on structs. I know applying DRY too early is bad too, but I wanted to raise this concern anyway.