v8 14.4 #31558
v8 14.4 #31558
Conversation
WalkthroughThis pull request updates Deno's dependency resolution and V8 integration. The deno_core workspace dependency switches from a published crate version to a local path reference. A default V8 flag is removed from the CLI arguments. NAPI string extraction methods and web encoding operations are migrated to use v8's newer write helpers (write_v2, write_utf8_v2) with explicit null termination and updated handling of invalid UTF-8 sequences, replacing previous approaches that handled termination differently. Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.toml(1 hunks)cli/args/mod.rs(0 hunks)ext/napi/js_native_api.rs(3 hunks)ext/web/lib.rs(1 hunks)
💤 Files with no reviewable changes (1)
- cli/args/mod.rs
🧰 Additional context used
📓 Path-based instructions (2)
ext/**/lib.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Extensions should provide ops (operations) exposed to JavaScript in Rust code within
ext/<extension_name>/directories
Files:
ext/web/lib.rs
**/*.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/web/lib.rsext/napi/js_native_api.rs
⚙️ CodeRabbit configuration file
Don't worry about coverage of Rust docstrings. Don't be nitpicky about it. Leave it to the author's judgement if such a documentation is necessary.
Files:
ext/web/lib.rsext/napi/js_native_api.rs
⏰ 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). (3)
- GitHub Check: test release linux-aarch64
- GitHub Check: test release macos-x86_64
- GitHub Check: test release macos-aarch64
🔇 Additional comments (2)
ext/napi/js_native_api.rs (1)
2176-2188: LGTM! Correct UTF-8 write implementation.The pattern here is correct: create a buffer with
bufsize - 1capacity, write without auto-null-termination, manually append the null byte, and report the actual copied count. This correctly handles the null terminator accounting.ext/web/lib.rs (1)
399-406: LGTM! Correctly migrated to write_utf8_v2 API.The migration from
write_utf8towrite_utf8_v2withkReplaceInvalidUtf8flag correctly maintains the previous behavior of replacing invalid UTF-8 sequences with the Unicode replacement character (U+FFFD). Thencharscapture via the optional parameter is the proper pattern for the new API, correctly storing the number of UTF-16 code units inout_buf[0]and bytes written inout_buf[1].
| [workspace.dependencies] | ||
| deno_ast = { version = "=0.52.0", features = ["transpiling"] } | ||
| deno_core = { version = "0.372.0" } | ||
| deno_core = { path = "../deno_core/core" } #version = "0.372.0" } |
There was a problem hiding this comment.
Local path dependency should not be merged to main.
This changes deno_core to reference a local path (../deno_core/core) outside the repository. This will break builds for anyone who doesn't have the expected sibling directory structure. The commented-out version suggests this is a temporary development change.
Revert to the versioned dependency before merging, or update to the appropriate published version that includes the V8 14.4 changes.
-deno_core = { path = "../deno_core/core" } #version = "0.372.0" }
+deno_core = "0.372.0" # Update to version with V8 14.4 support📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| deno_core = { path = "../deno_core/core" } #version = "0.372.0" } | |
| deno_core = "0.372.0" # Update to version with V8 14.4 support |
🤖 Prompt for AI Agents
In Cargo.toml at line 66, the deno_core dependency was changed to a local path
reference (deno_core = { path = "../deno_core/core" }) which must not be merged;
revert this to a published version by replacing the path spec with the previous
or appropriate published version (e.g. deno_core = "0.372.0" or the correct
crate version that includes the V8 14.4 changes), remove any stray comment
braces, and run cargo update/cargo generate-lockfile so the lockfile reflects
the published dependency.
| let length = value.length().min(bufsize - 1); | ||
| let buffer = unsafe { std::slice::from_raw_parts_mut(buf as _, length) }; | ||
| value.write_one_byte_v2(scope, 0, buffer, v8::WriteFlags::kNullTerminate); | ||
| if !result.is_null() { | ||
| unsafe { | ||
| *result = copied; | ||
| *result = length; | ||
| } | ||
| } |
There was a problem hiding this comment.
Result length may be incorrect when kNullTerminate consumes a buffer slot.
With kNullTerminate, V8's write_one_byte_v2 writes min(string_length, buffer.len() - 1) characters plus a null terminator. However, the code pre-calculates length = value.length().min(bufsize - 1) and passes a buffer of exactly that size.
If value.length() < bufsize - 1, the buffer has value.length() slots, so V8 writes only value.length() - 1 characters + null, but you report length (which equals value.length()).
Consider matching the UTF-8 implementation pattern: pass the full available buffer and manually append the null terminator, or adjust the result to reflect actual bytes written.
} else if bufsize != 0 {
- let length = value.length().min(bufsize - 1);
- let buffer = unsafe { std::slice::from_raw_parts_mut(buf as _, length) };
- value.write_one_byte_v2(scope, 0, buffer, v8::WriteFlags::kNullTerminate);
+ let buffer = unsafe { std::slice::from_raw_parts_mut(buf as _, bufsize - 1) };
+ let copied = value.write_one_byte_v2(scope, 0, buffer, v8::WriteFlags::empty());
+ unsafe {
+ buf.add(copied).write(0);
+ }
if !result.is_null() {
unsafe {
- *result = length;
+ *result = copied;
}
}🤖 Prompt for AI Agents
In ext/napi/js_native_api.rs around lines 2132 to 2139, the code precomputes
length and hands a buffer sized to length while also using
v8::WriteFlags::kNullTerminate, which can cause the reported length to be one
too large when V8 consumes a slot for the terminator; instead follow the UTF‑8
pattern: pass the full available buffer to V8 (so it can write up to bufsize-1
chars), capture the actual number of bytes written by the write call (or compute
it from the returned/observed value), then manually append a null terminator at
the next slot and store the actual bytes written into *result; alternatively, if
you must keep kNullTerminate, adjust the reported result to subtract the
terminator slot when V8 consumed it.
| let length = value.length().min(bufsize - 1); | ||
| let buffer = unsafe { std::slice::from_raw_parts_mut(buf as _, length) }; | ||
| value.write_v2(scope, 0, buffer, v8::WriteFlags::kNullTerminate); | ||
| if !result.is_null() { | ||
| unsafe { | ||
| *result = copied; | ||
| *result = length; | ||
| } | ||
| } |
There was a problem hiding this comment.
Same result length issue as latin1 function.
This has the same problem as napi_get_value_string_latin1: when kNullTerminate is used with a buffer of exactly length elements, V8 writes length - 1 characters plus null, but the code reports length as the result.
Apply the same fix pattern as the UTF-8 version.
} else if bufsize != 0 {
- let length = value.length().min(bufsize - 1);
- let buffer = unsafe { std::slice::from_raw_parts_mut(buf as _, length) };
- value.write_v2(scope, 0, buffer, v8::WriteFlags::kNullTerminate);
+ let buffer = unsafe { std::slice::from_raw_parts_mut(buf, bufsize - 1) };
+ let copied = value.write_v2(scope, 0, buffer, v8::WriteFlags::empty());
+ unsafe {
+ *buf.add(copied) = 0;
+ }
if !result.is_null() {
unsafe {
- *result = length;
+ *result = copied;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let length = value.length().min(bufsize - 1); | |
| let buffer = unsafe { std::slice::from_raw_parts_mut(buf as _, length) }; | |
| value.write_v2(scope, 0, buffer, v8::WriteFlags::kNullTerminate); | |
| if !result.is_null() { | |
| unsafe { | |
| *result = copied; | |
| *result = length; | |
| } | |
| } | |
| let buffer = unsafe { std::slice::from_raw_parts_mut(buf, bufsize - 1) }; | |
| let copied = value.write_v2(scope, 0, buffer, v8::WriteFlags::empty()); | |
| unsafe { | |
| *buf.add(copied) = 0; | |
| } | |
| if !result.is_null() { | |
| unsafe { | |
| *result = copied; | |
| } | |
| } |
denoland/deno_core#1256
the changes in in node-api to what node itself is doing, so it should be alright.