Kill unwrap() before it kills us

.github/workflows/rust.yml

@@ -58,7 +58,7 @@ jobs:
       - uses: actions/checkout@v3
       - name: Clippy
         run: |
-          cargo clippy --workspace --all-features --all-targets -- --deny=warnings
+          cargo clippy --workspace --all-features --lib --bins -- --deny=warnings -D clippy::unwrap_used
 
   simulator:
     runs-on: blacksmith-4vcpu-ubuntu-2404

CONTRIBUTING.md

@@ -128,6 +128,14 @@ echo -1 | sudo tee /proc/sys/kernel/perf_event_paranoid
 cargo bench --bench benchmark -- --profile-time=5
 ```
 
+## Coding Style
+
+### Error handling
+
+* Turso is a library, which means we prefer error handling over crashing.
+* However, we care about correctness more than crashing, which means we use assertions liberally.
+* We don't use `unwrap()` in Turso code, except for tests. Instead, we implement error handling or -- when not possible -- use expect() to clearly communicate that this is an invariant (assertion).
+
 ## Debugging bugs
 
 ### Query execution debugging

cli/build.rs

@@ -13,15 +13,15 @@ fn main() {
     println!("cargo::rerun-if-changed=build.rs");
     println!("cargo::rerun-if-changed=manuals");
 
-    let out_dir = env::var_os("OUT_DIR").unwrap();
-    let syntax =
-        SyntaxDefinition::load_from_str(include_str!("./SQL.sublime-syntax"), false, None).unwrap();
+    let out_dir = env::var_os("OUT_DIR").expect("OUT_DIR not set by cargo");
+    let syntax = SyntaxDefinition::load_from_str(include_str!("./SQL.sublime-syntax"), false, None)
+        .expect("failed to load SQL syntax definition");
     let mut ps = SyntaxSet::new().into_builder();
     ps.add(syntax);
     let ps = ps.build();
     dump_to_uncompressed_file(
         &ps,
         Path::new(&out_dir).join("SQL_syntax_set_dump.packdump"),
     )
-    .unwrap();
+    .expect("failed to dump syntax set");
 }

core/build.rs

@@ -8,7 +8,7 @@ fn main() {
         println!("cargo::rerun-if-changed=build.rs");
     }
 
-    let out_dir = PathBuf::from(std::env::var("OUT_DIR").unwrap());
+    let out_dir = PathBuf::from(std::env::var("OUT_DIR").expect("OUT_DIR not set by cargo"));
     let built_file = out_dir.join("built.rs");
 
     built::write_built_file().expect("Failed to acquire build-time information");
@@ -19,7 +19,8 @@ fn main() {
         &built_file,
         format!(
             "{}\npub const BUILT_TIME_SQLITE: &str = \"{}\";\n",
-            fs::read_to_string(&built_file).unwrap(),
+            fs::read_to_string(&built_file)
+                .expect("built.rs should exist after built::write_built_file()"),
             sqlite_date
         ),
     )

core/ext/dynamic.rs

@@ -4,9 +4,10 @@ use crate::{
 };
 #[cfg(not(target_family = "wasm"))]
 use libloading::{Library, Symbol};
+use parking_lot::Mutex;
 use std::{
     ffi::{c_char, CString},
-    sync::{Arc, Mutex, OnceLock},
+    sync::{Arc, OnceLock},
 };
 use turso_ext::{ExtensionApi, ExtensionApiRef, ExtensionEntryPoint, ResultCode, VfsImpl};
 
@@ -52,12 +53,7 @@ impl Connection {
         let result_code = unsafe { entry(api_ptr) };
         if result_code.is_ok() {
             let extensions = get_extension_libraries();
-            extensions
-                .lock()
-                .map_err(|_| {
-                    LimboError::ExtensionError("Error locking extension libraries".to_string())
-                })?
-                .push((Arc::new(lib), api_ref));
+            extensions.lock().push((Arc::new(lib), api_ref));
             if self.is_db_initialized() {
                 self.parse_schema_rows()?;
             }
@@ -156,10 +152,7 @@ fn register_static_vfs_modules(_api: &mut ExtensionApi) {
 }
 
 pub fn add_vfs_module(name: String, vfs: Arc<VfsMod>) {
-    let mut modules = VFS_MODULES
-        .get_or_init(|| Mutex::new(Vec::new()))
-        .lock()
-        .unwrap();
+    let mut modules = VFS_MODULES.get_or_init(|| Mutex::new(Vec::new())).lock();
     if !modules.iter().any(|v| v.0 == name) {
         modules.push((name, vfs));
     }
@@ -169,7 +162,6 @@ pub fn list_vfs_modules() -> Vec<String> {
     VFS_MODULES
         .get_or_init(|| Mutex::new(Vec::new()))
         .lock()
-        .unwrap()
         .iter()
         .map(|v| v.0.clone())
         .collect()
@@ -179,6 +171,5 @@ pub fn get_vfs_modules() -> Vec<Vfs> {
     VFS_MODULES
         .get_or_init(|| Mutex::new(Vec::new()))
         .lock()
-        .unwrap()
         .clone()
 }

core/ext/vtab_xconnect.rs

@@ -42,7 +42,7 @@ pub unsafe extern "C" fn execute(
                     let args_slice = &mut std::slice::from_raw_parts_mut(args, arg_count as usize);
                     for (i, val) in args_slice.iter_mut().enumerate() {
                         stmt.bind_at(
-                            NonZeroUsize::new(i + 1).unwrap(),
+                            NonZeroUsize::new(i + 1).expect("i cannot be negative"),
                             Value::from_ffi(std::mem::take(val)).unwrap_or(Value::Null),
                         );
                     }

core/functions/datetime.rs

@@ -49,7 +49,9 @@ where
         return Value::Null;
     }
 
-    let value = values.next().unwrap();
+    let Some(value) = values.next() else {
+        return Value::Null;
+    };
     let value = value.as_value_ref();
     let format_str = if matches!(
         value,
@@ -80,11 +82,15 @@ where
 {
     let values = values.into_iter();
     if values.len() == 0 {
-        let now = parse_naive_date_time(Value::build_text("now")).unwrap();
+        let Some(now) = parse_naive_date_time(Value::build_text("now")) else {
+            return Value::Null;
+        };
         return format_dt(now, output_type, false);
     }
     let mut values = values.peekable();
-    let first = values.peek().unwrap();
+    let Some(first) = values.peek() else {
+        return Value::Null;
+    };
     if let Some(mut dt) = parse_naive_date_time(first) {
         // if successful, treat subsequent entries as modifiers
         modify_dt(&mut dt, values.skip(1), output_type)
@@ -231,18 +237,19 @@ fn apply_modifier(dt: &mut NaiveDateTime, modifier: &str, n_floor: &mut i64) ->
         }
         Modifier::StartOfMonth => {
             *dt = NaiveDate::from_ymd_opt(dt.year(), dt.month(), 1)
-                .unwrap()
-                .and_hms_opt(0, 0, 0)
-                .unwrap();
+                .and_then(|d| d.and_hms_opt(0, 0, 0))
+                .ok_or_else(|| InvalidModifier("failed to construct start of month".to_string()))?;
         }
         Modifier::StartOfYear => {
             *dt = NaiveDate::from_ymd_opt(dt.year(), 1, 1)
-                .unwrap()
-                .and_hms_opt(0, 0, 0)
-                .unwrap();
+                .and_then(|d| d.and_hms_opt(0, 0, 0))
+                .ok_or_else(|| InvalidModifier("failed to construct start of year".to_string()))?;
         }
         Modifier::StartOfDay => {
-            *dt = dt.date().and_hms_opt(0, 0, 0).unwrap();
+            *dt = dt
+                .date()
+                .and_hms_opt(0, 0, 0)
+                .ok_or_else(|| InvalidModifier("failed to construct start of day".to_string()))?;
         }
         Modifier::Weekday(day) => {
             let current_day = dt.weekday().num_days_from_sunday();
@@ -259,11 +266,18 @@ fn apply_modifier(dt: &mut NaiveDateTime, modifier: &str, n_floor: &mut i64) ->
         }
         Modifier::Utc => {
             // TODO: handle datetime('now', 'utc') no-op
-            let local_dt = chrono::Local.from_local_datetime(dt).unwrap();
+            let local_dt = chrono::Local
+                .from_local_datetime(dt)
+                .single()
+                .ok_or_else(|| {
+                    InvalidModifier("ambiguous local datetime during DST transition".to_string())
+                })?;
             *dt = local_dt.with_timezone(&Utc).naive_utc();
         }
         Modifier::Subsec => {
-            *dt = dt.with_nanosecond(dt.nanosecond()).unwrap();
+            *dt = dt
+                .with_nanosecond(dt.nanosecond())
+                .ok_or_else(|| InvalidModifier("failed to set nanoseconds".to_string()))?;
             return Ok(true);
         }
     }
@@ -482,7 +496,7 @@ fn get_date_time_from_time_value_string(value: &str) -> Option<NaiveDateTime> {
 
     // First, try to parse as date-only format
     if let Ok(date) = NaiveDate::parse_from_str(value, date_only_format) {
-        return Some(date.and_time(NaiveTime::from_hms_opt(0, 0, 0).unwrap()));
+        return NaiveTime::from_hms_opt(0, 0, 0).map(|time| date.and_time(time));
     }
 
     for format in &datetime_formats {
@@ -598,10 +612,9 @@ fn is_leap_second(dt: &NaiveDateTime) -> bool {
 
 fn get_max_datetime_exclusive() -> NaiveDateTime {
     // The maximum date in SQLite is 9999-12-31
-    NaiveDateTime::new(
-        NaiveDate::from_ymd_opt(10000, 1, 1).unwrap(),
-        NaiveTime::from_hms_milli_opt(00, 00, 00, 000).unwrap(),
-    )
+    let date = NaiveDate::from_ymd_opt(10000, 1, 1).expect("10000-01-01 is valid");
+    let time = NaiveTime::from_hms_milli_opt(00, 00, 00, 000).expect("00:00:00.000 is valid");
+    NaiveDateTime::new(date, time)
 }
 
 /// Modifier doc https://www.sqlite.org/lang_datefunc.html#modifiers
@@ -824,8 +837,15 @@ where
         return Value::Null;
     }
 
-    let start = parse_naive_date_time(values.next().unwrap());
-    let end = parse_naive_date_time(values.next().unwrap());
+    let Some(start_val) = values.next() else {
+        return Value::Null;
+    };
+    let Some(end_val) = values.next() else {
+        return Value::Null;
+    };
+
+    let start = parse_naive_date_time(start_val);
+    let end = parse_naive_date_time(end_val);
 
     match (start, end) {
         (Some(start), Some(end)) => {

core/io/clock.rs

@@ -72,15 +72,17 @@ impl std::ops::Add<Duration> for Instant {
     type Output = Instant;
 
     fn add(self, rhs: Duration) -> Self::Output {
-        self.checked_add_duration(&rhs).unwrap()
+        self.checked_add_duration(&rhs)
+            .expect("duration addition overflow")
     }
 }
 
 impl std::ops::Sub<Duration> for Instant {
     type Output = Instant;
 
     fn sub(self, rhs: Duration) -> Self::Output {
-        self.checked_sub_duration(&rhs).unwrap()
+        self.checked_sub_duration(&rhs)
+            .expect("duration subtraction underflow")
     }
 }
 

core/io/completions.rs

@@ -272,7 +272,9 @@ impl Completion {
     }
 
     pub(super) fn get_inner(&self) -> &Arc<CompletionInner> {
-        self.inner.as_ref().unwrap()
+        self.inner
+            .as_ref()
+            .expect("completion inner should be initialized")
     }
 
     pub fn needs_link(&self) -> bool {

core/io/memory.rs

@@ -59,7 +59,12 @@ impl IO for MemoryIO {
                 }),
             );
         }
-        Ok(files.get(path).unwrap().clone())
+        Ok(files
+            .get(path)
+            .ok_or(crate::LimboError::InternalError(
+                "file should exist after insert".to_string(),
+            ))?
+            .clone())
     }
     fn remove_file(&self, path: &str) -> Result<()> {
         let mut files = self.files.lock();

core/mvcc/cursor.rs

@@ -5,7 +5,7 @@ use crate::mvcc::database::{MVTableId, MvStore, Row, RowID, RowKey, RowVersionSt
 use crate::storage::btree::{BTreeCursor, BTreeKey, CursorTrait};
 use crate::translate::plan::IterationDirection;
 use crate::types::{IOResult, ImmutableRecord, RecordCursor, SeekKey, SeekOp, SeekResult};
-use crate::{return_if_io, Result};
+use crate::{return_if_io, LimboError, Result};
 use crate::{Pager, Value};
 use std::any::Any;
 use std::cell::{Ref, RefCell};
@@ -110,7 +110,9 @@ impl<Clock: LogicalClock + 'static> MvccLazyCursor<Clock> {
                     };
                     {
                         let mut record = self.get_immutable_record_or_create();
-                        let record = record.as_mut().unwrap();
+                        let record = record.as_mut().ok_or(LimboError::InternalError(
+                            "immutable record not initialized".to_string(),
+                        ))?;
                         record.invalidate();
                         record.start_serialization(&row.data);
                     }
@@ -119,7 +121,10 @@ impl<Clock: LogicalClock + 'static> MvccLazyCursor<Clock> {
                         Ref::filter_map(self.reusable_immutable_record.borrow(), |opt| {
                             opt.as_ref()
                         })
-                        .unwrap();
+                        .ok()
+                        .ok_or(LimboError::InternalError(
+                            "immutable record not initialized".to_string(),
+                        ))?;
                     Ok(IOResult::Done(Some(record_ref)))
                 }
             }
@@ -587,10 +592,21 @@ impl<Clock: LogicalClock + 'static> CursorTrait for MvccLazyCursor<Clock> {
             panic!("BTreeKey::maybe_rowid() should return Some(rowid) for table rowid keys");
         };
         let row_id = RowID::new(self.table_id, RowKey::Int(rowid));
-        let record_buf = key.get_record().unwrap().get_payload().to_vec();
+        let record_buf = key
+            .get_record()
+            .ok_or(LimboError::InternalError(
+                "BTreeKey should have a record".to_string(),
+            ))?
+            .get_payload()
+            .to_vec();
         let num_columns = match key {
             BTreeKey::IndexKey(record) => record.column_count(),
-            BTreeKey::TableRowId((_, record)) => record.as_ref().unwrap().column_count(),
+            BTreeKey::TableRowId((_, record)) => record
+                .as_ref()
+                .ok_or(LimboError::InternalError(
+                    "TableRowId should have a record".to_string(),
+                ))?
+                .column_count(),
         };
         let row = crate::mvcc::database::Row::new(row_id, record_buf, num_columns);
 
@@ -770,7 +786,7 @@ impl<Clock: LogicalClock + 'static> CursorTrait for MvccLazyCursor<Clock> {
     fn invalidate_record(&mut self) {
         self.get_immutable_record_or_create()
             .as_mut()
-            .unwrap()
+            .expect("immutable record should be initialized")
             .invalidate();
         self.record_cursor.borrow_mut().invalidate();
     }