-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Implement non_send_field_in_send_ty
lint
#7709
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @xFrednet (or someone else) soon. Please see the contribution instructions for more information. |
☔ The latest upstream changes (presumably #7715) made this pull request unmergeable. Please resolve the merge conflicts. |
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.
The implementation logic is already looking good. I find it awesome to see the security lints coming to Clippy. Thank you for the research you've done in this regard and the work you're putting in the implementation!
Most of my comments are in regard to the documentation and the lint message construction. :)
The push was to resolve the merge conflict. I'll start working on the feedbacks shortly. |
I've addressed the PR feedbacks. Could you take another look at the PR? I left a few comments above unresolved, so that it is easier for you to leave additional feedbacks. Please resolve them if you are satisfied :) |
Thank you, I'll have a look at it over the week 🙃 |
☔ The latest upstream changes (presumably #7673) made this pull request unmergeable. Please resolve the merge conflicts. |
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 version is looking good, I think the logic is ready. Your lint test cases are awesome, I found a test case for everything I was expecting. There are still some NITs when it comes to the lint messages and some formatting, but all of them should hopefully be simple to fix. Then it should be ready for merge 🙃.
You can also rebase right away to resolve the merge conflicts.
Finished addressing the comments :) |
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.
Thank you for the fast changes! Three more NITs that I've missed so far (The last reviews mainly focussed on the logic). Then I'm happy to merge this 🙃.
I've also used our lintcheck tool and everything looks fine. The lint had no false positives for our default list of crates.
Regarding the lint group, you set the group to nursery
as a precaution, is that correct? I think it would be good to keep it, until the next beta has been branched off (that should be in about 13 - 19 days). Then I would move the lint to the suspicious
group to have it warn-by-default and get some feedback from the nightly users. We currently don't have a process to automatically to do this. Therefore, we should create an issue to change the group, once this is merged. 🙃
I initially put The last commit should have addressed the review comments. I'll restart working on uninit_vec lint after this. |
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.
Everything looks good to me. Thank you for all the work you put into this lint! :)
@bors r+ FYI: I updated the lint description with the new lint name 🙃. And this comment should merge it 🎉 |
📌 Commit fb0353b has been approved by |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
changelog: Implement [
non_send_fields_in_send_ty
] lintFixes #7703