Merge 'Kill unwrap() calls in VDBE module' from Pekka Enberg

Reviewed-by: Jussi Saurio <[email protected]>

Closes #4014

Author: penberg

core/vdbe/builder.rs

@@ -365,9 +365,10 @@ impl ProgramBuilder {
     ) -> crate::Result<usize> {
         tracing::debug!("alloc cursor: {:?} {:?}", key, index.index_method.is_some());
         let module = index.index_method.as_ref();
-        if module.is_some_and(|m| !m.definition().backing_btree) {
-            let module = module.unwrap().clone();
-            return Ok(self._alloc_cursor_id(key, CursorType::IndexMethod(module)));
+        if let Some(m) = module {
+            if !m.definition().backing_btree {
+                return Ok(self._alloc_cursor_id(key, CursorType::IndexMethod(m.clone())));
+            }
         }
         Ok(self._alloc_cursor_id(key, CursorType::BTreeIndex(index.clone())))
     }
@@ -544,11 +545,11 @@ impl ProgramBuilder {
         });
         for resolved_offset in self.label_to_resolved_offset.iter_mut() {
             if let Some((old_offset, target)) = resolved_offset {
-                let new_offset = self
-                    .insns
-                    .iter()
-                    .position(|(_, index)| *old_offset == *index as u32)
-                    .unwrap() as u32;
+                let new_offset =
+                    self.insns
+                        .iter()
+                        .position(|(_, index)| *old_offset == *index as u32)
+                        .expect("instruction offset must exist") as u32;
                 *resolved_offset = Some((new_offset, *target));
             }
         }
@@ -1064,7 +1065,7 @@ impl ProgramBuilder {
     }
 
     pub fn emit_column_or_rowid(&mut self, cursor_id: CursorID, column: usize, out: usize) {
-        let (_, cursor_type) = self.cursor_ref.get(cursor_id).unwrap();
+        let (_, cursor_type) = self.cursor_ref.get(cursor_id).expect("cursor_id is valid");
         if let CursorType::BTreeTable(btree) = cursor_type {
             let column_def = btree
                 .columns
@@ -1084,7 +1085,7 @@ impl ProgramBuilder {
     }
 
     fn emit_column(&mut self, cursor_id: CursorID, column: usize, out: usize) {
-        let (_, cursor_type) = self.cursor_ref.get(cursor_id).unwrap();
+        let (_, cursor_type) = self.cursor_ref.get(cursor_id).expect("cursor_id is valid");
 
         use crate::translate::expr::sanitize_string;
 
@@ -1114,9 +1115,10 @@ impl ProgramBuilder {
                         .chunks_exact(2)
                         .map(|pair| {
                             // We assume that sqlite3-parser has already validated that
-                            // the input is valid hex string, thus unwrap is safe.
-                            let hex_byte = std::str::from_utf8(pair).unwrap();
-                            u8::from_str_radix(hex_byte, 16).unwrap()
+                            // the input is valid hex string, thus expect is safe.
+                            let hex_byte =
+                                std::str::from_utf8(pair).expect("parser validated hex string");
+                            u8::from_str_radix(hex_byte, 16).expect("parser validated hex digit")
                         })
                         .collect(),
                 ),

core/vdbe/execute.rs

@@ -561,7 +561,7 @@ pub fn insn_to_row(
                         name
                     }
                     CursorType::BTreeIndex(index) => {
-                        let name = &index.columns.get(*column).unwrap().name;
+                        let name = &index.columns.get(*column).expect("column index out of bounds").name;
                         Some(name)
                     }
                     CursorType::MaterializedView(table, _) => {
@@ -571,7 +571,7 @@ pub fn insn_to_row(
                     CursorType::Pseudo(_) => None,
                     CursorType::Sorter => None,
                     CursorType::IndexMethod(..) => None,
-                    CursorType::VirtualTable(v) => v.columns.get(*column).unwrap().name.as_ref(),
+                    CursorType::VirtualTable(v) => v.columns.get(*column).expect("column index out of bounds").name.as_ref(),
                 };
                 (
                     "Column",
@@ -1017,8 +1017,8 @@ pub fn insn_to_row(
                             SortOrder::Asc => "",
                             SortOrder::Desc => "-",
                         };
-                        if collation.is_some() {
-                            format!("{sign}{}", collation.unwrap())
+                        if let Some(coll) = collation {
+                            format!("{sign}{coll}")
                         } else {
                             format!("{sign}B")
                         }

core/vdbe/explain.rs

@@ -1276,7 +1276,9 @@ const fn get_insn_virtual_table() -> [InsnFunction; InsnVariants::COUNT] {
 
     let mut insn = 0;
     while insn < InsnVariants::COUNT {
-        result[insn] = InsnVariants::from_repr(insn as u8).unwrap().to_function();
+        result[insn] = InsnVariants::from_repr(insn as u8)
+            .expect("insn index should be valid within COUNT")
+            .to_function();
         insn += 1;
     }
 

core/vdbe/insn.rs

@@ -60,7 +60,7 @@ fn construct_like_regex_with_escape(pattern: &str, escape: char) -> Regex {
         .case_insensitive(true)
         .dot_matches_new_line(true)
         .build()
-        .unwrap()
+        .expect("constructed LIKE regex pattern should be valid")
 }
 
 // Implements GLOB pattern matching. Caches the constructed regex if a cache is provided
@@ -173,7 +173,8 @@ fn construct_glob_regex(pattern: &str) -> Result<Regex, LimboError> {
     regex_pattern.push('$');
 
     if bracket_closed {
-        Ok(Regex::new(&regex_pattern).unwrap())
+        Regex::new(&regex_pattern)
+            .map_err(|e| LimboError::InternalError(format!("invalid GLOB regex pattern: {e}")))
     } else {
         Err(LimboError::Constraint(
             "blob pattern is not closed".to_string(),

core/vdbe/likeop.rs

@@ -997,7 +997,7 @@ impl Program {
                         .connection
                         .view_transaction_states
                         .get(view_name)
-                        .unwrap()
+                        .expect("view should have transaction state")
                         .get_table_deltas();
 
                     let schema = self.connection.schema.read();
@@ -1067,7 +1067,7 @@ impl Program {
                     let Some(tx_id) = conn.get_mv_tx_id() else {
                         return Ok(IOResult::Done(()));
                     };
-                    let state_machine = mv_store.commit_tx(tx_id, &conn).unwrap();
+                    let state_machine = mv_store.commit_tx(tx_id, &conn)?;
                     program_state.commit_state = CommitState::CommitingMvcc { state_machine };
                 }
                 let CommitState::CommitingMvcc { state_machine } = &mut program_state.commit_state
@@ -1314,7 +1314,12 @@ impl<'a> FromValueRow<'a> for &'a Value {
 
 impl Row {
     pub fn get<'a, T: FromValueRow<'a> + 'a>(&'a self, idx: usize) -> Result<T> {
-        let value = unsafe { self.values.add(idx).as_ref().unwrap() };
+        let value = unsafe {
+            self.values
+                .add(idx)
+                .as_ref()
+                .expect("row value pointer should be valid")
+        };
         let value = match value {
             Register::Value(value) => value,
             _ => unreachable!("a row should be formed of values only"),
@@ -1323,7 +1328,12 @@ impl Row {
     }
 
     pub fn get_value(&self, idx: usize) -> &Value {
-        let value = unsafe { self.values.add(idx).as_ref().unwrap() };
+        let value = unsafe {
+            self.values
+                .add(idx)
+                .as_ref()
+                .expect("row value pointer should be valid")
+        };
         match value {
             Register::Value(value) => value,
             _ => unreachable!("a row should be formed of values only"),

core/vdbe/mod.rs

@@ -1,9 +1,10 @@
 use turso_parser::ast::SortOrder;
 
+use parking_lot::RwLock;
 use std::cmp::{Eq, Ord, Ordering, PartialEq, PartialOrd, Reverse};
 use std::collections::BinaryHeap;
 use std::rc::Rc;
-use std::sync::{atomic, Arc, RwLock};
+use std::sync::{atomic, Arc};
 use tempfile;
 
 use crate::types::IOCompletions;
@@ -158,10 +159,7 @@ impl Sorter {
                 SortState::InitHeap => {
                     turso_assert!(
                         !self.chunks.iter().any(|chunk| {
-                            matches!(
-                                *chunk.io_state.read().unwrap(),
-                                SortedChunkIOState::WaitingForWrite
-                            )
+                            matches!(*chunk.io_state.read(), SortedChunkIOState::WaitingForWrite)
                         }),
                         "chunks should been written"
                     );
@@ -255,7 +253,7 @@ impl Sorter {
                 // Make sure all chunks read at least one record into their buffer.
                 turso_assert!(
                     !self.chunks.iter().any(|chunk| matches!(
-                        *chunk.io_state.read().unwrap(),
+                        *chunk.io_state.read(),
                         SortedChunkIOState::WaitingForRead
                     )),
                     "chunks should have been read"
@@ -332,11 +330,12 @@ impl Sorter {
             None => {
                 let temp_dir = tempfile::tempdir()?;
                 let chunk_file_path = temp_dir.as_ref().join("chunk_file");
-                let chunk_file = self.io.open_file(
-                    chunk_file_path.to_str().unwrap(),
-                    OpenFlags::Create,
-                    false,
-                )?;
+                let chunk_file_path_str = chunk_file_path.to_str().ok_or_else(|| {
+                    LimboError::InternalError("temp file path is not valid UTF-8".to_string())
+                })?;
+                let chunk_file =
+                    self.io
+                        .open_file(chunk_file_path_str, OpenFlags::Create, false)?;
                 self.temp_file = Some(TempFile {
                     _temp_dir: temp_dir,
                     file: chunk_file.clone(),
@@ -438,7 +437,7 @@ impl SortedChunk {
                     }
 
                     if self.records.is_empty() {
-                        let mut buffer_ref = self.buffer.write().unwrap();
+                        let mut buffer_ref = self.buffer.write();
                         let buffer = buffer_ref.as_mut_slice();
                         let mut buffer_offset = 0;
                         while buffer_offset < buffer_len {
@@ -449,8 +448,7 @@ impl SortedChunk {
                                         (record_size as usize, bytes_read)
                                     }
                                     Err(LimboError::Corrupt(_))
-                                        if *self.io_state.read().unwrap()
-                                            != SortedChunkIOState::ReadEOF =>
+                                        if *self.io_state.read() != SortedChunkIOState::ReadEOF =>
                                     {
                                         // Failed to decode a partial varint.
                                         break;
@@ -460,7 +458,7 @@ impl SortedChunk {
                                     }
                                 };
                             if record_size > buffer_len - (buffer_offset + bytes_read) {
-                                if *self.io_state.read().unwrap() == SortedChunkIOState::ReadEOF {
+                                if *self.io_state.read() == SortedChunkIOState::ReadEOF {
                                     crate::bail_corrupt_error!("Incomplete record");
                                 }
                                 break;
@@ -489,13 +487,13 @@ impl SortedChunk {
                     self.next_state = NextState::Finish;
                     // This check is done to see if we need to read more from the chunk before popping the record
                     if self.records.len() == 1
-                        && *self.io_state.read().unwrap() != SortedChunkIOState::ReadEOF
+                        && *self.io_state.read() != SortedChunkIOState::ReadEOF
                     {
                         // We've consumed the last record. Read more payload into the buffer.
                         if self.chunk_size - self.total_bytes_read.load(atomic::Ordering::SeqCst)
                             == 0
                         {
-                            *self.io_state.write().unwrap() = SortedChunkIOState::ReadEOF;
+                            *self.io_state.write() = SortedChunkIOState::ReadEOF;
                         } else {
                             let c = self.read()?;
                             if !c.succeeded() {
@@ -513,9 +511,9 @@ impl SortedChunk {
     }
 
     fn read(&mut self) -> Result<Completion> {
-        *self.io_state.write().unwrap() = SortedChunkIOState::WaitingForRead;
+        *self.io_state.write() = SortedChunkIOState::WaitingForRead;
 
-        let read_buffer_size = self.buffer.read().unwrap().len() - self.buffer_len();
+        let read_buffer_size = self.buffer.read().len() - self.buffer_len();
         let read_buffer_size = read_buffer_size
             .min(self.chunk_size - self.total_bytes_read.load(atomic::Ordering::SeqCst));
 
@@ -535,12 +533,12 @@ impl SortedChunk {
 
             let bytes_read = bytes_read as usize;
             if bytes_read == 0 {
-                *chunk_io_state_copy.write().unwrap() = SortedChunkIOState::ReadEOF;
+                *chunk_io_state_copy.write() = SortedChunkIOState::ReadEOF;
                 return;
             }
-            *chunk_io_state_copy.write().unwrap() = SortedChunkIOState::ReadComplete;
+            *chunk_io_state_copy.write() = SortedChunkIOState::ReadComplete;
 
-            let mut stored_buf_ref = stored_buffer_copy.write().unwrap();
+            let mut stored_buf_ref = stored_buffer_copy.write();
             let stored_buf = stored_buf_ref.as_mut_slice();
             let mut stored_buf_len = stored_buffer_len_copy.load(atomic::Ordering::SeqCst);
 
@@ -566,8 +564,8 @@ impl SortedChunk {
         record_size_lengths: Vec<usize>,
         chunk_size: usize,
     ) -> Result<Completion> {
-        assert!(*self.io_state.read().unwrap() == SortedChunkIOState::None);
-        *self.io_state.write().unwrap() = SortedChunkIOState::WaitingForWrite;
+        assert!(*self.io_state.read() == SortedChunkIOState::None);
+        *self.io_state.write() = SortedChunkIOState::WaitingForWrite;
         self.chunk_size = chunk_size;
 
         let buffer = Buffer::new_temporary(self.chunk_size);
@@ -592,7 +590,7 @@ impl SortedChunk {
             let Ok(bytes_written) = res else {
                 return;
             };
-            *chunk_io_state_copy.write().unwrap() = SortedChunkIOState::WriteComplete;
+            *chunk_io_state_copy.write() = SortedChunkIOState::WriteComplete;
             let buf_len = buffer_ref_copy.len();
             if bytes_written < buf_len as i32 {
                 tracing::error!("wrote({bytes_written}) less than expected({buf_len})");
@@ -684,7 +682,9 @@ impl Ord for SortableImmutableRecord {
                     // SAFETY: these were checked to be valid UTF-8 on construction.
                     &left, &right,
                 ),
-                _ => this_key_value.partial_cmp(&other_key_value).unwrap(),
+                _ => this_key_value
+                    .partial_cmp(&other_key_value)
+                    .expect("sorter values of the same column should be comparable"),
             };
             if !cmp.is_eq() {
                 return match column_order {

core/vdbe/sorter.rs

@@ -205,7 +205,10 @@ impl Value {
 
         // Convert the word to lowercase for consistent lookups
         let word = word.to_lowercase();
-        let first_letter = word.chars().next().unwrap();
+        let first_letter = word
+            .chars()
+            .next()
+            .expect("word should not be empty after validation");
 
         // Remove all occurrences of 'h' and 'w' except the first letter
         let code: String = word
@@ -559,7 +562,7 @@ impl Value {
         }
 
         let f: f64 = crate::numeric::str_to_f64(format!("{f:.precision$}"))
-            .unwrap()
+            .expect("formatted float should always parse successfully")
             .into();
 
         Value::Float(f)
@@ -987,7 +990,10 @@ impl Value {
             return Value::Null;
         }
 
-        let separator = match registers.next().unwrap() {
+        let separator = match registers
+            .next()
+            .expect("registers should have at least one element after length check")
+        {
             Value::Null | Value::Blob(_) => return Value::Null,
             v => format!("{v}"),
         };
@@ -1040,7 +1046,7 @@ pub fn construct_like_regex(pattern: &str) -> Regex {
         .case_insensitive(true)
         .dot_matches_new_line(true)
         .build()
-        .unwrap()
+        .expect("constructed LIKE regex pattern should be valid")
 }
 
 #[cfg(test)]