Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

n-api: use napi_get_all_property_names #601

Merged
merged 2 commits into from
Sep 22, 2020
Merged

n-api: use napi_get_all_property_names #601

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5b02650
Use `napi_get_all_property_names` API.
goto-bus-stop Apr 10, 2020
5eca8b8
Upgrade nodejs-sys
goto-bus-stop Apr 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/neon-runtime/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ license = "MIT/Apache-2.0"
[dependencies]
cfg-if = "0.1.9"
neon-sys = { version = "=0.4.1", path = "../neon-sys", optional = true }
nodejs-sys = { version = "0.2.0", optional = true }
nodejs-sys = { version = "0.7.0", optional = true }

[features]
default = []
Expand Down
75 changes: 11 additions & 64 deletions crates/neon-runtime/src/napi/object.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,6 @@ use std::mem::MaybeUninit;

use nodejs_sys as napi;

use array;
use convert;
use tag;
use raw::{Env, Local};

/// Mutates the `out` argument to refer to a `napi_value` containing a newly created JavaScript Object.
Expand All @@ -15,70 +12,20 @@ pub unsafe extern "C" fn new(out: &mut Local, env: Env) {
/// Mutates the `out` argument to refer to a `napi_value` containing the own property names of the
/// `object` as a JavaScript Array.
pub unsafe extern "C" fn get_own_property_names(out: &mut Local, env: Env, object: Local) -> bool {
// Node.js 13+ have `napi_get_all_property_names`, which does the conversion right and allows
// us to ask for only own properties or prototype properties or anything we like.
// Unfortunately, earlier versions do not support that method, so we have to implement it
// manually.
//
// So we use a temporary array for the raw names:
let mut raw_names = MaybeUninit::uninit();
if napi::napi_get_property_names(env, object, raw_names.as_mut_ptr()) != napi::napi_status::napi_ok {
return false;
}
// And a "fixed" array for the actual return value:
let mut fixed_names = MaybeUninit::uninit();
if napi::napi_create_array(env, fixed_names.as_mut_ptr()) != napi::napi_status::napi_ok {
let mut property_names = MaybeUninit::uninit();

if napi::napi_get_all_property_names(
env,
object,
napi::napi_key_collection_mode::napi_key_own_only,
napi::napi_key_filter::napi_key_all_properties | napi::napi_key_filter::napi_key_skip_symbols,
napi::napi_key_conversion::napi_key_numbers_to_strings,
property_names.as_mut_ptr(),
) != napi::napi_status::napi_ok {
return false;
}

let raw_names = raw_names.assume_init();
let fixed_names = fixed_names.assume_init();

*out = fixed_names;

let raw_len = array::len(env, raw_names);
let mut fixed_len = 0;
for index in 0..raw_len {
let mut property_name: Local = std::mem::zeroed();

// In general, getters may cause arbitrary JS code to be run, but this is a newly created
// Array from an official internal API so it doesn't do anything strange.
if !get_index(&mut property_name, env, raw_names, index) {
continue;
}

// Before https://github.com/nodejs/node/pull/27524, `napi_get_property_names` would return
// numbers for numeric indices instead of strings.
// Make sure we always return strings.
let property_name = if !tag::is_string(env, property_name) {
let mut stringified: Local = std::mem::zeroed();
// If we can't convert to a string, something went wrong.
if !convert::to_string(&mut stringified, env, property_name) {
return false;
}
stringified
} else {
property_name
};

let mut is_own_property = false;
// May return a non-OK status if `key` is not a string or a Symbol, but here it is always
// a string.
if napi::napi_has_own_property(env, object, property_name, &mut is_own_property as *mut _) != napi::napi_status::napi_ok {
return false;
}

if !is_own_property {
continue;
}

let mut dummy = false;
// If we can't convert assign to this array, something went wrong.
if !set_index(&mut dummy, env, fixed_names, fixed_len, property_name) {
return false;
}
fixed_len += 1;
}
*out = property_names.assume_init();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed this. I'll write a ticket for some clean-up in neon. This should probably also be using a MaybeUninit on the calling side or be returning an Option instead fo a boolean.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is a common pattern throughout neon-runtime. Since these runtime functions don't need to be FFI-safe anymore with N-API, we can safely use things like Options for this stuff, but it probably needs to wait until the NAN backend is removed.


true
}
Expand Down