Skip to content

unnecessary_safety_doc considered harmful #9986

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
asomers opened this issue Nov 29, 2022 · 1 comment · Fixed by #9989
Closed

unnecessary_safety_doc considered harmful #9986

asomers opened this issue Nov 29, 2022 · 1 comment · Fixed by #9989
Assignees
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have

Comments

@asomers
Copy link

asomers commented Nov 29, 2022

Summary

I disagree with the unnecessary_safety_doc warning. That's because a function's Safety section can document more than just the minimum requirements for calling the function safely. For example:

  • It can document why a function is safe, even though the programmer expects that it wouldn't be:
/// # Safety
///
/// Safe because the pointer argument is validated through some mysterious back channel
pub fn foo(*const MyStruct) {...}

or this real-life example

/// # Safety
/// Safe because the pointer argument is unused
// But it's required to be present because the API is defined according to some external
// interface specification
#[no_mangle]
pub extern "C" fn foo(_td: *mut thread_data) -> libc::c_int {...}

or this real-life example: https://github.com/asomers/blosc-rs/blob/03c25105e4d1b78af8c1af2cba98f5c390dd7c9c/blosc/src/lib.rs#L363

  • It can document how to use a function "safely", even though the compiler doesn't technically consider it unsafe.
/// # Safety
/// Though technically safe to use, this function is vulnerable to a race condition
/// and may return wrong results unless the caller does such and such.
pub fn foo() -> i64 {...}

or

/// # Safety
/// The returned pointer's lifetime will be such and such, and the caller is responsible
/// for freeing it with `Box::from_raw`.
pub fn foo() -> *const MyStruct {...}

Lint Name

unnecessary_safety_doc

Reproducer

I tried this code:

/// # Safety
///
/// Safe because the pointer argument is unused.
#[no_mangle]
pub extern "C" fn fio_bfffs_commit(_td: *mut thread_data) -> libc::c_int {
    unimplemented!()
}

I saw this happen:

warning: safe function's docs have unnecessary `# Safety` section

I expected to see this happen:
No warning

Version

rustc 1.67.0-nightly (2585bcea0 2022-11-28)
binary: rustc
commit-hash: 2585bcea0bc2a9c42a4be2c1eba5c61137f2b167
commit-date: 2022-11-28
host: x86_64-unknown-freebsd
release: 1.67.0-nightly
LLVM version: 15.0.4

Additional Labels

No response

@asomers asomers added C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have labels Nov 29, 2022
@xFrednet
Copy link
Member

I agree with this assessment. The lint should at least not be warn by default.

@rustbot claim

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants