Do not do eager formatting of error strings (#5233)

Co-authored-by: Andrew Duffy <[email protected]>

Author: gatesn

vortex-array/src/arrays/list/array.rs

@@ -5,7 +5,7 @@ use std::sync::Arc;
 
 use num_traits::AsPrimitive;
 use vortex_dtype::{DType, NativePType, match_each_integer_ptype, match_each_native_ptype};
-use vortex_error::{VortexExpect, VortexResult, vortex_bail, vortex_ensure};
+use vortex_error::{VortexExpect, VortexResult, vortex_bail, vortex_ensure, vortex_panic};
 
 use crate::arrays::{ListVTable, PrimitiveVTable};
 use crate::compute::{min_max, sub_scalar};
@@ -191,7 +191,7 @@ impl ListArray {
 
                 vortex_ensure!(
                     max_offset
-                        <= P::try_from(elements.len()).vortex_expect(&format!(
+                        <= P::try_from(elements.len()).unwrap_or_else(|_| vortex_panic!(
                             "Offsets type {} must be able to fit elements length {}",
                             <P as NativePType>::PTYPE,
                             elements.len()

vortex-array/src/compute/conformance/cast.rs

@@ -2,7 +2,7 @@
 // SPDX-FileCopyrightText: Copyright the Vortex contributors
 
 use vortex_dtype::{DType, Nullability, PType};
-use vortex_error::{VortexExpect as _, VortexUnwrap};
+use vortex_error::{VortexExpect as _, VortexUnwrap, vortex_panic};
 use vortex_scalar::Scalar;
 
 use crate::Array;
@@ -120,10 +120,12 @@ fn test_cast_to_non_nullable(array: &dyn Array) {
         }
         cast(array, &array.dtype().as_nonnullable())
             .err()
-            .vortex_expect(&format!(
-                "arrays with nulls should error when casting to non-nullable {}",
-                array,
-            ));
+            .unwrap_or_else(|| {
+                vortex_panic!(
+                    "arrays with nulls should error when casting to non-nullable {}",
+                    array,
+                )
+            });
     }
 }
 
@@ -190,14 +192,16 @@ fn test_cast_to_primitive(array: &dyn Array, target_ptype: PType, test_round_tri
             &DType::Primitive(target_ptype, array.dtype().nullability()),
         )
         .err()
-        .vortex_expect(&format!(
-            "Cast must fail because some values are out of bounds. {} {:?} {:?} {} {}",
-            target_ptype,
-            min,
-            max,
-            array,
-            array.display_values(),
-        ));
+        .unwrap_or_else(|| {
+            vortex_panic!(
+                "Cast must fail because some values are out of bounds. {} {:?} {:?} {} {}",
+                target_ptype,
+                min,
+                max,
+                array,
+                array.display_values(),
+            )
+        });
         return;
     }
 
@@ -206,11 +210,13 @@ fn test_cast_to_primitive(array: &dyn Array, target_ptype: PType, test_round_tri
         array,
         &DType::Primitive(target_ptype, array.dtype().nullability()),
     )
-    .vortex_expect(&format!(
-        "Cast must succeed because all values are within bounds. {} {}",
-        target_ptype,
-        array.display_values(),
-    ));
+    .unwrap_or_else(|e| {
+        vortex_panic!(
+            "Cast must succeed because all values are within bounds. {} {}: {e}",
+            target_ptype,
+            array.display_values(),
+        )
+    });
     assert_eq!(array.validity_mask(), casted.validity_mask());
     for i in 0..array.len().min(10) {
         let original = array.scalar_at(i);

vortex-array/src/patches.rs

@@ -85,7 +85,11 @@ impl PatchesMetadata {
     #[inline]
     pub fn chunk_offsets_dtype(&self) -> Option<DType> {
         self.chunk_offsets_ptype
-            .map(|t| PType::try_from(t).vortex_expect(&format!("invalid i32 value {t} for PType")))
+            .map(|t| {
+                PType::try_from(t)
+                    .map_err(|e| vortex_err!("invalid i32 value {t} for PType: {}", e))
+                    .vortex_expect("invalid i32 value for PType")
+            })
             .map(|ptype| DType::Primitive(ptype, NonNullable))
     }
 

vortex-duckdb/src/duckdb/table_filter.rs

@@ -8,7 +8,7 @@ use std::ptr;
 
 use cpp::duckdb_vx_table_filter;
 use num_traits::AsPrimitive;
-use vortex::error::VortexExpect;
+use vortex::error::vortex_panic;
 
 use crate::cpp::idx_t;
 use crate::duckdb::{Expression, Value, ValueRef};
@@ -47,7 +47,7 @@ impl<'a> IntoIterator for &'a TableFilterSet {
     fn into_iter(self) -> Self::IntoIter {
         Box::new((0..self.len()).map(move |i| {
             self.get(i)
-                .vortex_expect(format!("inside filter set bounds {i}").as_str())
+                .unwrap_or_else(|| vortex_panic!("inside filter set bounds {i}"))
         }))
     }
 }

vortex-error/src/lib.rs

@@ -325,7 +325,13 @@ pub trait VortexExpect {
 
     /// Returns the value of the result if it is Ok, otherwise panics with the error.
     /// Should be called only in contexts where the error condition represents a bug (programmer error).
-    fn vortex_expect(self, msg: &str) -> Self::Output;
+    ///
+    /// # `&'static` message lifetime
+    ///
+    /// The panic string argument should be a string literal, hence the `&'static` lifetime. If
+    /// you'd like to panic with a dynamic format string, consider using `unwrap_or_else` combined
+    /// with the `vortex_panic!` macro instead.
+    fn vortex_expect(self, msg: &'static str) -> Self::Output;
 }
 
 impl<T, E> VortexExpect for Result<T, E>
@@ -335,7 +341,7 @@ where
     type Output = T;
 
     #[inline(always)]
-    fn vortex_expect(self, msg: &str) -> Self::Output {
+    fn vortex_expect(self, msg: &'static str) -> Self::Output {
         self.map_err(|err| err.into())
             .unwrap_or_else(|e| vortex_panic!(e.with_context(msg.to_string())))
     }
@@ -345,7 +351,7 @@ impl<T> VortexExpect for Option<T> {
     type Output = T;
 
     #[inline(always)]
-    fn vortex_expect(self, msg: &str) -> Self::Output {
+    fn vortex_expect(self, msg: &'static str) -> Self::Output {
         self.unwrap_or_else(|| {
             let err = VortexError::AssertionFailed(
                 msg.to_string().into(),

vortex-session/src/lib.rs

@@ -77,10 +77,9 @@ impl SessionExt for VortexSession {
         Ref(self
             .0
             .get(&TypeId::of::<V>())
-            .vortex_expect(&format!(
-                "Session has not been initialized with {}",
-                type_name::<V>()
-            ))
+            .unwrap_or_else(|| {
+                vortex_panic!("Session has not been initialized with {}", type_name::<V>())
+            })
             .map(|v| {
                 (**v)
                     .as_any()
@@ -96,10 +95,9 @@ impl SessionExt for VortexSession {
         RefMut(
             self.0
                 .get_mut(&TypeId::of::<V>())
-                .vortex_expect(&format!(
-                    "Session has not been initialized with {}",
-                    type_name::<V>()
-                ))
+                .unwrap_or_else(|| {
+                    vortex_panic!("Session has not been initialized with {}", type_name::<V>())
+                })
                 .map(|v| {
                     (**v)
                         .as_any_mut()