missing_unsafe_doc, explain why an unsafe block is safe/sound

[ShadowJonathan]

What it does

Kinda like missing_safety_doc, but this lint would warn about a missing comment on each unsafe {} block, for which a comment must exist explaining why the unsafe block is safe/sound.

Categories (optional)

• Kind: style

What is the advantage of the recommended code over the original code

There'd be no recommended code, but the lint would suggest adding

// Safety: [...]

To the new code as a start. The lint would also raise a warning if that comment is copied verbatim.

Drawbacks

This would be rather bothersome to those who know what they're doing, or only make small unsafe blocks all around.

Example

pub(crate) fn coerce<T>(ffi_pointer: *const usize) -> T {
  unsafe {
      some_unchecked_function(pointer)
  }
}

Could be written as:

pub(crate) fn coerce<T>(ffi_pointer: *const usize) -> T {
  // Safety: function visibility is crate-internal, ffi_pointer could only be constructed in this crate.
  unsafe {
      some_unchecked_function(pointer)
  }
}

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

[MTRNord]

I would like to have this as it also improves overall crate quality as people who are less familiar with the code inside of unsafe (as it can be a C binding for example) would get a better understanding of this code which I think is overall leading to better documentation quality for contributors of crates.

[LeSeulArtichaut]

This also works really well with the #![deny(unsafe_op_in_unsafe_fn)], which forces you to put unsafe blocks around unsafe operations in unsafe functions, which in turn requires that you write a SAFETY comment

[LeSeulArtichaut]

I'll try to take a stab at this. @rustbot claim

[xFrednet]

The PR has been closed due to inactivity. My understanding is that @LeSeulArtichaut will not continue to work on this for now. I'll therefore unassign him. The PR still looks like a good starting point if someone else wants to take over 🙃

[Serial-ATA]

I'll try. @rustbot claim

[Serial-ATA]

Should this check all unsafe blocks or just those in safe functions?

[xFrednet]

It would make sense to have it on every unsafe block IMO. 🙃

[Serial-ATA]

I've got it working, but I can't figure out how to do the suggestion.

format!("// Safety: ...\n{}", snippet(cx, block.span, ".."))

The newline breaks the snippet line.

help: consider adding a safety comment
   |
LL ~     // Safety: ...
LL + unsafe {}
   |

Should I not be using span_lint_and_sugg?

[ShadowJonathan]

Should this check all unsafe blocks or just those in safe functions?

Every one, yes, unconditionally. Unless it's explicitly opted out (#[allow]).

[xFrednet]

Should I not be using span_lint_and_sugg?

For this lint you can just use span_lint as it was used in #7557. span_lint_and_sugg is usually better, but this lint requires the user to add some additional information anyway. If you want to give an example, you can use clippy_utils::source::indent_of to get the indention of the unsafe block and then clippy_utils::source::reindent_multiline to fix the indention in the suggestion. 🙃

[ShadowJonathan]

One thing i'm not sure about is if the suggestion will clash with all-uppercase SAFETY: i've seen in some codebases and linting combinations, could that maybe also be taken into account?

[Serial-ATA]

Thanks, that had me confused for awhile.

One thing i'm not sure about is if the suggestion will clash with all-uppercase SAFETY: i've seen in some codebases and linting combinations, could that maybe also be taken into account?

What I have now doesn't care about case since I saw your examples used "Safety" and #7557 uses "SAFETY"

[Serial-ATA]

Nevermind, I finally figured it out :)

Question

I assume you'd also want to accept

// Safety: ...
let _ = unsafe {};

over

let _ =
	// Safety: ...
	unsafe {}

but since I'm using check_block I can't figure out how to check if the Block is part of a Local. Is there a simple way to check for that, or should this just not be handled?

[ShadowJonathan]

I assume you'd also want to accept

Yup, would make sense. Though i think it'd get complicated once it gets nested enough (think let _ = run(|| run2(|| unsafe {}));), just a thought.