Conversation
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
LGTM — pure dedup: jsc::URL becomes a re-export of bun_url::whatwg::URL plus a UrlJsc extension trait, matching the existing StringJsc/SysErrorJsc pattern. The inline nit is doc-only.
Extended reasoning...
Overview
This PR collapses a duplicate opaque FFI type. src/jsc/URL.rs previously defined its own bun_opaque::opaque_ffi! { pub struct URL; } plus ~15 getter/constructor wrappers that were byte-for-byte equivalents of bun_url::whatwg::URL (src/url/lib.rs:48-211). It now pub uses that type and keeps only the two JSC-dependent entry points (from_js, href_from_js) as a UrlJsc extension trait — the same pattern already used for StringJsc, SysErrorJsc, ZigStringJsc, and SliceWithUnderlyingStringJsc in src/jsc/lib.rs. lib.rs re-exports the trait; fetch.rs imports it as UrlJsc as _.
Security risks
None. No parsing, validation, allocation, or FFI signature changes. Both the deleted and the retained URL are the same bun_opaque::opaque_ffi! ZST handle, so URL__fromJS(...) -> *mut URL is ABI-identical before and after.
Level of scrutiny
Low. This is code motion + deletion, split 2/8 from #32023 at a maintainer's request, with the union verified byte-identical to the CI-green parent. I checked every removed method has an equivalent on bun_url::whatwg::URL (getters, from_string/from_utf8, file_url_from_string/path_from_file_url, destroy, hash/fragment_identifier), and grepped call sites: href_from_js is only used in fetch.rs (trait now imported); URL::destroy in js_valkey.rs/SocketAddress.rs and URL::path_from_file_url/file_url_from_string in BunObject.rs/JSSourceMap.rs/fetch.rs all resolve to inherent methods on the re-exported type without needing the trait.
Other factors
The one finding is a doc-drift nit in src/CLAUDE.md (URL::href_from_string is now only a free function, not an associated fn) — no callers use that spelling, so nothing breaks. This doesn't warrant blocking a mechanical dedup.
There was a problem hiding this comment.
LGTM — pure deduplication, the deleted jsc::URL surface is byte-for-byte covered by bun_url::whatwg::URL. The one inline nit (4-line comment) is style-only and non-blocking.
Extended reasoning...
Overview
This PR replaces src/jsc/URL.rs's duplicate URL struct + inherent impl with pub use bun_url::whatwg::URL, moves the two JSC-dependent entry points (from_js, href_from_js) onto a new UrlJsc extension trait, re-exports that trait from bun_jsc's crate root, adds the trait import to the sole caller (fetch.rs), and updates src/CLAUDE.md's URL section. Net: ~130 lines deleted, ~20 added. Split 2 of 8 from #32023.
Security risks
None. No new parsing, no new FFI surface, no user-input handling changes. The two remaining extern "C" decls (URL__fromJS, URL__getHrefFromJS) and their call_check_slow wrappers are unchanged from the pre-PR bodies.
Level of scrutiny
Low-to-medium. Pure code motion/deletion with no intended behavior change. I verified:
- Every inherent method deleted from
src/jsc/URL.rs(from_string,from_utf8,destroy,protocol/href/username/password/search/host/hostname/port/pathname/hash/fragment_identifier,file_url_from_string,path_from_file_url) exists onbun_url::whatwg::URL(src/url/lib.rs:128-210) with matching signatures, so callers inSocketAddress.rs,BunObject.rs,js_valkey.rs, andJSSourceMap.rsstill resolve. URL::href_from_js/URL::from_jscallers: onlyfetch.rs(4 sites), which gets theUrlJsc as _import in this PR. No other file calls these, so no missing trait imports.- The
*mut URLinURL__fromJS's return type now namesbun_url::whatwg::URLinstead of the localopaque_ffi!struct — both are opaque#[repr(C)]handles to the same C++WTF::URL, so the ABI is unchanged. href_from_string/joinwere dropped as associated fns (free-function-only inbun_url); grep confirms zeroURL::href_from_string/URL::joincall sites, andsrc/CLAUDE.mdwas updated in d912d24 to reflect this (my previous nit, now resolved).
Other factors
- The prior review nit (stale
URL::href_from_stringdoc reference) was addressed in d912d24 and the thread is resolved. - The comment-cop bot flagged three comments; the inline nit on this run clarifies that only
URL.rs:6-9is a real 3-line-max violation (the other two flags are 2-line doc comments and can be dismissed). This is style-only and doesn't affect correctness — fine to fix on merge or in a follow-up. - The parent PR's CI ran 285/286 green on the byte-identical union of all splits.
There was a problem hiding this comment.
LGTM — pure code motion, both prior nits addressed.
What was reviewed:
- Confirmed every deleted inherent method (
from_string/from_utf8/destroy/getters/whole-string conversions) exists onbun_url::whatwg::URLwith matching signatures. - Grepped all
jsc::URL::call sites: onlyfetch.rsuses the trait methods (href_from_js), and it importsUrlJsc as _; the rest (JSSourceMap.rs,BunObject.rs,SocketAddress.rs,js_valkey.rs) use inherent methods that resolve through the re-export. - Verified 9ba009a dropped the 4-line comment and d912d24 fixed the
src/CLAUDE.mddoc drift; comment-cop flags on the two 2-line doc comments were false positives.
Extended reasoning...
Overview
Split 2 of 8 from #32023. Replaces the duplicate impl URL in src/jsc/URL.rs with pub use bun_url::whatwg::URL; and moves the two JSC-dependent entry points (href_from_js, from_js) onto a new UrlJsc extension trait. src/jsc/lib.rs re-exports the trait alongside URL and drops a stale comment. fetch.rs gains a one-line UrlJsc as _ import (the only caller of the trait methods). src/CLAUDE.md updates the URL-parsing section to reflect the new shape.
Security risks
None. No parsing, validation, or FFI-boundary logic changed — the extern-C bodies of href_from_js/from_js are byte-identical to before (same call_check_slow wrapper, same URL__getHrefFromJS/URL__fromJS externs). All deleted methods are provably present on the re-exported type.
Level of scrutiny
Low. This is mechanical deduplication of an internal Rust type re-export, part of a maintainer-requested split of an already-reviewed parent PR. The union of splits is stated byte-identical to the parent's CI-green head. I verified via grep that every jsc::URL:: call site in the tree resolves either through the re-exported inherent methods or (for fetch.rs only) through the imported trait, so no callers are broken.
Other factors
Both nits from my earlier passes are resolved: the href_from_string doc reference now points at the free function (d912d24), and the over-length comment above the re-export was dropped entirely (9ba009a). All inline threads are marked resolved. The remaining comment-cop flags on 2-line doc comments were false positives per rule #13's 3-line limit. No outstanding reviewer feedback.
There was a problem hiding this comment.
LGTM — both earlier nits (doc drift, 4-line comment) are addressed; pure dedup with no behavior change.
What was reviewed:
- Verified every deleted inherent method on the old
jsc::URL(getters,from_string/from_utf8,file_url_from_string/path_from_file_url,destroy) exists with matching signatures onbun_url::whatwg::URL, and both types arebun_opaque::opaque_ffi!ZSTs soURL__fromJS -> *mut URLis ABI-identical. - Grepped for all
URL::from_js/URL::href_from_jscallers — onlyfetch.rs, which now importsUrlJsc as _. - New tests are additive and exercise the rewired paths (URL-object arg, invalid-proxy Dead-tag branch,
preconnecttoString propagation).
Extended reasoning...
Overview
Split 2 of 8 from #32023. src/jsc/URL.rs (153 → 30 lines) drops its duplicated opaque URL type and inherent impl block in favor of pub use bun_url::whatwg::URL, keeping only the two JSC-dependent entry points (href_from_js, from_js) as a new UrlJsc extension trait. src/jsc/lib.rs re-exports the trait and drops a stale comment; fetch.rs gains UrlJsc as _ in its import list; src/CLAUDE.md is updated to describe the re-export; two test files gain coverage for the rewired call sites.
Security risks
None. No parsing, validation, or auth logic changes — the same URL__fromJS / URL__getHrefFromJS C++ externs are called through identical call_check_slow wrappers. The opaque handle type is nominally different but structurally identical (bun_opaque::opaque_ffi! in both crates), so the FFI ABI is unchanged.
Level of scrutiny
Low-to-medium. This is mechanical deduplication of a duplicated FFI wrapper against its canonical definition in bun_url. I confirmed via grep that bun_url::whatwg::URL carries every method being deleted here (including destroy), and that fetch.rs is the only Rust file invoking the two methods that moved to the trait. The bodies of href_from_js/from_js are byte-identical to the pre-PR inherent impls.
Other factors
Both nits from my earlier passes are resolved: the src/CLAUDE.md URL section now points at the free bun_url:: functions and documents UrlJsc, and the 4-line explanatory comment above the re-export was dropped in 9ba009a. All inline threads (including the comment-cop flags) are marked resolved. The added tests follow existing patterns in their files and lock in the observable behavior of the rewired entry points. The parent PR's CI ran 285/286 green on the union of all splits.
2f5dd36 to
9ae3ff5
Compare
|
Rebased cleanly onto the updated base branch to clear the merge conflict; diff content is unchanged and tests pass locally. CI running on bf0847d. Review is LGTM with all threads resolved. |
a7e3331 to
03f04da
Compare
There was a problem hiding this comment.
LGTM — all three earlier findings are addressed (whatwg::URL now uses opaque_ffi! so auto-trait bounds are preserved, the dead free path_from_file_url is dropped, and the proxy tests use the describe-scoped url).
What was reviewed:
jsc::URL→bun_url::whatwg::URLre-export: getters/constructors/destroyare byte-equivalent to the deleted jsc versions;from_stringwidenedpub(crate)(&String)→pub(String)is behaviorally identical sincebun_core::String: Copy.UrlJsctrait:from_js/href_from_jsbodies unchanged;fetch.rsimports the trait so method resolution still finds them.- Duplicate
URL__*extern declarations across the two crates name the same C symbols with identical signatures — harmless. - New tests exercise the rewired entry points (URL-object arg, invalid-proxy Dead-tag branch, preconnect
toStringthrow) and follow sibling conventions.
Extended reasoning...
Overview
Collapses the duplicated jsc::URL opaque-FFI surface into a pub use bun_url::whatwg::URL, keeping only the two JSC-dependent entry points (from_js, href_from_js) as a UrlJsc extension trait. bun_url gains bun_opaque as a dep so whatwg::URL is declared via opaque_ffi! (restoring !Send + !Sync + !Freeze + !Unpin), and picks up the getters (username/password/host/port), destroy(), and by-value file_url_from_string/path_from_file_url that previously lived only on the jsc twin. fetch.rs gets a one-line UrlJsc as _ import; src/CLAUDE.md is updated; five small tests lock in the rewired paths. Split 2 of 8 from #32023, which a maintainer asked to be broken up.
Security risks
None. Pure code motion around an opaque WTF::URL FFI handle. No new parsing, no user-input validation changes, no auth/crypto/permissions surface. The extern "C" declarations that now appear in both crates name identical C symbols with identical signatures (Rust permits redundant extern decls; the linker resolves to one definition).
Level of scrutiny
Moderate. It touches FFI type identity and cross-crate re-exports, which merits care around auto-trait bounds and ABI — but that was the substance of my earlier review, and the fix (commit 739eb49) uses opaque_ffi! exactly as suggested, so jsc::URL is now nominally and structurally the same type it was before. Every deleted method on the jsc side has a byte-equivalent counterpart on the re-exported type; the only signature change (from_string &String → String by value, pub(crate) → pub) is a no-op given bun_core::String: Copy and is required for jsc callers to reach it.
Other factors
Three prior inline findings from this reviewer were all addressed in b346c21 and 739eb49. The bug-hunting pass on the current head found nothing. The one remaining unresolved thread (the opaque_ffi! note at src/url/lib.rs:185) is fixed in code — the GitHub thread just wasn't clicked resolved. The unresolved comment-cop bot flag at line 50 targets the pre-existing extern "C" SAFETY block comment, not new code. New tests follow the sibling conventions in their files (await Bun.sleep(2) matches every neighbor in fetch-args.test.ts's "does not send a request when" block).
b5b9aaa to
23cbfb4
Compare
…, now live again via the jsc re-export
…free path_from_file_url
…to-trait opt-outs
739eb49 to
bf0847d
Compare
| /// The host WITHOUT the port (opposite of JS `host`; with-port form is [`URL::hostname`]). | ||
| pub fn host(&self) -> String { | ||
| URL__host(self) |
There was a problem hiding this comment.
🟡 The hostname() doc at line 150 still points to "bun_jsc::URL::host" for the port-less form — a reference written when bun_jsc::URL was a distinct type. This PR unifies them (jsc::URL is now a re-export of this struct), so that reference is now circular, names an upward-tier crate bun_url doesn't depend on, and is inconsistent with the [URL::hostname] intradoc form the new host() doc at line 164 uses. Update it to [URL::host].
Extended reasoning...
What the issue is
The hostname() doc comment in src/url/lib.rs (lines 147-150) reads:
/// Returns the host WITH the port.
///
/// Note that this does NOT match JS `hostname`, which excludes the port (that
/// port-less form is `bun_jsc::URL::host`).
This PR adds host() to the same impl block a few lines below (line 164) with the reciprocal cross-reference written as an intradoc link:
/// The host WITHOUT the port (opposite of JS `host`; with-port form is [`URL::hostname`]).
pub fn host(&self) -> String { ... }So one direction of the host↔hostname cross-reference pair uses the intradoc [URL::…] form, and the other still uses the old cross-crate bun_jsc::URL::host spelling.
Why the old spelling is now stale
Before this PR, bun_jsc::URL (declared via opaque_ffi! in src/jsc/URL.rs) and bun_url::whatwg::URL were two distinct nominal types. The hostname() doc's "that port-less form is bun_jsc::URL::host" was accurate: it pointed at a genuinely different type that carried host(), while whatwg::URL did not.
This PR changes that: src/jsc/URL.rs:6 now reads pub use bun_url::whatwg::URL;, and host() is added directly to whatwg::URL at line 165. So after this PR:
bun_jsc::URL::hostandbun_url::whatwg::URL::hostare the same method — the reference is now self-referential rather than pointing at a sibling type.bun_urldoes not depend onbun_jsc(bun_jscis upward-tier and depends onbun_url), sobun_jsc::URL::hostcannot resolve as an intradoc link from this crate and reads as if there were still two URL types.- The PR itself establishes the
[URL::…]convention on the newhost()doc, so leaving the old spelling onhostname()makes the pair internally inconsistent within the sameimplblock the PR is editing.
Step-by-step proof
- Pre-PR:
src/jsc/URL.rscontainedbun_opaque::opaque_ffi! { pub struct URL; }with an inherentfn host(&self)— a distinct type frombun_url::whatwg::URL. Thehostname()doc'sbun_jsc::URL::hostreference correctly pointed cross-crate at that other type. - This PR deletes
jsc::URL's inherenthost()and replaces the struct withpub use bun_url::whatwg::URL;(src/jsc/URL.rs:6). - This PR adds
fn host(&self)atsrc/url/lib.rs:165, in the sameimpl URLblock ashostname(). - Therefore
bun_jsc::URL::hostnow resolves (via re-export) to line 165 — the very method sitting 15 lines below the doc comment that references it. - This PR writes the new
host()doc as "with-port form is [URL::hostname]" — the correct local intradoc form. - The reciprocal
hostname()doc at line 150 was not updated to match, leaving one direction stale and the other current.
Why existing checks don't catch it
The reference is written as a plain code span (`bun_jsc::URL::host`), not an intradoc link ([…]), so rustdoc's broken-link lint stays silent. And since bun_jsc re-exports this exact type, anyone following the reference by hand would land back at line 165 — technically correct, just misleadingly circuitous.
Impact
None at runtime — doc-only. The staleness is created by this PR (via the type unification) and the inconsistency is introduced by this PR (one direction of the cross-reference pair updated, the other not), so it's in scope to fix here rather than pre-existing. REVIEW.md's "Match the exact file's local conventions" and "One source of truth; update every consumer atomically" both apply: the PR touched this impl block heavily and established the [URL::…] convention on host().
Suggested fix
Change line 150 to use the same intradoc form the PR uses on line 164:
/// Note that this does NOT match JS `hostname`, which excludes the port (that
/// port-less form is [`URL::host`]).
What this does
jsc::URLduplicated the JSC-agnostic surface ofbun_url::whatwg::URL(constructors, getters,destroy, whole-string conversions). It is now apub useof that type; only the two entry points that needJSValue/JSGlobalObject(from_js,href_from_js) remain here, as theUrlJscextension trait.fetch.rsimports the trait (this was the compile break in the original PR's first CI run).Split 2 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).Verification
cargo check), and the union of all eight splits is byte-identical to Deduplicate jsc bindings, runtime api, test matchers, and shell builtins #32023's headf29ce8fe4, which CI ran at 285/286 green (the one red was the fleet-widenapi.test.tsGC-timing flake on windows x64-baseline).fetch(new URL(...)), the invalid-proxy rejection (string and{url}object forms, thehref_from_jsDead-tag branch), andfetch.preconnectwith a URL object, an unparseable string, and a throwingtoString(theJsResultpropagation path). All pass identically on the release build and this branch's debug build.no test proof · iteration 9 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/fetch/fetch-preconnect.test.ts