Improve `unused_unsafe` lint by steffahn · Pull Request #93678 · rust-lang/rust...

By the way, this is the first time I’ve touched rustc source code (prior contributions of mine were standard-library only).

Main motivation: Fixes some issues with the current behavior. This PR is more-or-less completely re-implementing the unused_unsafe lint; it’s also only done in the MIR-version of the lint, the set of tests for the -Zthir-unsafeck version no longer succeeds (and is thus disabled, see lint-unused-unsafe.rs).

On current nightly,

unsafe fn unsf() {}

fn inner_ignored() {
    unsafe {
        #[allow(unused_unsafe)]
        unsafe {
            unsf()
        }
    }
}

doesn’t create any warnings. This situation is not unrealistic to come by, the inner unsafe block could e.g. come from a macro. Actually, this PR even includes removal of one unused unsafe in the standard library that was missed in a similar situation. (The inner unsafe coming from an external macro hides the warning, too.)

The reason behind this problem is how the check currently works:

• While generating MIR, it already skips nested unsafe blocks (i.e. unsafe nested in other unsafe) so that the inner one is always the one considered unused
• To differentiate the cases of no unsafe operations inside the unsafe vs. a surrounding unsafe block, there’s some ad-hoc magic walking up the HIR to look for surrounding used unsafe blocks.

There’s a lot of problems with this approach besides the one presented above. E.g. the MIR-building uses checks for unsafe_op_in_unsafe_fn lint to decide early whether or not unsafe blocks in an unsafe fn are redundant and ought to be removed.

#[allow(unsafe_op_in_unsafe_fn)]
unsafe fn granular_disallow_op_in_unsafe_fn() {
    unsafe {
        #[deny(unsafe_op_in_unsafe_fn)]
        {
            unsf();
        }
    }
}

error: call to unsafe function is unsafe and requires unsafe block (error E0133)
  --> src/main.rs:13:13
   |
13 |             unsf();
   |             ^^^^^^ call to unsafe function
   |
note: the lint level is defined here
  --> src/main.rs:11:16
   |
11 |         #[deny(unsafe_op_in_unsafe_fn)]
   |                ^^^^^^^^^^^^^^^^^^^^^^
   = note: consult the function's documentation for information on how to avoid undefined behavior

warning: unnecessary `unsafe` block
  --> src/main.rs:10:5
   |
