feat(ext/node): add napi_create_object_with_properties API
#31443
Conversation
WalkthroughThis PR introduces a new N-API function Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
ext/napi/generated_symbol_exports_list_linux.def(1 hunks)ext/napi/generated_symbol_exports_list_macos.def(1 hunks)ext/napi/generated_symbol_exports_list_windows.def(1 hunks)ext/napi/js_native_api.rs(1 hunks)ext/napi/sym/symbol_exports.json(1 hunks)tests/napi/object_test.js(1 hunks)tests/napi/src/object.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or uselldbdirectly
Useeprintln!()ordbg!()macros for debug prints in Rust code
Files:
ext/napi/js_native_api.rstests/napi/src/object.rs
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js}: For JavaScript runtime debugging, enable V8 inspector with--inspect-brkflag and connect Chrome DevTools tochrome://inspect
Useconsole.log()for debug prints in JavaScript runtime code
Files:
tests/napi/object_test.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test debug linux-aarch64
- GitHub Check: test debug macos-x86_64
- GitHub Check: lint debug macos-x86_64
- GitHub Check: build libs
- GitHub Check: lint debug windows-x86_64
- GitHub Check: lint debug linux-x86_64
🔇 Additional comments (8)
ext/napi/generated_symbol_exports_list_windows.def (1)
96-98: Windows export entry looks correctSymbol name and ordering (between
napi_create_objectandnapi_create_promise) are consistent with the other export lists and JSON manifest.ext/napi/generated_symbol_exports_list_linux.def (1)
1-1: Linux symbol list updated consistently
"napi_create_object_with_properties"is added in the expected position alongside the other object-related symbols.ext/napi/generated_symbol_exports_list_macos.def (1)
94-96: macOS export looks alignedThe
_napi_create_object_with_propertiessymbol is correctly exported and ordered after_napi_create_object.ext/napi/sym/symbol_exports.json (1)
96-98: Symbol manifest entry is consistentThe JSON export list now includes
"napi_create_object_with_properties"in the same relative position as the platform-specific.deffiles. No issues.tests/napi/object_test.js (1)
40-65: New N-API object tests look solidThe three tests exercise non-empty, empty, and custom-prototype cases and validate both shape and prototype behavior (via the
testmethod). This provides good coverage for the new API.tests/napi/src/object.rs (3)
11-21: Extern declaration matches the intended N-API shapeThe experimental
napi_create_object_with_propertiesdeclaration lines up with the expected N-API signature and is fine as a temporary bridge until napi-sys grows this API.
81-205: Rust tests nicely cover the new API (and highlight the 0-property edge case)The three helpers exercise:
- non-empty properties with a
nullprototype,- the
property_count == 0case with null pointers for the property arrays,- and a custom prototype carrying a method plus an own data property.
Once the implementation in
napi_create_object_with_propertiesis adjusted to avoidfrom_raw_partson null pointers whenproperty_count == 0, these tests give good coverage of the new behavior.
216-230: Init wiring for new test exports looks correctThe new entries in the
propertiesarray correctly expose all three test helpers under the expected names; this integrates cleanly with the existing init pattern.
| #[napi_sym] | ||
| fn napi_create_object_with_properties<'s>( | ||
| env_ptr: *mut Env, | ||
| prototype_or_null: napi_value<'s>, | ||
| property_names: *const napi_value<'s>, | ||
| property_values: *const napi_value<'s>, | ||
| property_count: usize, | ||
| result: *mut napi_value<'s>, | ||
| ) -> napi_status { | ||
| let env = check_env!(env_ptr); | ||
| check_arg!(env, result); | ||
|
|
||
| if property_count > 0 { | ||
| check_arg!(env, property_names); | ||
| check_arg!(env, property_values); | ||
| } | ||
|
|
||
| unsafe { | ||
| v8::callback_scope!(unsafe scope, env.context()); | ||
|
|
||
| let prototype = if let Some(proto) = *prototype_or_null { | ||
| proto | ||
| } else { | ||
| v8::null(scope).into() | ||
| }; | ||
|
|
||
| let names = std::slice::from_raw_parts(property_names, property_count); | ||
| let values = std::slice::from_raw_parts(property_values, property_count); | ||
|
|
||
| for name in names { | ||
| if let Some(name_val) = **name { | ||
| if !name_val.is_name() { | ||
| return napi_name_expected; | ||
| } | ||
| } else { | ||
| return napi_invalid_arg; | ||
| } | ||
| } | ||
|
|
||
| let mut v8_names = Vec::with_capacity(property_count); | ||
| let mut v8_values = Vec::with_capacity(property_count); | ||
|
|
||
| for i in 0..property_count { | ||
| if let Some(name) = *names[i] { | ||
| v8_names.push(name.try_cast::<v8::Name>().unwrap()); | ||
| } | ||
| if let Some(value) = *values[i] { | ||
| v8_values.push(value); | ||
| } | ||
| } | ||
|
|
||
| *result = v8::Object::with_prototype_and_properties( | ||
| scope, prototype, &v8_names, &v8_values, | ||
| ) | ||
| .into(); | ||
| } | ||
|
|
||
| return napi_clear_last_error(env_ptr); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the file under review
find . -name "js_native_api.rs" -type fRepository: denoland/deno
Length of output: 170
🏁 Script executed:
#!/bin/bash
# Look for test files related to napi_create_object_with_properties
find . -type f -name "*.rs" | head -20 && \
rg -l "napi_create_object_with_properties" --type rustRepository: denoland/deno
Length of output: 170
🏁 Script executed:
#!/bin/bash
# Search for tests that call this function with property_count == 0
rg -A5 -B5 "property_count.*0\|napi_create_object_with_properties" --type rust -C3Repository: denoland/deno
Length of output: 170
🌐 Web query:
Rust std::slice::from_raw_parts null pointer UB undefined behavior zero length
💡 Result:
Short answer: It's UB. std::slice::from_raw_parts / from_raw_parts_mut require the pointer to be non-null and properly aligned even when len == 0 — passing a null pointer to produce a reference is undefined behavior. Use NonNull::dangling() (or a valid non-null pointer) for zero-length slices instead. [1][2][3]
References
- std::slice::from_raw_parts / from_raw_parts_mut safety section (non-null required even for zero-length slices). [1]
- core::ptr::slice_from_raw_parts docs note a raw slice with a null pointer is dangerous and that a slice reference must never have a null pointer. [2]
- from_raw_parts_mut explicit wording about non-null for zero-length slices and NonNull::dangling(). [3]
[1] https://doc.rust-lang.org/std/slice/fn.from_raw_parts.html
[2] https://doc.rust-lang.org/std/ptr/fn.slice_from_raw_parts.html
[3] https://doc.rust-lang.org/std/slice/fn.from_raw_parts_mut.html
🌐 Web query:
Node.js NAPI napi_create_object_with_properties prototype documentation
💡 Result:
There is no napi_create_object_with_properties in upstream Node.js N-API. The official Node.js N-API uses napi_create_object and napi_define_properties (or napi_create_object_with_named_properties in some forks). Example Node.js N-API prototypes from the official docs:
-
napi_create_object:
napi_status napi_create_object(napi_env env, napi_value* result). [1] -
napi_define_properties:
napi_status napi_define_properties(napi_env env,
napi_value object,
size_t property_count,
const napi_property_descriptor* properties); [1]
The symbol napi_create_object_with_properties appears in other runtimes/forks (e.g., Harmony/Ark/other Node-API variants) with this prototype:
napi_status napi_create_object_with_properties(napi_env env,
napi_value* result,
size_t property_count,
const napi_property_descriptor* properties);
(Used in Harmony/Ark Node-API documentation.) [2][3]
References
- Official Node.js N-API docs (shows napi_create_object and napi_define_properties). [1]
- Harmony/Ark Node-API docs (shows napi_create_object_with_properties / napi_create_object_with_named_properties). [2][3]
Sources:
[1] Node.js N-API documentation (napi_create_object / napi_define_properties).
[2] Harmony/Ark Node-API — napi_create_object_with_properties.
[3] Harmony/Ark Node-API — napi_create_object_with_named_properties.
Based on my verification through web searches:
-
Rust
from_raw_partsUB with null pointers: ✅ CONFIRMED- The official Rust documentation explicitly states that
std::slice::from_raw_partsrequires a non-null and properly aligned pointer even whenlen == 0. Passing a null pointer to produce a slice reference is undefined behavior, regardless of slice length.
- The official Rust documentation explicitly states that
-
The review's suggested fix is correct: The approach of using empty slices (
&[]) whenproperty_count == 0and avoiding thefrom_raw_partscall with null pointers is the recommended Rust pattern. -
Prototype semantics concern: The function signature with
prototype_or_nullappears to be non-standard (official Node.js N-API doesn't have this parameter innapi_create_object_with_properties). The review's request to verify against upstream is reasonable, though this appears to be a Deno-specific extension.
The review comment is accurate and the critical issue is justified.
Undefined behavior when calling with property_count == 0 and null pointers
The code unconditionally constructs slices via std::slice::from_raw_parts(property_names, property_count) and std::slice::from_raw_parts(property_values, property_count), even when property_count == 0. Passing a null pointer to from_raw_parts is undefined behavior in Rust, even for zero-length slices—the pointer must always be non-null and properly aligned.
The existing guard if property_count > 0 only validates pointers via check_arg! but does not prevent the null pointers from reaching from_raw_parts. This creates UB when tests (or callers) invoke this function with property_count == 0 and null pointer arguments.
Fix: Only construct slices when property_count > 0; use empty slices otherwise:
- let names = std::slice::from_raw_parts(property_names, property_count);
- let values = std::slice::from_raw_parts(property_values, property_count);
+ let (names, values): (&[napi_value<'s>], &[napi_value<'s>]) =
+ if property_count == 0 {
+ (&[], &[])
+ } else {
+ (
+ std::slice::from_raw_parts(property_names, property_count),
+ std::slice::from_raw_parts(property_values, property_count),
+ )
+ };Also verify the intended semantics for prototype_or_null (whether nullptr prototype maps to JS null or the default Object.prototype) against the upstream NAPI specification or Node.js PR you're aligning with.
1 participant
Closes #31433