ACP: Referencing existing safety contracts in unsafe API documentation

[yilin0518]

Proposal

Problem statement

The standard library contains unsafe APIs whose safety requirements are identical to, derived from, or composed of the safety requirements of related APIs.

For example, many unsafe methods on NonNull<T> correspond to unsafe functions with the same name in the ptr module. And the unsafe methods on NonNull<T> describe the safety requirements by referring to the corresponding ptr functions:

    /// See [`ptr::write`] for safety concerns and examples.
    ///
    /// [`ptr::write`]: crate::ptr::write()
    #[rustc_const_stable(feature = "const_ptr_write", since = "1.83.0")]
    pub const unsafe fn write(self, val: T)
    where
        T: Sized,
    {
        // SAFETY: the caller must uphold the safety contract for `write`.
        unsafe { ptr::write(self.as_ptr(), val) }
    }

Some unsafe APIs only use a reference to the authoritative safety documentation:

/// path: alloc/alloc.rs
/// # Safety
///
/// See [`GlobalAlloc::dealloc`].
#[stable(feature = "global_alloc", since = "1.28.0")]
#[inline]
#[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
pub unsafe fn dealloc(ptr: *mut u8, layout: Layout) {
    // SAFETY: Upheld by caller.
    unsafe { dealloc_nonnull(NonNull::new_unchecked(ptr), layout) }
}

The two cases are acceptable because they both indicate the safety requirements. However, the safety documentation of the following APIs could be made more explicit:

• DisjointBitOr::disjoint_bitor
• FunnelShift::unchecked_funnel_shl
• FunnelShift::unchecked_funnel_shr

Also, the documentation of the atomic operation APIs in intrinsic module could likewise be improved.

• atomic_and
• atomic_cxchg
• atomic_cxchgweak
• atomic_load
• atomic_max
• atomic_min
• atomic_nand
• atomic_or
• atomic_store
• atomic_umax
• atomic_umin
• atomic_xadd
• atomic_xchg
• atomic_xor
• atomic_xsub

Motivating examples or use cases

The first three APIs use the same discription which don't indicate safety directly:

    /// See [`super::unchecked_funnel_shl`]; we just need the trait indirection to handle
    /// different types since calling intrinsics with generics doesn't work.
    unsafe fn unchecked_funnel_shl(self, right: Self, shift: u32) -> Self;

Also, the trait methods use different parameter name from the corresponding intrinsic functions.

The atomic operation APIs in intrinsic are marked unsafe, but their documentation does not state their safety requirements:

/// Loads the current value of the pointer.
/// `T` must be an integer or pointer type.
///
/// The stabilized version of this intrinsic is available on the
/// [`atomic`] types via the `load` method. For example, [`AtomicBool::load`].
#[rustc_intrinsic]
#[rustc_nounwind]
pub const unsafe fn atomic_load<T: Copy, const ORD: AtomicOrdering, const VOLATILE: bool>(
    src: *const T,
) -> T;

A simple idea is that since this function is unsafe, a # Safety section needs to be added to explain why it is unsafe(even without a # Safety section, it needs a reason about why it is unsafe).

For these atomic operations, they pass raw pointer as argument so the caller need to ensure that this pointer is valid. When VOLATILE is false, it is equivalent to Atomic<T>::from_ptr() followed by Atomic<T>::load().

Solution sketch

For the first three APIs, we can add a # Safety section that makes it explicit that the linked documentation defines the safety contract:

    /// We just need the trait indirection to handle different
    /// types since calling intrinsics with generics doesn't work.
    ///
    /// # Safety
    /// See [`super::disjoint_bitor`].
    unsafe fn disjoint_bitor(self, other: Self) -> Self;

Optionaly, the documentation may also specify the correspondence between parameters. For example, self corresponds to a, and other corresponds to b.

For these atomic APIs, the # Safety section can refer to the corresponding atomic APIs instead of duplicating their safety requirements:

