-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Purge remaining port-batch markers missed by the phase cleanup #30885
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
19f5f99
baeeacb
37d1fcf
6278f0a
fdb1b94
c4167e7
00b0f83
f71122d
6515182
ae94b8d
15f8418
cb164c7
5fe5886
01a6457
a0b9a57
e5c687d
b6aa67c
f400d2c
357cef7
c1238ca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,10 +60,10 @@ bun_core::declare_scope!(LinkerCtx, visible); | |
| bun_core::declare_scope!(TreeShake, hidden); | ||
|
|
||
| // ══════════════════════════════════════════════════════════════════════════ | ||
| // CYCLEBREAK(b0): vtable instance for `bun_crash_handler::BundleGenerateChunkVTable` | ||
| // (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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Two more empty-backtick artifacts of the Extended reasoning...What was left behindHEAD commit dd85136 ("Fix remaining empty-backtick artifacts, stranded punctuation, and inconsistent docs") swept the
// `LinkerGraph` SoA accessors are real now (`` on
// `JSAst`/`JSMeta`/`File`); the submodule bodies un-gate against those.The
/// `offset_of!``) and forward.
The Why this is in scopeBoth files are in this PR's 177 changed files:
So both fall under the verification claim " This is the same in-diff-file-survivor class as resolved comments #5 (custom.rs Step-by-step proof
Impact and fixZero runtime impact — comment-only doc text. Per the PR's triage rule ("keep the substance, drop the batch-number framing"):
Non-blocking nit; flagging only because both are exactly the artifact class dd85136 claims to have closed out, in files the PR already edits. |
||
| // path — crash trace only). `crash_handler` holds erased `(*const | ||
| // LinkerContext, *const Chunk, *const PartRange)`; the bundler supplies the | ||
| // formatter that knows their layout. Mirrors src/crash_handler/crash_handler.zig:135. | ||
| // ══════════════════════════════════════════════════════════════════════════ | ||
| #[cfg(feature = "show_crash_trace")] | ||
| bun_crash_handler::link_impl_BundleGenerateChunkCtx! { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -488,11 +488,6 @@ export var __callDispose = (stack, error, hasError) => { | |||||||||||||||||||
|
|
||||||||||||||||||||
| // ══════════════════════════════════════════════════════════════════════════ | ||||||||||||||||||||
| // Per-file parse worker — `getAST`/`getCodeForParseTask`/`runFromThreadPool`. | ||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 🟡 cb787f9 swept most of the 22:40 comment's list (including ParseTask.rs:493 here) but skipped two: Extended reasoning...What was left behindHEAD 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: Cluster (1): comment-#23 remainder — partial-fix follow-up
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
|
||||||||||||||||||||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 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-editedast/expr.rs; (2) nine\`\`-gatedartifacts 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) plusbundler/ParseTask.rs:493andinstall/PackageManager.rs:69TODO(b0); (3)(phase-d)injsc/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 isproperties/mod.rs:232-234— the doc-comment ONpub 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 hitsrc/ast/fold_string_addition.rs:7reads:Round-[CDEGH]is the first targeted pattern in the PR description, andrg '[Rr]ound-[A-Z]' srcreturns only this one hit tree-wide. The identical '(round-C draft)' phrasing was already swept fromjs_parser/fold.rs:23in this PR (commit 01c5536, resolved #3252795348/#3252809425) — this is the same artifact insrc/ast/rather thansrc/js_parser/, so the 'rg [Rr]ound-[A-Z] src/js_parser → 0' verification didn't cover it.src/ast/expr.rsIS 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 siblings908f16f swept the
\`\`-gatedempty-backtick artifacts from PR-edited css files (custom.rs ×4, font.rs, css_parser.rs — resolved #3252714625), butrg '\`-gated' src/css` still returns 8 hits in their parent/sibling modules:properties/mod.rspub mod font;/pub mod custom;rules/mod.rsrules/style.rsrules/property.rsselectors/selector.rsselectors/parser.rsPlus two non-css siblings of PR-edited files:
bundler/ParseTask.rs:493(\`\`-gated, sibling of bundle_v2.rs/options.rs) andinstall/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 thepub 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/ siblingsThe PR description explicitly lists
phase-damong targeted patterns, and the diff removes it fromparse_stmt.rs:37andHashObject.rs:193. After this PR,rg '\(phase-d\)|^//! phase-d'returns four hits:jsc/AbortSignal.rsPORT NOTE (phase-d):jsc/HTTPServerAgent.rsPORT NOTE (phase-d):runtime/hw_exports.rs//! phase-d:runtime/server/RequestContext.rsper phase-d rulesRequestContext.rsadditionally carries 4×TODO(b2-blocked)(lines 185/1030/2795/3186) — the form this PR converted ~20× toTODO(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)
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.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'):
-gated**: each '-gated' → 'gated' or rephrase; properties/mod.rs:232-234 should be aligned with custom.rs's rewritten doc.TODO(b0)→ `TODO(port)`.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.rssince none of the survivor files are in the diff.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks — cb787f9 handled all three clusters:
rg '[Rr]ound-[A-Z]' srcis 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.)