Clean up Zig-port phase comments and trivial lint warnings#30877
Conversation
…ixes Remove ~1,750 stale 'Phase A'/'Phase B' references left over from the Zig->Rust port (the phases are complete; the references confused the intent of TODO/PERF/PORT NOTE comments). Comments that encoded real deferred work keep the substance and drop the phase framing; comments that described a finished process step are removed. Also fix the trivial cargo check lints surfaced along the way: unused imports, unnecessary unsafe, unreachable-pub items, ambiguous glob re-exports, unused must-use results, and a private-type-in-public-alias. Two SAFETY/rustdoc comments that referenced removed methods are rewritten to name the current API.
|
Updated 3:30 AM PT - May 16th, 2026
❌ @Jarred-Sumner, your commit cc8b835 has some failures in 🧪 To try this PR locally: bunx bun-pr 30877That installs a local version of the PR into your bun-30877 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (12)
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/install/npm.rs (1)
1252-1257:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winConvert
SaveTask<'a>to own scope data instead of borrowing across threadpool task lifetime.
SaveTask<'a>capturesscope: &'a registry::Scopeand is scheduled asynchronously on the threadpool. InPackageManager/runTasks.rs:581, the scope is borrowed frommanagerviascope_for_package_name(). There is no guarantee thatmanagerremains valid until the async task completes and dereferences the scope inSaveTask::run(), creating a potential use-after-free UB.The codebase already documents this concern (lines 1251–1252): "TODO(port): lifetime —
scopeis borrowed across a thread boundary; Zig assumed the Registry.Scope outlives the threadpool task."Convert to owned task data: either wrap scope in
Arc(if shared across threads safely) or clone the required scope fields intoSaveTaskto eliminate the borrow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/install/npm.rs` around lines 1252 - 1257, SaveTask<'a> currently stores scope: &'a registry::Scope and is moved into the threadpool (via bun_core::heap::into_raw(SaveTask::new(...))), causing a potential use-after-free when PackageManager::runTasks (call site: scope_for_package_name() in PackageManager/runTasks.rs) borrows manager-owned scope; change SaveTask to own the scope data instead of borrowing: replace scope: &'a registry::Scope with an owned type (e.g., Arc<registry::Scope> or a small owned struct containing only the fields SaveTask::run() needs), update the SaveTask::new invocation to clone/wrap the scope (at the call site where scope_for_package_name() is used), and adjust SaveTask::run() to use the owned scope type so no reference to manager is required across the thread boundary.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/css/values/color.rs`:
- Line 1025: The doc comment currently includes a code literal containing the
text "// TODO(port): could be derived with a proc-macro." which renders as a
literal code fragment; replace that doc comment with a plain TODO-style doc
comment containing the same message (e.g., change the comment that contains "//
TODO(port): could be derived with a proc-macro." to a normal doc comment line
without backticks) so the note reads as a conventional TODO note in the
documentation; keep the wording and align formatting with surrounding doc
comments.
In `@src/install/yarn.rs`:
- Around line 480-482: The assignment to entry.resolved currently stores
git_info.url (which lacks the rewritten owned "https://github.com/..." value for
github: inputs); change Entry.resolved to hold an owned value (e.g., Cow<'a,
[u8]> or add an owned_url field on Entry), update the assignment site where
entry.resolved = Some(git_info.url) to store the rewritten owned_url instead,
and update the other assignment site(s) that set entry.resolved similarly; then
adjust downstream consumers in this file to read entry.resolved via
entry.resolved.as_deref() (or equivalent) so they see the rewritten owned URL.
---
Outside diff comments:
In `@src/install/npm.rs`:
- Around line 1252-1257: SaveTask<'a> currently stores scope: &'a
registry::Scope and is moved into the threadpool (via
bun_core::heap::into_raw(SaveTask::new(...))), causing a potential
use-after-free when PackageManager::runTasks (call site:
scope_for_package_name() in PackageManager/runTasks.rs) borrows manager-owned
scope; change SaveTask to own the scope data instead of borrowing: replace
scope: &'a registry::Scope with an owned type (e.g., Arc<registry::Scope> or a
small owned struct containing only the fields SaveTask::run() needs), update the
SaveTask::new invocation to clone/wrap the scope (at the call site where
scope_for_package_name() is used), and adjust SaveTask::run() to use the owned
scope type so no reference to manager is required across the thread boundary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 32d3b10b-1221-4e3f-8e1a-11326eb18b02
📒 Files selected for processing (300)
src/ast/ast_memory_allocator.rssrc/ast/b.rssrc/ast/e.rssrc/ast/expr.rssrc/ast/g.rssrc/ast/import_record.rssrc/ast/known_global.rssrc/ast/lexer_tables.rssrc/ast/lib.rssrc/ast/loader.rssrc/ast/new_store.rssrc/ast/nodes.rssrc/ast/stmt.rssrc/ast/symbol.rssrc/ast/ts.rssrc/base64/lib.rssrc/brotli/lib.rssrc/bun.rssrc/bun_alloc/basic.rssrc/bun_alloc/heap_breakdown.rssrc/bun_alloc/lib.rssrc/bun_alloc/memory.rssrc/bun_core/Global.rssrc/bun_core/Progress.rssrc/bun_core/bounded_array.rssrc/bun_core/deprecated.rssrc/bun_core/env.rssrc/bun_core/env_var.rssrc/bun_core/external_shared.rssrc/bun_core/fmt.rssrc/bun_core/lib.rssrc/bun_core/output.rssrc/bun_core/result.rssrc/bun_core/string/MutableString.rssrc/bun_core/string/SmolStr.rssrc/bun_core/string/StringBuilder.rssrc/bun_core/string/StringJoiner.rssrc/bun_core/string/escapeRegExp.rssrc/bun_core/string/immutable.rssrc/bun_core/string/immutable/escapeHTML.rssrc/bun_core/string/immutable/unicode.rssrc/bun_core/string/immutable/visible.rssrc/bun_core/string/mod.rssrc/bun_core/util.rssrc/bundler/AstBuilder.rssrc/bundler/Chunk.rssrc/bundler/Graph.rssrc/bundler/HTMLImportManifest.rssrc/bundler/LinkerContext.rssrc/bundler/LinkerGraph.rssrc/bundler/OutputFile.rssrc/bundler/ParseTask.rssrc/bundler/ServerComponentParseTask.rssrc/bundler/ThreadPool.rssrc/bundler/barrel_imports.rssrc/bundler/bundled_ast.rssrc/bundler/defines.rssrc/bundler/entry_points.rssrc/bundler/lib.rssrc/bundler/linker_context/OutputFileListBuilder.rssrc/bundler/linker_context/computeChunks.rssrc/bundler/linker_context/computeCrossChunkDependencies.rssrc/bundler/linker_context/convertStmtsForChunk.rssrc/bundler/linker_context/findAllImportedPartsInJSOrder.rssrc/bundler/linker_context/findImportedCSSFilesInJSOrder.rssrc/bundler/linker_context/findImportedFilesInCSSOrder.rssrc/bundler/linker_context/generateChunksInParallel.rssrc/bundler/linker_context/generateCompileResultForCssChunk.rssrc/bundler/linker_context/generateCompileResultForHtmlChunk.rssrc/bundler/linker_context/postProcessCSSChunk.rssrc/bundler/linker_context/postProcessJSChunk.rssrc/bundler/linker_context/writeOutputFilesToDisk.rssrc/bundler/ungate_support.rssrc/bundler_jsc/lib.rssrc/bunfig/bunfig.rssrc/cares_sys/c_ares.rssrc/cares_sys/lib.rssrc/clap/args.rssrc/clap/comptime.rssrc/clap/lib.rssrc/codegen/process_windows_translate_c.rssrc/collections/StaticHashMap.rssrc/collections/array_list.rssrc/collections/bit_set.rssrc/collections/comptime_string_map.rssrc/collections/lib.rssrc/collections/linear_fifo.rssrc/collections/multi_array_list.rssrc/collections/pool.rssrc/crash_handler/handle_oom.rssrc/crash_handler/lib.rssrc/css/css_modules.rssrc/css/css_parser.rssrc/css/dependencies.rssrc/css/error.rssrc/css/generics.rssrc/css/lib.rssrc/css/media_query.rssrc/css/printer.rssrc/css/properties/align.rssrc/css/properties/animation.rssrc/css/properties/background.rssrc/css/properties/border.rssrc/css/properties/border_image.rssrc/css/properties/border_radius.rssrc/css/properties/custom.rssrc/css/properties/flex.rssrc/css/properties/font.rssrc/css/properties/margin_padding.rssrc/css/properties/masking.rssrc/css/properties/size.rssrc/css/properties/svg.rssrc/css/properties/transform.rssrc/css/properties/transition.rssrc/css/properties/ui.rssrc/css/rules/container.rssrc/css/rules/font_face.rssrc/css/rules/keyframes.rssrc/css/rules/layer.rssrc/css/rules/mod.rssrc/css/rules/page.rssrc/css/rules/property.rssrc/css/rules/supports.rssrc/css/selectors/builder.rssrc/css/selectors/parser.rssrc/css/selectors/selector.rssrc/css/sourcemap.rssrc/css/targets.rssrc/css/values/angle.rssrc/css/values/calc.rssrc/css/values/color.rssrc/css/values/css_string.rssrc/css/values/easing.rssrc/css/values/ident.rssrc/css/values/image.rssrc/css/values/length.rssrc/css/values/number.rssrc/css/values/percentage.rssrc/css/values/size.rssrc/css/values/syntax.rssrc/css/values/time.rssrc/css/values/url.rssrc/dns/lib.rssrc/dotenv/env_loader.rssrc/errno/lib.rssrc/event_loop/AnyTaskWithExtraContext.rssrc/event_loop/DeferredTaskQueue.rssrc/event_loop/EventLoopTimer.rssrc/event_loop/ManagedTask.rssrc/event_loop/SpawnSyncEventLoop.rssrc/exe_format/pe.rssrc/glob/GlobWalker.rssrc/glob/lib.rssrc/glob/matcher.rssrc/http/AsyncHTTP.rssrc/http/HTTPContext.rssrc/http/HTTPRequestBody.rssrc/http/HTTPThread.rssrc/http/HeaderBuilder.rssrc/http/InternalState.rssrc/http/Signals.rssrc/http/h2_client/dispatch.rssrc/http/h3_client/AltSvc.rssrc/http/h3_client/Stream.rssrc/http/lib.rssrc/http/lshpack.rssrc/http/ssl_config.rssrc/http/websocket.rssrc/http/zlib.rssrc/http_jsc/websocket_client.rssrc/http_jsc/websocket_client/WebSocketDeflate.rssrc/http_jsc/websocket_client/WebSocketUpgradeClient.rssrc/http_types/ETag.rssrc/http_types/MimeType.rssrc/http_types/URLPath.rssrc/ini/lib.rssrc/install/NetworkTask.rssrc/install/PackageInstall.rssrc/install/PackageInstaller.rssrc/install/PackageManager/CommandLineArguments.rssrc/install/PackageManager/PackageJSONEditor.rssrc/install/PackageManager/PackageManagerResolution.rssrc/install/PackageManager/UpdateRequest.rssrc/install/PackageManager/WorkspacePackageJSONCache.rssrc/install/PackageManager/install_with_manager.rssrc/install/PackageManager/patchPackage.rssrc/install/PackageManager/security_scanner.rssrc/install/PackageManager/updatePackageJSONAndInstall.rssrc/install/PackageManagerTask.rssrc/install/TarballStream.rssrc/install/dependency.rssrc/install/extract_tarball.rssrc/install/hoisted_install.rssrc/install/hosted_git_info.rssrc/install/integrity.rssrc/install/isolated_install.rssrc/install/isolated_install/Installer.rssrc/install/isolated_install/Store.rssrc/install/isolated_install/Symlinker.rssrc/install/lib.rssrc/install/lifecycle_script_runner.rssrc/install/lockfile.rssrc/install/lockfile/Buffers.rssrc/install/lockfile/OverrideMap.rssrc/install/lockfile/Package.rssrc/install/lockfile/Package/Scripts.rssrc/install/lockfile/Package/WorkspaceMap.rssrc/install/lockfile/Tree.rssrc/install/lockfile/bun.lock.rssrc/install/lockfile/bun.lockb.rssrc/install/lockfile/lockfile_json_stringify_for_debugging.rssrc/install/lockfile/printer/Yarn.rssrc/install/lockfile/printer/tree_printer.rssrc/install/migration.rssrc/install/npm.rssrc/install/padding_checker.rssrc/install/patch_install.rssrc/install/postinstall_optimizer.rssrc/install/repository.rssrc/install/resolvers/folder_resolver.rssrc/install/windows-shim/BinLinkingShim.rssrc/install/windows-shim/bun_shim_impl.rssrc/install/yarn.rssrc/install_jsc/dependency_jsc.rssrc/install_jsc/ini_jsc.rssrc/install_jsc/npm_jsc.rssrc/install_jsc/update_request_jsc.rssrc/install_types/lib.rssrc/io/MaxBuf.rssrc/io/PipeReader.rssrc/io/PipeWriter.rssrc/io/heap.rssrc/io/lib.rssrc/io/posix_event_loop.rssrc/js_parser/lexer.rssrc/js_parser/lib.rssrc/js_parser/p.rssrc/js_parser/parse/parse_entry.rssrc/js_parser/parse/parse_prefix.rssrc/js_parser/parse/parse_property.rssrc/js_parser/parse/parse_stmt.rssrc/js_parser/parse/parse_typescript.rssrc/js_parser/parser.rssrc/js_parser/scan/scan_symbols.rssrc/js_parser/typescript.rssrc/js_parser/visit/visit_binary.rssrc/js_parser/visit/visit_expr.rssrc/js_parser/visit/visit_stmt.rssrc/js_parser_jsc/Macro.rssrc/js_parser_jsc/expr_jsc.rssrc/js_parser_jsc/lib.rssrc/js_printer/lib.rssrc/jsc/BunCPUProfiler.rssrc/jsc/CallFrame.rssrc/jsc/ConsoleObject.rssrc/jsc/Counters.rssrc/jsc/DOMURL.rssrc/jsc/DeprecatedStrong.rssrc/jsc/GarbageCollectionController.rssrc/jsc/JSArray.rssrc/jsc/JSCell.rssrc/jsc/JSPromise.rssrc/jsc/JSPropertyIterator.rssrc/jsc/JSRef.rssrc/jsc/JSSecrets.rssrc/jsc/ModuleLoader.rssrc/jsc/NodeModuleModule.rssrc/jsc/PluginRunner.rssrc/jsc/RefString.rssrc/jsc/RuntimeTranspilerCache.rssrc/jsc/SavedSourceMap.rssrc/jsc/SystemError.rssrc/jsc/Task.rssrc/jsc/URL.rssrc/jsc/ZigStackFramePosition.rssrc/jsc/ZigStackTrace.rssrc/jsc/bindgen.rssrc/jsc/btjs.rssrc/jsc/codegen.rssrc/jsc/comptime_string_map_jsc.rssrc/jsc/event_loop.rssrc/jsc/generated.rssrc/jsc/generated_classes_list.rssrc/jsc/host_fn.rssrc/jsc/ipc.rssrc/jsc/lib.rssrc/jsc/rare_data.rssrc/jsc/resolve_path_jsc.rssrc/jsc/resolver_jsc.rssrc/jsc/static_export.rssrc/jsc/uuid.rssrc/jsc/virtual_machine_exports.rssrc/jsc/web_worker.rssrc/libarchive/lib.rssrc/libarchive_sys/bindings.rssrc/lolhtml_sys/lol_html.rssrc/md/ansi_renderer.rssrc/md/html_renderer.rssrc/md/links.rssrc/md/parser.rs
💤 Files with no reviewable changes (3)
- src/http/h3_client/Stream.rs
- src/js_parser_jsc/lib.rs
- src/cares_sys/lib.rs
| /// Trait every colorspace implements. The Zig used `@field(this, "x")` over the | ||
| /// first three struct fields plus `alpha`; here we expose them by index. | ||
| /// `// TODO(port): Phase B may want to derive this with a proc-macro.` | ||
| /// `// TODO(port): could be derived with a proc-macro.` |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Simplify the doc comment syntax.
The current syntax embeds a comment inside a code literal inside a doc comment, which displays the literal text `// TODO(port): ...` rather than a conventional TODO note.
📝 Clearer doc comment
-/// `// TODO(port): could be derived with a proc-macro.`
+/// TODO(port): could be derived with a proc-macro.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// `// TODO(port): could be derived with a proc-macro.` | |
| /// TODO(port): could be derived with a proc-macro. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/css/values/color.rs` at line 1025, The doc comment currently includes a
code literal containing the text "// TODO(port): could be derived with a
proc-macro." which renders as a literal code fragment; replace that doc comment
with a plain TODO-style doc comment containing the same message (e.g., change
the comment that contains "// TODO(port): could be derived with a proc-macro."
to a normal doc comment line without backticks) so the note reads as a
conventional TODO note in the documentation; keep the wording and align
formatting with surrounding doc comments.
| // Fix: change Entry.resolved to Cow<'a, [u8]> (or store the | ||
| // owned buffer on Entry) so `owned_url` can be assigned here. | ||
| entry.resolved = Some(git_info.url); |
There was a problem hiding this comment.
Fix unresolved github: URL rewrite in entry.resolved.
Line 482 still stores git_info.url, which does not carry the rewritten owned https://github.com/... value for github: inputs. This leaves incorrect resolution behavior in place.
Suggested direction
- pub resolved: Option<&'a [u8]>,
+ pub resolved: Option<std::borrow::Cow<'a, [u8]>>,- entry.resolved = Some(git_info.url);
+ entry.resolved = Some(
+ git_info
+ .owned_url
+ .map(std::borrow::Cow::Owned)
+ .unwrap_or_else(|| std::borrow::Cow::Borrowed(git_info.url))
+ );Then adjust downstream reads to use entry.resolved.as_deref().
If you want, I can draft the full follow-up patch touching Entry, both assignment sites, and all entry.resolved consumers in this file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/install/yarn.rs` around lines 480 - 482, The assignment to entry.resolved
currently stores git_info.url (which lacks the rewritten owned
"https://github.com/..." value for github: inputs); change Entry.resolved to
hold an owned value (e.g., Cow<'a, [u8]> or add an owned_url field on Entry),
update the assignment site where entry.resolved = Some(git_info.url) to store
the rewritten owned_url instead, and update the other assignment site(s) that
set entry.resolved similarly; then adjust downstream consumers in this file to
read entry.resolved via entry.resolved.as_deref() (or equivalent) so they see
the rewritten owned URL.
| pub router: Option<Router<'a>>, | ||
| pub source_map: options::SourceMapOption, | ||
|
|
||
| // B-2 un-gated: real `crate::linker::Linker` so | ||
| // `crate::linker::Linker` so |
There was a problem hiding this comment.
🟡 After commit b51c41b fixed the B-1/B-2 stragglers, ~26 references to the PR's primary target pattern (Phase A / Phase B / Phase-A) still survive in seven files this PR already edits — transpiler.rs (10), bundle_v2.rs (8), options.rs (4), PackageManager.rs (2), renamer.rs:25, AsyncModule.rs:9, Progress.rs:66. Each of these files has hunks in this PR removing other Phase references, so these look like grep-pattern misses (e.g. Phase B: / Phase-A / Phase B-3 variants) rather than intentional exclusions. Repro: grep -rnE '\bPhase[- ]?[AB]\b' src/ --include='*.rs' (77 repo-wide). Comment-only — worth one more sweep since it's the PR's stated purpose.
Extended reasoning...
What was missed
The PR's stated purpose is removing the ~1,750 stale "Phase A" / "Phase B" references from the Zig→Rust port. After commit b51c41b ("Purge remaining B-1/B-2 port-batch comments") addressed the earlier review comment about B-1/B-2 stragglers, ~77 references to the primary Phase[- ]?[AB] pattern still survive repo-wide. 26 of those are in seven files that this PR already edits:
src/bundler/transpiler.rs: 132, 879, 1470, 1509, 1525, 1892, 2187, 2292, 2306, 2359src/bundler/bundle_v2.rs: 303, 613, 1591, 2900, 5416, 5973, 6851, 7601src/bundler/options.rs: 417, 1509, 1793, 1980src/install/PackageManager.rs: 366, 816src/js_printer/renamer.rs: 25src/jsc/AsyncModule.rs: 9src/bun_core/Progress.rs: 66 (B-0 round 1)
Why these are oversights, not intentional exclusions
Every one of these seven files appears in the PR's changed-files list, and each has hunks in this PR that rewrite or remove other Phase references. For example:
transpiler.rs— this PR removes fourB-2 un-gated:headers at the lines that became 144/630/791/2278, yet leaves 10Phase A/Breferences in the same file untouched.bundle_v2.rs— this PR rewrites theB-2 un-gated headerat line 1 and theB-2 un-gated implcomment at ~157, yet leaves 8Phase A/Breferences including line 303's// Phase-A draft body — gated until lower-tier crate surfaces solidify.options.rs— this PR rewrites 6B-2 type aliases/B-3/TODO(b2-blocked)comments, yet leaves 4Phase Breferences.
The surviving references are exactly the patterns this PR transforms hundreds of times elsewhere: profile in Phase B → profile if hot, Phase-A Str convention → dropped, Phase B: confirm X → TODO(port): confirm X, Phase B threads 'bump → thread 'bump once ….
Not a duplicate of the earlier review comment
The earlier inline comment (#3252413043, now resolved by commit b51c41b) covered the secondary B-1/B-2/TODO(b1) pattern in bun_core/lib.rs + http/lib.rs — different files, different pattern. This finding covers the primary Phase A/Phase B pattern (the PR title's literal target) in seven different files, and remains after b51c41b. The author already acted on the earlier comment, so there's precedent that this feedback is actionable.
Step-by-step proof
- PR description: "Removes the ~1,750 stale 'Phase A' / 'Phase B' references the Zig→Rust port left across ~600 files."
git log --oneline -1→ HEAD isb51c41b8 Purge remaining B-1/B-2 port-batch comments(the follow-up that fixed the earlier comment).grep -rnE '\bPhase[- ]?[AB]\b' src/ --include='*.rs' | wc -l→ 77.grep -rnE '\bPhase[- ]?[AB]\b' src/bundler/transpiler.rs→ 10 hits (132/879/1470/1509/1525/1892/2187/2292/2306/2359).- The PR diff for
transpiler.rsshows hunks at lines 144, 630, 791, 2278 removing// B-2 un-gated:headers — proving the file is in scope and the same kind of comment is being removed from it. - Spot-check
transpiler.rs:2292:// PERF(port): outermatch formatis one extra branch — profile in Phase B.— identical shape to dozens ofprofile in Phase B→profile if hotrewrites elsewhere in this PR (e.g.bun_alloc/lib.rs,base64/lib.rs,css/properties/align.rs).
The pattern is consistent with grep-pattern misses on variants like Phase B: (colon-suffixed), Phase-A (hyphenated mid-word), and Phase B-3 (suffixed) that the bulk-rewrite regex didn't catch.
Impact and fix
Comment-only; zero runtime effect. Nit severity. But since removing these references is the PR's sole stated purpose ("Removes the ~1,750 stale 'Phase A' / 'Phase B' references"), and the author has already done two follow-up commits to catch stragglers, one more grep -rnE '\bPhase[- ]?[AB]\b' src/ --include='*.rs' sweep would finish the job for the files already in this diff. The ~51 hits in files not touched by this PR can reasonably wait for a follow-up.
| // Round-C lowered `const JSX: JSXTransformType` → `J: JsxT`, so `BinaryExpressionVisitor` carries | ||
| // the parser generics directly. | ||
| // Phase B diff readers should map: | ||
| // Diff readers should map: |
There was a problem hiding this comment.
🟡 Commit cc8b835 applied #3252612660's case-sensitive \bround-[A-E]\b grep, so ~37 references using the capitalized Round-[A-G] form and lowercase round-G form survived in files this PR already edits — most visibly here at line 17 ("Round-C lowered…"), two lines above the "Phase B diff readers" → "Diff readers" rewrite this PR made at line 19. Other survivors: js_parser/p.rs (43/154/198/891/6835/7649/7651/7741/7865/8370/8837/8849/9326), visit/{visit_expr.rs:41, visit_stmt.rs:70, mod.rs:42}, parse/{parse_prefix.rs:29/31, parse_stmt.rs:35/37, parse_property.rs:29, parse_typescript.rs:37, …}, parser.rs:15, lib.rs:966, bundler/transpiler.rs:2362. Repro: grep -rnE '\b(Round-[A-G]|round-[FG])\b' src/ --include='*.rs' → 37 hits. Comment-only — same straggler class, one more case-insensitive sweep with the wider letter range finishes it.
Extended reasoning...
What was missed
Commit cc8b835 ("Clean up remaining round-[A-E]/Track-A/gated-draft port-batch markers") addressed the previous comment #3252612660's repro grep — \bround-[A-E]\b, case-sensitive, letters A–E only. That grep is now nearly clean (3 hits, all in files NOT in this PR's changed-files list: fold.rs, scan_side_effects.rs, fold_string_addition.rs), so a developer re-running #3252612660 would consider it resolved for PR-touched files.
But 37 references using two variants that grep cannot match still survive in files this PR already edits:
- Capitalized
Round-[A-G]—Round-C,Round-D,Round-E,Round-G(capital R doesn't match a case-sensitive\bround-) - Lowercase
round-G— letter G is outside the[A-E]character class
Repro: grep -rnE '\b(Round-[A-G]|round-[FG])\b' src/ --include='*.rs' → exactly 37 hits, 36 in src/js_parser/.
Why these are oversights, not intentional exclusions
The most direct evidence is right here at visit_binary.rs:17–19. The PR diff shows:
// Round-C lowered `const JSX: JSXTransformType` → `J: JsxT`, so `BinaryExpressionVisitor` carries
// the parser generics directly.
-// Phase B diff readers should map:
+// Diff readers should map:Line 19 was rewritten ("Phase B diff readers" → "Diff readers") with line 17's "Round-C lowered…" left as unchanged context two lines above — a phase reference rewritten adjacent to a surviving one in the same comment block.
The same adjacency pattern repeats in p.rs: cc8b835's diff against that file shows ~47 lines changed, removing lowercase round-[A-E] lines (e.g. -// round-D widens this, -// round-B Expr methods), while leaving 13 capitalized/round-G references in the same file untouched (43/154/198/891/6835/7649/7651/7741/7865/8370/8837/8849/9326). The author swept the file with the exact case-sensitive [A-E] regex from #3252612660 — which is precisely why Round-D (capital R) and round-G (G > E) survived.
Of the 37 hits, at least 14 distinct files are in this PR's changed-files list: p.rs, visit_binary.rs, visit_expr.rs, visit_stmt.rs, parser.rs, lib.rs, parse_prefix.rs, parse_stmt.rs, parse_property.rs, parse_typescript.rs, plus bundler/transpiler.rs (which #3252525489 already covers for Phase[- ]?[AB] but not for Round-G).
Not a duplicate of #3252612660
Comment #3252612660's repro grep is case-sensitive \bround-[A-E]\b. None of these 37 hits match it: every one is either capital-R Round- or lowercase round-G (G outside [A-E]). The line numbers cited in #3252612660 for p.rs (56/66/68/590/723/1360/1605/1665/3708/3856/5070/6614/6726/6852/8323/8325) are all different from the 13 surviving p.rs hits — confirming cc8b835 cleaned the lowercase [A-E] occurrences and left the capital-R / round-G forms untouched.
This is the same straggler class but a distinct lexical pattern with a distinct repro — exactly the relationship that comments #3252413043 (B-1/B-2), #3252525489 (Phase[- ]?[AB]), and #3252612660 (round-[A-E]) already have to one another as separate non-duplicate nits, each of which the author has acted on with a follow-up commit.
Step-by-step proof
git log --oneline -1→ HEAD iscc8b835b Clean up remaining round-[A-E]/Track-A/gated-draft port-batch markers— the commit message names the case-sensitive [A-E] regex.grep -rnE '\bround-[A-E]\b' src/ --include='*.rs'(comment #3252612660's repro) → 3 hits, none in PR-touched files. Comment #3252612660 is effectively resolved for this PR's scope.grep -rnE '\b(Round-[A-G]|round-[FG])\b' src/ --include='*.rs' | wc -l→ 37.sed -n '15,21p' src/js_parser/visit/visit_binary.rs→ line 17 reads// Round-C lowered …, line 19 reads// Diff readers should map:(rewritten by this PR from// Phase B diff readers should map:).grep -nE '\bround-[A-E]\b' src/js_parser/p.rs→ 0 hits (cc8b835 cleaned them).grep -nE '\b(Round-[A-G]|round-G)\b' src/js_parser/p.rs→ 13 hits — disjoint patterns in a file cc8b835 actively edited.
Impact and fix
Comment-only; zero runtime effect. Nit severity. But since this is the PR's sole stated purpose and the author has done five follow-up commits on this exact feedback class (most recently cc8b835, the HEAD), one more sweep finishes the job:
grep -rniE '\bround-[A-G]\b' src/ --include='*.rs'(case-insensitive -i + [A-G] catches everything in one pass — all 40 remaining hits including the 3 lowercase [A-E] stragglers in non-PR files). Suggested rewrites following the PR's own conventions: "Round-C lowered const JSX …" → drop the "Round-C" prefix; "moved to ungated impl (round-G)" → "moved to ungated impl"; "Round-D/E modules: stub re-exports" → "Stub re-exports"; "Round-G fix:" → drop the prefix or fold into PORT NOTE:.
…0877) ## What Removes the ~1,750 stale "Phase A" / "Phase B" references the Zig→Rust port left across ~600 files. The port phases are complete; the references confuse what's a real TODO vs. a finished process step. Comments that encode real deferred work (e.g. `PERF(port): was X — profile in Phase B`) keep the substance and drop the phase framing (`PERF(port): was X — profile if hot.`). Comments that only describe past process steps are removed. Also fixes the trivial lint warnings cargo check surfaced along the way: unused imports, an unnecessary `unsafe` block over a safe `extern "C" fn`, `unreachable_pub` items, ambiguous glob re-exports, an unused `#[must_use]` result, and a private-type-in-public-alias. Two SAFETY/rustdoc comments that referenced API methods removed in a follow-up are rewritten to name the current entry points. ## What this is not No behavior changes. No public API changes. The hive-pool deprecation warnings (`HiveArrayFallback::get/try_get`) are not silenced here — the call sites are migrated to the safe API in a follow-up PR. ## Verification - `cargo check --workspace` clean for everything this PR touches - `cargo fmt --all` applied - `bun run rust:check-all` (all 6 target triples)
What
Removes the ~1,750 stale "Phase A" / "Phase B" references the Zig→Rust port left across ~600 files. The port phases are complete; the references confuse what's a real TODO vs. a finished process step. Comments that encode real deferred work (e.g.
PERF(port): was X — profile in Phase B) keep the substance and drop the phase framing (PERF(port): was X — profile if hot.). Comments that only describe past process steps are removed.Also fixes the trivial lint warnings cargo check surfaced along the way: unused imports, an unnecessary
unsafeblock over a safeextern "C" fn,unreachable_pubitems, ambiguous glob re-exports, an unused#[must_use]result, and a private-type-in-public-alias. Two SAFETY/rustdoc comments that referenced API methods removed in a follow-up are rewritten to name the current entry points.What this is not
No behavior changes. No public API changes. The hive-pool deprecation warnings (
HiveArrayFallback::get/try_get) are not silenced here — the call sites are migrated to the safe API in a follow-up PR.Verification
cargo check --workspaceclean for everything this PR touchescargo fmt --allappliedbun run rust:check-all(all 6 target triples)