Skip to content

Strip comment blocks longer than 3 lines from .rs files#31201

Open
robobun wants to merge 11 commits into
mainfrom
farm/a9f7cf29/strip-long-rs-comments
Open

Strip comment blocks longer than 3 lines from .rs files#31201
robobun wants to merge 11 commits into
mainfrom
farm/a9f7cf29/strip-long-rs-comments

Conversation

@robobun
Copy link
Copy Markdown
Collaborator

@robobun robobun commented May 22, 2026

What

Adds scripts/strip-long-rs-comments.ts and applies it across the tree.

The script removes every //-style comment block longer than 3 lines (i.e. 4+ consecutive comment-only lines) from git-tracked *.rs files, except:

  • a block that is the first non-blank content in the file (top-of-file header),
  • // HOST_EXPORT(...) marker lines, which src/codegen/generate-host-exports.ts scrapes to emit extern "C" thunks — those are kept even when the surrounding prose is removed, and
  • any block containing SAFETY, a line opening with a case-insensitive Safety:-style marker, or a /// # Safety heading — clippy::undocumented_unsafe_blocks is deny workspace-wide and matches the marker case-insensitively, so those justifications are kept in full.

A "comment block" is a maximal run of lines whose trimmed content starts with // (covers //, ///, //!). Inline /* ... */ and trailing // ... after code aren't touched. vendor/, packages/ (externally published crates) and scripts/verify-baseline-static/ are skipped.

When a block is removed, adjacent blank lines are collapsed so the result doesn't leave double blanks or a stray blank after {.

Result

1062 files changed, 649 insertions(+), 70918 deletions(-)

Script usage

bun scripts/strip-long-rs-comments.ts           # apply in-place
bun scripts/strip-long-rs-comments.ts --dry     # report only
bun scripts/strip-long-rs-comments.ts --min N   # threshold (default 4)
bun scripts/strip-long-rs-comments.ts a.rs b.rs # only these files

Rebase onto main

Main gained 9 commits touching 10 .rs files after the original pass (WebKit upgrade, yaml/parser/shell/patch fixes). Conflicts in src/js_parser/parser.rs, src/jsc/JSType.rs and src/runtime/shell/states/Expansion.rs were resolved by taking main's version of each file and re-running the script on it, so the upstream code changes (JSType renumbering, shell meta_offsets rework, parser changes) are preserved exactly and only their comments are stripped. The last commit re-runs the script over src/js_parser/parse/{mod,parse_stmt}.rs and src/parsers/yaml.rs, which gained 4+-line comment blocks from upstream commits that merged cleanly.

