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

[arc_with_non_send_sync] Improve suggested resolution #11772

Merged
merged 1 commit into from Nov 8, 2023
Merged

[arc_with_non_send_sync] Improve suggested resolution #11772

merged 1 commit into from Nov 8, 2023

Conversation

m-rph
Copy link
Contributor

@m-rph m-rph commented Nov 7, 2023

Fixes: #11714

Improved the lint message for [arc_with_non_send_sync] to suggest using RC unless user needs an Arc, then suggests wrapping in a mutex, and then suggests implementing Sync and Send.


changelog: [arc_with_non_send_sync]: Suggest RC over unsafe impl of Send and Sync

@rustbot
Copy link
Collaborator

rustbot commented Nov 7, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @blyxyas (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Nov 7, 2023
Copy link
Member

@blyxyas blyxyas left a comment

Choose a reason for hiding this comment

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

This is pretty much ready, just a little improvement that I think would improve readability.

clippy_lints/src/arc_with_non_send_sync.rs Outdated Show resolved Hide resolved
Copy link
Member

@blyxyas blyxyas left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! ❤️ Could you squash the 4 commits into a single one?

@m-rph
Copy link
Contributor Author

m-rph commented Nov 8, 2023

Great! :D

I don't have write access so I can't squash-merge, and squash - (force) push would need another approval.

If that's fine I will get to it!

Fixes: #11714
changelog: [`arc_with_non_send_sync`]: Suggest RC over unsafe impl

Co-authored-by: Alejandra González <blyxyas@gmail.com>
@blyxyas
Copy link
Member

blyxyas commented Nov 8, 2023

Yeah don't worry, use git rebase -i HEAD~4 (replace "pick" with "s" in those four commits), adjust the commit messages and just git push -f. Rebasing is the scenario where a force push would be accepted.

@m-rph
Copy link
Contributor Author

m-rph commented Nov 8, 2023

Awesome, thanks Alejandra!

@blyxyas
Copy link
Member

blyxyas commented Nov 8, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented Nov 8, 2023

📌 Commit 399fe32 has been approved by blyxyas

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Nov 8, 2023

⌛ Testing commit 399fe32 with merge e16d42f...

@bors
Copy link
Collaborator

bors commented Nov 8, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: blyxyas
Pushing e16d42f to master...

@bors bors merged commit e16d42f into rust-lang:master Nov 8, 2023
5 checks passed
@m-rph m-rph deleted the 11714 branch November 8, 2023 22:45
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.

arc_with_non_send_sync can mislead users into unsafe impls
4 participants