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?