refactor(md/parse): two-phase parse - #11256
Conversation
|
github-actions
Bot
added
A-Parser
✅ Organic activityNo automation signals detected in the analyzed events. This is an automated analysis by AgentScan |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughMarkdown parsing now records paragraph and ATX heading inline regions for deferred processing. Link-reference definitions use source ranges and checkpoint restoration. The lexer and token source support bounded ranges. A new inline phase reparses deferred regions after definition collection, then replaces events and merges trivia and diagnostics. Tests cover headings, nested blockquote lists, emphasis delimiters, CST round-tripping, and diagnostic ordering. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
crates/biome_parser/src/diagnostic.rs (1)
159-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new public accessor and retire the duplicate.
span()is a new public API without rustdoc.diagnostic_range()at line 373 already returns the same field asOption<&TextRange>for crate-internal use.TextRangeisCopy, sospan()covers both needs andmerge_diagnosticscan call it.As per coding guidelines: "Use rustdoc documentation for documenting new features".
♻️ Proposed change
+ /// Returns the source range the diagnostic points at, if any. pub fn span(&self) -> Option<TextRange> { self.span }🤖 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 `@crates/biome_parser/src/diagnostic.rs` around lines 159 - 161, Add rustdoc documentation to the public Diagnostic::span accessor, remove the duplicate diagnostic_range method, and update merge_diagnostics to use span() instead.Source: Coding guidelines
crates/biome_markdown_parser/src/lexer/mod.rs (2)
256-274: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
checked_subfor the byte beforestart.
usize::from(start) - 1is safe today only because||short-circuits whenstart == 0. A later reordering of the two arms would introduce an underflow panic.checked_subremoves the dependency on evaluation order.♻️ Proposed tweak
- after_newline: start == TextSize::from(0) - || matches!( - source.as_bytes().get(usize::from(start) - 1), - Some(b'\n' | b'\r') - ), + after_newline: match usize::from(start).checked_sub(1) { + None => true, + Some(previous) => { + matches!(source.as_bytes().get(previous), Some(b'\n' | b'\r')) + } + },🤖 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 `@crates/biome_markdown_parser/src/lexer/mod.rs` around lines 256 - 274, Update from_valid_range to compute the preceding byte index with checked_sub before calling source.as_bytes().get, preserving the existing after_newline behavior for start at zero and positions preceded by newline characters without relying on short-circuit evaluation order.
248-254: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a lexer test with a bounded range.
The only caller,
parse_inline_fragment, always passessource.len()as the range end. Every newself.endbound is therefore exercised withend == source.len(), which is the old behaviour. The bounds inis_eof,byte_at,is_ordered_list_marker_at, andis_at_trailing_hash_closing_whitespacehave no coverage for a genuinely truncated range, and a regression there would be silent.Please add a lexer test that builds a lexer over a strict sub-range and asserts EOF at the range end.
crates/biome_markdown_parser/src/lexer/tests.rsalready has theassert_lex!scaffolding.As per coding guidelines: "All code changes must include appropriate tests; parser changes must cover valid and error cases".
🤖 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 `@crates/biome_markdown_parser/src/lexer/mod.rs` around lines 248 - 254, Add a lexer test in the existing tests module using the assert_lex! scaffolding that constructs a lexer with a range ending before source.len(). Assert that the lexer reports EOF at the bounded range end, exercising the from_range path and the self.end checks in is_eof and related byte-bound methods without changing production behavior.Source: Coding guidelines
crates/biome_markdown_parser/src/lib.rs (1)
45-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis assertion cannot fail, and it hides the real case.
parse_deferred_inlinescallsoutput.deferred_inlines.clear()unconditionally before it returns, so thisdebug_assert!always holds. It does not verify that the records were resolved.The case worth detecting is different:
parse_deferred_inlineshas threecontinuepaths that skip a record, andparse_inline_fragmentcan returnNone. Those records are then dropped with no signal, and the first-pass events stay in the tree. Please either assert on a counter of unresolved records, or drop the assertion so it does not read as a guarantee.🤖 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 `@crates/biome_markdown_parser/src/lib.rs` around lines 45 - 49, Replace the vacuous debug_assert! on output.deferred_inlines in the parser flow with validation that detects records skipped by parse_deferred_inlines, including continue paths and parse_inline_fragment returning None; alternatively remove the assertion entirely so it does not imply successful resolution. Anchor the change to parse_deferred_inlines and preserve the existing parsing behavior.crates/biome_markdown_parser/src/inline_phase.rs (1)
107-112: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMerge the trivia lists instead of sorting the whole document.
reparsed_rangesis ordered,output.triviais in source order, andinline_triviais produced in source order per fragment. A fullsort_by_keyover every trivia piece in the document is O(n log n) on each parse that reparses at least one fragment. A merge is O(n) and matches what the diagnostics path already does withmerge_diagnostics.For a performance-focused PR this is worth aligning.
🤖 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 `@crates/biome_markdown_parser/src/inline_phase.rs` around lines 107 - 112, The trivia handling after overlaps_ordered_ranges should merge the existing output.trivia and newly produced inline_trivia in source order instead of sorting the entire document with Trivia::offset. Reuse or mirror the linear merge approach used by merge_diagnostics, preserving ordering and avoiding the O(n log n) sort.
🤖 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 `@crates/biome_markdown_parser/src/inline_phase.rs`:
- Around line 148-160: In the fragment splice flow around parser.cur_range(),
retain the existing debug_assert_eq! but add a release-safe check for the
end-offset mismatch before calling wrapper.complete or constructing output.
Return None when parser.cur_range().start() differs from
deferred.source_range.end(), preserving the first-pass events instead of
splicing an invalid event stream.
In `@crates/biome_markdown_parser/src/parser.rs`:
- Around line 808-838: Update MarkdownParser::rewind to return immediately when
link_reference_definitions_len already matches the current definition count, and
change link-reference definition storage so each range retains its normalized
hash alongside it. Rebuild ranges_by_hash during truncation by reusing the
stored hashes instead of calling normalized_label_hash or re-normalizing labels,
while preserving existing truncation behavior.
---
Nitpick comments:
In `@crates/biome_markdown_parser/src/inline_phase.rs`:
- Around line 107-112: The trivia handling after overlaps_ordered_ranges should
merge the existing output.trivia and newly produced inline_trivia in source
order instead of sorting the entire document with Trivia::offset. Reuse or
mirror the linear merge approach used by merge_diagnostics, preserving ordering
and avoiding the O(n log n) sort.
In `@crates/biome_markdown_parser/src/lexer/mod.rs`:
- Around line 256-274: Update from_valid_range to compute the preceding byte
index with checked_sub before calling source.as_bytes().get, preserving the
existing after_newline behavior for start at zero and positions preceded by
newline characters without relying on short-circuit evaluation order.
- Around line 248-254: Add a lexer test in the existing tests module using the
assert_lex! scaffolding that constructs a lexer with a range ending before
source.len(). Assert that the lexer reports EOF at the bounded range end,
exercising the from_range path and the self.end checks in is_eof and related
byte-bound methods without changing production behavior.
In `@crates/biome_markdown_parser/src/lib.rs`:
- Around line 45-49: Replace the vacuous debug_assert! on
output.deferred_inlines in the parser flow with validation that detects records
skipped by parse_deferred_inlines, including continue paths and
parse_inline_fragment returning None; alternatively remove the assertion
entirely so it does not imply successful resolution. Anchor the change to
parse_deferred_inlines and preserve the existing parsing behavior.
In `@crates/biome_parser/src/diagnostic.rs`:
- Around line 159-161: Add rustdoc documentation to the public Diagnostic::span
accessor, remove the duplicate diagnostic_range method, and update
merge_diagnostics to use span() instead.
🪄 Autofix
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: CHILL
Plan: Pro
Run ID: 5bcef1dc-1cca-45b1-b19b-902e1caddd04
⛔ Files ignored due to path filters (1)
crates/biome_markdown_formatter/tests/specs/markdown/reference_link.md.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (13)
crates/biome_markdown_formatter/tests/specs/markdown/reference_link.mdcrates/biome_markdown_parser/src/inline_phase.rscrates/biome_markdown_parser/src/lexer/mod.rscrates/biome_markdown_parser/src/lib.rscrates/biome_markdown_parser/src/link_reference.rscrates/biome_markdown_parser/src/parser.rscrates/biome_markdown_parser/src/syntax/header.rscrates/biome_markdown_parser/src/syntax/link_block.rscrates/biome_markdown_parser/src/syntax/mod.rscrates/biome_markdown_parser/src/to_html.rscrates/biome_markdown_parser/src/token_source.rscrates/biome_markdown_parser/tests/cst_invariants.rscrates/biome_parser/src/diagnostic.rs
💤 Files with no reviewable changes (1)
- crates/biome_markdown_parser/src/link_reference.rs
Merging this PR will improve performance by 66.42%
Performance Changes
Tip Curious why this is faster? Comment Comparing 
Footnotes
|
Parser conformance results onjs/262
jsx/babel
markdown/commonmark
symbols/microsoft
ts/babel
ts/microsoft
|
2 participants
Summary
This PR should increase the speed of our markdown parser.
The previous version was parsing the same document twice. This is correct because that's how CommonMark suggests implementing the MD parser; however, our implementation was using a full-fledged parsing phase only to retrieve the link references.
This PR refactors the markdown parsing into exactly two phases:
Hopefully this should make our parser a bit faster.
Test Plan
Existing tests should pass.
Docs
N/A
Designed by me, implemented with a coding agent, and fixed by me.