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

Implement non_send_field_in_send_ty lint #7709

Merged
merged 12 commits into from
Oct 3, 2021
Merged

Conversation

Qwaz
Copy link
Contributor

@Qwaz Qwaz commented Sep 23, 2021

changelog: Implement [non_send_fields_in_send_ty] lint

Fixes #7703

@rust-highfive
Copy link

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.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 23, 2021
@bors
Copy link
Collaborator

bors commented Sep 24, 2021

☔ The latest upstream changes (presumably #7715) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Member

@xFrednet xFrednet left a 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. :)

clippy_lints/src/non_send_field_in_send_ty.rs Outdated Show resolved Hide resolved
clippy_lints/src/non_send_field_in_send_ty.rs Outdated Show resolved Hide resolved
clippy_lints/src/non_send_field_in_send_ty.rs Outdated Show resolved Hide resolved
clippy_lints/src/non_send_field_in_send_ty.rs Outdated Show resolved Hide resolved
clippy_lints/src/non_send_field_in_send_ty.rs Outdated Show resolved Hide resolved
tests/ui/non_send_field_in_send_ty.stderr Outdated Show resolved Hide resolved
clippy_lints/src/non_send_field_in_send_ty.rs Outdated Show resolved Hide resolved
clippy_lints/src/non_send_field_in_send_ty.rs Outdated Show resolved Hide resolved
@xFrednet xFrednet added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Sep 25, 2021
@Qwaz
Copy link
Contributor Author

Qwaz commented Sep 27, 2021

The push was to resolve the merge conflict. I'll start working on the feedbacks shortly.

@Qwaz
Copy link
Contributor Author

Qwaz commented Sep 28, 2021

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

@xFrednet
Copy link
Member

Thank you, I'll have a look at it over the week 🙃

@xFrednet xFrednet added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Sep 29, 2021
@bors
Copy link
Collaborator

bors commented Sep 30, 2021

☔ The latest upstream changes (presumably #7673) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Member

@xFrednet xFrednet left a 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.

clippy_lints/src/non_send_field_in_send_ty.rs Outdated Show resolved Hide resolved
clippy_lints/src/non_send_field_in_send_ty.rs Outdated Show resolved Hide resolved
clippy_lints/src/non_send_field_in_send_ty.rs Outdated Show resolved Hide resolved
clippy_lints/src/non_send_field_in_send_ty.rs Outdated Show resolved Hide resolved
clippy_lints/src/non_send_field_in_send_ty.rs Outdated Show resolved Hide resolved
clippy_lints/src/non_send_field_in_send_ty.rs Outdated Show resolved Hide resolved
clippy_lints/src/utils/conf.rs Outdated Show resolved Hide resolved
clippy_lints/src/non_send_field_in_send_ty.rs Outdated Show resolved Hide resolved
@Qwaz
Copy link
Contributor Author

Qwaz commented Oct 1, 2021

Finished addressing the comments :)

Copy link
Member

@xFrednet xFrednet left a 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. 🙃

clippy_lints/src/non_send_field_in_send_ty.rs Outdated Show resolved Hide resolved
clippy_lints/src/non_send_field_in_send_ty.rs Outdated Show resolved Hide resolved
clippy_lints/src/utils/conf.rs Outdated Show resolved Hide resolved
@Qwaz
Copy link
Contributor Author

Qwaz commented Oct 2, 2021

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.

I initially put nursery category because I was a little intimidated by "most likely" part of the description "code that is most likely wrong or useless". However, your report about lintcheck result regained me some confidence :) I think it's a good idea to start collecting feedbacks from the nightly users.

The last commit should have addressed the review comments. I'll restart working on uninit_vec lint after this.

Copy link
Member

@xFrednet xFrednet left a 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! :)

@xFrednet
Copy link
Member

xFrednet commented Oct 3, 2021

@bors r+

FYI: I updated the lint description with the new lint name 🙃.

And this comment should merge it 🎉

@bors
Copy link
Collaborator

bors commented Oct 3, 2021

📌 Commit fb0353b has been approved by xFrednet

@bors
Copy link
Collaborator

bors commented Oct 3, 2021

⌛ Testing commit fb0353b with merge 33c34fb...

@bors
Copy link
Collaborator

bors commented Oct 3, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 33c34fb to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Lint: drop_non_send
5 participants