From 8cfd9968fa9ef79777ac2e7dfcef8f1ed618b78b Mon Sep 17 00:00:00 2001 From: Leo Kettmeir Date: Thu, 17 Oct 2024 12:51:33 -0700 Subject: [PATCH] refactor(ext/napi): use concrete error types (#26186) --- Cargo.lock | 1 + ext/napi/Cargo.toml | 1 + ext/napi/lib.rs | 45 ++++++++++++++++++++++++++------------------- runtime/errors.rs | 11 +++++++++++ 4 files changed, 39 insertions(+), 19 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 205804abe4..ae4501c8dc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1772,6 +1772,7 @@ dependencies = [ "deno_core", "deno_permissions", "libloading 0.7.4", + "thiserror", ] [[package]] diff --git a/ext/napi/Cargo.toml b/ext/napi/Cargo.toml index 5405d6280c..68d281ccc6 100644 --- a/ext/napi/Cargo.toml +++ b/ext/napi/Cargo.toml @@ -17,3 +17,4 @@ path = "lib.rs" deno_core.workspace = true deno_permissions.workspace = true libloading = { version = "0.7" } +thiserror.workspace = true diff --git a/ext/napi/lib.rs b/ext/napi/lib.rs index 4500c66fd4..0b2b3eb5e7 100644 --- a/ext/napi/lib.rs +++ b/ext/napi/lib.rs @@ -6,8 +6,6 @@ #![deny(clippy::missing_safety_doc)] use core::ptr::NonNull; -use deno_core::error::type_error; -use deno_core::error::AnyError; use deno_core::op2; use deno_core::parking_lot::RwLock; use deno_core::url::Url; @@ -20,6 +18,18 @@ use std::path::PathBuf; use std::rc::Rc; use std::thread_local; +#[derive(Debug, thiserror::Error)] +pub enum NApiError { + #[error("Invalid path")] + InvalidPath, + #[error(transparent)] + LibLoading(#[from] libloading::Error), + #[error("Unable to find register Node-API module at {}", .0.display())] + ModuleNotFound(PathBuf), + #[error(transparent)] + Permission(deno_core::error::AnyError), +} + #[cfg(unix)] use libloading::os::unix::*; @@ -482,14 +492,20 @@ deno_core::extension!(deno_napi, pub trait NapiPermissions { #[must_use = "the resolved return value to mitigate time-of-check to time-of-use issues"] - fn check(&mut self, path: &str) -> std::result::Result; + fn check( + &mut self, + path: &str, + ) -> Result; } // NOTE(bartlomieju): for now, NAPI uses `--allow-ffi` flag, but that might // change in the future. impl NapiPermissions for deno_permissions::PermissionsContainer { #[inline(always)] - fn check(&mut self, path: &str) -> Result { + fn check( + &mut self, + path: &str, + ) -> Result { deno_permissions::PermissionsContainer::check_ffi(self, path) } } @@ -512,7 +528,7 @@ fn op_napi_open( global: v8::Local<'scope, v8::Object>, buffer_constructor: v8::Local<'scope, v8::Function>, report_error: v8::Local<'scope, v8::Function>, -) -> std::result::Result, AnyError> +) -> Result, NApiError> where NP: NapiPermissions + 'static, { @@ -521,7 +537,7 @@ where let (async_work_sender, cleanup_hooks, external_ops_tracker, path) = { let mut op_state = op_state.borrow_mut(); let permissions = op_state.borrow_mut::(); - let path = permissions.check(&path)?; + let path = permissions.check(&path).map_err(NApiError::Permission)?; let napi_state = op_state.borrow::(); ( op_state.borrow::().clone(), @@ -540,7 +556,7 @@ where let type_tag = v8::Global::new(scope, type_tag); let url_filename = - Url::from_file_path(&path).map_err(|_| type_error("Invalid path"))?; + Url::from_file_path(&path).map_err(|_| NApiError::InvalidPath)?; let env_shared = EnvShared::new(napi_wrap, type_tag, format!("{url_filename}\0")); @@ -565,17 +581,11 @@ where // SAFETY: opening a DLL calls dlopen #[cfg(unix)] - let library = match unsafe { Library::open(Some(&path), flags) } { - Ok(lib) => lib, - Err(e) => return Err(type_error(e.to_string())), - }; + let library = unsafe { Library::open(Some(&path), flags) }?; // SAFETY: opening a DLL calls dlopen #[cfg(not(unix))] - let library = match unsafe { Library::load_with_flags(&path, flags) } { - Ok(lib) => lib, - Err(e) => return Err(type_error(e.to_string())), - }; + let library = unsafe { Library::load_with_flags(&path, flags) }?; let maybe_module = MODULE_TO_REGISTER.with(|cell| { let mut slot = cell.borrow_mut(); @@ -610,10 +620,7 @@ where // SAFETY: we are going blind, calling the register function on the other side. unsafe { init(env_ptr, exports.into()) } } else { - return Err(type_error(format!( - "Unable to find register Node-API module at {}", - path.display() - ))); + return Err(NApiError::ModuleNotFound(path)); }; let exports = maybe_exports.unwrap_or(exports.into()); diff --git a/runtime/errors.rs b/runtime/errors.rs index 99de6beb04..935f62d264 100644 --- a/runtime/errors.rs +++ b/runtime/errors.rs @@ -26,6 +26,7 @@ use deno_ffi::StaticError; use deno_kv::KvCheckError; use deno_kv::KvError; use deno_kv::KvMutationError; +use deno_napi::NApiError; use deno_net::ops::NetError; use deno_tls::TlsError; use deno_web::BlobError; @@ -174,6 +175,15 @@ pub fn get_nix_error_class(error: &nix::Error) -> &'static str { } } +fn get_napi_error_class(e: &NApiError) -> &'static str { + match e { + NApiError::InvalidPath + | NApiError::LibLoading(_) + | NApiError::ModuleNotFound(_) => "TypeError", + NApiError::Permission(e) => get_error_class_name(e).unwrap_or("Error"), + } +} + fn get_web_error_class(e: &WebError) -> &'static str { match e { WebError::Base64Decode => "DOMExceptionInvalidCharacterError", @@ -443,6 +453,7 @@ pub fn get_error_class_name(e: &AnyError) -> Option<&'static str> { deno_core::error::get_custom_error_class(e) .or_else(|| deno_webgpu::error::get_error_class_name(e)) .or_else(|| deno_websocket::get_network_error_class_name(e)) + .or_else(|| e.downcast_ref::().map(get_napi_error_class)) .or_else(|| e.downcast_ref::().map(get_web_error_class)) .or_else(|| { e.downcast_ref::()