Add undocumented_unsafe_blocks lint by Serial-ATA · Pull Request #7748 · rust-lang/rust-clippy (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation65 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
changelog: Added a new lint [undocumented_unsafe_blocks]
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.
Welcome to the project, it's nice to see that you got a working implementation 🙃.
@flip1995 would you maybe like to review this, as you've also reviewed #7557? Otherwise, I'll have a look at it during the next week 🙃
Yes, sure. I have time this afternoon.
r? @flip1995
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I had to guess, I assume, that you didn't look at #7557 before implementing this?
I think your approach and the approach taken in #7557 combined would be ideal. I left comments about that below. IMO the work done in #7557 is really valuable. The PR was only closed, because it was inactive.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Just missing one feature from the original suggestion in #7464;
The lint would also raise a warning if that comment is copied verbatim.
...
The following code would still raise a warning (not because of what transmute is doing here, but because the safety comment still has "...", as suggested. The developer would need to update it with an actual sentence.);
// Safety: ... let totally_a_usize = unsafe { std::mem::transmute::<String, usize>(string) };
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Impl LGTM overall now. A few minor improvements could be made.
Add undocumented_unsafe_block_safety lint Add undocumented_unsafe_blocks lint
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR should be almost done. I have some suggestions on how to simplify the code a bit.
The tests look awesome! The only test that is missing is a macro test. Two important tests here would be
unsafeblock in the macro call:m!(unsafe{ ... }unsafeblock in the macro definition
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
Just for posterity; the behaviour that makes sure that "..." is also checked and errored on is removed, I think that's fine for now to get this PR through, but I'd still like to see it somewhere in the future
One more thing that came to mind: What if the unsafe block has multi-line code in it? How does the suggestion look in that case? I expect that the full unsafe block is shown. We may want to avoid this, by using the span_until_char function ( or similar functions)
Looks great now! Could you address the multi-line unsafe block case? After that this should be good to go. 🚀
Did I not take care of that? The no_comment_match and internal_comment tests only show the first line now.
Did I not take care of that? The
no_comment_matchandinternal_commenttests only show the first line now.
Oh I missed that. In that case all good 👍
(no_comment_match doesn't test that, because the unsafe block is still empty and all on one line in this test case, but the other test case does)
Please squash some commits, so that we can merge this.
@bors r+
Thanks for sticking around through all the review iterations. Great work!
📌 Commit 412b862 has been approved by flip1995
46 tasks
bors added a commit that referenced this pull request
Fix SAFETY comment tag casing in undocumented_unsafe_blocks
This changes the lint introduced in #7748 to suggest adding a SAFETY comment instead of a Safety comment.
Searching for // Safety: in rust-lang/rust yields 67 results while // SAFETY: yields 1072.
I think it's safe to say that this comment tag is written in upper case, just like TODO, FIXME and so on are. As such I would expect this lint to follow the official convention as well.
Note that I intentionally introduced some casing diversity in tests/ui/undocumented_unsafe_blocks.rs to test more cases than just Safety:.
changelog: Capitalize SAFETY comment in [undocumented_unsafe_blocks]
Labels
Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)