New lint: Limit unsafe blocks to a single unsafe operation

[djkoloski]

What it does

Checks for unsafe blocks that contain more than one unsafe operation. Unsafe operations include:

• Dereferencing a raw pointer (*const T or *mut T)
• Calling an unsafe function or method
• Accessing or modifying a mutable static variable
• Accessing fields of unions

Lint Name

one_unsafe_op_per_unsafe_block

Category

restriction

Advantage

Combined with undocumented_unsafe_blocks, this lint ensures that each unsafe operation must be independently justified. Combined with unused_unsafe, this lint also ensures that we know when we eliminate unsafe operations through refactoring.

See the "Example" section for comprehensive examples.

Drawbacks

It may be awkward or noisy to break up a series of related unsafe operations into individual blocks.

Example

With undocumented_unsafe_blocks

/// Reads a `char` from the given pointer.
///
/// # Safety
///
/// `ptr` must point to four consecutive, initialized bytes which
/// form a valid `char` when interpreted in the native byte order.
unsafe fn read_char(ptr: *const u8) -> char {
    // SAFETY: The caller has guaranteed that the value pointed
    // to by `bytes` is a valid `char`.
    unsafe { char::from_u32_unchecked(*ptr.cast::<u32>()) }
}

Although the unsafe block is properly justified for char::from_u32_unchecked, there are actually two unsafe operations in it. The pointer dereference needs to be justified as well, but it's easy to accidentally include it without updating the justification. By limiting unsafe blocks to a single unsafe operation, this would be rewritten as:

/// Reads a `char` from the given pointer.
///
/// # Safety
///
/// - `ptr` must be 4-byte aligned, point to four consecutive
///   initialized bytes, and be valid for reads of 4 bytes.
/// - The bytes pointed to by `ptr` must represent a valid
///   `char` when interpreted in the native byte order.
unsafe fn read_char(ptr: *const u8) -> char {
    // SAFETY: `ptr` is 4-byte aligned, points to four consecutive
    // initialized bytes, and is valid for reads of 4 bytes.
    let int_value = unsafe { *ptr.cast::<u32>() };
    // SAFETY: The caller has guaranteed that the four bytes
    // pointed to by `bytes` represent a valid `char`.
    unsafe { char::from_u32_unchecked(int_value) }
}

Breaking up our unsafe block led us to justify dereferencing ptr, which in turn led us to update the function's safety requirements to accurately reflect the invariants used by the implementation.

With unused_unsafe

Refactoring from the same starting function as the example in undocumented_unsafe_blocks:

/// Reads a `char` from the given reference.
///
/// # Safety
///
/// `int` must represent a valid `char` when interpreted in
/// the native byte order.
unsafe fn read_char(int: &u32) -> char {
    // SAFETY: The caller has guaranteed that the four bytes
    // pointed to by `bytes` represent a valid `char`.
    unsafe { char::from_u32_unchecked(*int) }
}

By changing our function to accept &u32, we were able to remove the only unsafe operation from our first unsafe block. In turn, we were able to eliminate the unsafe block, prompting us to update our safety requirements for our function. In this way, refactoring a function to remove unsafe helps ensure that the safety documentation also gets updated to reflect the new requirements.

[Niki4tap]

I'll try to implement this

@rustbot claim

[llogiq]

Would the lint accept the same operation multiple times, e.g. two pointer derefs?

I do worry that such a lint might lead to very unidiomatic code, but as a restriction lint it might be a good addition for some use cases.

[Niki4tap]

Would the lint accept the same operation multiple times, e.g. two pointer derefs?

Actually not sure, maybe the the issue author will answer this better.

I do worry that such a lint might lead to very unidiomatic code, but as a restriction lint it might be a good addition for some use cases.

That's what I'm worried about as well, but this is a restriction lint, so developers using it will be very aware of this, and use it accordingly, maybe it doesn't make sense crate-wide, but restricting some specific module to it might be a good idea:

// very unsafe module
#[warn(clippy::one_unsafe_op_per_block)]
mod not_very_safe {
    // ...
}

[smoelius]

FWIW, this lint sounds very similar to #8022.

[Niki4tap]

The issue author seems a bit inactive on this, and since I've assigned myself, I'm going to push this issue a bit.

Assuming the lint should count any unsafe operation (8 pointer derefs -> 8 unsafe ops), I have the lint implemented and pretty much ready for PR (maybe some cleanup is needed, but the logic is already there).

As for the usefulness of this lint, I personally believe that it will be useful in cases where you're writing a safe wrapper for an unsafe function and you want this wrapper to be absolutely safe, although the code readability might suffer a bit, due to extensive use of unsafe blocks, this lint forces the developer to be explicit about each unsafe operation, and, as mentioned by the issue author, in combination with undocumented_unsafe_blocks, also forces the developer to justify each unsafe operation in a safety comment (for examples look at the original issue).

About the relation to #8022, I think this lint request shouldn't suffer from the pushback received there (although most of the points raised there are still valid for this lint as well), at least because this lint was initially proposed as a restriction lint, not pedantic.

tl;dr: when used right, this lint should be useful, and considering that it's a restriction lint, it shouldn't trigger for users that do not know how to use it correctly.

I will wait for comments on this for a bit, and open a PR if there are no additional concerns.