9  | unsafe fn granular_disallow_op_in_unsafe_fn() {
   | --------------------------------------------- because it's nested under this `unsafe` fn
10 |     unsafe {
   |     ^^^^^^ unnecessary `unsafe` block
   |
   = note: `#[warn(unused_unsafe)]` on by default

Here, the intermediate unsafe was ignored, even though it contains a unsafe operation that is not allowed to happen in an unsafe fn without an additional unsafe block.

Also closures were problematic and the workaround/algorithms used on current nightly didn’t work properly. (I skipped trying to fully understand what it was supposed to do, because this PR uses a completely different approach.)

fn nested() {
    unsafe {
        unsafe { unsf() }
    }
}

warning: unnecessary `unsafe` block
  --> src/main.rs:10:9
   |
9  |     unsafe {
   |     ------ because it's nested under this `unsafe` block
10 |         unsafe { unsf() }
   |         ^^^^^^ unnecessary `unsafe` block
   |
   = note: `#[warn(unused_unsafe)]` on by default

fn nested() {
    let _ = || unsafe {
        let _ = || unsafe { unsf() };
    };
}

warning: unnecessary `unsafe` block
 --> src/main.rs:9:16
  |
9 |     let _ = || unsafe {
  |                ^^^^^^ unnecessary `unsafe` block
  |
  = note: `#[warn(unused_unsafe)]` on by default

warning: unnecessary `unsafe` block
  --> src/main.rs:10:20
   |
10 |         let _ = || unsafe { unsf() };
   |                    ^^^^^^ unnecessary `unsafe` block

note that this warning kind-of suggests that both unsafe blocks are redundant

I also dislike the fact that it always suggests keeping the outermost unsafe. E.g. for

fn granularity() {
    unsafe {
        unsafe { unsf() }
        unsafe { unsf() }
        unsafe { unsf() }
    }
}

I prefer if rustc suggests removing the more-course outer-level unsafe instead of the fine-grained inner unsafe blocks, which it currently does on nightly:

warning: unnecessary `unsafe` block
  --> src/main.rs:10:9
   |
9  |     unsafe {
   |     ------ because it's nested under this `unsafe` block
10 |         unsafe { unsf() }
   |         ^^^^^^ unnecessary `unsafe` block
   |
   = note: `#[warn(unused_unsafe)]` on by default

warning: unnecessary `unsafe` block
  --> src/main.rs:11:9
   |
9  |     unsafe {
   |     ------ because it's nested under this `unsafe` block
10 |         unsafe { unsf() }
11 |         unsafe { unsf() }
   |         ^^^^^^ unnecessary `unsafe` block

warning: unnecessary `unsafe` block
  --> src/main.rs:12:9
   |
9  |     unsafe {
   |     ------ because it's nested under this `unsafe` block
...
12 |         unsafe { unsf() }
   |         ^^^^^^ unnecessary `unsafe` block

Needless to say, this PR addresses all these points. For context, as far as my understanding goes, the main advantage of skipping inner unsafe blocks was that a test case like

fn top_level_used() {
    unsafe {
        unsf();
        unsafe { unsf() }
        unsafe { unsf() }
        unsafe { unsf() }
    }
}

should generate some warning because there’s redundant nested unsafe, however every single unsafe block does contain some statement that uses it. Of course this PR doesn’t aim change the warnings on this kind of code example, because the current behavior, warning on all the inner unsafe blocks, makes sense in this case.

As mentioned, during MIR building all the unsafe blocks are kept now, and usage is attributed to them. The way to still generate a warning like

warning: unnecessary `unsafe` block
  --> src/main.rs:11:9
   |
9  |     unsafe {
   |     ------ because it's nested under this `unsafe` block
10 |         unsf();
11 |         unsafe { unsf() }
   |         ^^^^^^ unnecessary `unsafe` block
   |
   = note: `#[warn(unused_unsafe)]` on by default

warning: unnecessary `unsafe` block
  --> src/main.rs:12:9
   |
9  |     unsafe {
   |     ------ because it's nested under this `unsafe` block
...
12 |         unsafe { unsf() }
   |         ^^^^^^ unnecessary `unsafe` block

warning: unnecessary `unsafe` block
  --> src/main.rs:13:9
   |
9  |     unsafe {
   |     ------ because it's nested under this `unsafe` block
...
13 |         unsafe { unsf() }
   |         ^^^^^^ unnecessary `unsafe` block

in this case is by emitting a unused_unsafe warning for all of the unsafe blocks that are within a used unsafe block.

The previous code had a little HIR traversal already anyways to collect a set of all the unsafe blocks (in order to afterwards determine which ones are unused afterwards). This PR uses such a traversal to do additional things including logic like always warn for an unsafe block that’s inside of another used unsafe block. The traversal is expanded to include nested closures in the same go, this simplifies a lot of things.

The whole logic around unsafe_op_in_unsafe_fn is a little complicated, there’s some test cases of corner-cases in this PR. (The implementation involves differentiating between whether a used unsafe block was used exclusively by operations where allow(unsafe_op_in_unsafe_fn) was active.) The main goal was to make sure that code should compile successfully if all the unused_unsafe-warnings are addressed simultaneously (by removing the respective unsafe blocks) no matter how complicated the patterns of unsafe_op_in_unsafe_fn being disallowed and allowed throughout the function are.

One noteworthy design decision I took here: An unsafe block with allow(unused_unsafe) is considered used for the purposes of linting about redundant contained unsafe blocks. So while

#![deny(unused_unsafe)]

fn granularity() {
    unsafe { //~ ERROR: unnecessary `unsafe` block
        unsafe { unsf() }
        unsafe { unsf() }
        unsafe { unsf() }
    }
}

warns for the outer unsafe block,

#![deny(unused_unsafe)]

fn top_level_ignored() {
    #[allow(unused_unsafe)]
    unsafe {
        #[deny(unused_unsafe)]
        {
            unsafe { unsf() } //~ ERROR: unnecessary `unsafe` block
            unsafe { unsf() } //~ ERROR: unnecessary `unsafe` block
            unsafe { unsf() } //~ ERROR: unnecessary `unsafe` block
        }
    }
}

warns on the inner ones.

It makes sense to review commit-by-commit; I’ve e.g. split implementation from addressing tests, and within tests, there’s also distinguishing blessing changed output from previous tests from adding new tests.