undocumented_unsafe_blocks false positives around attributes

[ojeda]

Assuming the most relaxed setup, i.e. the defaults (accept-comment-above-attribute = true and accept-comment-above-statement = true), undocumented_unsafe_blocks seems to have false positives around attributes, since moving the attribute on top makes it work (i.e. it does not lint anymore) in all cases.

For instance, we had this code:

    // SAFETY: ...
    #[allow(clippy::unnecessary_cast)]
    return Err(unsafe { Error::from_errno_unchecked(err as core::ffi::c_int) });

Is this expected to trigger? It currently lints. Perhaps the developer is expected to put the // SAFETY comment inside Err:

    return Err(
        // SAFETY: ...
        #[allow(clippy::unnecessary_cast)]
        unsafe { Error::from_errno_unchecked(err as core::ffi::c_int) }
    );

That works (i.e. it does not lint anymore), although it may be debatable whether it looks/is better or not.

However, moving the attribute on top of the comment also makes it work (i.e. it does not lint anymore):

    #[allow(clippy::unnecessary_cast)]
    // SAFETY: ...
    return Err(unsafe { Error::from_errno_unchecked(err as core::ffi::c_int) });

which is a bit surprising. It seems like that is "close enough". Should that trigger, or not?

Moreover, testing a few more variations, I found that this would also lint:

    // SAFETY: ...
    #[allow(clippy::unnecessary_cast)]
    return unsafe { h() };

For this one, moving the comment below the attribute makes it pass too.

Below I leave a few more variations/test cases. Which ones should pass, or not? Given that all of them pass when the comment is moved below the attribute, it would seem that all of them were expected to pass (i.e. not lint).

#![allow(clippy::needless_return)]
#![allow(clippy::let_and_return)]

extern {
    fn h() -> i32;
}

pub fn f1a() -> i32 {
    // SAFETY: OK.
    #[allow(clippy::unnecessary_cast)]
    unsafe { h() }
}

pub fn f1b() -> Result<i32, i32> {
    // SAFETY: Warns.
    #[allow(clippy::unnecessary_cast)]
    Ok(unsafe { h() })
}

pub fn f1c() -> Result<i32, i32> {
    Ok(
        // SAFETY: OK.
        #[allow(clippy::unnecessary_cast)]
        unsafe { h() }
    )
}

pub fn f2a() -> i32 {
    // SAFETY: Warns.
    #[allow(clippy::unnecessary_cast)]
    return unsafe { h() };
}

pub fn f2b() -> Result<i32, i32> {
    // SAFETY: Warns.
    #[allow(clippy::unnecessary_cast)]
    return Ok(unsafe { h() });
}


pub fn f2c() -> Result<i32, i32> {
    return Ok(
        // SAFETY: OK.
        #[allow(clippy::unnecessary_cast)]
        unsafe { h() }
    );
}


pub fn f3a() -> i32 {
    // SAFETY: OK.
    #[allow(clippy::unnecessary_cast)]
    let x = unsafe { h() };

    x
}

pub fn f3b() -> Result<i32, i32> {
    // SAFETY: Warns.
    #[allow(clippy::unnecessary_cast)]
    let x = Ok(unsafe { h() });

    x
}

pub fn f3c() -> Result<i32, i32> {
    let x = Ok(
        // SAFETY: OK.
        #[allow(clippy::unnecessary_cast)]
        unsafe { h() }
    );

    x
}

[ojeda]

Not sure if I can do this, but:

@rustbot label +C-bug +I-false-negative

[ojeda]

Sorry, the main case is (I assume) a false positive (and likely all of the ones listed above):

@rustbot label -I-false-negative +I-false-positive

[roberthree]

I don't know exactly why specific examples don't lint yet, but I suspect the underlying problem is similar to #13039, #12720, #11709. An underlying problem related to It seems like that is "close enough" with safety comments on unsafe blocks is the arbitrarily large distance in the HIR/AST representation between the unsafe-block and the code closest to the safety comment, combined with looking at source lines to find the safety comment. That's why I proposed a different safety comment for unsafe-blocks in #13887, which moves the safety comment inside the block. But I don't know if this is a viable alternative in terms of code aesthetics. Feedback is welcome 😀
In light of #13317, how would you prefer to handle attributes between safety comments and their respective unsafe parts of the code?

[ojeda]

That's why I proposed a different safety comment for unsafe-blocks in #13887, which moves the safety comment inside the block. But I don't know if this is a viable alternative in terms of code aesthetics. Feedback is welcome 😀

Thanks for that PR -- it is an interesting proposal.

In light of #13317, how would you prefer to handle attributes between safety comments and their respective unsafe parts of the code?

Do you mean what happens if there is a // SAFETY comment above a safe attribute that could have been intended for an unsafe block (or unsafe impl etc.) below it? i.e. whether the following should trigger clippy::undocumented_unsafe_blocks or not?

// SAFETY: ...
#[expect(unused_variables)]
let x = unsafe { std::mem::zeroed::<u8>() };

[roberthree]

Yes, exactly. If I understand #13317 correctly, your example should trigger unnecessary_safety_comment and the fix would be:

#[expect(unused_variables)]
// SAFETY: ...
let x = unsafe { std::mem::zeroed::<u8>() };

But then I would assume that you wouldn't allow your initial issue example as a valid safety comment (regarding undocumented_unsafe_blocks):

// SAFETY: ...
#[allow(clippy::unnecessary_cast)]
return Err(unsafe { Error::from_errno_unchecked(err as core::ffi::c_int) });

[ojeda]

Yeah, given the existence of unsafe attributes, it sounds reasonable to have the // SAFETY comment below the attribute. Having said that, we have some times code like:

// A multi-line comment about what this part
// of the code does...
//
// SAFETY: ... may reference something in
// the multi-line comment above ...
#[allow(something)]
unsafe { some_code_here(); }

Which means now it would look like:

// A multi-line comment about what this part
// of the code does...
#[allow(something)]
// SAFETY: ... may reference something in
// the multi-line comment above ...
unsafe { some_code_here(); }

Which makes sense, though I think it will take some time to get used to.

And with your other proposal on top:

// A multi-line comment about what this part
// of the code does...
#[allow(something)]
unsafe {
    // SAFETY: ... may reference something in
    // the multi-line comment above ...
    some_code_here();
}