Skip to content

Deduplicate small helpers across crypto, api objects, sockets, valkey, napi, bake, and coverage - #37366

Open
robobun wants to merge 11 commits into
claude/split/install-clifrom
farm/df8b29f2/split-api-misc
Open

Deduplicate small helpers across crypto, api objects, sockets, valkey, napi, bake, and coverage#37366
robobun wants to merge 11 commits into
claude/split/install-clifrom
farm/df8b29f2/split-api-misc

Conversation

@robobun

@robobun robobun commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

What this does

A collection of independent, file-local dedups that did not warrant their own PRs: CryptoHasher digest-output parsing (output_digest, shared by the five digest* entry points), PasswordObject verify-algorithm parsing (shared by verify/verifySync), HashObject / JSON5Object (delegates to the shared bun_js_parser_jsc::expr_to_js) / MarkdownObject / csrf_jsc option parsing, ServerWebSocket and udp_socket send-path argument handling, socket_body's four identical callback-dispatch tails (call_socket_handler), the valkey command table, the NAPI napi_create_string_* prologue (napi_string_slice + set_string_result), bake's three init_transpiler variants (init_transpiler_impl), and CodeCoverage report emission.

Split 8 of 8 from #32023, which alii asked to be broken up. Pure code motion and deletion; no intended behavior change. Stacked on claude/split/install-cli (merge order: foundations → install-cli → these, in any order among themselves).

Notes from rebasing against main

Most of the conflict resolution from the parent PR's two rebases landed in these files; the surviving code keeps main's semantics in every case: socket_body uses main's Rc<Handlers> / exit_scope API (#31859, #33388); napi routes main's transfer_to_js leak fix (#32303) through the shared helper; JSON5Object keeps OwnedString from #32331; MarkdownObject routes both call sites through main's parser_err_to_js (which gained InputTooLarge/TooManyBlocks in #32752/#33078); csrf calls main's get_optional_slice method (#32355); bake signatures use crate::Result (#33909). PasswordObject::verify_sync's unknown-algorithm-with-pending-exception branch returned Ok(JSValue::ZERO) and now returns Err(JsError::Thrown) like its async twin; the host_fn boundary maps both to the same zero return (src/jsc/host_fn.rs).

Verification

alii and others added 11 commits June 9, 2026 13:19
Resolved src/io/posix_event_loop.rs: #32594 added PollMemoryPressure to
is_registered(), which this branch moved into impl_file_poll_flag_methods;
carried the flag into the macro body.

Restored ArrayHashMap::get_index_mut: gained a caller in
react_compiler/collections.rs via #32504.
Resolved src/bun_core/string/mod.rs: #33909 switched return types to
crate::CrateResult<()> in write_pre_quoted_string and quote_for_json,
which this branch restructures into the const-generic
write_pre_quoted_string_inner; carried CrateResult<()> into both the
adapter and _inner signatures.

Resolved src/router/lib.rs and src/sourcemap/Chunk.rs: #33909 touched
return types inside blocks this branch deletes (orphaned router test
scaffolding, uncalled print_source_map_contents); kept the deletions
after re-verifying zero callers on current main.

Restored Node::find_last in src/collections/pool.rs: gained a test
caller via #33311.

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

I reviewed this PR and didn't find any bugs. Because it consolidates logic across 17 files in many independent subsystems (napi FFI, socket callback dispatch, crypto digest paths, bake transpiler init, the valkey command-macro table, JSON5→shared expr_to_js), a human look would still be worthwhile — several of the dedups delegate to shared implementations whose edge-case behavior (e.g. object-key handling in bun_js_parser_jsc::expr_to_js, the unified BuildConfigSubset type) isn't byte-identical to what was removed.

Checked and ruled out:

  • is_array_buffer_like() matches the exact 14-variant list it replaces in HashObject.
  • The valkey cmd!/cmd_varargs! collapse preserves each call site's null-handling mode (strict/skip_null/required) against the original 7 macros.
  • napi_string_slice preserves the null/length/NAPI_AUTO_LENGTH branch order and transfer_to_js routing.
  • StaticCryptoHasher::digest_to_bytes still reads output_digest_slice (which aliases output_digest_buf in the None arm) after the output_digest refactor.
Extended reasoning...

Overview

This PR is split 8/8 of #32023 and deduplicates local helper logic across 17 files: CryptoHasher digest-output parsing, PasswordObject verify-algorithm parsing, HashObject/JSON5Object/MarkdownObject/csrf_jsc argument parsing, ServerWebSocket.publish* prologues, udp_socket setsockopt paths, socket_body callback-dispatch tails, the valkey command-macro table (7 macros → 2), the NAPI napi_create_string_* prologue, bake's init_transpiler variants (merged into init_transpiler_impl with BuildConfigSubset unified to a single type), the DevServer field-destructure exhaustiveness check (macro-ized), and CodeCoverage byte-offset→line resolution.

Security risks

None identified. The changes are refactors of argument parsing and dispatch; no new trust boundaries, no changes to validation ordering that would weaken checks. The CSRF option-parsing helpers preserve the same algorithm allowlist and non-empty-string checks.

Level of scrutiny

High. Although framed as pure code motion, several consolidations are semantic delegations rather than cut-and-paste: JSON5's expr_to_js now routes through bun_js_parser_jsc::expr_to_js (which uses put_to_property_key and create_empty_array+put_index instead of the removed put_may_be_index/create_array_from_iter, and errors on unknown node types instead of returning undefined); the bake keystone BuildConfigSubset mirror struct is deleted in favor of re-exporting bake_body::BuildConfigSubset (which carries two extra fields); and the valkey macro collapse re-encodes per-command null-handling as a mode token. These touch napi FFI (unsafe slice construction), socket re-entrancy paths, and bundler configuration — all areas where subtle divergence is easy to miss.

Other factors

No bugs were found by the multi-agent bug hunter. I spot-checked is_array_buffer_like (exact match), the napi slice helper (branch order preserved, transfer_to_js still used), the output_digest refactor in StaticCryptoHasher (the None branch still reads the fallback buffer via the returned &mut), and traced each valkey call site's mode against its original macro. The PR description documents the one intentional semantic change (verify_sync returning Err(JsError::Thrown) instead of Ok(JSValue::ZERO), which the host_fn boundary maps identically). Given the breadth (17 files, ~10 subsystems) and that this is the final split of a series a maintainer explicitly asked to be broken up, deferring to human review.

@robobun

robobun commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 6:05 AM PT - Aug 11th, 2026

@alii, your commit 9f5a05c has some failures in Build #91741 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 37366

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

bun-37366 --bun

@robobun
robobun force-pushed the claude/split/install-cli branch from 2f5dd36 to 9ae3ff5 Compare August 11, 2026 02:09
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