refactor: unsafe fn kernel types drop closure wrappers; narrow unreachable allow

claude · claude · commit c452fcb7fa69 · 2026-07-14T06:12:50.000Z
Make the per-site kernel type an unsafe fn pointer so unsafe #[target_feature] kernels coerce directly by name in the selector - no |i, o| unsafe { ... } closure wrappers. Safe kernels (scalar, NEON wrapper fns) coerce to unsafe fn pointers implicitly. The single dispatched call is wrapped in one narrow unsafe block whose SAFETY comment states the selector probed the required features. Move #[allow(unreachable_code)] from the enclosing function down onto the portable-default tail block itself, so it covers exactly the expression that is unreachable on aarch64 and nothing else. Verified with cargo check --target aarch64-unknown-linux-gnu on both crates: the aarch64 selector arms compile and the narrowed allow suppresses the warning with no other diagnostics. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DUFxXZLv4xxEqEYcoGWhfJ
diff --git a/encodings/fastlanes/src/bit_transpose/mod.rs b/encodings/fastlanes/src/bit_transpose/mod.rs
@@ -31,7 +31,7 @@ pub use validity::*;
 use vortex_buffer::CpuKernel;
 
 /// Signature shared by all 1024-bit transpose kernels.
-type TransposeKernel = fn(&[u8; 128], &mut [u8; 128]);
+type TransposeKernel = unsafe fn(&[u8; 128], &mut [u8; 128]);
 
 /// Base indices for the first 64 output bytes (lanes 0-7).
 /// Each entry indicates the starting input byte index for that output byte group.
@@ -50,63 +50,65 @@ const TRANSPOSE_8X8: u64 = 0x0000_0000_F0F0_F0F0;
 /// Transpose 1024-bits into FastLanes layout.
 ///
 /// Dispatch to the best available implementation, selected once on first call.
-// The aarch64 arm returns unconditionally (NEON needs no probe), making the portable
-// tail unreachable there.
-#[allow(unreachable_code)]
 #[inline]
 pub fn transpose_bits(input: &[u8; 128], output: &mut [u8; 128]) {
     static KERNEL: CpuKernel<TransposeKernel> = CpuKernel::new();
-    KERNEL.get(|| {
+    let kernel = KERNEL.get(|| {
         #[cfg(target_arch = "x86_64")]
         {
             // VBMI is fastest
             if x86::has_vbmi() {
-                // SAFETY: the probe above confirms VBMI.
-                return |input, output| unsafe { x86::transpose_bits_vbmi(input, output) };
+                return x86::transpose_bits_vbmi;
             }
             if x86::has_bmi2() {
-                // SAFETY: the probe above confirms BMI2.
-                return |input, output| unsafe { x86::transpose_bits_bmi2(input, output) };
+                return x86::transpose_bits_bmi2;
             }
         }
+        // NEON is architecturally guaranteed on aarch64, so it needs no probe.
         #[cfg(target_arch = "aarch64")]
-        return |input, output| {
-            // SAFETY: NEON is architecturally guaranteed on aarch64.
-            unsafe { aarch64::transpose_bits_neon(input, output) }
-        };
-        scalar::transpose_bits_scalar
-    })(input, output)
+        return aarch64::transpose_bits_neon;
+        // The aarch64 arm above returns unconditionally, making this portable default
+        // unreachable there.
+        #[allow(unreachable_code)]
+        {
+            scalar::transpose_bits_scalar
+        }
+    });
+    // SAFETY: the selector only returns kernels that are safe or whose required CPU
+    // features were probed before selection.
+    unsafe { kernel(input, output) }
 }
 
 /// Untranspose 1024-bits from FastLanes layout.
 ///
 /// Dispatch untranspose to the best available implementation, selected once on first call.
-// The aarch64 arm returns unconditionally (NEON needs no probe), making the portable
-// tail unreachable there.
-#[allow(unreachable_code)]
 #[inline]
 pub fn untranspose_bits(input: &[u8; 128], output: &mut [u8; 128]) {
     static KERNEL: CpuKernel<TransposeKernel> = CpuKernel::new();
-    KERNEL.get(|| {
+    let kernel = KERNEL.get(|| {
         #[cfg(target_arch = "x86_64")]
         {
             // VBMI is fastest
             if x86::has_vbmi() {
-                // SAFETY: the probe above confirms VBMI.
-                return |input, output| unsafe { x86::untranspose_bits_vbmi(input, output) };
+                return x86::untranspose_bits_vbmi;
             }
             if x86::has_bmi2() {
-                // SAFETY: the probe above confirms BMI2.
-                return |input, output| unsafe { x86::untranspose_bits_bmi2(input, output) };
+                return x86::untranspose_bits_bmi2;
             }
         }
+        // NEON is architecturally guaranteed on aarch64, so it needs no probe.
         #[cfg(target_arch = "aarch64")]
-        return |input, output| {
-            // SAFETY: NEON is architecturally guaranteed on aarch64.
-            unsafe { aarch64::untranspose_bits_neon(input, output) }
-        };
-        scalar::untranspose_bits_scalar
-    })(input, output)
+        return aarch64::untranspose_bits_neon;
+        // The aarch64 arm above returns unconditionally, making this portable default
+        // unreachable there.
+        #[allow(unreachable_code)]
+        {
+            scalar::untranspose_bits_scalar
+        }
+    });
+    // SAFETY: the selector only returns kernels that are safe or whose required CPU
+    // features were probed before selection.
+    unsafe { kernel(input, output) }
 }
 
 #[cfg(test)]
diff --git a/vortex-buffer/src/bit/select.rs b/vortex-buffer/src/bit/select.rs
@@ -83,28 +83,30 @@ pub fn bit_select(bytes: &[u8], offset: usize, len: usize, nth: usize) -> Option
 ///
 /// If `chunk_index < chunks.len()`, the target bit is inside that chunk and `remaining`
 /// is the rank *within* that chunk. Otherwise all chunks were consumed.
-type ScanChunks = fn(&[[u8; 64]], usize, usize) -> (usize, usize, usize);
+type ScanChunks = unsafe fn(&[[u8; 64]], usize, usize) -> (usize, usize, usize);
 
-// The aarch64 arm returns unconditionally (NEON needs no probe), making the portable
-// tail unreachable there.
-#[allow(unreachable_code)]
 #[inline]
 fn scan_chunks(chunks: &[[u8; 64]], remaining: usize, pos: usize) -> (usize, usize, usize) {
     static KERNEL: CpuKernel<ScanChunks> = CpuKernel::new();
-    KERNEL.get(|| {
+    let kernel = KERNEL.get(|| {
         #[cfg(target_arch = "x86_64")]
         {
             if is_x86_feature_detected!("avx512f") && is_x86_feature_detected!("avx512vpopcntdq") {
-                // SAFETY: the probe above confirms the required target features.
-                return |chunks, remaining, pos| unsafe {
-                    scan_chunks_avx512_vpopcnt(chunks, remaining, pos)
-                };
+                return scan_chunks_avx512_vpopcnt;
             }
         }
         #[cfg(target_arch = "aarch64")]
         return scan_chunks_neon;
-        scan_chunks_scalar
-    })(chunks, remaining, pos)
+        // The aarch64 arm above returns unconditionally (NEON needs no probe), making
+        // this portable default unreachable there.
+        #[allow(unreachable_code)]
+        {
+            scan_chunks_scalar
+        }
+    });
+    // SAFETY: the selector only returns kernels that are safe or whose required CPU
+    // features were probed before selection.
+    unsafe { kernel(chunks, remaining, pos) }
 }
 
 #[cfg(target_arch = "aarch64")]
@@ -282,25 +284,27 @@ fn scan_words_scalar(
 
 // ── In-chunk select ─────────────────────────────────────────────────────
 
-type SelectInChunk = fn(&[u8; 64], usize) -> usize;
+type SelectInChunk = unsafe fn(&[u8; 64], usize) -> usize;
 
 /// Position of the `nth` set bit inside a 64-byte chunk (0-indexed).
 #[inline]
 fn select_in_chunk(chunk: &[u8; 64], nth: usize) -> usize {
     static KERNEL: CpuKernel<SelectInChunk> = CpuKernel::new();
-    KERNEL.get(|| {
+    let kernel = KERNEL.get(|| {
         #[cfg(target_arch = "x86_64")]
         {
             if is_x86_feature_detected!("avx512f")
                 && is_x86_feature_detected!("avx512vpopcntdq")
                 && is_x86_feature_detected!("avx512vbmi2")
             {
-                // SAFETY: the probe above confirms the required target features.
-                return |chunk, nth| unsafe { select_in_chunk_vbmi2(chunk, nth) };
+                return select_in_chunk_vbmi2;
             }
         }
         select_in_chunk_scalar
-    })(chunk, nth)
+    });
+    // SAFETY: the selector only returns kernels that are safe or whose required CPU
+    // features were probed before selection.
+    unsafe { kernel(chunk, nth) }
 }
 
 #[cfg(target_arch = "x86_64")]
@@ -364,22 +368,24 @@ fn count_ones_chunk(chunk: &[u8; 64]) -> usize {
 
 // ── In-word select ──────────────────────────────────────────────────────
 
-type SelectInWord = fn(u64, usize) -> usize;
+type SelectInWord = unsafe fn(u64, usize) -> usize;
 
 /// Position of the `nth` set bit inside a u64 (0-indexed, little-endian bit order).
 #[inline]
 fn select_in_word(word: u64, nth: usize) -> usize {
     static KERNEL: CpuKernel<SelectInWord> = CpuKernel::new();
-    KERNEL.get(|| {
+    let kernel = KERNEL.get(|| {
         #[cfg(target_arch = "x86_64")]
         {
             if is_x86_feature_detected!("bmi2") {
-                // SAFETY: the probe above confirms the required target feature.
-                return |word, nth| unsafe { select_in_word_bmi2(word, nth) };
+                return select_in_word_bmi2;
             }
         }
         select_in_word_scalar
-    })(word, nth)
+    });
+    // SAFETY: the selector only returns kernels that are safe or whose required CPU
+    // features were probed before selection.
+    unsafe { kernel(word, nth) }
 }
 
 /// BMI2: deposit a single bit at the nth set-bit position, then count trailing zeros.
diff --git a/vortex-buffer/src/dispatch.rs b/vortex-buffer/src/dispatch.rs
@@ -17,8 +17,13 @@
 //! * The **portable default** is the plain tail expression. Because the architecture
 //!   arms return early, no `#[cfg(not(any(...)))]` negation is ever needed. An arm
 //!   that returns unconditionally (e.g. NEON, architecturally guaranteed on aarch64)
-//!   makes the tail unreachable on that architecture — silence it with
-//!   `#[allow(unreachable_code)]` on the enclosing function.
+//!   makes the tail unreachable on that architecture — wrap just the tail in an
+//!   `#[allow(unreachable_code)]` block.
+//!
+//! When kernels are `unsafe` `#[target_feature]` functions, make `F` an
+//! `unsafe fn(...)` pointer type: the bare kernel names then coerce directly (no
+//! closure wrappers), and the one dispatched call is wrapped in `unsafe` with a
+//! SAFETY comment stating that the selector probed the required features.
 //!
 //! Races are benign: the slot only ever holds valid function pointers of the same
 //! type, and every candidate must compute the same result.
