Skip to content

Use ??= for rule-options dispatch (fix tsc stack overflow on parser.hera)#86

Draft
STRd6 wants to merge 2 commits into
mainfrom
fix-civet-typecheck-overflow
Draft

Use ??= for rule-options dispatch (fix tsc stack overflow on parser.hera)#86
STRd6 wants to merge 2 commits into
mainfrom
fix-civet-typecheck-overflow

Conversation

@STRd6
Copy link
Copy Markdown
Collaborator

@STRd6 STRd6 commented Apr 30, 2026

Summary

  • 0.9.6's const $$final = X$0() || X$1() || … rule-options dispatcher introduced a regression: tsc stack-overflows when typechecking heavily mutually-recursive grammars, including Civet's parser.hera (~9.5k lines, ~290 multi-alternative rules).
  • Switched the dispatcher to a per-statement ??= reassignment chain. Same runtime semantics, same control flow, but each alternative call is typechecked independently instead of being unified into one contextually-typed || expression.
  • Kept the variable name $$final and the type-narrowing rationale (the wide MaybeResult<X | Y | …> annotation, computed via Unwrap<ReturnType<typeof X$i>>) intact.

Root cause

With the ||-chain const initializer, TS resolves the entire chain's contextual type in one pass. Combined with rule bodies that materialize typeof $$r (the const-ternary-with-comma form for $skip-using handlers), the recursion through Civet's mutually-recursive rule graph blows the stack. Per-statement ??= short-circuits this: each rule call's return type is resolved on its own line.

The fix is a single tryBlock := swap in compileRule. See the comment block in the diff for the analysis.

Test plan

  • pnpm test — 146 unit tests pass (incl. the runtime $skip test that actually executes a generated parser).
  • pnpm test:typed-parser-samples — strict-mode tsc on the bundled sample grammars still passes.
  • Civet's parser.hera end-to-end (hera.compilecivet.compiletsc -p tsconfig.json) completes without Maximum call stack size exceeded. Repro is wired up at /tmp/hera-civet-repro/ (a small build.mjs harness + stub modules + tsconfig.json); happy to upstream it under perf/ or as a regression test if useful.

Size impact (Civet's parser.hera, post-Civet TS output)

bytes vs 0.9.5 vs 0.9.6 (broken)
0.9.5 1,098,598 +143 KB
0.9.6 (main, broken) 955,426 −143 KB
this PR 960,242 −138 KB (−12.6%) +5 KB (+0.5%)

Almost all of 0.9.6's size win is preserved.

🤖 Generated with Claude Code

The `||` chain put every alternative call into a single contextually-typed
expression that TS resolves all-at-once.  Combined with rule bodies that
materialize `typeof $$r` (the const-ternary-with-comma form for $skip-using
handlers), TS exceeds the call stack typechecking Civet's parser.hera (~9.5k
lines, ~290 multi-alternative rules).  Per-statement `??=` lets each call's
return type be resolved independently.

Verified: Civet's parser.hera now typechecks end-to-end through hera.compile
+ civet.compile + tsc.  Hera's own test suite (146 tests) and typed-parser-
samples (strict tsc on the bundled grammars) still pass.  Generated parser
size is ~+0.5% vs 0.9.6 on parser.hera, ~-12.6% vs 0.9.5.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 30, 2026

Greptile Summary

This PR fixes a TypeScript stack overflow regression introduced in 0.9.6 by replacing the ||-chained const initializer for the rule-options dispatcher with a let declaration followed by per-statement ??= reassignments. The semantic equivalence holds because MaybeResult<T> is defined as ParseResult<T> | undefinedParseResult<T> is an interface (always truthy, always non-nullish) and undefined is the sole failure value (both falsy and nullish), so || and ??= short-circuit identically for every value in this type.

Confidence Score: 5/5

Safe to merge — single, well-reasoned change with verified semantic equivalence and a full passing test suite.

The switch from ||-chain to ??= chain is semantically equivalent because MaybeResult<T> is ParseResult<T> | undefined: ParseResult is an interface (always truthy and non-nullish) and undefined is the only failure value (both falsy and nullish). No behavioral difference exists for any value in this type, the fix is narrowly scoped to a single codegen site, and 146 unit tests plus typed-parser-sample tests confirm correctness.

No files require special attention.

Important Files Changed

Filename Overview
source/compiler.civet Replaces the single-expression `const $$final = X$0()

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["compileRule(options, name, rule)"] --> B{Top-level choice?}
    B -- No --> C["compileRuleBodyInline → single function"]
    B -- Yes --> D["Emit per-alternative helper functions\nX$0, X$1, … X$N"]
    D --> E["Build finalWideDecl\n(MaybeResult union type annotation)"]
    E --> F["Build tryBlock via args.map"]
    F --> G["i === 0 →\nlet $$final<type> = X$0($$ctx, $$state)"]
    F --> H["i > 0 →\n$$final ??= X$i($$ctx, $$state)"]
    G --> I["Emit dispatcher function:\nreturn $$final"]
    H --> I
    I --> J["MaybeResult<T> = ParseResult<T> | undefined\n??= ≡ || for this type:\nParseResult → truthy & non-nullish\nundefined → falsy & nullish"]
Loading

Reviews (1): Last reviewed commit: "Use `??=` instead of `||` chain for rule..." | Re-trigger Greptile

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 30, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (5a16cdb) to head (f19d2d7).

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #86   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            9         9           
  Lines         1605      1611    +6     
  Branches       256       259    +3     
=========================================
+ Hits          1605      1611    +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@STRd6 STRd6 marked this pull request as draft April 30, 2026 23:44
@STRd6
Copy link
Copy Markdown
Collaborator Author

STRd6 commented Apr 30, 2026

Actually still investigating this, it's a complex interaction with the hera types being emitted and the deeply recursive Civet grammar.

Generates a 200-rule mutually-recursive grammar in-memory, runs it through
the same pipeline Civet's plugin uses (`hera.compile({ module: true })` →
`civet.compile({ js: false })` → tsc), and asserts no `Maximum call stack
size exceeded`.

The synthetic threshold for the overflow on the broken compiler is N≈175
rules; testing at N=200 leaves a small margin so minor TS-version drift
doesn't make the test flaky.  TS lands the overflow in different internals
(`isConstContext`, `resolveEntityName`, `instantiateTypeWithSingleGeneric-
CallSignature`, …) depending on which frame happens to run out of stack
first — the regex match on the shared error message is robust to that.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@STRd6
Copy link
Copy Markdown
Collaborator Author

STRd6 commented May 1, 2026

This can likely be fixed on the Civet side with more / better annotations in parser.hera

@edemaine
Copy link
Copy Markdown
Collaborator

edemaine commented May 1, 2026

I'm surprised it makes a difference - the const that the || chain gets assigned to still has an explicit type, right?

Just checking: did you turn on types: true when calling Hera? Without that, it tries to typecheck the js version which means lots of inference.

@STRd6
Copy link
Copy Markdown
Collaborator Author

STRd6 commented May 1, 2026

I'm surprised it makes a difference

I think it only makes a difference because our current parser.hera isn't annotated enough so that the recursive rules end up blowing the TS stack. I'm leaning towards the correct solution being annotating the Civet grammar better and closing this out. For deeply combinatoric grammars using types we should fix it at the grammar rather than trying to work around it.

0.9.3 also had a change that exploded as well so I think it's not really this particular change but any type inference that gets too recursive without enough types to break parser cycles..

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.

2 participants