From 23882271b261de262ce90cd411e75f9b7c303e12 Mon Sep 17 00:00:00 2001
From: David Herman <david.herman@gmail.com>
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<I, O, D>(
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<I, O, D>(env: Env, status: napi::Status, data
..
} = *Box::<Data<I, O, D>>::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<T> Drop for ThreadsafeFunction<T> {
}
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 <david.herman@gmail.com>
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<T> Drop for ThreadsafeFunction<T> {
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 <david.herman@gmail.com>
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<T> Drop for ThreadsafeFunction<T> {
}
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 <david.herman@gmail.com>
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<I, O, D>(
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<I, O, D>(env: Env, status: napi::Status, data
} = *Box::<Data<I, O, D>>::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<T> Drop for ThreadsafeFunction<T> {
}
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 <david.herman@gmail.com>
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 <david.herman@gmail.com>
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<I, O, D>(env: Env, status: napi::Status, data
..
} = *Box::<Data<I, O, D>>::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