A second rebase onto current main (18 further commits, 17 touching .rs files) had a single conflict, src/js_parser/parse/parse_typescript.rs (from #31261). It was resolved the same way: take main's version of the file and re-run the script on it, so the upstream parser change is preserved exactly and only its comments are stripped. A follow-up commit strips the 12 comment blocks (58 lines) that the newly merged upstream commits introduced (src/md/links.rs, src/md/inlines.rs, src/js_parser/p.rs, src/js_parser/parse/parse_stmt.rs, src/js_parser/lower/lower_decorators.rs).

A third rebase picked up #31240, #31264 and #31272 (3 commits, 3 .rs files), which landed while the previous head was in CI. Conflicts in src/bun_core/lib.rs and src/runtime/shell/states/Expansion.rs were resolved the same way — take main's version of the file and re-run the script on it — so the upstream fixes (the b.len() < a.len() guard in eql_case_insensitive_ascii, the component-leading-! glob wrapping) are preserved exactly and only their comments are stripped. A follow-up commit strips the one 4-line block #31240 added to src/js_parser/scan/scan_imports.rs, which had merged cleanly.

A fourth rebase picked up the next 10 upstream commits (67 .rs files: the round-3 hardening pass #31221 plus CSS/parser/markdown/JSONC/CryptoHasher fixes). 15 files conflicted — 14 on the main strip commit and src/md/inlines.rs on a follow-up commit — and were resolved the same way: take main's version of the file and re-run the script, so the upstream code changes are preserved exactly and only their comments are stripped. A follow-up commit strips the 33 new 4+-line blocks (173 lines) those commits introduced across the 25 files that merged cleanly.

A fifth rebase picked up the next 6 upstream commits, dominated by #31254 (pub(crate) restriction + dead-code removal across 789 .rs files). 240 files conflicted on the main strip commit and were resolved the same way — take main's version of each file and re-run the script — plus 5 files that #31254 deleted upstream (src/meta/*, src/url_jsc/lib.rs, src/libarchive_sys/bindings.rs, src/codegen/process_windows_translate_c.rs) are deleted here too rather than resurrected. A follow-up commit strips the 2 new 4+-line blocks the remaining upstream commits introduced in cleanly-merged files. The diffstat shrank slightly because upstream's dead-code removal deleted some previously-stripped files and lines.

A sixth rebase picked up the next 11 upstream commits (11 .rs files: CSS nesting/@container minification fixes, js_parser decorator/auto-accessor/TS-depth fixes, node:http2 GOAWAY + HEADERS/CONTINUATION work, js_printer fixes). Only src/css/rules/mod.rs and src/css/rules/style.rs conflicted, on the main strip commit, and were resolved the same way — take main's version of the file and re-run the script — so the upstream changes are preserved exactly and only their comments are stripped. A follow-up commit strips the 6 new 4+-line blocks (25 lines) the remaining upstream commits introduced in cleanly-merged files (src/js_parser/parse/mod.rs, src/js_parser/visit/visit_stmt.rs, src/runtime/api/bun/h2_frame_parser.rs).

Verification

  • Relative to current main, the .rs diff removes only comment lines; the 229 added lines are the cargo fmt reflow that follows block removal, one pub use reorder from autofix.ci, and 7 short comment lines (the 2-line divergence note in lifecycle_script_runner.rs and the reorder-rationale notes in the js_parser substitution-failure arms)
  • All 90 // HOST_EXPORT(...) markers present on main are still present; zero SAFETY / # Safety lines removed, and the one mixed-case // Safety: block in the tree (src/jsc/AsyncModule.rs) is kept
  • No raw-string literal content touched (checked the multi-line literals in the upstream-changed files byte-for-byte)
  • cargo fmt --all -- --check is clean with the pinned nightly-2026-05-06
  • bun scripts/strip-long-rs-comments.ts --dry over the tree reports nothing left to strip (idempotent)
  • test/internal/strip-long-rs-comments.test.ts → 5 pass / 0 fail (unit cases + tree-is-clean scan)
  • bun bd built and linked cleanly on the pre-rebase revision of this diff; ./build/debug/bun-debug -e 'console.log(1+1)'2

@robobun
Copy link
Copy Markdown
Collaborator Author

robobun commented May 22, 2026

Updated 3:26 PM PT - May 24th, 2026

@robobun, your commit 737377c is building: #57744

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 22, 2026

Review Change Stack

Warning

Review limit reached

@robobun, we couldn't start this review because you've used your available PR reviews for now.

Your plan includes 5 reviews of capacity. Refill in 30 minutes and 52 seconds.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more review capacity refills, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 7e7b5023-638c-408c-969a-73863d1b162d

📥 Commits

Reviewing files that changed from the base of the PR and between 11d5016 and 737377c.

📒 Files selected for processing (259)
  • scripts/strip-long-rs-comments.ts
  • src/analytics/lib.rs
  • src/analytics/schema.rs
  • src/api/lib.rs
  • src/ast/ast_memory_allocator.rs
  • src/ast/ast_result.rs
  • src/ast/b.rs
  • src/ast/base.rs
  • src/ast/binding.rs
  • src/ast/char_freq.rs
  • src/ast/e.rs
  • src/ast/expr.rs
  • src/ast/fold_string_addition.rs
  • src/ast/g.rs
  • src/ast/import_record.rs
  • src/ast/known_global.rs
  • src/ast/lexer_tables.rs
  • src/ast/lib.rs
  • src/ast/loader.rs
  • src/ast/new_store.rs
  • src/ast/nodes.rs
  • src/ast/op.rs
  • src/ast/runtime.rs
  • src/ast/s.rs
  • src/ast/scope.rs
  • src/ast/server_component_boundary.rs
  • src/ast/stmt.rs
  • src/ast/symbol.rs
  • src/ast/transpiler_cache.rs
  • src/ast/ts.rs
  • src/base64/lib.rs
  • src/boringssl/lib.rs
  • src/boringssl_sys/boringssl.rs
  • src/boringssl_sys/lib.rs
  • src/brotli/lib.rs
  • src/brotli_sys/brotli_c.rs
  • src/bun_alloc/MaxHeapAllocator.rs
  • src/bun_alloc/MimallocArena.rs
  • src/bun_alloc/NullableAllocator.rs
  • src/bun_alloc/ast_alloc.rs
  • src/bun_alloc/c_thunks.rs
  • src/bun_alloc/fallback/z.rs
  • src/bun_alloc/hashbrown_bridge.rs
  • src/bun_alloc/heap_breakdown.rs
  • src/bun_alloc/lib.rs
  • src/bun_alloc/maybe_owned.rs
  • src/bun_alloc/memory.rs
  • src/bun_alloc/stack_fallback.rs
  • src/bun_bin/lib.rs
  • src/bun_bin/phase_c_exports.rs
  • src/bun_core/Global.rs
  • src/bun_core/Progress.rs
  • src/bun_core/atomic_cell.rs
  • src/bun_core/bounded_array.rs
  • src/bun_core/build.rs
  • src/bun_core/debug.rs
  • src/bun_core/deprecated.rs
  • src/bun_core/env.rs
  • src/bun_core/env_var.rs
  • src/bun_core/external_shared.rs
  • src/bun_core/feature_flags.rs
  • src/bun_core/fmt.rs
  • src/bun_core/heap.rs
  • src/bun_core/hint.rs
  • src/bun_core/lib.rs
  • src/bun_core/output.rs
  • src/bun_core/result.rs
  • src/bun_core/string/HashedString.rs
  • src/bun_core/string/MutableString.rs
  • src/bun_core/string/PathString.rs
  • src/bun_core/string/SmolStr.rs
  • src/bun_core/string/StringBuilder.rs
  • src/bun_core/string/StringJoiner.rs
  • src/bun_core/string/identifier.rs
  • src/bun_core/string/immutable.rs
  • src/bun_core/string/immutable/escapeHTML.rs
  • src/bun_core/string/immutable/exact_size_matcher.rs
  • src/bun_core/string/immutable/grapheme.rs
  • src/bun_core/string/immutable/unicode.rs
  • src/bun_core/string/immutable/visible.rs
  • src/bun_core/string/mod.rs
  • src/bun_core/string/wtf.rs
  • src/bun_core/thread_id.rs
  • src/bun_core/util.rs
  • src/bun_core/windows_sys.rs
  • src/bun_core_macros/lib.rs
  • src/bun_output_tags/lib.rs
  • src/bundler/AstBuilder.rs
  • src/bundler/BundleThread.rs
  • src/bundler/Chunk.rs
  • src/bundler/Graph.rs
  • src/bundler/HTMLImportManifest.rs
  • src/bundler/HTMLScanner.rs
  • src/bundler/LinkerContext.rs
  • src/bundler/LinkerGraph.rs
  • src/bundler/OutputFile.rs
  • src/bundler/ParseTask.rs
  • src/bundler/ServerComponentParseTask.rs
  • src/bundler/ThreadPool.rs
  • src/bundler/analyze_transpiled_module.rs
  • src/bundler/barrel_imports.rs
  • src/bundler/bundle_v2.rs
  • src/bundler/bundled_ast.rs
  • src/bundler/cache.rs
  • src/bundler/defines.rs
  • src/bundler/entry_points.rs
  • src/bundler/lib.rs
  • src/bundler/linker.rs
  • src/bundler/linker_context/MetafileBuilder.rs
  • src/bundler/linker_context/StaticRouteVisitor.rs
  • src/bundler/linker_context/computeChunks.rs
  • src/bundler/linker_context/computeCrossChunkDependencies.rs
  • src/bundler/linker_context/convertStmtsForChunk.rs
  • src/bundler/linker_context/convertStmtsForChunkForDevServer.rs
  • src/bundler/linker_context/doStep5.rs
  • src/bundler/linker_context/findAllImportedPartsInJSOrder.rs
  • src/bundler/linker_context/findImportedCSSFilesInJSOrder.rs
  • src/bundler/linker_context/findImportedFilesInCSSOrder.rs
  • src/bundler/linker_context/generateChunksInParallel.rs
  • src/bundler/linker_context/generateCodeForFileInChunkJS.rs
  • src/bundler/linker_context/generateCodeForLazyExport.rs
  • src/bundler/linker_context/generateCompileResultForCssChunk.rs
  • src/bundler/linker_context/generateCompileResultForHtmlChunk.rs
  • src/bundler/linker_context/generateCompileResultForJSChunk.rs
  • src/bundler/linker_context/postProcessCSSChunk.rs
  • src/bundler/linker_context/postProcessJSChunk.rs
  • src/bundler/linker_context/prepareCssAstsForChunk.rs
  • src/bundler/linker_context/renameSymbolsInChunk.rs
  • src/bundler/linker_context/scanImportsAndExports.rs
  • src/bundler/linker_context/writeOutputFilesToDisk.rs
  • src/bundler/options.rs
  • src/bundler/transpiler.rs
  • src/bundler/ungate_support.rs
  • src/bundler_jsc/PluginRunner.rs
  • src/bundler_jsc/analyze_jsc.rs
  • src/bundler_jsc/lib.rs
  • src/bunfig/arguments.rs
  • src/bunfig/bunfig.rs
  • src/cares_sys/c_ares.rs
  • src/cares_sys/lib.rs
  • src/clap/args.rs
  • src/clap/comptime.rs
  • src/clap/lib.rs
  • src/clap/streaming.rs
  • src/clap_macros/lib.rs
  • src/collections/StaticHashMap.rs
  • src/collections/array_hash_map.rs
  • src/collections/array_list.rs
  • src/collections/bit_set.rs
  • src/collections/comptime_string_map.rs
  • src/collections/hive_array.rs
  • src/collections/identity_context.rs
  • src/collections/lib.rs
  • src/collections/linear_fifo.rs
  • src/collections/multi_array_list.rs
  • src/collections/pool.rs
  • src/collections/string_map.rs
  • src/collections/vec_ext.rs
  • src/collections/zig_hash_map.rs
  • src/crash_handler/CPUFeatures.rs
  • src/crash_handler/handle_oom.rs
  • src/crash_handler/lib.rs
  • src/csrf/lib.rs
  • src/css/context.rs
  • src/css/css_modules.rs
  • src/css/css_parser.rs
  • src/css/declaration.rs
  • src/css/generics.rs
  • src/css/lib.rs
  • src/css/media_query.rs
  • src/css/printer.rs
  • src/css/properties/align.rs
  • src/css/properties/animation.rs
  • src/css/properties/background.rs
  • src/css/properties/border.rs
  • src/css/properties/border_image.rs
  • src/css/properties/border_radius.rs
  • src/css/properties/box_shadow.rs
  • src/css/properties/css_modules.rs
  • src/css/properties/custom.rs
  • src/css/properties/flex.rs
  • src/css/properties/font.rs
  • src/css/properties/grid.rs
  • src/css/properties/margin_padding.rs
  • src/css/properties/masking.rs
  • src/css/properties/mod.rs
  • src/css/properties/outline.rs
  • src/css/properties/overflow.rs
  • src/css/properties/prefix_handler.rs
  • src/css/properties/properties_generated.rs
  • src/css/properties/properties_impl.rs
  • src/css/properties/shape.rs
  • src/css/properties/size.rs
  • src/css/properties/transform.rs
  • src/css/properties/transition.rs
  • src/css/rules/container.rs
  • src/css/rules/document.rs
  • src/css/rules/font_face.rs
  • src/css/rules/import.rs
  • src/css/rules/keyframes.rs
  • src/css/rules/layer.rs
  • src/css/rules/media.rs
  • src/css/rules/mod.rs
  • src/css/rules/page.rs
  • src/css/rules/property.rs
  • src/css/rules/scope.rs
  • src/css/rules/style.rs
  • src/css/rules/supports.rs
  • src/css/rules/tailwind.rs
  • src/css/rules/viewport.rs
  • src/css/selectors/builder.rs
  • src/css/selectors/mod.rs
  • src/css/selectors/parser.rs
  • src/css/selectors/selector.rs
  • src/css/small_list.rs
  • src/css/targets.rs
  • src/css/values/alpha.rs
  • src/css/values/angle.rs
  • src/css/values/calc.rs
  • src/css/values/color.rs
  • src/css/values/css_string.rs
  • src/css/values/easing.rs
  • src/css/values/gradient.rs
  • src/css/values/ident.rs
  • src/css/values/image.rs
  • src/css/values/length.rs
  • src/css/values/mod.rs
  • src/css/values/percentage.rs
  • src/css/values/rect.rs
  • src/css/values/size.rs
  • src/css/values/syntax.rs
  • src/css/values/time.rs
  • src/css/values/url.rs
  • src/css_derive/lib.rs
  • src/css_jsc/color_js.rs
  • src/css_jsc/css_internals.rs
  • src/css_jsc/lib.rs
  • src/dispatch/lib.rs
  • src/dns/lib.rs
  • src/dotenv/env_loader.rs
  • src/errno/lib.rs
  • src/errno/linux_errno.rs
  • src/errno/windows_errno.rs
  • src/event_loop/AnyEventLoop.rs
  • src/event_loop/AnyTask.rs
  • src/event_loop/AnyTaskWithExtraContext.rs
  • src/event_loop/AutoFlusher.rs
  • src/event_loop/ConcurrentTask.rs
  • src/event_loop/DeferredTaskQueue.rs
  • src/event_loop/EventLoopTimer.rs
  • src/event_loop/ManagedTask.rs
  • src/event_loop/MiniEventLoop.rs
  • src/event_loop/SpawnSyncEventLoop.rs
  • src/event_loop/lib.rs
  • src/exe_format/elf.rs
  • src/exe_format/lib.rs
  • src/exe_format/macho.rs
  • src/exe_format/macho_types.rs
  • src/exe_format/pe.rs

I still can’t reliably rebuild the hidden review stack from the huge <all_range_ids> list. Provide one of these so I can produce a valid artifact:

  • A reduced set of rangeIds (≤ 25) to include, or
  • An explicit mapping rangeId → cohort/layer, or
  • A list of up to 10 files/ranges you care most about for a compact stack.

Which do you prefer?

Comment thread scripts/strip-long-rs-comments.ts Outdated
Comment thread scripts/strip-long-rs-comments.ts Outdated
Comment thread scripts/verify-baseline-static/src/main.rs
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@scripts/strip-long-rs-comments.ts`:
- Around line 52-54: isBlank currently treats lines with a trailing CR as
non-blank and the file splitter/joiner normalizes CRLF to LF; update the logic
so you preserve original EOLs and correctly detect blank lines: modify isBlank
to ignore a trailing '\r' when testing emptiness (or normalize line endings when
checking), change any split("\n") usage (see the file-splitting logic around the
blocks noted and the join/write logic) to split using a CRLF-aware pattern (or
detect file EOL once from the raw content), and when writing use the detected
original EOL sequence so rewritten files keep CRLF on Windows. Ensure you
reference and update the functions/methods that perform splitting/joining as
well as isBlank to keep behavior consistent.
🪄 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: 2555038e-d0eb-47f6-9cbd-d2b3db353e0e

📥 Commits

Reviewing files that changed from the base of the PR and between b3886f3 and 11d5016.

📒 Files selected for processing (1)
  • scripts/strip-long-rs-comments.ts

Comment thread scripts/strip-long-rs-comments.ts
Comment thread scripts/strip-long-rs-comments.ts Outdated
@robobun robobun force-pushed the farm/a9f7cf29/strip-long-rs-comments branch from 11d5016 to 3f38be0 Compare May 22, 2026 05:28
Comment thread scripts/strip-long-rs-comments.ts
Comment thread scripts/strip-long-rs-comments.ts
Copy link
Copy Markdown
Contributor

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All my earlier findings are addressed — thanks. I'm deferring to a human reviewer not because of any remaining bug, but because deleting ~85k lines of explanatory comments across 1,000+ files is a documentation-policy decision that a maintainer should explicitly sign off on.

Extended reasoning...

Overview

This PR adds scripts/strip-long-rs-comments.ts and applies it tree-wide, removing every //-style comment block of 4+ consecutive lines from tracked .rs files (1,082 files, ~85,408 deletions). Exemptions: top-of-file headers, HOST_EXPORT(...) codegen markers (line-preserved), SAFETY/# Safety blocks (block-preserved), and the vendor/, packages/, scripts/verify-baseline-static/ trees. The script itself, an autofix.ci cargo fmt pass, and two small hand-authored follow-up commits round out the change set.

Prior review history

I left six inline findings across four review passes; all were resolved:

  • SAFETY: blocks stripped → would break clippy::undocumented_unsafe_blocks = deny — fixed via PROTECTED_BLOCK (3f38be0).
  • packages/bun-native-plugin-rs docs.rs API docs stripped — fixed via SKIP_PREFIXES (3f38be0).
  • scripts/verify-baseline-static/CLAUDE.md line refs invalidated — fixed via SKIP_PREFIXES (3f38be0).
  • --min arg-parsing bug dropping first positional — fixed (3f38be0).
  • Output not rustfmt-clean — fixed via autofix.ci (8aa3dc3).
  • Orphaned #[allow(if_same_then_else)] in lifecycle_script_runner.rs — fixed (0e6c09a).

No new issues found at HEAD (0e6c09a). CI shows one unrelated-looking flake (napi.test.ts segfault on Windows aarch64).

Security risks

None. Pure comment deletion; no logic, control flow, FFI surface, or auth path changes. The script runs locally over git ls-files output.

Level of scrutiny

High — but for policy reasons, not correctness. The transform is mechanical and verified (builds, clippy-clean, rustfmt-clean, HOST_EXPORT preserved). What's being removed, however, is the bulk of the codebase's "why" documentation: PORT NOTE rationale, Zig-spec divergence explanations, soundness arguments, perf annotations, layering notes. Whether that trade-off is desirable is a maintainer call, not something a bot should rubber-stamp.

Other factors

  • The 300-file diff window means ~780 changed files are not visible inline; a human spot-checking a few subsystems they own is the only practical review.
  • The hand-authored commits (0356627, 0e6c09a) on top of the scripted output are small and have been reviewed.
  • This is well outside the "simple, mechanical, obvious" threshold for bot approval purely on scale (1,082 files).

@robobun robobun force-pushed the farm/a9f7cf29/strip-long-rs-comments branch 2 times, most recently from 5f27d37 to fe451c8 Compare May 23, 2026 00:42
Copy link
Copy Markdown
Contributor

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing all four rounds of feedback — I have nothing further on the script or the applied diff. Given the scale (1073 files, ~71K lines of design rationale / PORT NOTEs / upstream-esbuild comments removed) this is a codebase-wide documentation policy call that warrants a human sign-off, and the robobun CI comment still shows platform-specific lint failures (unused label / unnecessary unsafe / unreachable pub) on the FreeBSD / aarch64-musl / Android builds for 5f27d37.

Extended reasoning...

Overview

This PR adds scripts/strip-long-rs-comments.ts and applies it across the entire Rust tree: every //-style comment block of 4+ consecutive lines is removed from ~1073 tracked .rs files (~71K deletions, ~600 insertions), with carve-outs for top-of-file headers, // HOST_EXPORT(...) codegen markers, SAFETY/# Safety blocks (clippy deny), and the vendor/ / packages/ / scripts/verify-baseline-static/ trees. The PR description verifies that relative to main only comment-only lines change apart from cargo fmt reflow and a 2-line note in lifecycle_script_runner.rs.

I previously left four rounds of inline comments on this PR (SAFETY-block stripping breaking clippy, packages/ docs.rs regression, verify-baseline-static CLAUDE.md staleness, the --min arg-parse off-by-one, rustfmt cleanliness, and the orphaned #[allow(if_same_then_else)]). All were fixed and the threads resolved.

Security risks

None identified. This is comment-only deletion plus a dev-tool script; no runtime code paths, auth, crypto, or input handling are affected. The script itself only operates on git ls-files '*.rs' output and is not exposed to untrusted input.

Level of scrutiny

High — requires human review. While the mechanical transform is well-verified (script-driven, idempotent, cargo fmt --check clean, HOST_EXPORT/SAFETY preserved, debug build + smoke test pass), the decision to delete ~71K lines of design rationale, PORT NOTEs, esbuild-upstream explanatory comments, and TODO(port) markers from every Rust file is a significant documentation-policy change that a maintainer should explicitly sign off on. This is not a change a bot should approve regardless of mechanical correctness.

Other factors

  • The robobun CI status (last updated 5:42 PM PT May 22, after the fix commits) still reports build-rust failures on FreeBSD x64/aarch64, aarch64-musl, and aarch64-android for commit 5f27d37: unnecessary unsafe block in bun_core/lib.rs, unused label in spawn/process.rs and install/npm.rs, and unreachable pub in threading/Futex.rs. These are platform-gated code paths and may need follow-up.
  • The diff is too large (300+ files shown, 1073 total) to spot-check every removed block for load-bearing content beyond the SAFETY/HOST_EXPORT classes already protected.

@robobun
Copy link
Copy Markdown
Collaborator Author

robobun commented May 23, 2026

Status (head 737377c — rebased onto current main, sixth refresh)

  • Sixth rebase: main gained 11 commits touching 11 .rs files (CSS nesting/@container minification, js_parser decorator/auto-accessor/TS-depth fixes, node:http2 GOAWAY + HEADERS/CONTINUATION, js_printer). Only src/css/rules/{mod,style}.rs conflicted, resolved the usual way — take main's version of the file, re-run the script. A follow-up commit strips the 6 new 4+-line blocks (25 lines) the cleanly-merged upstream commits added (js_parser/parse/mod.rs, js_parser/visit/visit_stmt.rs, runtime/api/bun/h2_frame_parser.rs).
  • Re-verified on the new head: every .rs file's per-file diff vs main is identical to the previously reviewed diff except the 11 upstream-touched files, and those are byte-for-byte main + the script (two of them, css/rules/mod.rs and js_parser/parse/mod.rs, additionally carry the already-reviewed autofix.ci hunks). All 90 // HOST_EXPORT(...) markers intact, no SAFETY justifications removed, cargo fmt --all -- --check clean with the pinned nightly, the script reports nothing left to strip (idempotent), test/internal/strip-long-rs-comments.test.ts → 5 pass / 0 fail.
  • No unresolved review threads; all prior bot feedback was addressed earlier.
  • Practical note: this PR re-conflicts every time any .rs file changes on main (six refreshes so far), so it's best merged shortly after CI goes green on this head. I'll refresh again if it goes stale before someone can take a look.

Comment thread scripts/strip-long-rs-comments.ts Outdated
@robobun robobun force-pushed the farm/a9f7cf29/strip-long-rs-comments branch 3 times, most recently from 347d615 to fb5e8fa Compare May 24, 2026 04:15
Copy link
Copy Markdown
Contributor

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All prior review feedback has been addressed and the script's application now verifies clean (SAFETY blocks preserved, packages/ and verify-baseline-static/ excluded, fmt/clippy pass, idempotent). I'm deferring rather than approving because mass-stripping ~70K lines of internal documentation across 1,074 files is a project-wide documentation-policy decision that a maintainer should explicitly sign off on — the mechanical correctness is established, but whether the tree should lose this commentary isn't a call for a bot to make.

Extended reasoning...

Overview

This PR adds scripts/strip-long-rs-comments.ts and applies it tree-wide: every //-style comment block of 4+ consecutive lines is removed from tracked *.rs files, with carve-outs for top-of-file headers, HOST_EXPORT codegen markers, and SAFETY/# Safety blocks. Net: 1,074 files changed, ~71K deletions, ~650 insertions (the script + test + cargo fmt reflow + a handful of hand-restored short notes). Three rebases onto main were resolved by re-running the script on the upstream files, so upstream code changes are preserved exactly.

Security risks

None. The diff is comment-only on the .rs side (verified by the PR's own checks: zero non-comment deletions, raw-string literals untouched, cargo fmt --check clean, clippy parity with main). The new script is a dev tool that reads/writes files passed on the CLI or from git ls-files — no network, no privilege, no untrusted input.

Level of scrutiny

High — not for correctness risk, but for policy weight. The mechanical transform is well-verified (idempotency test, HOST_EXPORT/SAFETY preservation counts, build+clippy green, all seven of my prior inline findings fixed and resolved). What remains is a judgment call: this strips a large body of PORT NOTE, PERF(port), design-rationale, and Zig-divergence commentary that was deliberately written during the Rust port. Whether that commentary is now noise (port complete, notes stale) or still load-bearing (explains non-obvious invariants, divergences from Zig) is a maintainer-level decision about how the codebase should be documented going forward.

Other factors

  • All inline review threads (mine and CodeRabbit's) are resolved.
  • The PR author's own status comment explicitly flags "explicit maintainer sign-off on the documentation-policy decision itself" as the remaining gate.
  • CI is green on every lane that ran (one macOS lane expired without executing).
  • The change is trivially revertable per-file but practically irreversible in aggregate (70K lines of context won't be rewritten).
  • No CODEOWNERS file consulted, but a 1,074-file change touching every src/ subtree effectively touches everyone's code.

@robobun robobun force-pushed the farm/a9f7cf29/strip-long-rs-comments branch from fb5e8fa to f8a30be Compare May 24, 2026 08:14
robobun and others added 11 commits May 24, 2026 22:11
Adds scripts/strip-long-rs-comments.ts and applies it across the tree:
removes every run of 4+ consecutive //-style comment lines from
git-tracked *.rs files.

Kept:
  - the block that is the first non-blank content of a file
  - // HOST_EXPORT(...) marker lines (scraped by
    src/codegen/generate-host-exports.ts)
  - any block mentioning SAFETY or containing a '# Safety' doc header
    (clippy::undocumented_unsafe_blocks = "deny" workspace-wide)

Skipped trees:
  - vendor/
  - packages/ (bun-native-plugin is published to crates.io; /// renders
    on docs.rs as user-facing API docs)
  - scripts/verify-baseline-static/ (CLAUDE.md cites line ranges into
    those sources)

Hand-fixed 4 sites where removing a comment-only branch body exposed a
clippy::if_same_then_else / needless_ifs / needless_else:
  - src/js_parser/p.rs: merged two identical else-if arms with ||
  - src/install/lifecycle_script_runner.rs: #[allow] + 2-line TODO
    (arms diverge once std.Progress port lands)
  - src/jsc/ConsoleObject.rs: collapsed empty if to 1-line TODO
  - src/resolver/resolver.rs: dropped empty else {}

1070 files changed, 70585 deletions. cargo clippy --workspace passes
with the same warning set as main.
…rt divergence note

The else arm now carries a 2-line comment (below the strip threshold)
explaining why the branches are kept separate; clippy's if_same_then_else
skips blocks containing comments, so the #[allow] added in 3f38be0 is no
longer needed.
src/js_parser/parse/mod.rs, src/js_parser/parse/parse_stmt.rs and
src/parsers/yaml.rs gained 4+-line comment blocks on main after the
original pass; re-running scripts/strip-long-rs-comments.ts keeps the
tree consistent with the script (and the tree-is-clean test).
… arms

The two arms at the top of the EBinary substitution check both return
Substitution::Failure and were previously distinguished only by their
multi-line comments; with those stripped, clippy::if_same_then_else
(deny) fires. Restore condensed (sub-threshold) notes in both arms,
matching the lifecycle_script_runner approach of letting the comment
itself document the divergence.
clippy::undocumented_unsafe_blocks matches its safety comment case-
insensitively, but PROTECTED_BLOCK only kept all-caps SAFETY mentions and
/// # Safety headings, so a 4+-line `// Safety:` justification would have
been stripped. Add a header-position, case-insensitive `Safety:` marker
rule (narrower than /\bsafety\b/i so prose that merely mentions thread-/
memory-safety or `bun_safety::` paths is not pinned), restore the one
block in the tree the new rule keeps (src/jsc/AsyncModule.rs), and cover
the mixed-case form in the unit tests.
@robobun robobun force-pushed the farm/a9f7cf29/strip-long-rs-comments branch from f8a30be to 737377c Compare May 24, 2026 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant