Conversation
|
Warning Review limit reachedYour included review limit has been reached. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 31 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset (next review available in 45 seconds), then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the two flags are threaded correctly and gated so flags-off behavior is unchanged. The inline note about underline is informational; it's a heavier change and reasonable to defer.
Extended reasoning...
Overview
Implements two documented-but-no-op Bun.markdown options: hardSoftBreaks (adds a Flags field, threads it through to_flags(), and OR's it into the existing hard-break condition in the newline handler) and collapseWhitespace (adds a whitespace-run branch to process_inline_content and a trailing-whitespace strip before soft breaks). Five files touched: three in src/md/, two test files.
Security risks
None. Pure markdown-rendering logic on already-merged inline content; no new syscalls, allocations, or FFI. All new slice accesses are bounds-checked (end < content.len(), sp > text_start, i > text_start).
Level of scrutiny
Medium — parser hot path, but the change is small and additive. I verified via build_mark_char_map (parser.rs:351-355) that space/tab/CR are only mark chars when collapse_whitespace is set, so with the flag off the new branch is unreachable via the fast-path skip and the CommonMark spec suite is unaffected. Traced the whitespace-run → newline handoff for "a \n" (still a hard break), "a \n" (collapses before soft break), "a\t\n" with hardSoftBreaks (tab stripped, hard break), and single-space (no-op, end - i == 1 && c == ' '). Confirmed the option parser accepts snake_case so md-spec.test.ts's --fhard-soft-breaks → hard_soft_breaks mapping works, and that the spec-hard-soft-breaks.txt fixture exists.
Other factors
Tests are exact-value .toBe() assertions covering both flag states, code-span/code-block preservation, and the hard-break interaction; the md4c spec file is now enabled. The one finding (underline is a third parsed-but-dropped option) is a nit — it needs emph_char threaded into emit_emph_open_tags/close_tags, which is a separate feature implementation, not a mechanical sibling of this fix.
…ions Both options were parsed from the JS options object but never reached the inline processor, so they were silently ignored. - hard_soft_breaks now threads through Flags and forces every soft break to a hard break, dropping trailing whitespace before it, matching md4c's MD_FLAG_HARD_SOFT_BREAKS. - collapse_whitespace now collapses non-trivial whitespace runs (longer than one char, or containing a tab/CR) in normal text into a single space, matching md4c's MD_FLAG_COLLAPSEWHITESPACE. Runs immediately before a newline are left to the existing hard-break detection. Enables md4c's spec-hard-soft-breaks.txt in the spec test suite.
0469333 to
d2d159c
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — both flags are correctly threaded through and gated so default behavior is unchanged.
What was reviewed:
- Traced the new whitespace-collapse branch in
process_inline_content: space/tab/CR are only inmark_char_mapwhencollapse_whitespaceis set (parser.rs:339-343), so the branch is unreachable with the flag off; the run-before-\ndeferral preserves the two-space hard-break rule. - Verified
hard_soft_breaksreuses the existing trailing-space stripping, andspec-hard-soft-breaks.txtmaps via--fhard-soft-breaks→hard_soft_breaksinBOOL_FIELD_SETTERS. - Prior feedback (the
underline/latexMathsibling exclusion, comment-cop length) is addressed and resolved.
Extended reasoning...
Overview
This PR wires up two documented-but-no-op Bun.markdown options: hardSoftBreaks (adds a hard_soft_breaks field to Flags, threads it through Options::to_flags(), and ORs it into the existing hard-break condition in the newline branch of process_inline_content) and collapseWhitespace (adds a whitespace-run branch to process_inline_content that emits a single space for non-trivial runs, plus a trailing-whitespace strip before soft breaks). ~37 net production lines across three files, plus 63 lines of new tests and one line enabling md4c's checked-in spec-hard-soft-breaks.txt.
Security risks
None. This is a pure text-transformation flag in the markdown inline parser — no allocation, no FFI, no user-controlled sizes, no external data. The new branches only read and re-emit slices of the already-bounded merged-line buffer.
Level of scrutiny
Low-to-medium. Both new code paths are gated on flags that default to false, and build_mark_char_map (parser.rs:339-343) only marks space/tab/CR when collapse_whitespace is set — so with defaults the new collapse branch is provably unreachable and the || self.flags.hard_soft_breaks clause is a no-op. The full CommonMark spec suite (1082 tests) still passes, confirming no behavior change with the flags off. The flag-on paths are covered by exact-output assertions across the variant matrix (single space vs run, tab, inside emphasis/link labels, code spans/blocks untouched, two-trailing-spaces still <br>, disabled-by-default) plus md4c's own spec fixture.
Other factors
I traced the trickier interactions by hand: (1) a whitespace run immediately before \n is left for the newline branch (i = end; continue; without touching text_start), so "a \n" under collapseWhitespace still hits emit_end - sp >= 2 and produces <br>; (2) a single plain space skips the emit (end - i > 1 || c != b' ' is false) and stays part of the enclosing Normal text run; (3) spec-hard-soft-breaks.txt uses --fhard-soft-breaks, which renderMarkdown maps to snake_case hard_soft_breaks, matching BOOL_FIELD_SETTERS. All prior review threads are resolved: my earlier note on underline being a sibling instance was addressed by an intentional-exclusion note in the description linking #39493, and the comment-cop length complaints were fixed in e633315. No CODEOWNERS on src/md/.
|
Updated 8:44 PM PT - Aug 20th, 2026
✅ @robobun, your commit e63331534bf334f0a608f96c2e9a9007055be8ae passed in 🧪 To try this PR locally: bunx bun-pr 39495That installs a local version of the PR into your bun-39495 --bun |
#43745) ### Problem - 16 struct fields in 11 crates are never read. Two options are never set: `BufferWriter.append_null_byte` is never `true`, `TransposeState.import_record_tag` is never `Some`. - No lint reports them. rustc's `dead_code` pass counts `x.f = v` as a use of `f`. hawk counts the initializer as a reference. ### Fix - Delete each field with its initializers and stores, and the two branches that the options guard. 18 files, 105 lines removed. - Candidates come from a compiler probe: `#[deprecated]` on 12,134 struct fields, `cargo check --force-warn deprecated` for Linux, Windows, and macOS, then a syn pass marks each use as read or write. 134 fields have no read. Most stay (Notes). - Verified: `bun bd`, `bun run rust:check-all` (12 of 12 targets), `cargo check --workspace --all-targets`, and the tests in the Notes. - Self-reviewed: 4 concerns raised, 2 addressed. Rejected as outside a deletion: the now vestigial `written_without_trailing_zero()` and an older stale comment. ### Background - `MultiArrayList<T>` keeps each field of `T` in its own column. Code reads a column by name as a string (`items::<"name", _>()`), so the compiler sees no read. Those fields stay. - `BufferWriter` is the JS printer's output buffer. `done()` could append a NUL byte. No caller asks for it. ### Downsides - `bun_core::output::Source` becomes `Send + Sync`, because its raw pointer fields are gone. It lives in a `thread_local!` only. - The `bun_resolver::Result` in `_resolve` now drops when `_resolve` returns. It has no `Drop` effect: paths are borrowed, fds are `Copy`. <details><summary>Notes</summary> #### Removed, one line each - `bun_core::output::Source`: `buffered_stream`, `buffered_error_stream`, `stream`, `error_stream` (`*mut io::Writer`). `init()` cached them, nothing read them. The accessors of the same names return `*_backing.new_interface()`, which is a pointer cast with no side effect. - `bun_bundler`: `InputFileInfo.import_count` (`MetafileBuilder.rs`). - `bun_install`: `SecurityScanSubprocess.stderr_data` (an empty `Vec` that is never filled, the scanner's stderr is inherited), `PackageManager.root_progress_node`. The `progress.start(b"", 0)` call stays, because it starts the progress root. - `bun_js_parser`: `PropertyOpts.async_range`, `ScanPassResult.approximate_newline_count`, `TransposeState.import_record_tag`. #16624 replaced the only setter of the tag with `import_loader`, which stays. - `bun_jsc`: `VirtualMachine.has_terminated` (its readers were debug panics in `enqueue_task_concurrent`, which #37075 removed), `ResolveFunctionResult.result` (seven stores, no read). Nothing borrows from the stored value: `path` and `query_string` point into the resolver's arena and the specifier. - `bun_js_printer`: `BufferWriter.append_null_byte`, with the seven `= false` stores in `bun_jsc` and `bun_runtime`. - `bun_libarchive`: `BufferReadStream.reading`. - `bun_runtime`: `WindowsState.is_server` (`ipc.rs`, Windows only). Its last reader went away in #18688, before the Rust port. - `bun_csrf`: `GenerateOptions.encoding`. `csrf_jsc.rs` encodes the token itself, and `VerifyOptions.encoding` stays. - `bun_resolver`: `DataURL.url` (always `String::EMPTY`). - `bun_s3_signing`: `S3CredentialsWithOptions.virtual_hosted_style`. No code names it. Every reader uses `credentials.virtual_hosted_style` on the inner `S3Credentials`. #### Passed the probe, kept on purpose - `MultiArrayList` columns: fields of `JSMeta`, `File`, `InputFile`, `BundledAst`, `Entry`, `Node`, `WatchItem`, `LineOffsetTable`, `ServerComponentBoundary`, and others. - Owners of memory that other fields borrow: `ParseResult.source_contents_backing`, `LinkerContext.unique_key_buf`, `OutputFile.owned_src_path_text`, `HTTPResponseMetadata.owned_buf`, `PackageJSON.source_contents` and `json_tape`, `MatchedRoute.pathname_backing`, `SignResult.content_md5`, `KEventWaker.machport_buf`. - Values whose drop has an effect: `Repl.last_error` (a GC protect), `SecurityScanSubprocess.process`, `Watcher.thread`. - Options that are unfinished features or open bugs, not dead code: `md::Options.hard_soft_breaks` and `underline` (#39495, #39493), `jsc::virtual_machine::Options.dns_result_order` (#40703), `P.has_top_level_return` (#40840), `ReactRefresh.last_hook_seen` and `force_reset`, `DebugOptions.dump_environment_variables` (the `--dump-environment-variables` flag is parsed and ignored). - Kept by a comment in the source: `AllocatorConfiguration.long_running`, `DumpStackTraceOptions.skip_*`, `NameOfSymbol.has_property_key_comment`. - Removal needs more than a deletion: `PostgresSQLQuery` `Flags.is_done` (set by the JS `done()` call), `WorkerPipe.done` (leaves two empty reader callbacks), `WTFTimer.repeat` (leaves an unused FFI parameter), `MaxHeapAllocator.len` (leaves an empty `reset()`), `ReadToEndResult.err` (callers drop read errors, which looks like a bug), `Subcommand::Pack` with `PACK_PARAMS` (`bun pm pack` runs as `Subcommand::Pm`, so the pack help text is unreachable). #### Self-review Three independent read-only passes tried to prove each deletion wrong (readers through raw pointers, `offset_of!`, C++ layout mirrors, macros, other `cfg`s, unit tests, the Zig originals, drop effects). None found a reader. Concerns raised: the `Send + Sync` change of `Source` (now in Downsides), the style of the kept `progress.start` call (now `let _ =`, as in `install_with_manager.rs`), `BufferWriter::written_without_trailing_zero()` (nine callers, it only strips NUL bytes that the printer never appends now, a follow-up), and a comment above `ResolveFunctionResult.path` that names a function which no longer exists (it predates this change). #### Other scans of this run, all clean or already in an open pull request - A relink of the debug binary with `--gc-sections --print-gc-sections`, and a zero-mention identifier index over `src/`, `packages/`, `scripts/` and `build/debug/codegen/`. - `macro_rules!` without an invocation, Cargo features that nothing enables, `#if 0` and never-true `#if` blocks in the C++ bindings, patches that no dependency script applies, unused exports under `scripts/`, `tsc --noUnusedLocals` over `src/js` and `scripts/`. - A syn scan for enum variants that no expression constructs. The hits are FFI code tables, derive-constructed variants, or already claimed. #### Overlap with the open dead-code pull requests - Every removed line was checked per file against the diffs of the 36 open dead-code pull requests. None of them deletes these lines. Several touch the same files in other places (`output.rs`, `VirtualMachine.rs`, `parser.rs`, `p.rs`, `js_printer/lib.rs`, `jsc_hooks.rs`, `PackageManager.rs`, `data_url.rs`). #### Tests run with the debug build `test/js/bun/util/csrf.test.ts` (31 pass), `test/bundler/metafile.test.ts` (65 pass), `test/js/bun/archive.test.ts` (108 pass), `test/bundler/transpiler/transpiler.test.js` (222 pass), `test/js/bun/resolve/import-empty.test.js`, `test/js/bun/resolve/esModule.test.ts`, `test/cli/install/bun-install-security-provider.test.ts` (43 pass), `test/internal/source-lints/` (195 pass). Also a `data:` URL import, and `bun install` under a pty so that the progress root starts. No test is added. The change deletes fields that nothing reads, so there is no behavior to assert, and `test/internal/source-lints/CLAUDE.md` asks for no tests that pin dead symbols. </details>
This branch has not been deployed
1 participant
Fixes #39491
Problem
Bun.markdown.html("a\nb\n", { hardSoftBreaks: true })returns"<p>a\nb</p>\n"andBun.markdown.html("a b\n", { collapseWhitespace: true })returns"<p>a b</p>\n": both options are documented (docs/runtime/markdown.mdx,Bun.markdown.Options) but do nothing.hard_soft_breaksis parsed intoOptions(src/md/root.rs) but dropped byOptions::to_flags();Flags(src/md/types.rs) had no such field.collapse_whitespacereachesFlagsand marks space/tab/CR in the mark char map (src/md/parser.rs:351), butprocess_inline_content(src/md/inlines.rs) had no branch for those characters, so they fell through as literal text.Fix
hard_soft_breakstoFlagsand threads it throughto_flags(). The newline handler inprocess_inline_contentnow forces a hard break when the flag is set, dropping trailing whitespace before the break, matching md4c'sMD_FLAG_HARD_SOFT_BREAKS(first call above now returns"<p>a<br />\nb</p>\n").process_inline_content: whencollapse_whitespaceis set, a non-trivial run (longer than one character, or not a plain space) in normal text is emitted as a single space, matching md4c'sMD_FLAG_COLLAPSEWHITESPACE(second call above now returns"<p>a b</p>\n"). A run immediately before a newline is left for the existing hard-break detection, so"a \n"still produces<br />. Code spans and code blocks are untouched (they never go through this path).spec-hard-soft-breaks.txtis now enabled in test/js/bun/md/md-spec.test.ts and passes; all 1082 tests in test/js/bun/md/ pass (including the full CommonMark spec suite, so no behavior change with the flags off).underlineandlatexMathare the same bug class but are span-type options handled separately in Bun.markdown: implement the latexMath and underline options #39493, so they are not touched here.Background
Bun.markdownis a Rust port of md4c. Block structure is parsed first; each leaf block's lines are merged into one buffer and walked byprocess_inline_content, which dispatches on a per-parser "mark char map" (a 256-bit set of characters that need special handling, built once from the flags).MD_TEXT_SOFTBR) render as\nand hard breaks (MD_TEXT_BR) as<br />\n.MD_FLAG_HARD_SOFT_BREAKSturns every soft break into a hard break;MD_FLAG_COLLAPSEWHITESPACEreplaces each whitespace run that is more than a single space with one space.[review] gate passed · iteration 0 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file
root cause · written by the author bot
The hardSoftBreaks and collapseWhitespace options were parsed from the JavaScript options object but never took effect, because hard_soft_breaks was dropped in Options::to_flags and the inline processing loop had no handling for either behavior, so soft breaks always became Softbr and whitespace runs passed through as plain text. The fix threads both flags through to the parser, makes the newline handling emit a hard break and strip preceding trailing whitespace when hard_soft_breaks is set, and adds a branch that collapses non-trivial whitespace runs into a single space when collapse_white…