#![deny(clippy::undocumented_unsafe_blocks)]: Add and fix existing safety comments (#1305)

* Part of #1270.

This adds `#![deny(clippy::undocumented_unsafe_blocks)]` to the
`librav1d` crate. Since we don't yet run `clippy` in CI, this doesn't do
anything yet, but that's good, since we can turn that on once we fix all
of the errors. This PR only turns on the lint and fixes the existing
safety comments into ones that the lint recognizes.

Author: kkysen

include/dav1d/data.rs

@@ -30,8 +30,10 @@ impl From<Dav1dData> for Rav1dData {
             m,
         } = value;
         Self {
-            // Safety: `r#ref` is a [`RawCArc`] originally from [`CArc`].
-            data: r#ref.map(|r#ref| unsafe { CArc::from_raw(r#ref) }),
+            data: r#ref.map(|r#ref| {
+                // SAFETY: `r#ref` is a [`RawCArc`] originally from [`CArc`].
+                unsafe { CArc::from_raw(r#ref) }
+            }),
             m: m.into(),
         }
     }

include/dav1d/picture.rs

@@ -449,24 +449,36 @@ impl From<Dav1dPicture> for Rav1dPicture {
         } = value;
         Self {
             // We don't `.update_rav1d()` [`Rav1dSequenceHeader`] because it's meant to be read-only.
-            // Safety: `raw` came from [`RawArc::from_arc`].
-            seq_hdr: seq_hdr_ref.map(|raw| unsafe { raw.into_arc() }),
+            seq_hdr: seq_hdr_ref.map(|raw| {
+                // SAFETY: `raw` came from [`RawArc::from_arc`].
+                unsafe { raw.into_arc() }
+            }),
             // We don't `.update_rav1d()` [`Rav1dFrameHeader`] because it's meant to be read-only.
-            // Safety: `raw` came from [`RawArc::from_arc`].
-            frame_hdr: frame_hdr_ref.map(|raw| unsafe { raw.into_arc() }),
-            // Safety: `raw` came from [`RawArc::from_arc`].
-            data: data_ref.map(|raw| unsafe { raw.into_arc() }),
+            frame_hdr: frame_hdr_ref.map(|raw| {
+                // SAFETY: `raw` came from [`RawArc::from_arc`].
+                unsafe { raw.into_arc() }
+            }),
+            data: data_ref.map(|raw| {
+                // SAFETY: `raw` came from [`RawArc::from_arc`].
+                unsafe { raw.into_arc() }
+            }),
             stride,
             p: p.into(),
             m: m.into(),
-            // Safety: `raw` came from [`RawArc::from_arc`].
-            content_light: content_light_ref.map(|raw| unsafe { raw.into_arc() }),
-            // Safety: `raw` came from [`RawArc::from_arc`].
-            mastering_display: mastering_display_ref.map(|raw| unsafe { raw.into_arc() }),
+            content_light: content_light_ref.map(|raw| {
+                // SAFETY: `raw` came from [`RawArc::from_arc`].
+                unsafe { raw.into_arc() }
+            }),
+            mastering_display: mastering_display_ref.map(|raw| {
+                // Safety: `raw` came from [`RawArc::from_arc`].
+                unsafe { raw.into_arc() }
+            }),
             // We don't `.update_rav1d` [`Rav1dITUTT35`] because never read it.
-            // Safety: `raw` came from [`RawArc::from_arc`].
             itut_t35: itut_t35_ref
-                .map(|raw| unsafe { raw.into_arc() })
+                .map(|raw| {
+                    // SAFETY: `raw` came from [`RawArc::from_arc`].
+                    unsafe { raw.into_arc() }
+                })
                 .unwrap_or_default(),
         }
     }
@@ -740,7 +752,7 @@ impl Rav1dPicAllocator {
             ..Default::default()
         };
         let mut pic_c = pic.to::<Dav1dPicture>();
-        // Safety: `pic_c` is a valid `Dav1dPicture` with `data`, `stride`, `allocator_data` unset.
+        // SAFETY: `pic_c` is a valid `Dav1dPicture` with `data`, `stride`, `allocator_data` unset.
         let result = unsafe { (self.alloc_picture_callback)(&mut pic_c, self.cookie) };
         result.try_to::<Rav1dResult>().unwrap()?;
         // `data`, `stride`, and `allocator_data` are the only fields set by the allocator.
@@ -777,7 +789,7 @@ impl Rav1dPicAllocator {
             allocator_data,
             ..Default::default()
         };
-        // Safety: `pic_c` contains the same `data` and `allocator_data`
+        // SAFETY: `pic_c` contains the same `data` and `allocator_data`
         // that `Self::alloc_picture_data` set, which now get deallocated here.
         unsafe {
             (self.release_picture_callback)(&mut pic_c, self.cookie);

lib.rs

@@ -6,6 +6,7 @@
 )]
 #![deny(unsafe_op_in_unsafe_fn)]
 #![allow(clippy::all)]
+#![deny(clippy::undocumented_unsafe_blocks)]
 
 #[cfg(not(any(feature = "bitdepth_8", feature = "bitdepth_16")))]
 compile_error!("No bitdepths enabled. Enable one or more of the following features: `bitdepth_8`, `bitdepth_16`");

src/align.rs

@@ -169,14 +169,14 @@ impl<T: Copy, C: AlignedByteChunk> AlignedVec<T, C> {
 
     /// Extract a slice containing the entire vector.
     pub fn as_slice(&self) -> &[T] {
-        // Safety: The first `len` elements have been
+        // SAFETY: The first `len` elements have been
         // initialized to `T`s in `Self::resize_with`.
         unsafe { slice::from_raw_parts(self.as_ptr(), self.len) }
     }
 
     /// Extract a mutable slice of the entire vector.
     pub fn as_mut_slice(&mut self) -> &mut [T] {
-        // Safety: The first `len` elements have been
+        // SAFETY: The first `len` elements have been
         // initialized to `T`s in `Self::resize_with`.
         unsafe { slice::from_raw_parts_mut(self.as_mut_ptr(), self.len) }
     }

src/c_arc.rs

@@ -11,7 +11,7 @@ use std::sync::Arc;
 
 pub fn arc_into_raw<T: ?Sized>(arc: Arc<T>) -> NonNull<T> {
     let raw = Arc::into_raw(arc).cast_mut();
-    // Safety: [`Arc::into_raw`] never returns null.
+    // SAFETY: [`Arc::into_raw`] never returns null.
     unsafe { NonNull::new_unchecked(raw) }
 }
 
@@ -106,7 +106,7 @@ impl<T: ?Sized> AsRef<T> for CArc<T> {
             }
         }
 
-        // Safety: [`Self::stable_ref`] is a ptr
+        // SAFETY: [`Self::stable_ref`] is a ptr
         // derived from [`Self::owner`]'s through [`CBox::as_ref`]
         // and is thus safe to dereference.
         // The [`CBox`] is [`Pin`]ned and
