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

Add lint for "deprecated" attribute on structs and enums #365

Merged
merged 9 commits into from
Feb 12, 2023

Conversation

Daaiid
Copy link
Contributor

@Daaiid Daaiid commented Feb 10, 2023

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.

This was mostly done copying and adjusting code from the "must_use" attribute lint.
Copy link
Owner

@obi1kenobi obi1kenobi left a 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 {
Copy link
Owner

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

@Daaiid
Copy link
Contributor Author

Daaiid commented Feb 11, 2023

Thank you very much for the warm welcome. Glad to help!

I took your suggestion and implemented it more generally with ImplOwner. Renamed everything to better reflect what the query does and added the tests, even for unions for future-proofing.

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

Copy link
Owner

@obi1kenobi obi1kenobi left a 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!

Comment on lines 2 to 4
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].",
Copy link
Owner

@obi1kenobi obi1kenobi Feb 12, 2023

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.

Suggested change
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].",

Copy link
Owner

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.",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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
Copy link
Owner

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.

@obi1kenobi
Copy link
Owner

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.

@Daaiid
Copy link
Contributor Author

Daaiid commented Feb 12, 2023

I love that you were able to suggest good solutions to the things that irked me the most. I'll gladly implement those suggestions.

@obi1kenobi obi1kenobi enabled auto-merge (squash) February 12, 2023 14:31
@obi1kenobi
Copy link
Owner

obi1kenobi commented Feb 12, 2023

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.

@obi1kenobi obi1kenobi merged commit 4472828 into obi1kenobi:main Feb 12, 2023
@Daaiid
Copy link
Contributor Author

Daaiid commented Feb 12, 2023

Awesome! Thanks for your guidance and I'll gladly help more in the future.

@obi1kenobi obi1kenobi changed the title Add lint for "deprecated" attribute on structs Add lint for "deprecated" attribute on structs and enums Feb 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants