`clippy::multiple_unsafe_ops_per_block` and `unused_unsafe` not handled properly in nested unsafe calls/closures

[meadowsys]

Summary

I am calling an unsafe function, passing in a callback in a parameter, and inside that parameter I am calling another unsafe function. This works fine without touching any lints, but recently I enabled clippy::multiple_unsafe_ops_per_block (to combine it with clippy::undocumented_unsafe_blocks, but I think that's irrelevant to this bug), and it's emitting a warning that this unsafe block contains two unsafe operations. It does indeed... so I wrapped the inner one in an unsafe block. This warning doesn't go away from that, and causes rustc's builtin unused_unsafe lint to trigger.

I am not completely sure whether to report this here or if I should have done so on the compiler repo, since it involves a rustc builtin lint as well. This would probably require more changes than just a clippy change, I imagine.

Lint Name

clippy::multiple_unsafe_ops_per_block, and rustc's unused_unsafe

Reproducer

This code triggers clippy::multiple_unsafe_ops_per_block:

unsafe fn takes_closure(_: impl Fn()) {}
unsafe fn do_unsafe_op() {}

fn example() {
    // SAFETY: comment would be here
    unsafe {
        takes_closure(|| {
            do_unsafe_op()
        })
    }
}

This code still triggers the lint, but also triggers unused_unsafe

fn example() {
    // SAFETY: comment would be here
    unsafe {
        takes_closure(|| {
            // SAFETY: comment would be here
            unsafe { do_unsafe_op() }
        })
    }
}

I saw this happen:

warning: unnecessary `unsafe` block
  --> src/test.rs:33:13
   |
30 |     unsafe {
   |     ------ because it's nested under this `unsafe` block
...
33 |             unsafe { do_unsafe_op() }
   |             ^^^^^^ unnecessary `unsafe` block
   |
   = note: `#[warn(unused_unsafe)]` on by default

warning: this `unsafe` block contains 2 unsafe operations, expected only one
  --> src/test.rs:30:5
   |
30 | /     unsafe {
31 | |         takes_closure(|| {
32 | |             // SAFETY: comment would be here
33 | |             unsafe { do_unsafe_op() }
34 | |         })
35 | |     }
   | |_____^
   |
note: unsafe function call occurs here
  --> src/test.rs:31:9
   |
31 | /         takes_closure(|| {
32 | |             // SAFETY: comment would be here
33 | |             unsafe { do_unsafe_op() }
34 | |         })
   | |__________^
note: unsafe function call occurs here
  --> src/test.rs:33:22
   |
33 |             unsafe { do_unsafe_op() }
   |                      ^^^^^^^^^^^^^^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#multiple_unsafe_ops_per_block
note: the lint level is defined here
  --> src/test.rs:6:5
   |
6  |     clippy::multiple_unsafe_ops_per_block,
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I expected no warnings from this.

Side note... while writing this, I had a realisation, that I could refactor the closure expression to its own variable outside the unsafe block, and that would satisfy the lint. But I think this is still good to bring up anyways

Version

rustc 1.81.0-nightly (e9e6e2e44 2024-06-28)
binary: rustc
commit-hash: e9e6e2e444c30c23a9c878a88fbc3978c2acad95
commit-date: 2024-06-28
host: aarch64-apple-darwin
release: 1.81.0-nightly
LLVM version: 18.1.7

Additional Labels

No response

[roberthree]

Looking at the current lint code, this is the expected behaviour. Currently, for each relevant unsafe-block unsafe operations are counted by visiting each nested expression via for_each_expr. Nested unsafe-blocks are ignored in this traversal while still visiting their contained expressions, and therefore clippy::multiple_unsafe_ops_per_block is not even aware of these nested unsafe labels. That's why it doesn't matter if you have multiple nested unsafe-blocks or just one.

As you wrote, refactoring the code into multiple statements, i.e. separating the expressions, should always be possible, and is even intended by the lint's example. I'm not sure if this lint should allow nested unsafe-blocks, given unused_unsafe and the fact that one can and probably should refactor such situations. But if so, one could stop (?) traversing deeper when a nested unsafe-block is detected.

On the other hand, assuming that unused_unsafe is related to rustc_mir_build::errors::UnusedUnsafe and in particular rustc_mir_build::errors::UnusedUnsafeEnclosing, the error detected here, then the explanation of unused_unsafe needs to be updated regarding disallowing nested unsafe-blocks.

[tamird]

The original example no longer triggers unused_unsafe, or at least I can't make that happen: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=552cbe4113565f854f40be944248ce93

Refactoring into multiple statements is possible, but kind of sucks when the safety of the inner block relies on properties provided by the outer block. Here's an example from Linux:

        // SAFETY: We only return `false` if the `work_struct` is already in a workqueue. The other
        // `__enqueue` requirements are not relevant since `W` is `Send` and static.
        //
        // Furthermore, if the C workqueue code accesses the pointer after this call to
        // `__enqueue`, then the work item was successfully enqueued, and `bindings::queue_work_on`
        // will have returned true. In this case, `__enqueue` promises that the raw pointer will
        // stay valid until we call the function pointer in the `work_struct`, so the access is ok.
        unsafe {
            w.__enqueue(move |work_ptr| {
                // SAFETY: `__enqueue` guarantees that the pointer is valid for the duration of this
                // closure.
                unsafe {
                    bindings::queue_work_on(
                        bindings::wq_misc_consts_WORK_CPU_UNBOUND as _,
                        queue_ptr,
                        work_ptr,
                    )
                }
            })
        }

but I can't write this because the lint triggers on the outer block.

EDIT: just kidding, unused_unsafe does trigger, I missed it because it comes from rustc rather than clippy.