Skip to content

fix(napi-derive): make_ref with Vec<&S> cases#2589

Merged
Brooooooklyn merged 2 commits into
mainfrom
05-02-fix_napi-derive_make_ref_with_vec_s_cases
May 2, 2025
Merged

fix(napi-derive): make_ref with Vec<&S> cases#2589
Brooooooklyn merged 2 commits into
mainfrom
05-02-fix_napi-derive_make_ref_with_vec_s_cases

Conversation

@Brooooooklyn

@Brooooooklyn Brooooooklyn commented May 2, 2025

Copy link
Copy Markdown
Member

Brooooooklyn commented May 2, 2025

Copy link
Copy Markdown
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@Brooooooklyn Brooooooklyn requested a review from Copilot May 2, 2025 14:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 fixes the handling of Vec<&S> cases in napi-derive and adjusts the way references are created in threadsafe functions. Key changes include:

  • Updating the Ref::new invocation in the tsfn module.
  • Refactoring inline closure usage into a standalone make_ref function.
  • Enhancing type-checking logic to correctly detect Vec with reference types.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
examples/napi-compat-mode/src/napi4/tsfn.rs Updated Ref::new invocation to match the expected API signature.
crates/backend/src/codegen/fn.rs Removed the inline make_ref closure, introduced a helper function, and added logic to detect Vec<&S> cases.
Comments suppressed due to low confidence (2)

examples/napi-compat-mode/src/napi4/tsfn.rs:89

  • Changing the parameter from '&ctx.env' to 'ctx.env' appears intentional; please ensure this adjustment aligns with the updated Ref::new API and does not introduce any unexpected ownership or lifetime issues.
let option_ref = Ref::new(ctx.env, &options);

crates/backend/src/codegen/fn.rs:764

  • [nitpick] Extracting 'make_ref' into a dedicated function improves code reuse; ensure that all previous inline references to the make_ref closure have been correctly updated and that its behavior is covered by tests.
fn make_ref(input: TokenStream) -> TokenStream {

Comment thread crates/backend/src/codegen/fn.rs Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Benchmark

Details
Benchmark suite Current: 702e78a Previous: 3c169b7 Ratio
noop#napi-rs 83911031 ops/sec (±0.3%) 84181770 ops/sec (±0.17%) 1.00
noop#JavaScript 817161831 ops/sec (±0.11%) 817602110 ops/sec (±0.1%) 1.00
Plus number#napi-rs 26757752 ops/sec (±0.45%) 26811460 ops/sec (±0.55%) 1.00
Plus number#JavaScript 814719297 ops/sec (±0.31%) 817187605 ops/sec (±0.1%) 1.00
Create buffer#napi-rs 620921 ops/sec (±13.06%) 651230 ops/sec (±12.44%) 1.05
Create buffer#JavaScript 3494405 ops/sec (±2.37%) 3910832 ops/sec (±2.9%) 1.12
createArray#createArrayJson 55315 ops/sec (±0.34%) 55318 ops/sec (±0.49%) 1.00
createArray#create array for loop 9658 ops/sec (±0.52%) 9962 ops/sec (±0.15%) 1.03
createArray#create array with serde trait 9709 ops/sec (±0.2%) 10307 ops/sec (±0.5%) 1.06
getArrayFromJs#get array from json string 23249 ops/sec (±0.43%) 23066 ops/sec (±0.33%) 0.99
getArrayFromJs#get array from serde 13029 ops/sec (±0.59%) 12384 ops/sec (±0.72%) 0.95
getArrayFromJs#get array with for loop 15985 ops/sec (±0.29%) 16021 ops/sec (±0.67%) 1.00
Get Set property#Get Set from native#u32 548743 ops/sec (±12.34%) 575650 ops/sec (±12.24%) 1.05
Get Set property#Get Set from JavaScript#u32 524183 ops/sec (±2.5%) 543464 ops/sec (±2.27%) 1.04
Get Set property#Get Set from native#string 547572 ops/sec (±11.36%) 560408 ops/sec (±10.71%) 1.02
Get Set property#Get Set from JavaScript#string 514894 ops/sec (±1.78%) 512758 ops/sec (±2.47%) 1.00
Async task#spawn task 26443 ops/sec (±0.27%) 25913 ops/sec (±1.05%) 0.98
Async task#ThreadSafeFunction 9444 ops/sec (±0.73%) 9569 ops/sec (±0.76%) 1.01
Async task#Tokio future to Promise 32092 ops/sec (±1.08%) 31887 ops/sec (±0.41%) 0.99
Query#query * 100 3641 ops/sec (±0.5%) 3657 ops/sec (±0.59%) 1.00
Query#query * 1 27361 ops/sec (±0.84%) 27922 ops/sec (±0.75%) 1.02

This comment was automatically generated by workflow using github-action-benchmark.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@Brooooooklyn Brooooooklyn force-pushed the 05-02-fix_napi-derive_make_ref_with_vec_s_cases branch from c0fe655 to 702e78a Compare May 2, 2025 15:02

Brooooooklyn commented May 2, 2025

Copy link
Copy Markdown
Member Author

Merge activity

  • May 2, 11:13 AM EDT: A user started a stack merge that includes this pull request via Graphite.
  • May 2, 11:13 AM EDT: @Brooooooklyn merged this pull request with Graphite.

@Brooooooklyn Brooooooklyn merged commit ebcf0ea into main May 2, 2025
@Brooooooklyn Brooooooklyn deleted the 05-02-fix_napi-derive_make_ref_with_vec_s_cases branch May 2, 2025 15:13
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.

Use after free for Vec<&Elem> in async funtions

2 participants