chore: remove stale 'cfg-gated'/'un-gate'/'blocked_on' port-era comments - #32519
Merged
Conversation
…nts and collapse their shims
These comments were left behind from the incremental Zig->Rust port,
claiming code was cfg-gated or blocked on dependencies that have since
landed. 150 of 158 such markers were verified stale and removed; the
remaining 8 accurately describe real platform/feature #[cfg(...)] gating.
Shims that existed only because their comment claimed the canonical
impl was gated are collapsed into the canonical:
repl.rs: global_clear_exception/global_to_js_value/
vm_set_execution_forbidden/vm_mut -> JSGlobalObject/VM/VirtualMachine
inherent methods
StandaloneModuleGraph.rs: inlined open+mkdir+retry -> bun_sys::File::make_open
js_parser/visit/mod.rs: local stmts_to_single_stmt_ -> P::stmts_to_single_stmt
js_parser/lower/lower_esm_exports_hmr.rs: local generate_temp_ref ->
P::generate_temp_ref
css/selectors/selector.rs: dead 'let _ = arguments;' after real to_css_raw
Collaborator
Author
|
Updated 10:42 PM PT - Jun 19th, 2026
✅ @robobun, your commit 5f3539bf6bb62380d5160b1aabcddf1c8ff8add5 passed in 🧪 To try this PR locally: bunx bun-pr 32519That installs a local version of the PR into your bun-32519 --bun |
Contributor
|
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:
WalkthroughThis PR removes hundreds of stale ChangesIncremental un-gating cleanup with targeted functional completions
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
…gated comments Adds test/internal/port-era-markers.test.ts which scans src/**/*.rs for port-era comment jargon (blocked_on, un-gate, un-gates, un-gated, ungated, re-gated, empty-backtick-gated) that this PR drove to zero, preventing reintroduction. Also fixes the last 7 occurrences the test flagged: five empty-backtick '-gated' comments in timer/mod.rs, jsc_hooks.rs, RequestContext.rs, css/rules/mod.rs, css_jsc/lib.rs, plus rewords util.rs spawn_ffi and sys/windows Win32Error comments.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…ond replace_decl block
Collaborator
Author
|
Re: duplicate flag for #30885: that PR targets a different set of port-batch markers ( |
…note, and CSS-modules ref-arm in is_selector_unused Replaces the three TODO markers flagged by diff hygiene with real code: - JSValue::call: debug-build event-loop bookkeeping (js_call_count_outside_tick_queue / last_fn_name), matching JSValue.zig and the existing JSPromise.rs resolve/reject pattern. - css/declaration.rs: wire the secondary location note for ComposesState::DisallowNotSingleClass via warn_fmt_with_notes, matching declaration.zig. - css/selectors/selector.rs: resolve CSS-modules symbol refs in is_selector_unused via IdentOrRef::as_original_string instead of skipping them.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/jsc/JSValue.rs`:
- Around line 1644-1654: The debug block uses the `?` operator on
`self.get_name(global)` when setting `loop_.debug.last_fn_name`, which can cause
an early return from the `call()` method in debug builds while the same code
path does not exist in release builds. This creates inconsistent behavior
between debug and release builds where debug builds may fail while release
builds succeed. Remove the `?` operator from the `self.get_name(global)?` call
and instead handle the error by either ignoring it (letting the assignment be
skipped if it fails) or providing a sensible fallback value (such as a default
string). This ensures debug instrumentation does not alter the observable
control flow behavior of the function.
🪄 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: 495c1bb0-8b83-4adc-9894-6a9437cfb466
📒 Files selected for processing (3)
src/css/declaration.rssrc/css/selectors/selector.rssrc/jsc/JSValue.rs
…s; swallow get_name error in debug bookkeeping
- De-nest four redundant '{ }' blocks left behind after removing
blocked_on comments (FontFace/Keyframes arms, composes_state if-block,
CustomFunction tail).
- Remove two orphaned bare '//' separator lines in font_face.rs that
rustfmt does not strip.
- Trim custom_ident_to_css doc comment which still claimed write_ident
was gated.
- JSValue::call debug bookkeeping: swallow get_name error via 'if let Ok'
so debug instrumentation does not short-circuit ahead of the actual
call; any pending exception from get_name is still surfaced by the
subsequent from_js_host_call.
…ailing comment; make marker lint case-insensitive and sweep 9 capitalized instances - JSValue::call: read debug flags and drop the &mut EventLoop borrow before calling get_name (which may re-enter JS), then re-borrow to assign last_fn_name, per the event_loop_mut() contract. - css/rules/import.rs: delete orphaned 'silence unused-import warnings on the gated bodies' deps' trailer at EOF. - test/internal/port-era-markers.test.ts: add /i flag to all patterns; show flags in test names. - Sweep 9 capitalized Un-gate/Un-gated/Re-gated comments the case-sensitive grep missed (css_parser.rs, media_query.rs, webcore.rs, timer_object_internals.rs, node_fs.rs, AnyRequestContext.rs, spawn/process.rs, H2Client.rs, ini/lib.rs).
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/internal/port-era-markers.test.ts (1)
57-68:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winScope matching to comment text to avoid false-positive lint failures.
Line 63 applies banned regexes to every Rust line, but this test’s contract is “comment jargon.” That can fail on legitimate code/string content containing these tokens. Restrict matching to comment lines before running the banned patterns.
Suggested patch
for (const abs of rustSources) { const rel = path.relative(root, abs); const content = await file(abs).text(); const lines = content.split("\n"); for (const { pattern } of banned) { for (let i = 0; i < lines.length; i++) { - if (pattern.test(lines[i])) { + const line = lines[i]; + // Lint intent is stale marker jargon in comments. + if (!line.includes("//")) continue; + if (pattern.test(line)) { hits[pattern.source].push(`${rel}:${i + 1}`); } } } }🤖 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 `@test/internal/port-era-markers.test.ts` around lines 57 - 68, The banned pattern matching in the nested loop is applying regexes to every line of Rust source code, causing false positives when pattern tokens appear in legitimate code or strings. Modify the line-by-line iteration to first filter for comment lines only (lines that contain Rust comments starting with //) before testing against the banned patterns. Add a condition to check if the current line is a comment before executing the pattern.test(lines[i]) call, so the banned patterns are only matched against actual comment text.
🤖 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.
Outside diff comments:
In `@test/internal/port-era-markers.test.ts`:
- Around line 57-68: The banned pattern matching in the nested loop is applying
regexes to every line of Rust source code, causing false positives when pattern
tokens appear in legitimate code or strings. Modify the line-by-line iteration
to first filter for comment lines only (lines that contain Rust comments
starting with //) before testing against the banned patterns. Add a condition to
check if the current line is a comment before executing the
pattern.test(lines[i]) call, so the banned patterns are only matched against
actual comment text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a8e47871-b353-41da-9d45-3240639ed171
📒 Files selected for processing (12)
src/css/css_parser.rssrc/css/media_query.rssrc/css/rules/import.rssrc/http/H2Client.rssrc/ini/lib.rssrc/jsc/JSValue.rssrc/runtime/node/node_fs.rssrc/runtime/server/AnyRequestContext.rssrc/runtime/timer/timer_object_internals.rssrc/runtime/webcore.rssrc/spawn/process.rstest/internal/port-era-markers.test.ts
💤 Files with no reviewable changes (7)
- src/http/H2Client.rs
- src/ini/lib.rs
- src/spawn/process.rs
- src/runtime/server/AnyRequestContext.rs
- src/css/css_parser.rs
- src/runtime/webcore.rs
- src/css/rules/import.rs
… doc, zlib NodeMode re-export, masking FillRule import)
…notes and two more stale 'gated' comments in timer/mod.rs
…se host_fns.rs get_own shim - Collapse local JSC__JSValue__getOwn extern + get_own() wrapper in host_fns.rs into JSValue::get_own (the canonical, identical body). - Drop or reword stale 'is/stays/still gated' / '(gated)' comments in css/rules/mod.rs (3 sites), resolver/lib.rs, bun_core/lib.rs, bundler/linker.rs, bundler/transpiler.rs, runtime/ffi/ffi_body.rs, runtime/jsc_hooks.rs. - Remaining 'gated' occurrences in PR-edited files all describe real const-generic / #[cfg] / feature / runtime-conditional gating: http/lib.rs (SSL const-generic), install/lib.rs (shim_standalone feature), VirtualMachine.rs (0/1 transition), node_fs.rs (cfg(windows) and IS_U16 const-generic), Body.rs (require_array conditional).
This was referenced Jun 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Cleans up comments left behind from the incremental Zig→Rust port that claim code is "cfg-gated", "blocked_on X", or "will un-gate when Y lands", where X/Y have since landed and the claim is no longer true. Also collapses a handful of local shims whose only justification was one of those stale comments.
Before: 158 such markers across 87 files.
After: 8 remaining, all of which accurately describe real platform/feature
#[cfg(...)]gating (macOS/Windows image clipboard, POSIX/Windows stdio alias, WASM timer selection, Win32 x64 callconv extern blocks, debug-assertions context, the#[cfg(any())]Win32Error table handled by #31995).Shims collapsed into canonical impls
src/runtime/cli/repl.rs: removedglobal_clear_exception,global_to_js_value,vm_set_execution_forbidden,vm_mut(48 lines). Comment claimed "the canonical impls live in cfg-gated JSGlobalObject.rs / VM.rs (see src/jsc/lib.rs `_gated`)"; no_gatedmodule exists andJSGlobalObject::clear_exception/::to_js_value,VM::set_execution_forbidden,VirtualMachine::as_mutare all public and un-gated. 25 call sites rewired.src/standalone_graph/StandaloneModuleGraph.rs: replaced hand-inlined open+mkdir+retry withbun_sys::File::make_open. Comment claimed "src/sys/File.rsis still cfg-gated upstream";make_openis public atsrc/sys/file.rs:112and already used frombuild_command.rs.src/js_parser/visit/mod.rs: removed localstmts_to_single_stmt_(duplicate ofP::stmts_to_single_stmt). Comment claimed it was "``-gated (P.rs:6267, blocked on S::Block Default)".src/js_parser/lower/lower_esm_exports_hmr.rs: removed localgenerate_temp_ref(duplicate ofP::generate_temp_ref). Comment claimed "P::generate_temp_refis ``-gated in P.rs (round-6 re-gate)".src/js_parser/visit/mod.rs: removed deadlet _ = &mut j;keep-alive whose comment said "keep 'outer label live until #[cfg] un-gates".src/css/selectors/selector.rs: removed two deadlet _ = arguments;lines after realarguments.to_css_raw(dest)?calls.Comments rewritten or deleted
The rest are comment-only changes removing port-progress narrative ("cycle-5", "round-D/E/G/H", "phase-c/d", "tier-0", "reconciler-6 re-gate", "`` gates carry blocked_on notes") and stale blocker lists that name dependencies which now exist. Where a comment carried useful non-port information (aliasing rationale, init-order invariants, layering notes), that part is kept; only the stale gating claim is removed.
A few
blocked_oncomments insrc/css/described genuinely incomplete code (e.g.Property::longhand's no-oplh()stub, the CSS-modules ref-arm inselector_has_composes_for_property); those were reworded to plain TODOs rather than deleted.Verification
80 files changed, +171/-796.