fix(napi): skip nullish error causes when converting from Unknown#3143
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces centralized error-cause extraction in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use OpenGrep to find security vulnerabilities and bugs across 17+ programming languages.OpenGrep is compatible with Semgrep configurations. Add an |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/napi/src/error.rs (1)
695-717: Add one test on the actualFrom<Unknown>path.These tests pin the nullish predicate, but they would still pass if
extract_error_causeregressed in the object/property-access path. A small end-to-end case asserting that{ cause: undefined }and{ cause: null }produceError { cause: None, .. }would make this change much harder to break later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/napi/src/error.rs` around lines 695 - 717, Add an end-to-end test that exercises the From<Unknown> conversion path: construct an Unknown representing an object with a "cause" property set to Undefined and another with Null, convert each via From<Unknown> into the crate's Error type (the same path that calls extract_error_cause/should_extract_error_cause), and assert that the resulting Error.cause is None in both cases; place this new test alongside the existing tests in the tests module and reference the From<Unknown> conversion, extract_error_cause, and Error type so the object/property-access path is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/napi/src/error.rs`:
- Around line 671-676: In extract_error_cause, the call to
obj.get_named_property::<Unknown>("cause").ok() swallows N-API errors (including
Status::PendingException) and leaves pending JS exceptions uncleared; update
extract_error_cause to stop discarding errors: either propagate the error
(return Err) when get_named_property fails or call the helper macro
check_pending_exception! (or equivalent) to clear the pending exception before
returning None, ensuring pending exceptions are not left dangling; update the
logic around extract_error_cause and the get_named_property("cause") call
accordingly and add an end-to-end test that exercises the property-access path
(a getter/proxy that throws) to verify error conversion and clearing of pending
exceptions.
---
Nitpick comments:
In `@crates/napi/src/error.rs`:
- Around line 695-717: Add an end-to-end test that exercises the From<Unknown>
conversion path: construct an Unknown representing an object with a "cause"
property set to Undefined and another with Null, convert each via From<Unknown>
into the crate's Error type (the same path that calls
extract_error_cause/should_extract_error_cause), and assert that the resulting
Error.cause is None in both cases; place this new test alongside the existing
tests in the tests module and reference the From<Unknown> conversion,
extract_error_cause, and Error type so the object/property-access path is
covered.
|
@cursor review |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/napi/src/error.rs`:
- Around line 677-680: In extract_error_cause, don't hide errors from
value.get_type(): replace the matches!(value.get_type(), Ok(ValueType::Object))
check with a call that propagates failures (e.g. let ty = value.get_type()?; if
ty != ValueType::Object { return Ok(None); }) so actual get_type() errors (like
pending JS exceptions) are returned instead of being treated as Ok(None); keep
the rest of extract_error_cause unchanged.
bdebeeb to
9c08ca1
Compare
Avoid recursively converting `Error.cause` when it is `undefined` or `null`. This prevents host runtimes from observing spurious pending exceptions when `Unknown -> Error` conversion inspects nullable causes.
9c08ca1 to
ecec35d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/napi/src/error.rs`:
- Line 16: Remove the unused import JsObjectValue from the top of error.rs:
delete the line "use crate::bindgen_runtime::JsObjectValue;" since the file uses
Object::get_named_property() and Unknown-based methods (no JsObjectValue trait
methods), so eliminating this import removes the unused dependency and cleans up
the file.
|
One check is failing, but this isn't a problem with this PR; it seems to have been happening for a while. |
- Move cause extraction after reference creation in non-WASM path to preserve original ordering - Use unwrap_or(None) instead of returning cause-extraction errors, so the original error is never lost - Inline should_extract_error_cause into extract_error_cause - Replace unit tests with integration tests for nullish cause handling Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Background
I found this while cleaning up build failure handling in an unplugin-based workflow. The original goal was to make build failures propagate by throwing, but in deno that error did not only surface as an expected build failure and was also observed as an extra
unhandledrejection.At first I suspected error propagation in
unpluginorrolldown, and I narrowed the reproduction down from that direction. However, the issue turned out to be lower in the stack. While converting rejected values across the napi boundary intonapi::Error,Error.causewas still being recursively converted even when the cause wasundefinedornull.That behavior can be easy to miss in practice, but on some host runtimes, specifically Deno, it can leave an unnecessary pending exception behind, which later becomes observable as
unhandledrejection. Because of that, it seemed more appropriate to tighten theUnknown -> Errorconversion policy innapi-rsfirst instead of treating this as primarily arolldownor Deno issue.This change captures the host-independent rule behind that fix: nullish causes should not be reconstructed as nested errors.
Changes
Error.causeisundefinedornull.Notes for Reviewers
The observable failure here is host-specific, but this PR intentionally stops one level earlier and focuses on the policy that
napi-rsitself should guarantee.Because of that, the added tests do not prove that a host runtime no longer leaves a pending exception behind. Instead, they verify the narrower rule that nullish causes are not treated as nested errors. I plan to attach a separate reproduction script for the runtime-level behavior in the PR discussion rather than include it in this commit.
The main review question is whether skipping
undefinedandnullcauses is the right default fornapi::Error's error model.Testing
yarn workspace @examples/napi buildcargo test -p napi --libdeno -ER --allow-ffi examples/napi/repro-deno-unhandledrejection.mjs
Summary by CodeRabbit
Improvements
Tests