Skip to content

napi: map Float16Array in napi_get_typedarray_info/napi_create_typedarray - #34144

Merged
Jarred-Sumner merged 5 commits into
mainfrom
farm/bcdc51da/napi-float16-array
Jul 15, 2026
Merged

Jarred-Sumner merged 5 commits into
mainfrom
farm/bcdc51da/napi-float16-array

Conversation

@robobun

@robobun robobun commented Jul 14, 2026 •

Copy link
Copy Markdown
Collaborator

What

napi_is_typedarray reports true for a Float16Array, but napi_get_typedarray_info on that same value returns napi_invalid_arg (leaving every out-param untouched), and napi_create_typedarray(napi_float16_array, ...) also returns napi_invalid_arg. Node.js v26 ships napi_float16_array unconditionally (NODE_API_HAS_FLOAT16_ARRAY), so any addon that accepts user-supplied typed arrays works on Node and errors on Bun the moment a Float16Array crosses the ABI.

// view = new Float16Array([1.5, 2, 3, 4])
napi_is_typedarray(env, view, &is);                 // node: 1   bun: 1
napi_get_typedarray_info(env, view, &t, &len, ...); // node: ok, type=11, len=4
                                                    // bun : napi_invalid_arg
napi_create_typedarray(env, napi_float16_array, 4, ab, 0, &out);
                                                    // node: ok, [object Float16Array]
                                                    // bun : napi_invalid_arg

Why

The napi_typedarray_type ⇄ JSC type tables stop at napi_biguint64_array:

  • napi_typedarray_type::from_js_type in src/runtime/napi/napi_body.rs has no Float16Array arm, so napi_get_typedarray_info bails with invalid_arg when the caller asks for the type.
  • getTypedArrayTypeFromNAPI, createArrayBufferView, and the enum-range guard in napi_create_typedarray (src/jsc/bindings/napi.cpp) have no napi_float16_array case, so creation is rejected before reaching JSC.
  • The vendored src/runtime/napi/js_native_api_types.h predates the enum member entirely.

Fix

Add napi_float16_array (= 11) to the vendored header with the NODE_API_HAS_FLOAT16_ARRAY marker to match upstream, add the variant to the Rust enum and its from_js_type mapping, and add the napi_float16_array ⇄ JSC::TypeFloat16 / JSC::JSFloat16Array arms to the three switches in napi.cpp.

Test

test/napi/napi.test.ts (napi_get_typedarray_info > maps Float16Array to napi_float16_array ...) runs a native addon helper under both Bun and Node and asserts identical output: is_typedarray=1, info_status=0 type=11 length=4 byte_offset=4, first element bytes 0x3E00 (fp16 for 1.5), create_status=0, and created instanceof Float16Array=1. Before the fix, Bun reports info_status=1 type=999 and never gets to the create call.


no test proof · iteration 4 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/napi/napi.test.ts

@robobun

robobun commented Jul 14, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 2:25 PM PT - Jul 14th, 2026

❌ @robobun, your commit 7cc3ab5 has 1 failures in Build #72991 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 34144

That installs a local version of the PR into your bun-34144 executable, so you can run:

bun-34144 --bun

@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. test(napi): remove duplicate get_all_property_names helper #34141 - Both PRs remove the same duplicate get_all_property_names helper in the NAPI test addon (merge artifact from napi: fix napi_get_all_property_names(own_only, skip_strings|skip_symbols) #34126 and napi: stop napi_get_property_names poisoning JSC's per-Structure own-keys cache #34130); test(napi): remove duplicate get_all_property_names helper #34141 does only that fix, which is a strict subset of this PR

🤖 Generated with Claude Code

@robobun

robobun commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator Author

Not a duplicate: #34141 is the test-addon dedup only, this PR is the Float16Array napi mapping fix. The dedup is included here because the napi test addon does not compile on main without it, so the new test can't run otherwise. The js_test_helpers.cpp / module.js hunks are now byte-identical to #34141, so whichever lands first the other rebases cleanly.

@coderabbitai

coderabbitai Bot commented Jul 14, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Adds N-API Float16 typed-array enum support, maps Float16Array through type detection and creation, and adds standalone and concurrent regression tests covering metadata, backing storage, and constructed array instances.

Changes

Float16 typed-array support

Layer / File(s) Summary
Float16 type mapping
src/runtime/napi/js_native_api_types.h, src/runtime/napi/napi_body.rs
Adds the public and runtime float16_array enum values and maps JSType::Float16Array to the corresponding N-API type.
Float16 view creation
src/jsc/bindings/napi.cpp
Recognizes napi_float16_array and creates JSFloat16Array views through the typed-array creation path.
Float16 regression coverage
test/napi/napi-app/standalone_tests.cpp, test/napi/napi.test.ts
Tests typed-array metadata, backing-buffer offsets, Float16 creation, and Float16Array instance checks.

