Remove some panics from zerovec's derive

unicode-org · Feb 1, 2025 · 112a61e · 112a61e
1 parent 9920677
commit 112a61e
Show file tree

Hide file tree

Showing 4 changed files with 17 additions and 8 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,7 +9,9 @@
         - `LocaleExpander`, `LocaleDirectionality`, and `LocaleCanonicalizer` distinguish between `new_common()` and `new_extended()` constructors (unicode-org#5958)
     - `icu_segmenter`
         - Segmenters that can take a content locale now specify `_root()` on their default localeless constructors (unicode-org#5958)
-
+- Utils
+    - `zerovec`
+        - derive: Reduce number of panicky calls introduced by derive (unicode-org#6052)
 ## icu4x 2.0-beta1
 
 - Components

diff --git a/utils/zerovec/derive/src/make_varule.rs b/utils/zerovec/derive/src/make_varule.rs
@@ -414,7 +414,9 @@ fn make_encode_impl(
             let ty = &field.field.ty;
             let accessor = &field.accessor;
             quote!(
-                #[allow(clippy::indexing_slicing)] // generate_per_field_offsets produces valid indices
+                // generate_per_field_offsets produces valid indices,
+                // and we don't care about panics in Encode impls
+                #[allow(clippy::indexing_slicing)]
                 let out = &mut dst[#prev_offset_ident .. #prev_offset_ident + #size_ident];
                 let unaligned = zerovec::ule::AsULE::to_unaligned(self.#accessor);
                 let unaligned_slice = &[unaligned];
@@ -443,7 +445,10 @@ fn make_encode_impl(
                 debug_assert_eq!(self.encode_var_ule_len(), dst.len());
                 #encoders
 
-                #[allow(clippy::indexing_slicing)] // generate_per_field_offsets produces valid remainder
+
+                // generate_per_field_offsets produces valid remainders,
+                // and we don't care about panics in Encode impls
+                #[allow(clippy::indexing_slicing)]
                 let out = &mut dst[#remaining_offset..];
                 #last_encode_write
             }

diff --git a/utils/zerovec/derive/src/ule.rs b/utils/zerovec/derive/src/ule.rs
@@ -86,7 +86,8 @@ pub(crate) fn generate_ule_validators(
     utils::generate_per_field_offsets(fields, false, |field, prev_offset_ident, size_ident| {
         let ty = &field.field.ty;
         quote! {
-            #[allow(clippy::indexing_slicing)] // generate_per_field_offsets produces valid indices
+            // generate_per_field_offsets produces valid indices
+            #[allow(clippy::indexing_slicing)]
             <#ty as zerovec::ule::ULE>::validate_bytes(&bytes[#prev_offset_ident .. #prev_offset_ident + #size_ident])?;
         }
     })

diff --git a/utils/zerovec/derive/src/varule.rs b/utils/zerovec/derive/src/varule.rs
@@ -108,16 +108,17 @@ pub fn derive_impl(
                 }
                 #validators
                 debug_assert_eq!(#remaining_offset, #ule_size);
-                #[allow(clippy::indexing_slicing)] // TODO explain
-                let last_field_bytes = &bytes[#remaining_offset..];
+                // Safety: We just checked that we have enough bytes above
+                let last_field_bytes = bytes.get_unchecked(#remaining_offset..);
                 #last_field_validator
                 Ok(())
             }
             #[inline]
             unsafe fn from_bytes_unchecked(bytes: &[u8]) -> &Self {
                 // just the unsized part
-                #[allow(clippy::indexing_slicing)] // TODO explain
-                let unsized_bytes = &bytes[#ule_size..];
+                // Safety: The invariants of this function allow us to assume bytes is valid, and
+                // having at least #ule_size bytes is a validity constraint for the ULE type. 
+                let unsized_bytes = bytes.get_unchecked(#ule_size..);
                 let unsized_ref = <#unsized_field as zerovec::ule::VarULE>::from_bytes_unchecked(unsized_bytes);
                 // We should use the pointer metadata APIs here when they are stable: https://github.com/rust-lang/rust/issues/81513
                 // For now we rely on all DST metadata being a usize to extract it via a fake slice pointer