/// Loads the current value of the pointer.
/// `T` must be an integer or pointer type.
///
/// # Safety
///
/// * If `VOLATILE` is `true`, this is equivalent to [Atomic::load_volatile].
///   Refer to the documentation of that method for safety requirements.
///
/// * If `VOLATILE` is `false`, this is equivalent to [Atomic::from_ptr] followed
///   by [Atomic::load]. Refer to the documentation of [Atomic::from_ptr] for safety requirements.
///
/// The stabilized version of this intrinsic is available on the
/// [`atomic`] types via the `load` method. For example, [`AtomicBool::load`].
///
/// [Atomic::load_volatile]: AtomicI32::load_volatile
/// [Atomic::from_ptr]: AtomicI32::from_ptr
/// [Atomic::load]: AtomicI32::load
#[rustc_intrinsic]
#[rustc_nounwind]
pub const unsafe fn atomic_load<T: Copy, const ORD: AtomicOrdering, const VOLATILE: bool>(
    src: *const T,
) -> T;

Alternatives

Adding a # Safety section is a small documentation change. But I think it would help make the presentation of safety requirements throughout the standard library more consistent.

An ideal situation is: For every unsafe API, we can use a # Safety to denote the point this API must ensure or must not violate; At every call site, we can discharge every point to ensure the soundness. If the same safety requirement already exists in other place, just refer it in # Safety is easy.

Links and related work

I have provided two PRs concerning the APIs discussed above:

• Add safety section for DisjointBitor::disjoint_bitor
• Add safety section for atomic_load/store

I have also opened several PRs that improve safety documentation in the Rust
standard library. All of the following PRs have been merged:

• Add safety comments in alloc::Wtf8
• doc: document safety requirements for core WTF-8
• Clarify safety requirements for SIMD shl/shr and masked load/store
• Fix safety doc in intrinsics::simd
• Add align requirement in _mm_stream_si32 and _mm_stream_si64
• Add supplementary information for get_unchecked(mut)
• Fix inconsistent safety requirement in VecDeque::nonoverlapping_ranges
• Add safety section for SliceIndex::get_unchecked(mut)

[RalfJung]

Note that these are all internal APIs. So this isn't really an ACP (API change proposal) as no public API is affected.

It's more a question for t-libs about how they'd like to do safety comments on internal unsafe functions.

[hxuhack]

Note that these are all internal APIs. 

How can we determine whether an API is considered internal or public?

If an API is intended for internal use only, why not use pub(crate) for it, or introduce a new visibility mechanism (e.g., pub(std)) if this distinction cannot be expressed with the current visibility system?

[RalfJung]

How can we determine whether an API is considered internal or public?

If you need to enable an internal feature to call the API, it's internal.
The compiler will tell you when you enable an internal feature.
core_intrinsics is an internal feature.

[hxuhack]

I understand that they can only be accessed by enabling the feature gate, but this applies not only to the standard library, but also to third-party crates. So I'm not sure whether it is appropriate to consider them internal APIs.

[RalfJung]

The compiler literally tells you that they are internal when you try to use them. I don't know why you think that this is up for debate.

[hxuhack]

It might be because I wasn't aware that APIs protected by compiler gates are considered internal.

[RalfJung]

That is not what I said. I said

If you need to enable an internal feature to call the API, it's internal.
The compiler will tell you when you enable an internal feature.
core_intrinsics is an internal feature.

The compiler prints this when you enable core_intrinsics:

warning: the feature `core_intrinsics` is internal to the compiler or standard library
 --> src/main.rs:1:12
  |
1 | #![feature(core_intrinsics)]
  |            ^^^^^^^^^^^^^^^
  |
  = note: using it is strongly discouraged
  = note: `#[warn(internal_features)]` on by default

So, it's easy to tell that these APIs are internal.

[hxuhack]

Thanks, that clarifies the distinction.
However, it might not be obvious from the API documentation alone that these APIs are considered internal. Is that true?

[RalfJung]

That's correct. We should maybe have rustdoc add a special marker for *internal* unstable functions. We could also hide them from the docs completely. (But the docs are useful for internal work on the standard library.)