Possibly related PRs

  • oven-sh/bun#34132: Modifies overlapping N-API typed-array type mapping, information retrieval, and creation paths.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly states the Float16Array N-API mapping change and the affected APIs.
Description check ✅ Passed The description includes the problem, rationale, fix, and verification details required by the template.

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai 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.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/napi/napi-app/standalone_tests.cpp`:
- Around line 2661-2686: Guard the napi_instanceof call in the created-handle
validation block so it executes only when napi_create_typedarray returned
napi_ok. Preserve the existing default is_instance value and output, allowing
creation failures to be reported through create_status without passing the null
created handle to napi_instanceof.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 5a1e56a7-e762-46a7-83e0-3a22027b8610

📥 Commits

Reviewing files that changed from the base of the PR and between 616c623 and e7a7db7.

📒 Files selected for processing (7)
  • src/jsc/bindings/napi.cpp
  • src/runtime/napi/js_native_api_types.h
  • src/runtime/napi/napi_body.rs
  • test/napi/napi-app/js_test_helpers.cpp
  • test/napi/napi-app/module.js
  • test/napi/napi-app/standalone_tests.cpp
  • test/napi/napi.test.ts
💤 Files with no reviewable changes (1)
  • test/napi/napi-app/js_test_helpers.cpp

Comment thread test/napi/napi-app/standalone_tests.cpp
Comment thread test/napi/napi.test.ts Outdated

@claude claude 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.

LGTM — the Windows CRLF fix in ee4e350 addresses my earlier comment.

Checked: the three napi.cpp switch arms map to existing JSC::TypeFloat16/JSC::JSFloat16Array, matching sibling cases exactly; jsc::JSType::Float16Array exists for the Rust match arm; enum ordinal 11 matches upstream Node's napi_float16_array.
Verified fp16(1.5) = 0x3E00 for the test's expected output, and checkSameOutput cross-validates against Node.
The js_test_helpers.cpp/module.js dedup is byte-identical to #34141 as stated.

Extended reasoning...

Overview

Adds napi_float16_array support to Bun's N-API layer: one enum member in the vendored js_native_api_types.h, one Rust enum variant + match arm in napi_body.rs, and three switch-case arms in napi.cpp (getTypedArrayTypeFromNAPI, createArrayBufferView, and the napi_create_typedarray range guard). A native test helper in standalone_tests.cpp plus a checkSameOutput case in napi.test.ts exercise both directions. Also carries the #34141 test-addon dedup so the addon compiles.

Security risks

None. This is an enum-table extension in the N-API compatibility layer; no new allocations, lifetimes, or untrusted-input parsing.

Level of scrutiny

Low-to-medium. The production diff is purely mechanical — every added arm copies the exact shape of the adjacent napi_biguint64_array case with the type name swapped. I verified JSC::TypedArrayType::TypeFloat16, JSC::JSFloat16Array, and jsc::JSType::Float16Array all already exist in the codebase, and that the enum value 11 (following biguint64_array = 10) matches upstream Node.js. The #define NODE_API_HAS_FLOAT16_ARRAY marker mirrors Node's header. No CODEOWNERS cover these paths.

Other factors

Both prior review comments are addressed and resolved: CodeRabbit's napi_instanceof null-guard (1df2842) and my Windows CRLF split (ee4e350). The bug-hunting system found nothing on the current revision. The test is strong — checkSameOutput runs the same native helper under both Bun and an ABI-matching Node and asserts byte-identical stdout, so any divergence in type id, length, byte offset, data pointer arithmetic, or instanceof result would fail. The fp16 bit pattern for 1.5 (0x3E00) checks out.

@robobun

robobun commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator Author

CI on ee4e350: the new napi_get_typedarray_info > maps Float16Array ... test passed on every lane that ran napi.test.ts (no napi annotations on build 72857).

The reds are unrelated to this diff:

  • test/js/node/test/parallel/test-net-connect-memleak.js on alpine x64/x64-baseline: GC-timing assertion from the net/tls port, nothing to do with napi typedarray enum tables. Being tracked separately.
  • test-worker-message-port-transfer-terminate.js: pre-existing JSC assertion on the asan lane.
  • One :windows: x64-baseline shard failed Azure VM provisioning before running anything.
  • es-module-lexer, spawn.test.ts, bun-run-dir, webview-chrome, require-cache, test-net-write-slow: all flaked once and passed on retry.

Ready for review.

robobun added 5 commits July 14, 2026 18:25
…rray

napi_is_typedarray already reported true for a Float16Array, but both
napi_get_typedarray_info and napi_create_typedarray(napi_float16_array)
returned napi_invalid_arg because the napi_typedarray_type <-> JSC type
tables stopped at biguint64.

Add napi_float16_array (= 11, matching Node's js_native_api_types.h) to
the vendored header, to the Rust napi_typedarray_type enum and its
from_js_type mapping, and to the three switches in napi.cpp that drive
napi_create_typedarray.

Also dedup a get_all_property_names helper in the napi test addon that
was left doubly defined after #34126 and #34130 both added one; the
addon did not compile on main.
@robobun
robobun force-pushed the farm/bcdc51da/napi-float16-array branch from ee4e350 to 7cc3ab5 Compare July 14, 2026 18:27

@claude claude 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.

LGTM — mechanical enum-table extension for napi_float16_array, prior review feedback addressed and CI green.

What was reviewed:

  • The three C++ switch arms and the Rust from_js_type arm follow the sibling entries exactly; JSC::TypeFloat16/JSFloat16Array/jsc::JSType::Float16Array all already exist.
  • Grepped for other napi_typedarray_type ⇄ JSC mapping tables — none missed (array_buffer.rs already handles Float16 independently).
  • Test compares byte-for-byte against Node via checkSameOutput; the Windows CRLF and null-created napi_instanceof concerns from earlier review rounds are both fixed in the current diff.
Extended reasoning...

Overview

Adds napi_float16_array (= 11) to the N-API typed-array enum and wires it through every mapping table that previously stopped at napi_biguint64_array: the vendored js_native_api_types.h header (with the upstream NODE_API_HAS_FLOAT16_ARRAY marker), the Rust napi_typedarray_type enum and its from_js_type match, and three switches in napi.cpp (getTypedArrayTypeFromNAPI, createArrayBufferView, and the napi_create_typedarray range guard). A native test helper and a checkSameOutput test in napi.test.ts exercise both napi_get_typedarray_info and napi_create_typedarray against Node.

Security risks

None. This is a type-mapping table extension for a typed-array kind JSC already supports; no new parsing of untrusted input, no allocation-size arithmetic, no auth/crypto surface.

Level of scrutiny

Low-to-moderate. Each new arm is a one-line copy of the immediately preceding sibling with the type name substituted. I verified via grep that JSC::TypedArrayType::TypeFloat16, JSC::JSFloat16Array, and jsc::JSType::Float16Array are all pre-existing types already used in array_buffer.rs, JSType.rs, and other bindings, so there is no risk of a dangling reference. I also grepped src/ for every occurrence of napi_typedarray_type and napi_biguint64_array to confirm no other mapping table was missed — the only hits are the three files this PR touches plus a stale layering comment in array_buffer.rs.

Other factors

  • Both prior review comments (my Windows CRLF note and CodeRabbit's napi_instanceof null-handle guard) are addressed and marked resolved; the current diff reflects both fixes.
  • The test asserts exact status codes, type=11, length/byte_offset, the fp16 bit pattern 0x3E00 for 1.5, and instanceof Float16Array, all cross-checked against Node in the same run.
  • The author reports the new test passed on every CI lane; remaining reds are unrelated pre-existing flakes.
  • No CODEOWNERS entries cover src/runtime/napi/ or src/jsc/bindings/napi.cpp.
  • The earlier js_test_helpers.cpp/module.js dedup hunks were dropped after #34133 landed, so the diff is now purely the Float16 change.

@robobun

robobun commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator Author

CI on 7cc3ab5 (rebased): the new Float16Array test passed on every lane of build 72991. The only red annotation is test/js/node/test/parallel/test-net-connect-memleak.js (pre-existing on main, unrelated to napi typedarray enum tables). Everything else was [flaky] and passed on retry. Ready to merge.

@Jarred-Sumner
Jarred-Sumner merged commit 9f5cc6b into main Jul 15, 2026
75 of 77 checks passed
@Jarred-Sumner
Jarred-Sumner deleted the farm/bcdc51da/napi-float16-array branch July 15, 2026 00:09
robobun added a commit that referenced this pull request Jul 15, 2026
Resolve conflicts with #34144 (napi_float16_array): add Float16Array
to the NAPI_TYPED_ARRAY_CASE switch in napi_create_typedarray, keep
both the new view-validation tests and main's test_napi_float16_array,
and simplify the napi_get_typedarray_info guard now that from_js_type
covers every typed-array JSType.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants