Add undocumented_unsafe_blocks lint by Serial-ATA · Pull Request #7748 · rust-la... - JOYK Joy of Geek, Geek News, Link all geek

Conversation

Copy link

Contributor

Serial-ATA commented 20 days ago •

edited

changelog: Added a new lint [undocumented_unsafe_blocks]

Fixes #7464, #7238 (?)

Copy link

Collaborator

rust-highfive commented 20 days ago

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.

Copy link

Member

xFrednet commented 19 days ago

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

Copy link

Member

flip1995 commented 19 days ago

Yes, sure. I have time this afternoon.

r? @flip1995

Copy link

Member

flip1995 left a comment

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.

Copy link

Contributor

bors commented 18 days ago

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

Copy link

ShadowJonathan left a comment •

edited

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)
};

clippy_lints/src/undocumented_unsafe_blocks.rs

Outdated Show resolved

Serial-ATA

changed the title Add undocumented_unsafe_block_safety lint

Add undocumented_unsafe_blocks lint

17 days ago

Copy link

Member

flip1995 left a comment

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

unsafe block in the macro call: m!(unsafe{ ... }
unsafe block in the macro definition

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

Copy link

Contributor

bors commented 15 days ago

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

Copy link

Member

flip1995 commented 15 days ago

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)

Copy link

Member