Skip to content

refactor(parser): clean up diagnostics#24661

Merged
graphite-app[bot] merged 1 commit into
mainfrom
refactor/parser-diagnostics-cleanup
Jul 19, 2026
Merged

refactor(parser): clean up diagnostics#24661
graphite-app[bot] merged 1 commit into
mainfrom
refactor/parser-diagnostics-cleanup

Conversation

@Boshen

@Boshen Boshen commented Jul 19, 2026

Copy link
Copy Markdown
Member

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

@github-actions github-actions Bot added the A-parser Area - Parser label Jul 19, 2026
@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label Jul 19, 2026

Boshen commented Jul 19, 2026

Copy link
Copy Markdown
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
graphite-app Bot force-pushed the refactor/parser-diagnostics-cleanup branch from 2afdab7 to d814c72 Compare July 19, 2026 07:24
@codspeed-hq

codspeed-hq Bot commented Jul 19, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 62 untouched benchmarks
⏩ 14 skipped benchmarks1


Comparing refactor/parser-diagnostics-cleanup (2afdab7) with main (c91d2a4)

Open in CodSpeed

Footnotes

  1. 14 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@graphite-app
graphite-app Bot merged commit d814c72 into main Jul 19, 2026
30 checks passed
@graphite-app graphite-app Bot removed the 0-merge Merge with Graphite Merge Queue label Jul 19, 2026
@graphite-app
graphite-app Bot deleted the refactor/parser-diagnostics-cleanup branch July 19, 2026 07:27
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-parser Area - Parser

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant