From 23882271b261de262ce90cd411e75f9b7c303e12 Mon Sep 17 00:00:00 2001 From: David Herman Date: Fri, 22 Nov 2024 10:44:44 -0800 Subject: [PATCH 1/6] chore(neon): fix Clippy lints exposed by #1065 --- crates/neon/src/sys/async_work.rs | 6 ++-- crates/neon/src/sys/no_panic.rs | 26 ++++++++--------- crates/neon/src/sys/object.rs | 47 ++++++++++++++++--------------- crates/neon/src/sys/tsfn.rs | 4 +-- 4 files changed, 43 insertions(+), 40 deletions(-) diff --git a/crates/neon/src/sys/async_work.rs b/crates/neon/src/sys/async_work.rs index 10cec23ea..db668d507 100644 --- a/crates/neon/src/sys/async_work.rs +++ b/crates/neon/src/sys/async_work.rs @@ -74,7 +74,7 @@ pub unsafe fn schedule( Ok(()) => {} status => { // If queueing failed, delete the work to prevent a leak - napi::delete_async_work(env, *work); + napi::delete_async_work(env, *work).unwrap(); status.unwrap() } } @@ -150,7 +150,9 @@ unsafe extern "C" fn call_complete(env: Env, status: napi::Status, data .. } = *Box::>::from_raw(data.cast()); - napi::delete_async_work(env, work); + if napi::delete_async_work(env, work).is_err() { + panic!("failed to delete async work"); + } BOUNDARY.catch_failure(env, None, move |env| { // `unwrap` is okay because `call_complete` should be called exactly once diff --git a/crates/neon/src/sys/no_panic.rs b/crates/neon/src/sys/no_panic.rs index f51dd9216..516deaba8 100644 --- a/crates/neon/src/sys/no_panic.rs +++ b/crates/neon/src/sys/no_panic.rs @@ -235,9 +235,8 @@ unsafe fn error_from_panic(env: Env, panic: Panic) -> Local { unsafe fn set_property(env: Env, object: Local, key: &str, value: Local) { let key = create_string(env, key); - match napi::set_property(env, object, key, value) { - Err(_) => fatal_error("Failed to set an object property"), - Ok(()) => (), + if napi::set_property(env, object, key, value).is_err() { + fatal_error("Failed to set an object property"); } } @@ -271,9 +270,8 @@ unsafe fn external_from_panic(env: Env, panic: Panic) -> Local { let external = result.assume_init(); #[cfg(feature = "napi-8")] - match napi::type_tag_object(env, external, &*crate::MODULE_TAG) { - Err(_) => fail(), - Ok(()) => (), + if napi::type_tag_object(env, external, &*crate::MODULE_TAG).is_err() { + fail(); } external @@ -288,11 +286,14 @@ extern "C" fn finalize_panic(_env: Env, data: *mut c_void, _hint: *mut c_void) { #[track_caller] unsafe fn create_string(env: Env, msg: &str) -> Local { let mut string = MaybeUninit::uninit(); - let status = napi::create_string_utf8(env, msg.as_ptr().cast(), msg.len(), string.as_mut_ptr()); - match status { - Err(_) => fatal_error("Failed to create a String"), - Ok(()) => (), + if napi::create_string_utf8( + env, + msg.as_ptr().cast(), + msg.len(), + string.as_mut_ptr(), + ).is_err() { + fatal_error("Failed to create a String"); } string.assume_init() @@ -301,9 +302,8 @@ unsafe fn create_string(env: Env, msg: &str) -> Local { unsafe fn is_exception_pending(env: Env) -> bool { let mut throwing = false; - match napi::is_exception_pending(env, &mut throwing) { - Err(_) => fatal_error("Failed to check if an exception is pending"), - Ok(()) => (), + if napi::is_exception_pending(env, &mut throwing).is_err() { + fatal_error("Failed to check if an exception is pending"); } throwing diff --git a/crates/neon/src/sys/object.rs b/crates/neon/src/sys/object.rs index c9abfa006..79bffa958 100644 --- a/crates/neon/src/sys/object.rs +++ b/crates/neon/src/sys/object.rs @@ -7,7 +7,7 @@ use super::{ /// Mutates the `out` argument to refer to a `napi_value` containing a newly created JavaScript Object. pub unsafe fn new(out: &mut Local, env: Env) { - napi::create_object(env, out as *mut _); + assert!(napi::create_object(env, out as *mut _).is_ok()); } #[cfg(feature = "napi-8")] @@ -31,16 +31,15 @@ pub unsafe fn seal(env: Env, obj: Local) -> Result<(), napi::Status> { pub unsafe fn get_own_property_names(out: &mut Local, env: Env, object: Local) -> bool { let mut property_names = MaybeUninit::uninit(); - match napi::get_all_property_names( + if napi::get_all_property_names( env, object, napi::KeyCollectionMode::OwnOnly, napi::KeyFilter::ALL_PROPERTIES | napi::KeyFilter::SKIP_SYMBOLS, napi::KeyConversion::NumbersToStrings, property_names.as_mut_ptr(), - ) { - Err(_) => return false, - Ok(()) => (), + ).is_err() { + return false; } *out = property_names.assume_init(); @@ -81,15 +80,18 @@ pub unsafe fn get_string( // Not using `crate::string::new()` because it requires a _reference_ to a Local, // while we only have uninitialized memory. - match napi::create_string_utf8(env, key as *const _, len as usize, key_val.as_mut_ptr()) { - Err(_) => return false, - Ok(()) => (), + if napi::create_string_utf8( + env, + key as *const _, + len as usize, + key_val.as_mut_ptr(), + ).is_err() { + return false; } // Not using napi_get_named_property() because the `key` may not be null terminated. - match napi::get_property(env, object, key_val.assume_init(), out as *mut _) { - Err(_) => return false, - Ok(()) => (), + if napi::get_property(env, object, key_val.assume_init(), out as *mut _).is_err() { + return false; } true @@ -113,20 +115,19 @@ pub unsafe fn set_string( *out = true; - match napi::create_string_utf8(env, key as *const _, len as usize, key_val.as_mut_ptr()) { - Err(_) => { - *out = false; - return false; - } - Ok(()) => (), + if napi::create_string_utf8( + env, + key as *const _, + len as usize, + key_val.as_mut_ptr(), + ).is_err() { + *out = false; + return false; } - match napi::set_property(env, object, key_val.assume_init(), val) { - Err(_) => { - *out = false; - return false; - } - Ok(()) => (), + if napi::set_property(env, object, key_val.assume_init(), val).is_err() { + *out = false; + return false; } true diff --git a/crates/neon/src/sys/tsfn.rs b/crates/neon/src/sys/tsfn.rs index 408cfc850..5352b7e3b 100644 --- a/crates/neon/src/sys/tsfn.rs +++ b/crates/neon/src/sys/tsfn.rs @@ -182,10 +182,10 @@ impl Drop for ThreadsafeFunction { } unsafe { - napi::release_threadsafe_function( + assert!(napi::release_threadsafe_function( self.tsfn.0, napi::ThreadsafeFunctionReleaseMode::Release, - ); + ).is_ok()); }; } } From 8125e1593c9aa01ab176f1f8987b1c443357fadd Mon Sep 17 00:00:00 2001 From: David Herman Date: Fri, 22 Nov 2024 10:52:57 -0800 Subject: [PATCH 2/6] cargo fmt --all --- crates/neon/src/sys/no_panic.rs | 7 +------ crates/neon/src/sys/object.rs | 18 +++++------------- crates/neon/src/sys/tsfn.rs | 3 ++- 3 files changed, 8 insertions(+), 20 deletions(-) diff --git a/crates/neon/src/sys/no_panic.rs b/crates/neon/src/sys/no_panic.rs index 516deaba8..b5de6eeb2 100644 --- a/crates/neon/src/sys/no_panic.rs +++ b/crates/neon/src/sys/no_panic.rs @@ -287,12 +287,7 @@ extern "C" fn finalize_panic(_env: Env, data: *mut c_void, _hint: *mut c_void) { unsafe fn create_string(env: Env, msg: &str) -> Local { let mut string = MaybeUninit::uninit(); - if napi::create_string_utf8( - env, - msg.as_ptr().cast(), - msg.len(), - string.as_mut_ptr(), - ).is_err() { + if napi::create_string_utf8(env, msg.as_ptr().cast(), msg.len(), string.as_mut_ptr()).is_err() { fatal_error("Failed to create a String"); } diff --git a/crates/neon/src/sys/object.rs b/crates/neon/src/sys/object.rs index 79bffa958..98eb694e7 100644 --- a/crates/neon/src/sys/object.rs +++ b/crates/neon/src/sys/object.rs @@ -38,7 +38,9 @@ pub unsafe fn get_own_property_names(out: &mut Local, env: Env, object: Local) - napi::KeyFilter::ALL_PROPERTIES | napi::KeyFilter::SKIP_SYMBOLS, napi::KeyConversion::NumbersToStrings, property_names.as_mut_ptr(), - ).is_err() { + ) + .is_err() + { return false; } @@ -80,12 +82,7 @@ pub unsafe fn get_string( // Not using `crate::string::new()` because it requires a _reference_ to a Local, // while we only have uninitialized memory. - if napi::create_string_utf8( - env, - key as *const _, - len as usize, - key_val.as_mut_ptr(), - ).is_err() { + if napi::create_string_utf8(env, key as *const _, len as usize, key_val.as_mut_ptr()).is_err() { return false; } @@ -115,12 +112,7 @@ pub unsafe fn set_string( *out = true; - if napi::create_string_utf8( - env, - key as *const _, - len as usize, - key_val.as_mut_ptr(), - ).is_err() { + if napi::create_string_utf8(env, key as *const _, len as usize, key_val.as_mut_ptr()).is_err() { *out = false; return false; } diff --git a/crates/neon/src/sys/tsfn.rs b/crates/neon/src/sys/tsfn.rs index 5352b7e3b..99726e433 100644 --- a/crates/neon/src/sys/tsfn.rs +++ b/crates/neon/src/sys/tsfn.rs @@ -185,7 +185,8 @@ impl Drop for ThreadsafeFunction { assert!(napi::release_threadsafe_function( self.tsfn.0, napi::ThreadsafeFunctionReleaseMode::Release, - ).is_ok()); + ) + .is_ok()); }; } } From e41e3178c0df51efa3719a45cbead429fae4b5b2 Mon Sep 17 00:00:00 2001 From: David Herman Date: Fri, 22 Nov 2024 17:19:57 -0800 Subject: [PATCH 3/6] slightly more consistent style --- crates/neon/src/sys/no_panic.rs | 12 ++++++------ crates/neon/src/sys/object.rs | 2 +- crates/neon/src/sys/tsfn.rs | 12 +++++++----- 3 files changed, 14 insertions(+), 12 deletions(-) diff --git a/crates/neon/src/sys/no_panic.rs b/crates/neon/src/sys/no_panic.rs index b5de6eeb2..19336e60e 100644 --- a/crates/neon/src/sys/no_panic.rs +++ b/crates/neon/src/sys/no_panic.rs @@ -254,17 +254,17 @@ unsafe fn panic_msg(panic: &Panic) -> Option<&str> { unsafe fn external_from_panic(env: Env, panic: Panic) -> Local { let fail = || fatal_error("Failed to create a neon::types::JsBox from a panic"); let mut result = MaybeUninit::uninit(); - let status = napi::create_external( + + if napi::create_external( env, Box::into_raw(Box::new(DebugSendWrapper::new(panic))).cast(), Some(finalize_panic), ptr::null_mut(), result.as_mut_ptr(), - ); - - match status { - Ok(()) => (), - Err(_) => fail(), + ) + .is_err() + { + fail(); } let external = result.assume_init(); diff --git a/crates/neon/src/sys/object.rs b/crates/neon/src/sys/object.rs index 98eb694e7..fe049c98f 100644 --- a/crates/neon/src/sys/object.rs +++ b/crates/neon/src/sys/object.rs @@ -7,7 +7,7 @@ use super::{ /// Mutates the `out` argument to refer to a `napi_value` containing a newly created JavaScript Object. pub unsafe fn new(out: &mut Local, env: Env) { - assert!(napi::create_object(env, out as *mut _).is_ok()); + assert_eq!(napi::create_object(env, out as *mut _), Ok(())); } #[cfg(feature = "napi-8")] diff --git a/crates/neon/src/sys/tsfn.rs b/crates/neon/src/sys/tsfn.rs index 99726e433..ce793b7f5 100644 --- a/crates/neon/src/sys/tsfn.rs +++ b/crates/neon/src/sys/tsfn.rs @@ -182,11 +182,13 @@ impl Drop for ThreadsafeFunction { } unsafe { - assert!(napi::release_threadsafe_function( - self.tsfn.0, - napi::ThreadsafeFunctionReleaseMode::Release, - ) - .is_ok()); + assert_eq!( + napi::release_threadsafe_function( + self.tsfn.0, + napi::ThreadsafeFunctionReleaseMode::Release, + ), + Ok(()) + ); }; } } From 3031a974c58fed8cd3483295d3d1cb657b6ed8c7 Mon Sep 17 00:00:00 2001 From: David Herman Date: Mon, 25 Nov 2024 10:37:54 -0800 Subject: [PATCH 4/6] a few subtle logical and style improvements based on @kjvalencik's review: - style: don't `assert_eq!` of `Ok(())`, just `.unwrap()` - style: capitalize no_panic error message strings - logic: don't unwrap deleting async work if we're about to fail with a more clear error anyway - logic: when returning false to propagate exceptions, only do so on PendingException; panic on other errors --- crates/neon/src/sys/async_work.rs | 4 ++-- crates/neon/src/sys/object.rs | 36 +++++++++++++++++++------------ crates/neon/src/sys/reference.rs | 2 +- crates/neon/src/sys/tag.rs | 2 +- crates/neon/src/sys/tsfn.rs | 2 +- 5 files changed, 27 insertions(+), 19 deletions(-) diff --git a/crates/neon/src/sys/async_work.rs b/crates/neon/src/sys/async_work.rs index db668d507..338319f37 100644 --- a/crates/neon/src/sys/async_work.rs +++ b/crates/neon/src/sys/async_work.rs @@ -74,7 +74,7 @@ pub unsafe fn schedule( Ok(()) => {} status => { // If queueing failed, delete the work to prevent a leak - napi::delete_async_work(env, *work).unwrap(); + let _ = napi::delete_async_work(env, *work); status.unwrap() } } @@ -151,7 +151,7 @@ unsafe extern "C" fn call_complete(env: Env, status: napi::Status, data } = *Box::>::from_raw(data.cast()); if napi::delete_async_work(env, work).is_err() { - panic!("failed to delete async work"); + panic!("Failed to delete async work"); } BOUNDARY.catch_failure(env, None, move |env| { diff --git a/crates/neon/src/sys/object.rs b/crates/neon/src/sys/object.rs index fe049c98f..fe6ba1526 100644 --- a/crates/neon/src/sys/object.rs +++ b/crates/neon/src/sys/object.rs @@ -7,7 +7,7 @@ use super::{ /// Mutates the `out` argument to refer to a `napi_value` containing a newly created JavaScript Object. pub unsafe fn new(out: &mut Local, env: Env) { - assert_eq!(napi::create_object(env, out as *mut _), Ok(())); + napi::create_object(env, out as *mut _).unwrap(); } #[cfg(feature = "napi-8")] @@ -31,7 +31,7 @@ pub unsafe fn seal(env: Env, obj: Local) -> Result<(), napi::Status> { pub unsafe fn get_own_property_names(out: &mut Local, env: Env, object: Local) -> bool { let mut property_names = MaybeUninit::uninit(); - if napi::get_all_property_names( + match napi::get_all_property_names( env, object, napi::KeyCollectionMode::OwnOnly, @@ -39,9 +39,9 @@ pub unsafe fn get_own_property_names(out: &mut Local, env: Env, object: Local) - napi::KeyConversion::NumbersToStrings, property_names.as_mut_ptr(), ) - .is_err() { - return false; + Err(napi::Status::PendingException) => return false, + status => status.unwrap(), } *out = property_names.assume_init(); @@ -82,13 +82,15 @@ pub unsafe fn get_string( // Not using `crate::string::new()` because it requires a _reference_ to a Local, // while we only have uninitialized memory. - if napi::create_string_utf8(env, key as *const _, len as usize, key_val.as_mut_ptr()).is_err() { - return false; + match napi::create_string_utf8(env, key as *const _, len as usize, key_val.as_mut_ptr()) { + Err(napi::Status::PendingException) => return false, + status => status.unwrap(), } // Not using napi_get_named_property() because the `key` may not be null terminated. - if napi::get_property(env, object, key_val.assume_init(), out as *mut _).is_err() { - return false; + match napi::get_property(env, object, key_val.assume_init(), out as *mut _) { + Err(napi::Status::PendingException) => return false, + status => status.unwrap(), } true @@ -112,14 +114,20 @@ pub unsafe fn set_string( *out = true; - if napi::create_string_utf8(env, key as *const _, len as usize, key_val.as_mut_ptr()).is_err() { - *out = false; - return false; + match napi::create_string_utf8(env, key as *const _, len as usize, key_val.as_mut_ptr()) { + Err(napi::Status::PendingException) => { + *out = false; + return false; + } + status => status.unwrap(), } - if napi::set_property(env, object, key_val.assume_init(), val).is_err() { - *out = false; - return false; + match napi::set_property(env, object, key_val.assume_init(), val) { + Err(napi::Status::PendingException) => { + *out = false; + return false; + } + status => status.unwrap(), } true diff --git a/crates/neon/src/sys/reference.rs b/crates/neon/src/sys/reference.rs index a290cdd8a..b3d744153 100644 --- a/crates/neon/src/sys/reference.rs +++ b/crates/neon/src/sys/reference.rs @@ -31,7 +31,7 @@ pub unsafe fn unreference(env: Env, value: napi::Ref) { napi::reference_unref(env, value, result.as_mut_ptr()).unwrap(); if result.assume_init() == 0 { - assert_eq!(napi::delete_reference(env, value), Ok(())); + napi::delete_reference(env, value).unwrap(); } } diff --git a/crates/neon/src/sys/tag.rs b/crates/neon/src/sys/tag.rs index 3dcc888c0..588d7c71d 100644 --- a/crates/neon/src/sys/tag.rs +++ b/crates/neon/src/sys/tag.rs @@ -39,7 +39,7 @@ pub unsafe fn is_object(env: Env, val: Local) -> bool { pub unsafe fn is_array(env: Env, val: Local) -> bool { let mut result = false; - assert_eq!(napi::is_array(env, val, &mut result as *mut _), Ok(())); + napi::is_array(env, val, &mut result as *mut _).unwrap(); result } diff --git a/crates/neon/src/sys/tsfn.rs b/crates/neon/src/sys/tsfn.rs index ce793b7f5..7810244bd 100644 --- a/crates/neon/src/sys/tsfn.rs +++ b/crates/neon/src/sys/tsfn.rs @@ -182,7 +182,7 @@ impl Drop for ThreadsafeFunction { } unsafe { - assert_eq!( + debug_assert_eq!( napi::release_threadsafe_function( self.tsfn.0, napi::ThreadsafeFunctionReleaseMode::Release, From 4f0adfc9c9c6d217ba36858a3c9770fbec06d29e Mon Sep 17 00:00:00 2001 From: David Herman Date: Mon, 25 Nov 2024 10:41:06 -0800 Subject: [PATCH 5/6] cargo fmt --all --- crates/neon/src/sys/object.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/neon/src/sys/object.rs b/crates/neon/src/sys/object.rs index fe6ba1526..226239368 100644 --- a/crates/neon/src/sys/object.rs +++ b/crates/neon/src/sys/object.rs @@ -38,8 +38,7 @@ pub unsafe fn get_own_property_names(out: &mut Local, env: Env, object: Local) - napi::KeyFilter::ALL_PROPERTIES | napi::KeyFilter::SKIP_SYMBOLS, napi::KeyConversion::NumbersToStrings, property_names.as_mut_ptr(), - ) - { + ) { Err(napi::Status::PendingException) => return false, status => status.unwrap(), } From 77929a525a8281ae06215ac8c1293b7ba83ef0af Mon Sep 17 00:00:00 2001 From: David Herman Date: Tue, 26 Nov 2024 09:47:58 -0800 Subject: [PATCH 6/6] debug_assert_eq! on delete_async_work for consistency --- crates/neon/src/sys/async_work.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/neon/src/sys/async_work.rs b/crates/neon/src/sys/async_work.rs index 338319f37..e834576c6 100644 --- a/crates/neon/src/sys/async_work.rs +++ b/crates/neon/src/sys/async_work.rs @@ -150,9 +150,7 @@ unsafe extern "C" fn call_complete(env: Env, status: napi::Status, data .. } = *Box::>::from_raw(data.cast()); - if napi::delete_async_work(env, work).is_err() { - panic!("Failed to delete async work"); - } + debug_assert_eq!(napi::delete_async_work(env, work), Ok(())); BOUNDARY.catch_failure(env, None, move |env| { // `unwrap` is okay because `call_complete` should be called exactly once