refactor(parser): clean up diagnostics#24661
Merged
Merged
Conversation
Member
Author
Merge activity
|
Cleanup of `crates/oxc_parser/src/diagnostics.rs`. All changes are crate-internal (the `diagnostics` module is private and unused by other crates) and produce identical diagnostic output. ## Changes - **`duplicate_default_export`**: takes `Vec<Span>` instead of `impl IntoIterator<Item = Span>`. As a generic, this `#[cold]` constructor was monomorphized for whatever iterator type each caller passed (a `Chain<FilterMap<…>, FilterMap<…>>`), defeating the point of `#[cold]`. The caller now `.collect()`s only inside the `count() > 1` branch, so the common path stays allocation-free. - **`switch_multiple_default_clause`**: passes a stack array to `with_labels` instead of `vec![]` — it was the only `vec!` in the file where every other call site uses an array, and it avoided a throwaway heap allocation. - **`ts_error`**: dropped the `code` type parameter for a concrete `&'static str`. Every call site already passes a string literal, so the generality bought nothing. - **`#[cold]`**: added to the six diagnostic constructors that were missing it (`for_loop_let_reserved_word`, `cannot_appear_on_class_elements`, `cannot_appear_on_a_type_member`, `cannot_appear_on_a_parameter`, `cannot_appear_on_an_index_signature`, `accessor_modifier`). - **Parameter names**: renamed leftover codegen names (`x0`, `x1`, `span1`, `span2`, `a`, `b`, `x`) to descriptive ones across ~16 functions. Positional args mean no call sites change; e.g. `jsx_element_no_match(opening_span, closing_span, name)` now mirrors its sibling `jsx_fragment_no_match`. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
graphite-app
Bot
force-pushed
the
refactor/parser-diagnostics-cleanup
branch
from
July 19, 2026 07:24
2afdab7 to
d814c72
Compare
Merging this PR will not alter performance
Comparing Footnotes
|
graphite-app Bot
pushed a commit
that referenced
this pull request
Jul 19, 2026
> [!NOTE] > Draft. Builds on the merged diagnostics cleanup (#24661), and has been through two `/simplify` review passes. > > One design point a perf reviewer may ask about: `ParserCheckpoint` (copied on every `checkpoint`/`rewind`/`lookahead`) embeds `FatalError` **inline**, not boxed. Boxing it would shrink the checkpoint from 3 → 2 cache lines, but only by adding a heap allocation — counter to this PR's goal. The deferral already shrank the checkpoint substantially for free: `FatalError` previously held an inline `OxcDiagnostic` (~150 B), now a 72 B POD `ParserDiagnostic`. Parser diagnostics are now stored in a deferred (unmaterialized) form during parsing and materialized into `OxcDiagnostic` only once, at parse exit. ## Design A `parser_diagnostics!` macro in `diagnostics.rs` generates, from one entry per diagnostic: - an enum variant `ParserDiagnostic::Foo { .. }` holding only the POD arguments (`Span`, `&'a str`, `Copy` types); - a `#[cold] into_diagnostic()` match arm holding the eager construction (box + label strings + `format!`) — all the allocation/formatting work concentrated in one cold function; - an `#[inline]` constructor with the **same name and signature as before**, so the ~330 `diagnostics::foo(...)` call sites are unchanged. The parser/lexer error vecs and `FatalError` now carry `ParserDiagnostic<'a>`, so pushing an error, snapshotting the error set on `checkpoint()`, and discarding it on `rewind()` never allocate. Materialization happens at the two `parse()` / `parse_expression()` exit points. Foreign already-materialized diagnostics (from `oxc_regular_expression`) ride an `Eager(Box<OxcDiagnostic>)` escape hatch; the two module-record diagnostics that are built once at exit stay eager. ## Why This eliminates the eager boxing of diagnostics that are created and then thrown away during speculative parsing (arrow-function / type-vs-expression backtracking). On real-world files — all valid, so nearly every one of these allocations was a speculation-discarded diagnostic: | file | sys allocs before → after | |---|---| | checker.ts | 1818 → 23 | | antd.js | 3661 → 190 | | kitchen-sink.tsx | 2051 → 190 | | App.tsx | 360 → 23 | | binder.ts | 85 → 1 | Diagnostic output is byte-identical — parser conformance (Test262, Babel, TypeScript) snapshots are unchanged. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
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
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.
Cleanup of
crates/oxc_parser/src/diagnostics.rs. All changes are crate-internal (thediagnosticsmodule is private and unused by other crates) and produce identical diagnostic output.Changes
duplicate_default_export: takesVec<Span>instead ofimpl IntoIterator<Item = Span>. As a generic, this#[cold]constructor was monomorphized for whatever iterator type each caller passed (aChain<FilterMap<…>, FilterMap<…>>), defeating the point of#[cold]. The caller now.collect()s only inside thecount() > 1branch, so the common path stays allocation-free.switch_multiple_default_clause: passes a stack array towith_labelsinstead ofvec![]— it was the onlyvec!in the file where every other call site uses an array, and it avoided a throwaway heap allocation.ts_error: dropped thecodetype parameter for a concrete&'static str. Every call site already passes a string literal, so the generality bought nothing.#[cold]: added to the six diagnostic constructors that were missing it (for_loop_let_reserved_word,cannot_appear_on_class_elements,cannot_appear_on_a_type_member,cannot_appear_on_a_parameter,cannot_appear_on_an_index_signature,accessor_modifier).x0,x1,span1,span2,a,b,x) to descriptive ones across ~16 functions. Positional args mean no call sites change; e.g.jsx_element_no_match(opening_span, closing_span, name)now mirrors its siblingjsx_fragment_no_match.🤖 Generated with Claude Code