New lint proper_safety_comment as an alternative to undocumented_unsafe_blocks and unnecessary_safety_comment

[roberthree]

changelog: new lint: [proper_safety_comment]

This PR is about proposing an alternative to undocumented_unsafe_blocks and unnecessary_safety_comment that tries to address the following issues:

• unsafe extern { safe fn } support #13777
• unsafe extern support (undocumented_unsafe_blocks?) #13560
• Unsafe attributes support (undocumented_unsafe_blocks) #13316 and Unsafe attributes support (unnecessary_safety_comment) #13317
• undocumented_unsafe_blocks false positives around attributes #13189
• unnecessary_safety_comment false positive in _ = unsafe { foo_unsafe() } expression, conflicts with undocumented_unsafe_blocks correct analysis #13039
• undocumented_unsafe_blocks can be broken by rustfmt #13024
• undocumented_unsafe_blocks doesn't recognise safety comments before consts in impl blocks #12720 and undocumented_unsafe_blocks doesn't detect safety comment above associated constant #11709

Proposed new lint: PROPER_SAFETY_COMMENT

What it does

It requires proper safety comments at the barrier of Unsafety.
This includes any part of the code that needs to satisfy extra safety conditions:

• unsafe blocks (unsafe {})
• unsafe trait implementations (unsafe impl)
• unsafe external blocks (unsafe extern)
• unsafe attributes (#[unsafe(attr)])

Safety comments are non-doc line comments starting with SAFETY::

// SAFETY: A safety comment
// that can cover
// multiple lines.

Furthermore, it detects unnecessary safety comments for non-critical blocks, trait implementations and attributes. However, there can be false negatives.

Code that defines extra safety conditions is covered by clippy::missing_safety_doc and clippy::unnecessary_safety_doc

Why restrict this?

Breaking the safety barrier should not be done carelessly.
Proper documentation should be provided as to why each unsafe operation does not introduce undefined behavior.
Thinking about these safety requirements and writing them down can prevent incorrect implementations.
On the other hand, unnecessary safety comments are confusing and should not exist.

Example

unsafe fn f1() {}
fn f2() {
    unsafe { f1() }
}

unsafe trait A {}
unsafe impl A for () {}

unsafe extern {
    pub fn g1();
    pub unsafe fn g2();
    pub safe fn g3();
}

#[unsafe(no_mangle)]
fn h() {}

Use instead:

unsafe fn f1() {}
fn f2() {
    unsafe {
        // SAFETY: ...
        f1()
    }
}

unsafe trait A {}
// SAFETY: ...
unsafe impl A for () {}

// SAFETY: ...
unsafe extern {
    // SAFETY: ...
    pub fn g1();
    // SAFETY: ...
    pub unsafe fn g2();
    // SAFETY: ...
    pub safe fn g3();
}

// SAFETY: ...
#[unsafe(no_mangle)]
fn h() {}

Discussion

Moving the safety comment into the unsafe-block

undocumented_unsafe_blocks requires a safety comment above the block:

// SAFETY: ...
unsafe {}

While this is intuitive, there are a number of problems. Firstly, it does not play well with the style guide. As pointed out in #13024, rustfmt may break lines with an unsafe-block not at the beginning of the line into multiple ones.
undocumented_unsafe_blocks and unnecessary_safety_comment try to solve this problem by identifying valid parents of an unsafe-block and looking for the safety comment on top of them. But this can get quite complex:

// SAFETY: ...
let a =
    &unsafe { unsafe_fn() };

This has resulted in a few uncovered cases: #13189, #13039, #12720, #11709.

The main problem is that the distance between the unsafe-block and the safety comment in the AST/HIR representation is unknown and can be arbitrarily large.
On the other hand, just looking at the previous lines is also quite complicated if you want to avoid false negatives.

That's why proper_safety_comment requires a safety comment inside the unsafe-block at the top:

unsafe {
    // SAFETY: ...
    unsafe_fn()
}

This is well in line with the style guide of blocks and clearly links the safety comment to its unsafe-block. But it is also clear that this makes the code less compact:

if let Some(x) = unsafe {
    // SAFETY: ...
    unsafe_fn()
} {
    // ...
}

instead of

// SAFETY: ...
if let Some(x) = unsafe { unsafe_fn() } {
    // ...
}

However, more complex structures are more clearly documented:

let a = (
    unsafe {
        // SAFETY: ...
        unsafe_fn_1()
    },
    unsafe {
        // SAFETY: ...
        unsafe_fn_2()
    },
);

instead of

// SAFETY: ...
let a = (unsafe { unsafe_fn_1() }, unsafe { unsafe_fn_2() });

Excluding block comments

RFC 1574 recommends to avoid block comments and use line comments instead, as written down in Comments. This recommendation should be adopted for safety comments, which also simplifies the lint implementation, reducing false positives/negatives. Currently, proper_safety_comment requires non-doc line comments:

unsafe {
    // SAFETY: A safety comment
    // that can cover
    // multiple lines.
}

// SAFETY: A safety comment
// that can cover
// multiple lines.
unsafe impl A for ();

// SAFETY: A safety comment
// that can cover
// multiple lines.
unsafe extern {
    // SAFETY: A safety comment
    // that can cover
    // multiple lines.
    pub fn f();
}

// SAFETY: A safety comment
// that can cover
// multiple lines.
#[unsafe(no_mangle)]
fn h();

Question: Is there any need for non-doc block comments? Note that undocumented_unsafe_blocks allows block comments.

Excluding doc comments

Currently, proper_safety_comment does not allow doc comments.

Question: Is there any need for doc comments? Note that undocumented_unsafe_blocks allows doc comments.

Items in unsafe extern blocks

In addition to #13560 (safety comments for unsafe extern blocks), #13777 asks for comments on individual items. Currently, proper_safety_comment requires safety comments on both:

// SAFETY: ...
unsafe extern {
    // SAFETY: ...
    pub fn g1();
    // SAFETY: ...
    pub unsafe fn g2();
    // SAFETY: ...
    pub safe fn g3();
}

Question: Should there be a more individual solution?

Config parameters from UndocumentedUnsafeBlocks

accept-comment-above-statement

This config parameter avoids problems with rustfmt by allowing line breaks in statements:

// SAFETY: fail ONLY if `accept-comment-above-statement = false`
let some_variable_with_a_very_long_name_to_break_the_line =
    unsafe { a_function_with_a_very_long_name_to_break_the_line() };

In general, the following is allowed:

// SAFETY: ...
let x = unsafe { unsafe_fn() };

This is not needed any more with proper_safety_comment due to moving the safety comment into the unsafe-block.

accept-comment-above-attributes

This config parameter is in conflict with #13316 and #13317. proper_safety_comment requires safety comments on unsafe attributes and lints unnecessary safety comments on normal attributes. The idea of accept-comment-above-attributes is as follows:

// SAFETY: fail ONLY if `accept-comment-above-attribute = false`
#[allow(unsafe_code)]
unsafe {}

Currently, proper_safety_comment does not allow this and there is even no concept for it.

Question: Is there any use-case for accept-comment-above-attribute = false?

Unusual safety comments allowed with undocumented_unsafe_blocks

Examples have been taken from tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.rs.

fn safety_with_prepended_text() {
    // This is a test. safety:
    unsafe {}
}

fn non_ascii_comment() {
    // ॐ᧻໒ SaFeTy: ௵∰
    unsafe {};
}

proper_safety_comment does not allow "prepended text" and only allows SAFETY: as a safety comment label.

(Corner) cases to avoid false positive/negatives

This section is about collecting a list of all the complex corner cases that need to be addressed for a stable implementation.

• tests/ui/safety/proper_safety_comment/attribute.rs contains examples for attributes.
• tests/ui/safety/proper_safety_comment/block.rs contains examples for blocks.
• tests/ui/safety/proper_safety_comment/proc_macros.rs contains examples and their expected behaviour with respect to code generated by procedural macros. Question: Is the only way to detect procedural macros to compare the respective span with the actual source code?
• tests/ui/safety/proper_safety_comment/trait_impl.rs contains examples for trait implementations.
• tests/ui/safety/proper_safety_comment/unsafe_extern.rs contains examples for unsafe extern.

Question: What additional cases should be tested/supported?

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Manishearth (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[Manishearth]

Is there a reason to not just update the old lint?

[Manishearth]

Started a Zulip thread for that discussion https://rust-lang.zulipchat.com/#narrow/channel/257328-clippy/topic/proper_safety_comment.20lint

[ojeda]

Interesting proposal!

However, more complex structures are more clearly documented:

let a = (
    unsafe {
        // SAFETY: ...
        unsafe_fn_1()
    },
    unsafe {
        // SAFETY: ...
        unsafe_fn_2()
    },
);

instead of

// SAFETY: ...
let a = (unsafe { unsafe_fn_1() }, unsafe { unsafe_fn_2() });

Couldn't the latter have been written as

let a = (
    // SAFETY: ...
    unsafe { unsafe_fn_1() },
    // SAFETY: ...
    unsafe { unsafe_fn_2() },
);

?

[roberthree]

That's a good point, it's probably more about the formatter issue. Maybe a better example is:

// SAFETY: ...
if unsafe { unsafe_fn_01() } && unsafe { unsafe_fn_02() } {
    // ...
}

if
// SAFETY: ...
unsafe { unsafe_fn_01() } &&
// SAFETY: ...
unsafe { unsafe_fn_02() }
{
    // ...
}

if unsafe {
    // SAFETY: ...
    unsafe_fn_01()
} && unsafe {
    // SAFETY: ...
    unsafe_fn_02()
} {
    // ...
}

In general, I think you can always rewrite the code so that the safety comment above the unsafe-block works:

// SAFETY: old style, but formatting has broken the line
let a =
    &unsafe { unsafe_fn() };

let a =
    // SAFETY: old style, handling the linebreak
    &unsafe { unsafe_fn() };

let a = &unsafe {
    // SAFETY: new style, avoiding the formatter issue
    unsafe_fn()
};

But the old style only works reliably if the following unsafe-block starts the source line, otherwise the formatter may break the line and suddenly undocumented_unsafe_blocks is no longer satisfied. In my opinion, the formatter should not affect the "safety comment" lint. But the new style may not improve clarity, as @ojeda pointed out.

[pitaj]

To follow the lint naming conventions, this should be called improper_safety_comments.

[Jarcho]

Closing this since introducing a new lint to do what an existing lint does won't be merged. @roberthree if you want to open PRs extending the existing lint to handle extra cases please feel free to do so. If you want to try to push for safety comments being inside the block that's also fine, but it should be an extension to the existing lint and also be part of its own PR.