@@ -239,7 +239,7 @@ impl<T: ?Sized> CArc<T> {
     ///
     /// The [`RawCArc`] must be originally from [`Self::into_raw`].
     pub unsafe fn from_raw(raw: RawCArc<T>) -> Self {
-        // Safety: The [`RawCArc`] contains the output of [`Arc::into_raw`],
+        // SAFETY: The [`RawCArc`] contains the output of [`Arc::into_raw`],
         // so we can call [`Arc::from_raw`] on it.
         let owner = unsafe { raw.0.into_arc() };
         owner.into()

src/c_box.rs

@@ -70,7 +70,7 @@ unsafe impl<T: Sync + ?Sized> Sync for Unique<T> {}
 pub enum CBox<T: ?Sized> {
     Rust(Box<T>),
     C {
-        /// # Safety:
+        /// # SAFETY:
         ///
         /// * Never moved.
         /// * Valid to dereference.
@@ -103,12 +103,12 @@ impl<T: ?Sized> Drop for CBox<T> {
             Self::Rust(_) => {} // Drop normally.
             Self::C { data, free, .. } => {
                 let ptr = data.pointer.as_ptr();
-                // Safety: See below.
+                // SAFETY: See below.
                 // The [`FnFree`] won't run Rust's `fn drop`,
                 // so we have to do this ourselves first.
                 unsafe { drop_in_place(ptr) };
                 let ptr = ptr.cast();
-                // Safety: See safety docs on [`Self::data`] and [`Self::from_c`].
+                // SAFETY: See safety docs on [`Self::data`] and [`Self::from_c`].
                 unsafe { free.free(ptr) }
             }
         }
@@ -137,7 +137,7 @@ impl<T: ?Sized> CBox<T> {
     }
 
     pub fn into_pin(self) -> Pin<Self> {
-        // Safety:
+        // SAFETY:
         // If `self` is `Self::Rust`, `Box` can be pinned.
         // If `self` is `Self::C`, `data` is never moved until [`Self::drop`].
         unsafe { Pin::new_unchecked(self) }

src/disjoint_mut.rs

@@ -1002,6 +1002,7 @@ impl<V: Copy, C: AlignedByteChunk> DisjointMut<AlignedVec<V, C>> {
     }
 }
 
+#[allow(clippy::undocumented_unsafe_blocks)]
 #[test]
 fn test_overlapping_immut() {
     let mut v: DisjointMut<Vec<u8>> = Default::default();
@@ -1013,6 +1014,7 @@ fn test_overlapping_immut() {
     assert_eq!(guard1[2], guard2[0]);
 }
 
+#[allow(clippy::undocumented_unsafe_blocks)]
 #[test]
 #[cfg_attr(debug_assertions, should_panic)]
 fn test_overlapping_mut() {
@@ -1026,6 +1028,7 @@ fn test_overlapping_mut() {
     assert_eq!(guard1[2], 42);
 }
 
+#[allow(clippy::undocumented_unsafe_blocks)]
 #[cfg(debug_assertions)]
 #[test]
 fn test_pointer_write_debug() {
@@ -1052,6 +1055,7 @@ fn test_pointer_write_debug() {
 
 // Run with miri using the following command:
 // RUSTFLAGS="-C debug-assertions=off" cargo miri test
+#[allow(clippy::undocumented_unsafe_blocks)]
 #[cfg(not(debug_assertions))]
 #[test]
 fn test_pointer_write_release() {

src/filmgrain.rs

@@ -1,6 +1,4 @@
 #![deny(unsafe_op_in_unsafe_fn)]
-#![warn(clippy::all)]
-#![allow(clippy::too_many_arguments)]
 
 use crate::include::common::bitdepth::AsPrimitive;
 use crate::include::common::bitdepth::BitDepth;
@@ -288,7 +286,7 @@ unsafe extern "C" fn generate_grain_y_c_erased<BD: BitDepth>(
     data: &Dav1dFilmGrainData,
     bitdepth_max: c_int,
 ) {
-    // Safety: Casting back to the original type from the `generate_grain_y::Fn::call`.
+    // SAFETY: Casting back to the original type from the `generate_grain_y::Fn::call`.
     let buf = unsafe { &mut *buf.cast() };
     let data = &data.clone().into();
     let bd = BD::from_c(bitdepth_max);
@@ -448,7 +446,7 @@ fn generate_grain_uv_rust<BD: BitDepth>(
                         let (luma_y, luma_x) = is_sub.luma((y, x));
                         const _: () = IsSub::check_buf_index_all(&None::<GrainLut<()>>);
                         // The optimizer is not smart enough to deduce this on its own.
-                        // Safety: The above static check checks all maximum index possibilities.
+                        // SAFETY: The above static check checks all maximum index possibilities.
                         unsafe {
                             assume(luma_y < GRAIN_HEIGHT + 1 - 1);
                             assume(luma_x < GRAIN_WIDTH - 1);
@@ -492,9 +490,9 @@ unsafe extern "C" fn generate_grain_uv_c_erased<
     uv: intptr_t,
     bitdepth_max: c_int,
 ) {
-    // Safety: Casting back to the original type from the `generate_grain_uv::Fn::call`.
+    // SAFETY: Casting back to the original type from the `generate_grain_uv::Fn::call`.
     let buf = unsafe { &mut *buf.cast() };
-    // Safety: Casting back to the original type from the `generate_grain_uv::Fn::call`.
+    // SAFETY: Casting back to the original type from the `generate_grain_uv::Fn::call`.
     let buf_y = unsafe { &*buf_y.cast() };
     let data = &data.clone().into();
     let is_uv = uv != 0;
@@ -546,9 +544,9 @@ unsafe extern "C" fn fgy_32x32xn_c_erased<BD: BitDepth>(
     // SAFETY: Was passed as `FFISafe::new(_)` in `fgy_32x32xn::Fn::call`.
     let [dst_row, src_row] = [dst_row, src_row].map(|it| *unsafe { FFISafe::get(it) });
     let data = &data.clone().into();
-    // Safety: Casting back to the original type from the `fn` ptr call.
+    // SAFETY: Casting back to the original type from the `fn` ptr call.
     let scaling = unsafe { &*scaling.cast() };
-    // Safety: Casting back to the original type from the `fn` ptr call.
+    // SAFETY: Casting back to the original type from the `fn` ptr call.
     let grain_lut = unsafe { &*grain_lut.cast() };
     let bh = bh as usize;
     let row_num = row_num as usize;
@@ -883,13 +881,14 @@ unsafe extern "C" fn fguv_32x32xn_c_erased<
     src_row: *const FFISafe<Rav1dPictureDataComponentOffset>,
     luma_row: *const FFISafe<Rav1dPictureDataComponentOffset>,
 ) {
-    // SAFETY: Was passed as `FFISafe::new(_)` in `fguv_32x32xn::Fn::call`.
-    let [dst_row, src_row, luma_row] =
-        [dst_row, src_row, luma_row].map(|row| *unsafe { FFISafe::get(row) });
+    let [dst_row, src_row, luma_row] = [dst_row, src_row, luma_row].map(|row| {
+        // SAFETY: Was passed as `FFISafe::new(_)` in `fguv_32x32xn::Fn::call`.
+        *unsafe { FFISafe::get(row) }
+    });
     let data = &data.clone().into();
-    // Safety: Casting back to the original type from the `fn` ptr call.
+    // SAFETY: Casting back to the original type from the `fn` ptr call.
     let scaling = unsafe { &*scaling.cast() };
-    // Safety: Casting back to the original type from the `fn` ptr call.
+    // SAFETY: Casting back to the original type from the `fn` ptr call.
     let grain_lut = unsafe { &*grain_lut.cast() };
     let bh = bh as usize;
     let row_num = row_num as usize;

src/intra_edge.rs

@@ -344,7 +344,7 @@ impl<const SB128: bool, const N_BRANCH: usize, const N_TIP: usize>
         if cfg!(debug_assertions) {
             &edges[edge.index as usize]
         } else {
-            // Safety: Already checked in `Self::check_indices`, and `EdgeIndex`'s fields are private.
+            // SAFETY: Already checked in `Self::check_indices`, and `EdgeIndex`'s fields are private.
             unsafe { edges.get_unchecked(edge.index as usize) }
         }
     }

src/lf_mask.rs

@@ -250,7 +250,7 @@ fn mask_edges_inter(
     }
 
     for y in 0..h4 {
-        // SAFETY y < h4 and w4 - 1 < w4 so txa[0][0][y][w4 - 1] is initialized.
+        // SAFETY: y < h4 and w4 - 1 < w4 so txa[0][0][y][w4 - 1] is initialized.
         l[y] = unsafe { txa[0][0][y][w4 - 1].assume_init() };
     }
     // SAFETY: h4 - 1 < h4 and ..w4 < w4 so txa[1][0][h4 - 1][..w4] is

src/log.rs

@@ -48,7 +48,7 @@ impl fmt::Write for Dav1dLogger {
         // or the Rust API can be used instead.
         let fmt = c"%c";
         for &byte in s.as_bytes() {
-            // # Safety
+            // SAFETY:
             //
             // The first argument is `self.cookie`
             // and the rest are safe to call `printf` with,
@@ -120,7 +120,7 @@ mod marker {
     type Callback = extern "C" fn(cookie: *mut c_void, fmt: *const c_char);
 
     const fn cast(callback: Callback) -> Dav1dLoggerCallback {
-        // Safety: It should always be safe to ignore variadic args.
+        // SAFETY: It should always be safe to ignore variadic args.
         // Declaring a variadic `fn` is unstable, though, which is why we avoid that.
         unsafe { std::mem::transmute(callback) }
     }