Skip to content

Conversation

kjvalencik
Copy link
Member

This is exposed publicly which will likely be needed for macros. However, it's marked #[doc(hidden)] because we don't want to stabilize it. We can make it private if it turns out not to be necessary for the macro.

@kjvalencik kjvalencik requested a review from Copilot August 8, 2025 19:44
Copilot

This comment was marked as outdated.

Copy link

github-actions bot commented Aug 8, 2025

🐰 Bencher Report

Branchkv/wrap
Testbedubuntu-latest

🚨 3 Alerts

BenchmarkMeasure
Units
ViewBenchmark Result
(Result Δ%)
Lower Boundary
(Limit %)
Upper Boundary
(Limit %)
JsFunction::bindLatency
nanoseconds (ns)
📈 plot
🚷 threshold
🚨 alert (🔔)
181.57 ns
(-11.43%)Baseline: 204.99 ns
184.49 ns
(101.61%)

409.98 ns
(44.29%)
JsFunction::call_withLatency
nanoseconds (ns)
📈 plot
🚷 threshold
🚨 alert (🔔)
182.12 ns
(-10.69%)Baseline: 203.92 ns
183.52 ns
(100.77%)

407.83 ns
(44.66%)
JsFunction::call_withThroughput
operations / second (ops/s)
📈 plot
🚷 threshold
🚨 alert (🔔)
5,481,209.61 ops/s
(+12.13%)Baseline: 4,888,393.39 ops/s
0.00 ops/s
(0.00%)
5,377,232.73 ops/s
(101.93%)

Click to view all benchmark results
BenchmarkLatencyBenchmark Result
nanoseconds (ns)
(Result Δ%)
Lower Boundary
nanoseconds (ns)
(Limit %)
Upper Boundary
nanoseconds (ns)
(Limit %)
ThroughputBenchmark Result
operations / second (ops/s)
(Result Δ%)
Lower Boundary
operations / second (ops/s)
(Limit %)
Upper Boundary
operations / second (ops/s)
(Limit %)
JsFunction::bind📈 view plot
🚷 view threshold
🚨 view alert (🔔)
181.57 ns
(-11.43%)Baseline: 204.99 ns
184.49 ns
(101.61%)

409.98 ns
(44.29%)
📈 view plot
🚷 view threshold
5,291,751.34 ops/s
(+9.12%)Baseline: 4,849,476.68 ops/s
0.00 ops/s
(0.00%)
5,334,424.35 ops/s
(99.20%)
JsFunction::call📈 view plot
🚷 view threshold
200.92 ns
(-7.86%)Baseline: 218.04 ns
196.24 ns
(97.67%)
436.09 ns
(46.07%)
📈 view plot
🚷 view threshold
4,944,477.03 ops/s
(+8.19%)Baseline: 4,570,377.18 ops/s
0.00 ops/s
(0.00%)
5,027,414.90 ops/s
(98.35%)
JsFunction::call_with📈 view plot
🚷 view threshold
🚨 view alert (🔔)
182.12 ns
(-10.69%)Baseline: 203.92 ns
183.52 ns
(100.77%)

407.83 ns
(44.66%)
📈 view plot
🚷 view threshold
🚨 view alert (🔔)
5,481,209.61 ops/s
(+12.13%)Baseline: 4,888,393.39 ops/s
0.00 ops/s
(0.00%)
5,377,232.73 ops/s
(101.93%)

auto-exported-noop📈 view plot
🚷 view threshold
27.98 ns
(-7.52%)Baseline: 30.25 ns
27.23 ns
(97.32%)
60.51 ns
(46.24%)
📈 view plot
🚷 view threshold
35,689,306.39 ops/s
(+5.83%)Baseline: 33,721,701.48 ops/s
0.00 ops/s
(0.00%)
37,093,871.62 ops/s
(96.21%)
hello-world📈 view plot
🚷 view threshold
52.01 ns
(-8.80%)Baseline: 57.03 ns
51.33 ns
(98.69%)
114.06 ns
(45.60%)
📈 view plot
🚷 view threshold
19,189,285.59 ops/s
(+9.59%)Baseline: 17,510,430.41 ops/s
0.00 ops/s
(0.00%)
19,261,473.46 ops/s
(99.63%)
manually-exported-noop📈 view plot
🚷 view threshold
27.95 ns
(-1.23%)Baseline: 28.29 ns
25.46 ns
(91.12%)
56.59 ns
(49.39%)
📈 view plot
🚷 view threshold
35,719,035.87 ops/s
(+1.24%)Baseline: 35,282,712.05 ops/s
0.00 ops/s
(0.00%)
38,810,983.26 ops/s
(92.03%)
🐰 View full continuous benchmarking report in Bencher

Copy link

codecov bot commented Aug 8, 2025

Codecov Report

❌ Patch coverage is 86.25000% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.91%. Comparing base (5793b62) to head (3b3943e).

Files with missing lines Patch % Lines
crates/neon/src/object/wrap.rs 86.25% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1114      +/-   ##
==========================================
+ Coverage   82.85%   82.91%   +0.05%     
==========================================
  Files          74       75       +1     
  Lines        4701     4781      +80     
  Branches     4701     4781      +80     
==========================================
+ Hits         3895     3964      +69     
- Misses        713      724      +11     
  Partials       93       93              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements public but documentation-hidden wrap and unwrap functions for the Neon library, intended for use in macros while avoiding stabilization of the API.

  • Adds new wrap and unwrap functions with #[doc(hidden)] visibility for macro usage
  • Implements comprehensive error handling for wrapping operations including already-wrapped and foreign-type scenarios
  • Adds Node-API bindings and test coverage for the new wrapping functionality

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
crates/neon/src/object/wrap.rs New module implementing wrap/unwrap functionality with error handling and safety guarantees
crates/neon/src/object/mod.rs Exports wrap/unwrap functions as doc-hidden public API
crates/neon/src/sys/bindings/functions.rs Adds Node-API bindings for wrap and unwrap operations
test/napi/src/js/boxed.rs Test functions demonstrating wrap/unwrap usage
test/napi/lib/boxed.js JavaScript tests verifying wrap/unwrap behavior and error cases

@kjvalencik kjvalencik force-pushed the kv/wrap branch 2 times, most recently from d510307 to 1911b1e Compare August 8, 2025 20:31
Copy link
Collaborator

@dherman dherman left a comment

Choose a reason for hiding this comment

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

Thank you for implementing this! I made one logic suggestion—take a look and see what you want to do, but you don't need me to re-review afterwards.

Copy link
Collaborator

@dherman dherman left a comment

Choose a reason for hiding this comment

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

A couple more suggestions, but feel free to use your judgment and merge.

#[cfg(feature = "napi-8")]
match sys::type_tag_object(env, o, &*crate::MODULE_TAG) {
Err(sys::Status::InvalidArg) => {
sys::remove_wrap(env, o, ptr::null_mut()).unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd add a comment explaining why we're removing the wrap:

  • We can only guarantee our state invariants if we own the type tag
  • If we don't own the type tag, we don't know if someone else might be using the wrap
  • So we remove our wrap to allow the owner of the type tag to do what they want with the wrap state

One more thought: since we're removing the wrap, we could actually return ownership of the value back to the caller in the Err result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants