Purge remaining port-batch markers missed by the phase cleanup#30885
Purge remaining port-batch markers missed by the phase cleanup#30885Jarred-Sumner wants to merge 20 commits into
Conversation
|
Updated 8:31 PM PT - May 17th, 2026
✅ @Jarred-Sumner, your commit c1238ca547f36e4aa9ddd7db37263886915a855f passed in 🧪 To try this PR locally: bunx bun-pr 30885That installs a local version of the PR into your bun-30885 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughUpdates and standardizes comments, porting notes, and small formatting across many Rust files; includes two behavior-adjacent edits: explicit host export name for ChangesDocumentation and Porting Notes Cleanup
Possibly Related PRs
Suggested Reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — pure comment cleanup (port-batch marker purge) plus rustfmt whitespace from autofix.ci; no logic changes.
Extended reasoning...
Overview
Follow-up to #30877 that removes the long tail of Zig→Rust port-batch markers (Round-[C-H], round-G2, reconciler-6, B-0/B-3, Phase C/D/F, track-A, VERIFY-FIX(round1), peechy batch 2, cycle-5, Pass C, C-7/C-9, etc.) from comments across 38 files. I verified every hunk falls into one of three buckets: (a) comment-text rewrites that drop the batch-number framing while keeping the substantive note, (b) a single use crate::renamer; reorder in p.rs after its surrounding round-D/E comment was deleted, and (c) rustfmt whitespace reflow in bin.rs, bunx_command.rs, socket_body.rs, and PostgresSQLConnection.rs applied by the autofix.ci commit. The CodeRabbit walkthrough mentioned two "functional changes" (crc32 export name, postgres SASL match) but both are pre-existing — only the adjacent comment/indentation moved.
Security risks
None. No executable code paths, string literals, attributes, or cfg gates change value. The #![allow(unexpected_cfgs)] line in ffi_body.rs only loses comment text after //; the attribute itself is untouched.
Level of scrutiny
Low. This is mechanical documentation hygiene identical in character to the already-merged #30877, with the same triage rules spelled out in the description. None of the touched files are covered by CODEOWNERS.
Other factors
The robobun build failures are on secondary cross-compile targets (musl/android/freebsd) and include a build-cpp job that cannot be affected by Rust comment edits — consistent with infra flakiness rather than this diff. The PR description confirms cargo check --workspace and cargo fmt are clean. No bugs were flagged by the bug-hunting system.
There was a problem hiding this comment.
LGTM — comment-only cleanup plus rustfmt reflows; the inline nits are non-blocking.
Extended reasoning...
Overview
Follow-up to #30877: purges remaining port-batch markers (Round-[CDEGH], reconciler-6, B-0/B-3, Phase C/D/F, TODO(b0/b1/b2/b3), PORT_NOTES_PLAN R-2, VERIFY-FIX(round1), etc.) from comments across 57 files. Despite CodeRabbit's walkthrough mentioning "two functional code changes," inspection of the diff shows neither is functional: the #[bun_jsc::host_fn(export = ...)] attribute on crc32 is unchanged (only the comment above it was reworded), and the PostgresSQLConnection / bin.rs / bunx_command.rs / socket_body.rs hunks are pure rustfmt indentation from the autofix.ci commit. The one non-comment edit is the use crate::renamer; import in p.rs moving a few lines up — semantically identical.
Security risks
None. Every hunk is either a comment rewrite or whitespace reflow; no executable code paths, types, exports, or FFI surfaces change.
Level of scrutiny
Low. This is the mechanical long-tail of an already-merged comment purge (#30877), applying the same triage rule ("keep the substance, drop the batch-number framing") to patterns the earlier case-sensitive greps missed. cargo check --workspace is reported clean and the autofix.ci commit confirms no fmt drift. The diff is wide but each hunk is independently trivial to verify as comment-only.
Other factors
The four inline nits are all comment-hygiene observations (hardcoded line-number breadcrumbs, a few (b0)/NOALIAS_HUNT stragglers, and a stale paragraph in the PR description) — valid but non-blocking, and ironic-at-worst for a marker-cleanup PR. None affect correctness or warrant holding the merge. No outstanding human reviewer comments; CodeRabbit generated no actionable findings.
There was a problem hiding this comment.
Thanks for addressing the earlier nits in 5f7d7a2 — LGTM. The two remaining inline comments are more of the same long-tail markers and can be swept here or in a follow-up; non-blocking.
Extended reasoning...
Overview
Follow-up to #30877: purges the long tail of Zig-port batch/phase markers (Round-X, b0/b2-cycle, reconciler-6, Phase C/D/F, PORT_NOTES_PLAN R-2, etc.) across 57 files. The diff is almost entirely comment rewording. The only non-comment hunks are pure cargo fmt reflows (bin.rs, bunx_command.rs, socket_body.rs, PostgresSQLConnection.rs) and a trivial import reorder in p.rs that deletes the stale comment attached to it. No logic, types, control flow, or exported symbols change.
Since my last review
Commit 5f7d7a2 addressed all four nits I raised previously: the (~6600) line-number breadcrumbs in p.rs are gone, CYCLEBREAK(b0) → CYCLEBREAK in bundle_v2.rs, the NOALIAS_HUNT cluster E tails were dropped from PipeWriter.rs, and the PR description now correctly lists the PORT_NOTES_PLAN R-2 retargeting under What with the out-of-scope section reworded. The two new inline nits (bare (b2-cycle) parentheticals and a few stranded ``-gated / "next round" artifacts) are the same flavor of leftover marker — comment-only, zero runtime impact, and fine to sweep here or in the tracked follow-up.
Security risks
None. No executable code paths change; the rustfmt-only hunks were verified to be whitespace/indentation reflows of existing expressions.
Level of scrutiny
Low. This is mechanical comment hygiene with a uniform, easily-auditable pattern ("keep the substance, drop the batch-number framing"). cargo check --workspace is clean per the author, CI is building, and none of the touched paths are CODEOWNER-gated.
Other factors
The author was responsive to the prior round of feedback, and the remaining findings are explicitly nit-level and non-blocking. File count is high (57) but per-file change is tiny and homogeneous.
There was a problem hiding this comment.
LGTM — thanks for addressing the earlier nits. One minor inline note on the VirtualMachine.rs:3084 rewrite (comment-only, non-blocking).
Extended reasoning...
Overview
Follow-up to #30877: purges the long tail of port-batch markers (Round-X, reconciler-6, B-0/B-3, Phase C/D/F, PORT_NOTES_PLAN R-2, empty-backtick artifacts, etc.) across 57 files. Every hunk is a comment rewrite, a TODO(bN-*) → TODO(port) rename, or a cargo fmt whitespace reflow (bin.rs, bunx_command.rs, socket_body.rs, PostgresSQLConnection.rs). No code-path, type, or symbol changes — cargo check --workspace is the right verification bar and the description reports it clean.
Security risks
None. Comment-only edits; no auth/crypto/permission/input-handling logic touched.
Level of scrutiny
Low. This is mechanical cleanup following the exact triage rule established in #30877 ("keep the substance, drop the batch-number framing"). The only thing to check is that no rewrite accidentally changed semantics or dropped load-bearing information, and spot-checking the diff confirms substance is preserved (e.g. the PORT_NOTES_PLAN R-2 references now point at the in-tree bun_ptr::LaunderedSelf anchor instead of a deleted planning doc — strictly better).
Other factors
I left two rounds of nits earlier (line-number breadcrumbs, CYCLEBREAK(b0), NOALIAS_HUNT, stale PR description, surviving ``-gated/(b2-cycle)` artifacts). All six were addressed in 5f7d7a2 and 908f16f, and the PR description was updated to reflect the expanded scope. The one new inline nit on this revision (VirtualMachine.rs:3084 — the rewrite says `bun_http` is higher-tier when it's actually a direct dependency) is a minor factual slip in a comment, zero runtime impact, and easily fixed in a follow-up or before merge.
There was a problem hiding this comment.
LGTM — thanks for iterating; the HTTPVerboseLevel comment fix in aa3a97a looks correct. The remaining B-3 nit below is non-blocking and can go in a follow-up sweep.
Extended reasoning...
Overview
Comment-cleanup PR across 57 files purging port-batch markers (Round-X, B-0/B-3, Phase C/D/F, reconciler-6, PORT_NOTES_PLAN R-2, b2-cycle, empty-backtick artifacts) left over from the Zig→Rust port. The only non-comment changes are rustfmt reflows (bin.rs, bunx_command.rs, socket_body.rs, PostgresSQLConnection.rs) and one import reorder in p.rs (moving use crate::renamer; up with the other imports). No logic, types, or control flow changed.
Security risks
None. Comment text and whitespace only; no auth, crypto, input handling, or FFI surface touched beyond rewording explanatory prose.
Level of scrutiny
Low. Despite the 57-file footprint, every hunk is either (a) comment rewording, (b) comment deletion, or (c) rustfmt whitespace. cargo check --workspace is reported clean. I spot-checked the rustfmt hunks and the p.rs import move — all semantically identical.
Other factors
This is my fourth review pass. The author addressed all seven of my prior inline nits across commits 5f7d7a2 (line-number refs, CYCLEBREAK(b0), NOALIAS_HUNT), 908f16f (bare (b2-cycle) parentheticals, empty-backtick artifacts), and aa3a97a (the factually-wrong bun_http higher-tier comment, now replaced with a correct niche-optimization explanation at the field decl). The one new finding this round — two surviving B-3 markers in options.rs — is explicitly a comment-only nit with zero runtime impact and doesn't warrant another round-trip before merge.
There was a problem hiding this comment.
LGTM — comment-only cleanup, all prior review nits addressed. The remaining stragglers flagged inline are non-blocking and can roll into the tracked follow-up sweep.
Extended reasoning...
Overview
This PR is the second pass of a port-batch comment purge (follow-up to #30877), touching 57 .rs files. Every hunk falls into one of three buckets: (a) rewording/deleting comment lines that carried internal port-batch identifiers (Round-C/D/G, B-0/B-3, TODO(b1/b2-cycle), reconciler-6, PORT_NOTES_PLAN R-2, etc.), (b) pure cargo fmt reflows (bin.rs, bunx_command.rs, socket_body.rs, PostgresSQLConnection.rs), or (c) a one-line use reorder in p.rs. CodeRabbit's walkthrough flagged two "behavior-adjacent" edits (HashObject crc32 export, Postgres SASL base64 match), but on inspection both are comment/whitespace-only — the #[host_fn(export = ...)] attribute and the match arm were already present; only surrounding indentation/comment text changed.
Security risks
None. No string handling, no FFI signatures, no auth/crypto logic, no control flow is touched. The diff is comments and whitespace.
Level of scrutiny
Low. Comment-only changes in a large mechanical sweep; cargo check --workspace and cargo fmt are stated clean, and the rustfmt-only hunks corroborate that nothing semantic moved. The only risk class for a PR like this is introducing a factually wrong replacement comment — that happened once (the HTTPVerboseLevel tier claim at VirtualMachine.rs:3084), I flagged it, and the author fixed it in aa3a97a with a correct niche-optimization explanation.
Other factors
I've already posted 8 inline comments across four review rounds on this PR; the author addressed every one in follow-up commits (5f7d7a2 → 908f16f → aa3a97a → bcd3b50). The four new inline nits on this run are all scope-adjacent completeness notes (a reconciler-6 survivor in the parent module of an edited file, TODO(b1/b2) in Cargo.toml files the --type rust grep skipped, a draft-b1 prose form, and 11 unconverted Round-C boilerplate siblings in js_parser/). None affect correctness; they're grep-pattern stragglers for the already-tracked follow-up. Holding a comment-only PR for a fifth nit round would be churn — approving so the author can sweep the remainder at their discretion.
| // Expr — property/object/string accessor methods. These call into | ||
| // `E::Object::as_property` / `EString` methods backed by | ||
| // `bun_core::utf16_eql_string` / `to_utf8_alloc` and `Vec::deep_clone`. |
There was a problem hiding this comment.
🟡 Three more out-of-diff-sibling clusters of explicitly-targeted patterns survive (same class as #3253647490, different files): (1) src/ast/fold_string_addition.rs:7 '(round-C draft)' — the last [Rr]ound-[A-Z] hit tree-wide, sibling of PR-edited ast/expr.rs; (2) nine \`\`-gated artifacts in css/ parent modules of PR-edited custom.rs/font.rs/keyframes.rs/media.rs (properties/mod.rs:199/225/233, rules/mod.rs:348/540, rules/style.rs:43, rules/property.rs:66, selectors/selector.rs:573, selectors/parser.rs:4312) plus bundler/ParseTask.rs:493 and install/PackageManager.rs:69 TODO(b0); (3) (phase-d) in jsc/AbortSignal.rs:263, jsc/HTTPServerAgent.rs:45, runtime/hw_exports.rs:1, runtime/server/RequestContext.rs:328 (RequestContext.rs also carries 4× TODO(b2-blocked) + 4× Phase B, so it may fall under the #30879 carve-out). The strongest hit is properties/mod.rs:232-234 — the doc-comment ON pub mod custom; still says 'parse/to_css/deep_clone/eql/hash bodies remain internally ``-gated there', directly contradicting custom.rs's own doc which this PR just rewrote to drop that claim. Comment-only, zero runtime impact; per the triage rule each becomes 'gated' / drops the batch tag.
Extended reasoning...
What was left behind
This PR's verification claim ('grep for all known port-batch patterns in the diff's files → 0') holds — none of the files below are in the 83 changed files. But the established review pattern on this PR (resolved #3252700302/#3252763439/#3252788306/#3252788309 and open #3253647490, all swept by the author) is that explicitly-targeted patterns surviving in sibling/parent modules of PR-edited files get listed for completeness. Three more clusters of that shape survive that aren't covered by #3253647490 (which lists server/mod.rs, media.rs, ScopeFunctions.rs, phase_c_exports.rs):
(1) round-C — last tree-wide hit
src/ast/fold_string_addition.rs:7 reads:
EString::push/EString::clone_rope_nodesare still gated in E.rs (round-C draft); inline the minimal surface here so this file can un-gate
Round-[CDEGH] is the first targeted pattern in the PR description, and rg '[Rr]ound-[A-Z]' src returns only this one hit tree-wide. The identical '(round-C draft)' phrasing was already swept from js_parser/fold.rs:23 in this PR (commit 01c5536, resolved #3252795348/#3252809425) — this is the same artifact in src/ast/ rather than src/js_parser/, so the 'rg [Rr]ound-[A-Z] src/js_parser → 0' verification didn't cover it. src/ast/expr.rs IS in the changed-files list (track-A blocked_on removed at :285), making fold_string_addition.rs a same-directory sibling.
(2) \`\`-gated — css/ parent modules + two non-css siblings
908f16f swept the \`\`-gated empty-backtick artifacts from PR-edited css files (custom.rs ×4, font.rs, css_parser.rs — resolved #3252714625), but rg '\`-gated' src/css` still returns 8 hits in their parent/sibling modules:
| File | Line(s) | Relationship |
|---|---|---|
properties/mod.rs |
199/225/233 | parent declaring pub mod font; / pub mod custom; |
rules/mod.rs |
348/540 | parent declaring keyframes.rs/media.rs |
rules/style.rs |
43 | sibling of keyframes.rs/media.rs |
rules/property.rs |
66 | sibling of keyframes.rs/media.rs |
selectors/selector.rs |
573 | same crate |
selectors/parser.rs |
4312 | same crate |
Plus two non-css siblings of PR-edited files: bundler/ParseTask.rs:493 (\`\`-gated, sibling of bundle_v2.rs/options.rs) and install/PackageManager.rs:69 (TODO(b0) — the same B-0 tag this PR converts 4× in bundle_v2.rs; sibling of PR-edited bin.rs/lockfile.rs/Cargo.toml).
The strongest case is properties/mod.rs:232-234: it's the doc-comment ON the pub mod custom; declaration and reads 'parse/to_css/deep_clone/eql/hash bodies remain internally ``-gated there' — directly contradicting custom.rs's own module doc, which this PR rewrote at :9-18 to drop exactly that claim. The PR introduced this parent↔child contradiction (before the PR both said the same thing).
(3) phase-d / TODO(b2-blocked) — jsc/ + runtime/ siblings
The PR description explicitly lists phase-d among targeted patterns, and the diff removes it from parse_stmt.rs:37 and HashObject.rs:193. After this PR, rg '\(phase-d\)|^//! phase-d' returns four hits:
| File | Line | Marker | Relationship |
|---|---|---|---|
jsc/AbortSignal.rs |
263 | PORT NOTE (phase-d): |
sibling of 7 PR-edited src/jsc/ files |
jsc/HTTPServerAgent.rs |
45 | PORT NOTE (phase-d): |
(same) |
runtime/hw_exports.rs |
1 | //! phase-d: |
sibling of jsc_hooks.rs (~40 lines edited) |
runtime/server/RequestContext.rs |
328 | per phase-d rules |
sibling of ServerWebSocket.rs |
RequestContext.rs additionally carries 4× TODO(b2-blocked) (lines 185/1030/2795/3186) — the form this PR converted ~20× to TODO(port) in jsc_hooks.rs/VirtualMachine.rs/options.rs — plus 4× Phase B (127/167/1037/3194). Its unusually high survivor count suggests it may fall under the '#30879 still touches' carve-out, but the three single-marker (phase-d) files (AbortSignal/HTTPServerAgent/hw_exports) almost certainly don't.
Step-by-step proof (properties/mod.rs:233 — the PR-introduced contradiction)
- PR description lists 'stranded empty-backtick artifacts (``-gated)' as a targeted pattern.
- PR diff at
custom.rs:9-18rewrites the module doc from 'A few leaf calls … are still-gated in *other* files … Remaining internalgates carry blocked_on: notes for the next round' → 'A few leaf calls … have their bodies inlined verbatim undermod extbelow'. properties/mod.rs:232-234(the doc-comment ONpub mod custom;) still reads 'parse/to_css/deep_clone/eql/hash bodies remain internally ``-gated there' — pointing the reader at custom.rs.- A reader following that pointer to custom.rs finds a module doc that (as of this PR) says nothing of the sort.
- Before this PR, parent and child docs agreed (both said ``-gated). The PR rewrote the child and not the parent, introducing the contradiction.
Not a duplicate
Unresolved #3253647490 lists server/mod.rs, media.rs, ScopeFunctions.rs, phase_c_exports.rs — zero file overlap with any of the ~18 locations above. Resolved #3252714625 covered ``-gated only in PR-edited files (custom.rs/css_parser.rs/jsc_hooks.rs/VirtualMachine.rs, all fixed in 908f16f). None of the 22 prior PR comments mention fold_string_addition.rs, properties/mod.rs, rules/mod.rs, rules/style.rs, rules/property.rs, selectors/, ParseTask.rs, PackageManager.rs:69, AbortSignal.rs, HTTPServerAgent.rs, hw_exports.rs, or RequestContext.rs.
Impact and fix
Zero runtime impact — comment-only. Per the triage rule ('keep the substance, drop the batch-number framing'):
- fold_string_addition.rs:7: '(round-C draft)' → '(an earlier draft)' or drop the clause (same fix as fold.rs:23).
- **css/
-gated**: each '-gated' → 'gated' or rephrase; properties/mod.rs:232-234 should be aligned with custom.rs's rewritten doc. - ParseTask.rs:493: 'remain ``-gated' → 'remain gated'. PackageManager.rs:69:
TODO(b0)→ `TODO(port)`. - (phase-d):
PORT NOTE (phase-d):→PORT NOTE:;//! phase-d:→ drop the prefix; RequestContext.rs'sTODO(b2-blocked)→TODO(port)(or skip RequestContext.rs entirely if it's under the Replace HiveArrayFallback's uninit-pointer API with safe construction #30879 carve-out).
Non-blocking nit; flagging only because all three patterns are explicitly listed as targeted, the precedent on this PR is that out-of-diff sibling survivors get swept, and (1) closes out the PR's headline pattern tree-wide while (2) includes a contradiction this PR introduced.
Anchored on ast/expr.rs since none of the survivor files are in the diff.
There was a problem hiding this comment.
Thanks — cb787f9 handled all three clusters: rg '[Rr]ound-[A-Z]' src is now 0 tree-wide, the nine css/ ``-gated artifacts + ParseTask.rs:493 are gone, the properties/mod.rs:232-234 ↔ custom.rs contradiction is resolved, and AbortSignal/HTTPServerAgent/hw_exports dropped (phase-d). One straggler from cluster (2): `src/install/PackageManager.rs:69` still reads `// TODO(b0): RunCommand arrives from move-in` — same `TODO(b0)` → `TODO(port)` conversion as bundle_v2.rs:370/742. (RequestContext.rs left as-is per the #30879 carve-out the comment flagged.)
| @@ -488,11 +488,6 @@ export var __callDispose = (stack, error, hasError) => { | |||
|
|
|||
| // ══════════════════════════════════════════════════════════════════════════ | |||
| // Per-file parse worker — `getAST`/`getCodeForParseTask`/`runFromThreadPool`. | |||
There was a problem hiding this comment.
🟡 🟡 cb787f9 swept most of the 22:40 comment's list (including ParseTask.rs:493 here) but skipped two: install/PackageManager.rs:69 still reads TODO(b0): RunCommand arrives from move-in, and runtime/server/RequestContext.rs still carries (phase-d):328 + 4× TODO(b2-blocked) (:185/:1030/:2795/:3186) + 4× Phase B (:127/:167/:1037/:3194) + ``-gated:416 — gh pr view 30879 --json files confirms neither is in #30879, so the carve-out doesn't shield them. Two more out-of-diff sibling clusters not in any prior comment: (a) the hyphenated Phase-A/Phase-B form (which a space-anchored Phase A grep misses) survives in install/lib.rs:11/14/21/231/1079 (crate root declaring PR-edited bin.rs/lockfile.rs/Cargo.toml), install/TarballStream.rs:1405, runtime/bake/DevServer.rs:5410, runtime/server/server_body.rs:1378, runtime/api/bun/h2_frame_parser.rs:641; (b) TODO(b*)/``-gated in three crates this PR hasn't touched: js_printer/renamer.rs:668 TODO(b2-blocked), js_printer/lib.rs:6742 ``-gated (also factually stale — bun_ast::runtime is real per this PR's own jsc_hooks.rs BunWrap cleanup), css_jsc/lib.rs:13 ``-gated, collections/bit_set.rs:126/1516 TODO(b2)/TODO(b0), collections/lib.rs:30 TODO(b2-large). Comment-only, zero runtime impact; same out-of-diff-sibling class as resolved #22/#23/#24/#26 — flagging so they ride along rather than spawn another follow-up.
Extended reasoning...
What was left behind
HEAD commit cb787f9 ("Clean port-batch markers in css/ast/bundler/jsc/runtime sibling modules") was the response to the 22:40 review comment, which enumerated three out-of-diff-sibling clusters of explicitly-targeted patterns. cb787f9 addressed ~13 of the ~15 listed items: ast/fold_string_addition.rs, the nine css/ parent-module files, bundler/ParseTask.rs:493 (anchored here — the 5-line ``-gated block was deleted), and jsc/AbortSignal.rs / jsc/HTTPServerAgent.rs / runtime/hw_exports.rs. Two items from that comment's enumeration were skipped, and two further sibling clusters of the same targeted patterns survive that no prior comment has named. None of the survivor files are in the 98 changed files, so the per-diff-file verification claim ("grep for all known port-batch patterns in the diff's files → 0") technically holds — this is the weaker out-of-diff-sibling class established by resolved comments #3252788306 (lockfile.rs), #3252788309 (11 js_parser siblings), #3253647490 (server/mod.rs et al.), and #3253671780 (the 22:40 comment itself), all of which the author swept in follow-up commits.
Cluster (1): comment-#23 remainder — partial-fix follow-up
install/PackageManager.rs:69 reads // TODO(b0): RunCommand arrives from move-in (bun_runtime::cli::RunCommand → install). — the same TODO(b0) form this PR converts 4× in bundle_v2.rs (lines 370/495/742/5229 → TODO(port)). The 22:40 comment listed PackageManager.rs:69 alongside ParseTask.rs:493; cb787f9 fixed ParseTask.rs but not PackageManager.rs. No carve-out was ever suggested for this file.
runtime/server/RequestContext.rs carries 10 markers: per phase-d rules at :328; TODO(b2-blocked) at :185/:1030/:2795/:3186 (the same form this PR converted ~20× in jsc_hooks.rs/VirtualMachine.rs/options.rs); Phase B at :127/:167/:1037/:3194; and ``-gated at :416. The 22:40 comment hedged that this file "may fall under the #30879 carve-out" — gh pr view 30879 --json files confirms it does not, so the PR description's only stated carve-out doesn't apply.
This is the well-established partial-fix-follow-up pattern on this PR: resolved #3252795348 (fold.rs:23 — comment listed :23/:62, response handled :62 only), #3252811820 (visit/mod.rs:709), #3252967867 (BunWrap tail), and the 22:18 follow-up (server/mod.rs after 72cf6b6 handled 1-of-4) — each flagged the N-minus-k remainder after a response commit, and each was addressed.
Cluster (2): hyphenated Phase-A/Phase-B — grep blind spot
The PR description says #30877 swept Phase A/B and this PR catches "the long tail … the case-sensitive greps missed". The hyphenated form Phase-A / Phase-B — which a space-anchored 'Phase A' grep would miss, exactly the kind of grep blind spot the PR title references — survives in five sibling/parent modules of PR-edited files:
| File | Line(s) | Relationship to diff |
|---|---|---|
install/lib.rs |
11/14/21/231/1079 (5×) | crate root declaring PR-edited bin.rs/lockfile.rs/lockfile/Tree.rs; install/Cargo.toml is in the diff |
install/TarballStream.rs |
1405 (Phase-B) |
sibling of bin.rs/lockfile.rs |
runtime/bake/DevServer.rs |
5410 | sibling of PR-edited bake_body.rs |
runtime/server/server_body.rs |
1378 | sibling of PR-edited ServerWebSocket.rs (the 22:57 comment covers server/mod.rs:7 but not server_body.rs) |
runtime/api/bun/h2_frame_parser.rs |
641 | under runtime/api/, same tree as PR-edited HashObject.rs |
The hyphenated form is already established as in-scope on this PR — the unresolved 22:57 comment covers server/mod.rs:7 "full Phase-A draft", and install/lib.rs is the exact analogue of the lockfile.rs ← Tree.rs parent-module precedent from resolved #3252788306.
Cluster (3): TODO(b*) / ``-gated in untouched crates
Three crates carry the patterns this PR explicitly targets but have zero files in the diff and aren't named in any of the 26 prior PR comments:
js_printer/renamer.rs:668—TODO(b2-blocked): bun_core::env_var::BUN_DUMP_SYMBOLSjs_printer/lib.rs:6742—full `runtime.rs` is-gated upstream `` — also factually stale:bun_ast::runtime::Runtime::source_code()is real and this PR's own jsc_hooks.rs:3369 calls it (the BunWrap arm cleanup in 0fa0f77)css_jsc/lib.rs:13—``-gated on `bun_css::values::color::*`collections/bit_set.rs:126—TODO(b2): callers needing >64 bitscollections/bit_set.rs:1516—TODO(b0): strings arrives in bun_core via move-incollections/lib.rs:30—TODO(b2-large): heavy nightly-feature usage
This is admittedly the weakest scope relationship of any survivor finding on this PR — js_printer/css_jsc/collections are entirely separate crates, vs. all prior accepted comments which targeted parent/sibling modules within crates the PR already edits. However: js_printer is the direct counterpart of js_parser (18 files edited here) and is named in this PR's own cache.rs rewrite; css_jsc is the _jsc bridge for PR-edited css/; and the cross-crate Cargo.toml sweep precedent (resolved #3252788307 → 642550c) already reached into url/sourcemap/resolve_builtins which had zero .rs edits at the time. Surfacing these lets the author make an informed scope-cutoff decision rather than discover them in another follow-up.
Step-by-step proof
- The 22:40 timeline comment listed
install/PackageManager.rs:69 TODO(b0)andRequestContext.rs:328 phase-d + 4× TODO(b2-blocked) + 4× Phase BalongsideParseTask.rs:493. git diff 880ee892..HEAD --name-onlyshows ParseTask.rs IS in the 98 changed files (the diff at @@ -488 deletes the 5-line``-gated block); PackageManager.rs and RequestContext.rs are NOT (git logshows both last touched pre-PR).- At HEAD,
PackageManager.rs:69readsTODO(b0): RunCommand arrives from move-in— verified. - At HEAD,
RequestContext.rshas all 10 claimed markers at the exact lines listed — verified viased -n. gh pr view 30879 --json filesreturns neither file — the Replace HiveArrayFallback's uninit-pointer API with safe construction #30879 carve-out doesn't shield them.rg 'Phase-[AB]' src/install src/runtimereturns the 9 hyphenated hits listed in cluster (2) — none in the 98 changed files, none mentioned in any prior comment exceptserver/mod.rs:7(covered separately).rg 'TODO\(b[02]|``-gated' src/js_printer src/css_jsc src/collectionsreturns the 6 hits in cluster (3) — none in any prior comment.- PR description explicitly lists
B-0,Phase C/D/F,phase-d,TODO(b1)(via Clean up Zig-port phase comments and trivial lint warnings #30877), and "stranded empty-backtick artifacts (``-gated)" as targeted; Clean up Zig-port phase comments and trivial lint warnings #30877 explicitly targetedPhase A/BandTODO(b2-*).
Impact and fix
Zero runtime impact — comment-only. Per the PR's triage rule ("keep the substance, drop the batch-number framing, delete done-process comments"):
- PackageManager.rs:69:
TODO(b0)→TODO(port)(same conversion as bundle_v2.rs). - RequestContext.rs:
TODO(b2-blocked)→TODO(port)(4×); dropper phase-d rules/Phase B/in Phase Bframings;``-gated →gated. Phase-A/Phase-B: rewrite to plain prose (drafts,earlier draft,Resolved paths) or drop the qualifier — same rewrite the 22:57 comment prescribes forserver/mod.rs:7.- js_printer/css_jsc/collections:
TODO(b*)→TODO(port);``-gated →gated(or deletejs_printer/lib.rs:6742outright sincebun_ast::runtimeis real).
Non-blocking nit; flagging only because all patterns are in the description's explicitly-targeted list and the established precedent on this PR is that out-of-diff sibling survivors get swept in the same response commit rather than spawning another follow-up round.
There was a problem hiding this comment.
Thanks — d85dd77 handled two of cluster (3): js_printer/lib.rs:6742 (the stale -gated `runtime.rs` TODO deleted) and `css_jsc/lib.rs:13` (-gated → dropped). The rest of the comment is still outstanding at HEAD (e97f778):
Cluster (1) — neither file touched since pre-PR e520065:
install/PackageManager.rs:69still readsTODO(b0): RunCommand arrives from move-inruntime/server/RequestContext.rsstill carries all 10 markers (:127/:167/:185/:328/:416/:1030/:1037/:2795/:3186/:3194)
Cluster (2) — rg 'Phase-[AB]' src returns the same 9 hits: install/lib.rs:11/14/21/231/1079, install/TarballStream.rs:1405, runtime/bake/DevServer.rs:5410, runtime/server/server_body.rs:1378, runtime/api/bun/h2_frame_parser.rs:641 (plus server/mod.rs:7, already carved out).
Cluster (3) remainder — js_printer/renamer.rs:668 TODO(b2-blocked), collections/bit_set.rs:126/1516 TODO(b2)/TODO(b0), collections/lib.rs:30 TODO(b2-large).
Per the 22:58 #30879 carve-out (and robobun's HiveArrayFallback warning list on this build), DevServer.rs / server_body.rs / h2_frame_parser.rs / renamer.rs are plausibly shielded — happy to defer those. That still leaves PackageManager.rs:69, RequestContext.rs (which the comment already confirmed isn't in #30879), install/lib.rs (×5), TarballStream.rs:1405, and the three collections/ hits with no carve-out. Same fixes as before: TODO(b0)/TODO(b2*) → TODO(port); Phase-A/Phase-B → drop the qualifier; RequestContext.rs's ``-gated:416 → gated.
| /// Port of `VendorPrefix.toCss` (css_parser.zig); duplicates | ||
| /// `VendorPrefix::to_css` (css_parser.rs). |
There was a problem hiding this comment.
🟡 dd85136 applied the 'duplicates X' template to vendor_prefix_to_css here (responding to #3253712208) but left the same stale pattern on the next two helpers in the same section: custom_ident_to_css (:405-408 still says 'Both CustomIdent::to_css and Printer::write_ident are gated' — both are real, and :424 of this very function calls dest.write_ident(); plus :416-426 carries the { return …; } + unreachable serialize_identifier tail that 0fa0f77 already unwrapped for the BunWrap arm) and dashed_ident_to_css (:429-432/:440-441 still say write_dashed_ident is gated; it's defined at printer.rs:606). Per the template applied at :353/:388, both docs should become '/// Port of …; duplicates X (values/ident.rs)', the :416-426 tail unwrapped, and the two blocked_on: notes deleted. Comment-only + compiler-eliminated dead code; non-blocking.
Extended reasoning...
What was left behind
cb787f9 rewrote the section header at src/css/rules/mod.rs:347-351 to name THREE helpers — decl_block_to_css, VendorPrefix::to_css, CustomIdent::to_css — and say 'The canonical impls live outside rules/ (declaration.rs, css_parser.rs, values/ident.rs); these duplicates can be deleted once callers route through them directly.' The PR then applied the 'duplicates X' template to two of the three:
- :353-354
decl_block_to_css→ 'duplicatesDeclarationBlock::to_css_block' (cb787f9) - :388-389
vendor_prefix_to_css→ 'duplicatesVendorPrefix::to_css(css_parser.rs)' (dd85136, responding to #3253712208)
But the same stale pattern survives on the next two helpers in the section, plus a dead-code tail of exactly the kind this PR already cleaned in jsc_hooks.rs:
(a) :405-408 custom_ident_to_css still reads 'Both CustomIdent::to_css and Printer::write_ident are gated on the css_modules Pattern::write borrowck reshape; this is the non-css-module tail (serialize_identifier) that both share.' Both claims are false: CustomIdent::to_css is defined at values/ident.rs:511, Printer::write_ident at printer.rs:546, and the body at line 424 of this very function calls dest.write_ident(v, enabled). This now internally contradicts the :349 header that names values/ident.rs as the canonical home.
(b) :416-426 dead-code tail — :416-417 is a blocked_on: Printer::write_ident — … is gated; fall through to its unscoped tail note (the PR removed an identical blocked_on: paragraph from this same file at :531-535); :419-425 is a brace block containing an unconditional return dest.write_ident(...); :426 dest.serialize_identifier(v) is unreachable. This is byte-for-byte the same shape as the BunWrap arm in jsc_hooks.rs that 0fa0f77 unwrapped (resolved #3252967867/#3252985018).
(c) :429-432 / :440-441 dashed_ident_to_css — doc says 'The real printer method is gated on a borrowck reshape', and :440-441 is another blocked_on: Printer::write_dashed_ident — … is gated note. Printer::write_dashed_ident is defined at printer.rs:606 and DashedIdent::to_css (values/ident.rs:211) calls it directly.
Why this is in scope
rules/mod.rs IS in the PR's changed-files list (hunks at :347-354, :388-389, :531-535). Review #3253712208 flagged the :388 inconsistency relative to the PR-rewritten :349 header; dd85136 fixed only that one. This is the same partial-fix-follow-up class as resolved #3252795348 (fold.rs:23 — fixed :62, left :23), #3252811820 (visit/mod.rs:709), #3252967867 (BunWrap tail). The PR-rewritten header at :349 introduced the contradiction with :405-408 (before the PR, the header didn't claim the canonical lived in values/ident.rs). And the PR removed an identical blocked_on: paragraph from this same file at :531-535, so :416-417 and :440-441 are exactly the class it's already sweeping.
Step-by-step proof
- PR diff hunk @@ -344 rewrites the section header to name
values/ident.rsasCustomIdent::to_css's canonical home. - PR diff hunk @@ -387 (dd85136) applies the 'duplicates X' template to
vendor_prefix_to_css. - PR diff hunk @@ -534 deletes a
blocked_on:paragraph from this file. - :405-408 still says 'Both
CustomIdent::to_cssandPrinter::write_identare gated' — verified at HEAD. values/ident.rs:511definesCustomIdent::to_css;printer.rs:546defineswrite_ident— neither is gated.- :419 opens
{, :424 readsreturn dest.write_ident(v, enabled);(unconditional), :425 closes}; :426dest.serialize_identifier(v)is statically unreachable. - :440-441 reads
blocked_on: Printer::write_dashed_ident — … is gated;printer.rs:606defines it. - None of the 32 prior PR comments mention
custom_ident_to_css,dashed_ident_to_css, or rules/mod.rs:405-443. Not a duplicate of #3253712208 (which covered :388 only, now fixed).
Impact and fix
Comment-only + compiler-eliminated dead code; zero runtime impact. Per the triage rule and the template the PR applied at :353/:388:
- :405-408: → '/// Port of
CustomIdentFns.toCss; duplicatesCustomIdent::to_css(values/ident.rs).' - :416-426: delete the
blocked_on:note + unwrap the brace block + delete the unreachable :426, leaving justlet enabled = …; dest.write_ident(v, enabled)(matching the BunWrap-arm cleanup in 0fa0f77). - :429-432 / :440-441: → '/// Port of
DashedIdentFns.toCss; duplicatesDashedIdent::to_css(values/ident.rs).' and delete theblocked_on:note.
Non-blocking nit.
| pub fn init( | ||
| v2: &BundleV2<'_>, |
There was a problem hiding this comment.
🟡 Three port-process residue items survive in PR-edited bundler files: ThreadPool.rs:55 still reads callers (bundle_v2.rs draft) — the same stale-bv2_impl-draft reference class this PR explicitly targets and just removed at :208-210 of the same file ((bundle_v2.rs draft) → (bundle_v2.rs) or drop). linker.rs:42-45 carries 'gated in the draft body (transpiler.rs:1111)' — a hardcoded line-number reference of the same rot-prone class as resolved #3252700298 (and already stale: transpiler.rs:1111 now points at an unrelated zip-iterator body). bundle_v2.rs:287-288 is a done-process tombstone ('draft on_parse_task_complete … removed — canonical bodies live below') identical in shape to p.rs:7862 ('earlier draft body removed'), which this PR deleted. Comment-only, non-blocking; same in-diff-file-survivor class as the prior resolved nits.
Extended reasoning...
What was left behind
dd85136 brought ThreadPool.rs back into the diff (de-genericized init/init_with_pool at :208-256, fixed the empty-backtick at :639-642), and the same commit's broader sweep touched linker.rs and bundle_v2.rs. Three port-process residue items survive in those files:
| File:line | Text | Class |
|---|---|---|
ThreadPool.rs:55 |
kept raw so callers (bundle_v2.rs draft) can dereference |
stale bv2_impl draft reference |
linker.rs:42-45 |
gated in the draft body (\transpiler.rs:1111`)` |
rot-prone line-number ref + 'draft body' framing |
bundle_v2.rs:287-288 |
PORT NOTE: draft \on_parse_task_complete` / `deinit_without_freeing_arena` removed — canonical bodies live in the later impl blocks below.` |
done-process tombstone |
Why each is in scope
ThreadPool.rs:55 is the strongest case. The PR description explicitly lists 'stale bv2_impl draft references' as a targeted pattern, and (bundle_v2.rs draft) is precisely that construction — a parenthetical naming bundle_v2.rs with the 'draft' qualifier. dd85136 just deleted the same phrasing ~150 lines below in the same file (the old :208-210 'inside the gated bv2_impl draft module' was dropped when init was de-genericized to take &BundleV2<'_> directly). rg 'draft' src/bundler/ThreadPool.rs returns exactly this one hit. None of the prior PR comments cover :55 (resolved #3252903668 covered :210-211; #3252957211 covered :208-213/:642-646).
linker.rs:42-45 carries a hardcoded line-number reference, (\transpiler.rs:1111`), of the same rot-prone class that resolved comment #3252700298 flagged on p.rs (~6600/~6630) and the author removed in 5f7d7a2a. The reference has *already* rotted: transpiler.rs:1111now points at an unrelated.keys().iter().zip(...)body insideinit_resolver_options, not at any isCacheEnabledgate. The 'draft body' framing is the weaker half of this item, but the line-number reference is independently actionable per established precedent.linker.rs` IS in the diff (the PR rewrote :15 'B-3 collapses them' → 'until they are unified').
bundle_v2.rs:287-288 is a textbook done-process tombstone per the PR's 'delete done-process comments' triage rule. It records that draft method bodies were removed and the canonical ones live below — pure process history with no forward-looking content. This PR's own diff deleted an identically-shaped tombstone at p.rs:7862 ('compute_ts_enums_map() lives in the … impl block below (deduped — earlier draft body removed once both un-gated)' → 'compute_ts_enums_map() lives in the to_ast impl block below.'). bundle_v2.rs IS in the diff (×4 hunks).
Addressing the 'bare-draft is plain English' objection
One reasonable objection is that the bare word 'draft' is not one of the PR's targeted patterns — the description targets 'stale bv2_impl draft references' specifically, and the PR's own resolved comments (#3252788308 'an earlier draft enum', #3252809425 'gated draft') kept 'draft' as plain English while stripping the batch identifier. That objection is correct, and is why this finding excludes four bare-English 'draft' uses in PR-edited files (FileSink.rs:854 'distinct draft type', ParseTask.rs:19 'the draft used the', p.rs:6725 'The full draft uses', pretty_format.rs:711 'this draft handles bytes only') — those are descriptive prose conveying real information, not batch markers.
The three items above are not in that bare-English class:
- ThreadPool.rs:55's
(bundle_v2.rs draft)IS abv2_impl draftreference — the exact pattern the description names. - linker.rs:42's actionable issue is the rotted
transpiler.rs:1111line-number reference, independent of the word 'draft'. - bundle_v2.rs:287 is a done-process tombstone, actionable under the 'delete done-process comments' triage rule independent of the word 'draft'.
Step-by-step proof (ThreadPool.rs:55)
- PR description's targeted-pattern bullets include 'stale
bv2_impl draftreferences'. - dd85136 (HEAD) edits ThreadPool.rs at :205-256 (de-genericize
init/init_with_pool) and :639-642 (empty-backtick fix), so ThreadPool.rs is in the changed-files list and subject to the per-diff-file verification claim ('grep for all known port-batch patterns in the diff's files → 0'). - The pre-dd85136e text at :208-210 read 'inside the gated
bv2_impldraft module' — dd85136 deleted that whole 6-line PORT NOTE. - At HEAD, ThreadPool.rs:55 reads
kept raw so callers (bundle_v2.rs draft) can dereference for \wake_for_idle_events()`— the same 'bundle_v2`-as-draft' qualifier survives 153 lines above the deleted instance. - Per the triage rule ('keep the substance, drop the batch-number framing'),
(bundle_v2.rs draft)→(bundle_v2.rs), or drop the parenthetical (the 'kept raw so callers can dereference' substance stands on its own).
Impact and fix
Zero runtime impact — comment-only. Same in-diff-file-survivor class as resolved #3252714625/#3252763439/#3252809425 (all addressed by the author). Suggested fixes per the triage rule:
- ThreadPool.rs:55:
(bundle_v2.rs draft)→(bundle_v2.rs), or drop the parenthetical. - linker.rs:42-45: drop the
(\transpiler.rs:1111`)parenthetical (already stale) and 'in the draft body'; the substantive content is just 'isCacheEnabledis a hardfalse` in the Zig source; inline it here'. - bundle_v2.rs:287-288: delete both lines (done-process tombstone; the canonical
on_parse_task_complete/deinit_without_freeing_arenabodies are findable by name).
Non-blocking nit.
| // (cold-path §Dispatch — crash trace only). crash_handler (T1) holds erased | ||
| // `(*const LinkerContext, *const Chunk, *const PartRange)`; bundler supplies | ||
| // the formatter that knows their layout. Mirrors src/crash_handler/crash_handler.zig:135. | ||
| // Vtable instance for `bun_crash_handler::BundleGenerateChunkVTable` (cold |
There was a problem hiding this comment.
🟡 Two more empty-backtick artifacts of the () class survived dd85136e in PR-edited files: **`src/bundler/LinkerContext.rs:138`** reads " LinkerGraph SoA accessors are real now ( on `JSAst`/`JSMeta`/`File`)" (the gated-feature name was stripped, leaving ( on), and **`src/runtime/webcore/Body.rs:694`** reads "Recover the parent via offset_of!``) `` and forward." (orphaned `` ``) ``). Both files are in the changed-files list (LinkerContext.rs:60-67 dropped CYCLEBREAK(b0)`; Body.rs had six R-2 tags stripped at :89/:115/:1739/:2302/:2656), so the per-diff-file verification claim ("grep → 0") doesn't quite hold for this artifact class. Same in-diff-file-survivor pattern as resolved #5/#13/#26/#30 — comment-only, non-blocking.
Extended reasoning...
What was left behind
HEAD commit dd85136 ("Fix remaining empty-backtick artifacts, stranded punctuation, and inconsistent docs") swept the () ``-class survivors that the 23:17/23:36/23:40 follow-up threads enumerated (host_fns.rs:12, `rules/mod.rs:388-390`, `timer/mod.rs`). Two more occurrences of the same artifact class survive in files this PR edits:
src/bundler/LinkerContext.rs:138-139 reads:
// `LinkerGraph` SoA accessors are real now (`` on
// `JSAst`/`JSMeta`/`File`); the submodule bodies un-gate against those.The ( ontoken is a stranded empty-backtick artifact — an earlier rewrite stripped a feature-flag/gate name out of (X on JSAst/...) , leaving the bare plus the trailing on. This is exactly the pattern class the PR description lists as targeted: "Stranded empty-backtick artifacts ( -gated, () ) … from earlier rewrites".
src/runtime/webcore/Body.rs:694 reads:
/// `offset_of!``) and forward.
The ) is the trailing half of the same () pattern — presumably once read "(via offset_of!)" or "offset_of! (container_of!) and forward." before content was stripped, leaving the orphaned close-paren glued to empty backticks.
Why this is in scope
Both files are in this PR's 177 changed files:
LinkerContext.rswas edited at :60-67 (theCYCLEBREAK(b0)→ vtable-comment rewrite from resolved comment Copy source lines when generating error messages #3, addressed in 5f7d7a2).Body.rswas edited at :89/:115/:1739/:1743/:2302/:2656 by e97f778 (sixR-2workstream tags stripped).
So both fall under the verification claim "grep for all known port-batch patterns in the diff's files → 0". The () / -gated pattern is one of those known patterns (it's explicitly bullet-listed in the description), and a grep for it across the diff's file set returns exactly these two hits — the verification claim doesn't quite hold.
This is the same in-diff-file-survivor class as resolved comments #5 (custom.rs -gated, fixed in 908f16f9), #13 (visit/mod.rs:709 -gated, fixed in ba86ce4), #26/#30 (host_fns.rs:12 () ``, fixed in dd85136 itself). Both artifacts pre-date this PR (git blame → 23427db, the original Rust rewrite), so this is the "explicitly-targeted pattern surviving in a PR-edited file" class, not a PR-introduced regression.
Step-by-step proof
- PR description lists "Stranded empty-backtick artifacts (
,())" as a targeted pattern. - dd85136 (HEAD) swept this pattern at
host_fns.rs:12andrules/mod.rs:388-390— confirming() `` is in the active sweep set. src/bundler/LinkerContext.rsis in the changed-files list (diff hunk at :60-67); :138 reads(onJSAst/... `` at HEAD — verified.src/runtime/webcore/Body.rsis in the changed-files list (diff hunks at :89/:115/:1739/:2302/:2656); :694 reads`offset_of!) and forward. `` at HEAD — verified.rg '\(``|``\)'across the 177 PR-edited files returns exactly these two hits — the per-diff-file verification claim is contradicted by ≥2.- None of the 32 prior PR comments mention LinkerContext.rs:138 or Body.rs:694 (resolved Copy source lines when generating error messages #3 covered LinkerContext.rs:63, a different issue) — not a duplicate.
Impact and fix
Zero runtime impact — comment-only doc text. Per the PR's triage rule ("keep the substance, drop the batch-number framing"):
-
LinkerContext.rs:138: drop the empty parenthetical entirely — the
real nowclaim already conveys the substance:// `LinkerGraph` SoA accessors are real now on // `JSAst`/`JSMeta`/`File`; the submodule bodies un-gate against those.
(or just
//LinkerGraphSoA accessors are real now; the submodule bodies …) -
Body.rs:693-694: restore the dropped open-paren or simplify:
/// header carries the refcount + pool back-pointer. Recover the parent (via /// `offset_of!`) and forward.
Non-blocking nit; flagging only because both are exactly the artifact class dd85136 claims to have closed out, in files the PR already edits.
Verified each TODO against the code it references: - Linker .log assignment: linker.rs:59 defines pub log: *mut Log and the assignment exists at the call site; deleted the stale TODO. - ref_counted_resolved_source: the method exists on VirtualMachine and the watcher branch returns; there is no fall-through. Deleted the stale TODO. - bun_ast::runtime::Runtime::source_code(): defined in src/ast/runtime.rs and called at this site; deleted the stale TODO. - BUILTIN_MODULES: the static lives in bun_analytics::features (a Mutex<BTreeSet<&'static str>>), not bun_resolve_builtins. Reworded the TODO to point at the right static and shape. - HMR pending_internal_promise swap loop: the loop is implemented under is_watcher_enabled(); deleted the stale TODO. - AsyncModule arena hand-off: the enqueue site already defuses the guard via ScopeGuard::into_inner and hands the arena to the queue. Deleted the stale TODO. - update_date_header_timer_if_necessary (x2): the method exists on timer::All; reworded the TODOs to say only the call site is missing. - set_breakpoint_on_first_line: both Debugger field and runtime_transpiler_store helper exist; reworded to say only the plumbing at this call site is missing.
…onsistent docs - bundler/ThreadPool.rs: take main's post-#30886 version and drop the stranded empty-backtick parenthetical at the MimallocArena PORT NOTE - runtime/ffi/host_fns.rs: drop stranded empty-backtick parenthetical - css/rules/mod.rs: align vendor_prefix_to_css doc with the rewrite at :347-351 (canonical impl is in css_parser.rs, not lib.rs) - js_parser/parser.rs: delete dead ConvertESMExportsForHmr / ImportScanner stub modules (zero callers; real types live in lower/ and scan/) - runtime/api/html_rewriter.rs, runtime/node/node_zlib_binding.rs: fix //; and //. stranded by the R-2 tag strip
Jarred-Sumner
force-pushed
the
claude/comment-cleanup
branch
from
dd85136 to
c1238ca
Compare
| // The `&mut` originals above are kept until every type has migrated; once | ||
| // they're all on `sharedThis`, delete them and drop the `_shared` suffix. |
There was a problem hiding this comment.
🟡 The e97f778 R-2 sweep ran with --type rust, missing TypeScript: src/codegen/class-definitions.ts:121 still reads '(R-2 noalias re-entrancy)' and src/codegen/generate-classes.ts:2866 still reads '// R-2 Phase 3: default flipped to sharedThis: true' — and :2870-2871 explicitly cross-references 'src/jsc/host_fn.rs alongside the legacy &mut originals', whose paired comment this PR just rewrote at :510/:516-517 to drop both '(R-2 noalias re-entrancy)' and 'Phase 3 of R-2-design.md'. Same file-type-gap class as resolved #3252788307 (TOML, swept in 642550c); rg 'R-2' --glob '*.ts' returns ~21 hits across 9 .ts files total. Fix: '(R-2 noalias re-entrancy)' → '(noalias re-entrancy)'; 'R-2 Phase 3:' → drop the prefix. Comment-only, non-blocking.
Extended reasoning...
What was left behind
Commit e97f778 ('Drop bare R-2 workstream tags from comments tree-wide') swept ~330 R-2 tags from .rs files, and this PR's diff at src/jsc/host_fn.rs:506-517 rewrote the _shared-helpers comment block to drop '(R-2 noalias re-entrancy)' from :510 and replace 'Phase 3 of R-2-design.md deletes them' with 'once they're all on sharedThis, delete them' at :516-517. But the sweep evidently used --type rust and missed the TypeScript codegen files that carry the paired halves of those exact comments:
src/codegen/class-definitions.ts:121reads 'thisis live (R-2 noalias re-entrancy). The Rust impl must take' — the doc-comment on thesharedThisfield, which is the very flag host_fn.rs:509 references ('Emitted bygenerate-classes.tswhen a.classes.tsdefinition setssharedThis: true').src/codegen/generate-classes.ts:2866reads '// R-2 Phase 3: default flipped tosharedThis: true' — the same 'Phase 3' milestone host_fn.rs:516-517 just rewrote away. And :2870-2871 reads '_sharedhelpers live insrc/jsc/host_fn.rsalongside the legacy&mutoriginals' — an explicit cross-reference to the file this PR just edited.
So the PR converted the .rs side of two paired .rs↔.ts comments and left the .ts side carrying the tags it just stripped. Neither .ts file is in the 175 changed files, so the per-diff-file verification claim ('grep for all known port-batch patterns in the diff's files → 0') technically holds — this is the weaker out-of-diff file-type-gap class.
Why this is in scope (precedent)
This is the same file-type-gap class as resolved #3252788307 (TOML — 'the sweep evidently used --type rust', author swept 8 Cargo.toml files in 642550c) and the same 'rewrote one half of a paired comment' class as resolved #3252852393 (SASL.rs — 'ba86ce48 fixed one of a pair of comments and added a pointer at the unfixed one'). Both were addressed by the author. The PR description's last bullet under What is '~330 bare R-2: workstream tags retargeted to bun_ptr::LaunderedSelf or simplified' — these two are exactly that pattern, in a file type the grep didn't reach.
The full file-type gap is larger than the two anchored sites: rg 'R-2' --glob '*.ts' src returns ~21 hits across 9 files (response.classes.ts ×3, html_rewriter.classes.ts ×8, server.classes.ts ×3, sockets/valkey/ResumableSink/streams/BunObject.classes.ts ×1 each), mostly 'R-2 Phase 2/3:' tags on sharedThis: true lines. Flagging the two src/codegen/ hits specifically because they're the ones with the explicit paired-comment cross-reference to a file this PR rewrote.
Step-by-step proof
- PR diff at
src/jsc/host_fn.rs:510changes 'sharedThis: true(R-2 noalias re-entrancy).&mut Tcarries' → 'sharedThis: true.&mut Tcarries' — drops the '(R-2 noalias re-entrancy)' parenthetical. - PR diff at
src/jsc/host_fn.rs:516-517changes '(Phase 3 ofR-2-design.mddeletes them and drops the_sharedsuffix)' → 'once they're all onsharedThis, delete them and drop the_sharedsuffix' — drops the 'R-2-design.md Phase 3' reference. src/codegen/class-definitions.ts:121at HEAD reads '(R-2 noalias re-entrancy)' — the identical parenthetical the PR just dropped from host_fn.rs:510.src/codegen/generate-classes.ts:2866at HEAD reads '// R-2 Phase 3: default flipped' — the same Phase-3 reference the PR just dropped from host_fn.rs:516-517.src/codegen/generate-classes.ts:2870-2871reads '_sharedhelpers live insrc/jsc/host_fn.rsalongside the legacy&mutoriginals' — explicitly pointing the reader at the comment block this PR just rewrote. A reader following that pointer now finds text with no 'R-2' / 'Phase 3' framing, while the .ts side that sent them still carries it.- Neither
class-definitions.tsnorgenerate-classes.tsis in the 175 changed files;rg 'R-2' --glob '*.ts' src/codegenreturns exactly these two hits. - None of the 35+ prior PR comments mention .ts files or src/codegen/ — not a duplicate.
Impact and fix
Zero runtime impact — comment-only, in TypeScript codegen scripts (not emitted into generated Rust). Per the PR's triage rule ('keep the substance, drop the batch-number framing'):
- class-definitions.ts:121: '(R-2 noalias re-entrancy)' → '(noalias re-entrancy)' — same rewrite the PR applied at host_fn.rs:510.
- generate-classes.ts:2866: '// R-2 Phase 3: default flipped' → '// Default flipped' (or just drop the prefix) — same rewrite as host_fn.rs:516-517.
For completeness, the remaining ~19 'R-2 Phase 2/3:' tags in *.classes.ts files (response/html_rewriter/server/sockets/valkey/ResumableSink/streams/BunObject) are the same shape and could be swept in the same pass, though they don't carry the paired-comment cross-reference that anchors this finding to the diff.
Non-blocking nit; the author may reasonably defer .ts to a follow-up given how far this PR has already crept (175 files), but flagging per the established file-type-gap precedent.
| let mut params = route_param::List::default(); | ||
| // `defer params.deinit(allocator)` → Drop | ||
| // SAFETY (R-2): short-lived `&mut Router` for the route lookup; | ||
| // SAFETY (noalias re-entry): short-lived `&mut Router` for the route lookup; |
There was a problem hiding this comment.
🟡 nit: The e97f778 sweep mechanically replaced SAFETY (R-2): → SAFETY (noalias re-entry): here and at node_fs.rs:778/958, but at all three sites the body explicitly says there IS no re-entry ('is pure (no JS re-entry)' / 'never across a JS re-entry point' / 'held only across the libuv enqueue'). The original (R-2) was a bare workstream tag — per this PR's own pattern at non-re-entrant R-2 sites (e.g. filesystem_router.rs:473 // R-2: snapshot → // Snapshot), it should have been dropped → SAFETY:, not retargeted to a label whose substance doesn't fit. Same mechanical-rewrite-artifact class as resolved #3253749351 (//;///.) and #3252732677 (template misapplied).
Extended reasoning...
What happened
Commit e97f778 ('Drop bare R-2 workstream tags from comments tree-wide') swept ~330 R-2 tags. For most sites it simply dropped the tag (e.g. // R-2: \Cell` so host-fns take `&self`→// `Cell` so host-fns take `&self`). But for the SAFETY (R-2):form it did a mechanical substitution toSAFETY (noalias re-entry):` instead — which works at sites that do have re-entry, but at three sites the new label now reads against its own body:
| File:line | Label | Body says |
|---|---|---|
filesystem_router.rs:610 |
SAFETY (noalias re-entry): |
'match_page_with_allocator is pure (no JS re-entry)' |
node_fs.rs:778 |
SAFETY (noalias re-entry): |
'held only across the libuv enqueue … and never across a JS re-entry point' |
node_fs.rs:958 |
SAFETY (noalias re-entry): |
'held only across the libuv enqueue (copies path internally)' |
These are plain JsCell/get_mut() projections whose safety argument is precisely that no re-entry occurs over the borrow — so labelling them '(noalias re-entry)' is at best confusing and at face value self-contradicting.
Why this is a mechanical-rewrite artifact, not an intentional label
The PR establishes (noalias re-entry) as a substance marker for sites that do re-enter and do launder — e.g. PipeReader.rs:622 'noalias re-entry (see bun_ptr::LaunderedSelf): &mut parent carries LLVM noalias, but vtable.on_read_chunk below re-enters JS', and the ~10 similar sites in PipeWriter.rs/uws/lib.rs/multipart.rs. At those sites the label and body agree.
The original (R-2) at these three sites was not that — it was a bare workstream tag (the same one this PR's description says it's purging: '~330 bare R-2: workstream tags retargeted … or simplified'). The PR's own treatment of non-re-entrant R-2 sites in the same file shows the correct disposition: filesystem_router.rs:473 went from // R-2: snapshot the config fields… → // Snapshot the config fields…, and :1010 from // R-2: \create_query_object` writes…→// `create_query_object` writes…` — i.e. drop the tag, don't replace it with substance that doesn't fit.
Step-by-step proof
- The diff shows all three were mechanical
SAFETY (R-2):→SAFETY (noalias re-entry):substitutions in the e97f778 hunk (no body change). - At HEAD,
filesystem_router.rs:610-611reads:// SAFETY (noalias re-entry): short-lived \&mut Router` for the route lookup; `match_page_with_allocator` is pure (no JS re-entry)` — the parenthetical label asserts a hazard the body denies four words later. - At HEAD,
node_fs.rs:778-781reads:// SAFETY (noalias re-entry): single-JS-thread \JsCell` projection of the scratch path buffer; the borrow is held only across the libuv enqueue below (which copies `path` internally) and never across a JS re-entry point.` — same shape. - At HEAD,
node_fs.rs:958-959reads:// SAFETY (noalias re-entry): single-JS-thread \JsCell` projection; held only across the libuv enqueue (copies `path` internally).` — same shape. rg 'noalias re-entry \(see bun_ptr'returns ~10 sites where the label introduces a body that says 'but X re-enters …' — confirming '(noalias re-entry)' is used elsewhere in this PR as an assertion of re-entry, not a topic tag.- None of the 35+ prior PR comments cover these three lines — not a duplicate.
Addressing the charitable reading
One could read SAFETY (X): as a topic label rather than an assertion — i.e. '(noalias re-entry): regarding the noalias-re-entry concern, there is none here, so safe'. That's defensible in isolation, but it's inconsistent with the PR's own usage at the ~10 LaunderedSelf sites, where '(noalias re-entry)' introduces 'but X re-enters'. And it's inconsistent with the PR's own triage rule ('keep the substance, drop the batch-number framing'): (R-2) was pure batch framing, and the body already carries the substance ('no JS re-entry' / 'JsCell projection') — adding '(noalias re-entry)' is new framing, redundant with the body, that creates reading friction.
Precedent on this PR
Same mechanical-template-misapplied class as resolved #3252732677 (HTTPVerboseLevel — the (b2-cycle) → '(jsc/runtime crate cycle)' template was applied where it didn't fit, fixed in aa3a97a) and resolved #3253749351 (//;///. — the same e97f778 commit left stranded punctuation at two sites, fixed in dd85136). Both were addressed by the author.
Impact and fix
Zero runtime impact — comment-only, lowest severity. Suggested: SAFETY (noalias re-entry): → SAFETY: at all three sites, matching the PR's treatment of non-re-entrant R-2 elsewhere. Non-blocking nit.
2 participants
What
Follow-up to #30877. The phase-comment purge there caught
Phase A/B,B-1/B-2,TODO(b1),round-[A-E],Track-A, andgated draft. This catches the long tail of port-batch naming schemes the case-sensitive greps missed:Round-[CDEGH],round-G2,round-5/6(capitalized + alphabetic ranges beyond E)reconciler-6B-0,B-3 UNIFIED,B-0 round 1Phase C/D/F,phase-dtrack-A,VERIFY-FIX(round1),peechy batch 2,cycle-5,Pass C,C-7/C-9this round/the next round/phased port/ stalebv2_impl draftreferences,()) and stranded comment-marker punctuation (//;,//.) from earlier rewritesPORT_NOTES_PLAN R-2cross-references — all 26 retargeted tobun_ptr::LaunderedSelf, the in-tree home of the noalias-re-entry pattern they were anchoringR-2:workstream tags retargeted tobun_ptr::LaunderedSelfor simplifiedSame triage rules: keep the substance, drop the batch-number framing, delete done-process comments. False positives left alone:
tier-N(the Cargo crate-tier architecture term),Phase 1/2/3(algorithm phases),round-robin/rounds to(math/algorithm terms),Stage N(protocol stages).What this is not
Out of scope:
Verification
cargo check --workspacecleangrepfor all known port-batch patterns in the diff's files → 0cargo fmtper-crate clean (no new fmt drift)