feat(codegen)!: accept mutual (indirect) left recursion via hub inlining (#151)

[tinovyatkin]

Closes #151.

What

ANTLR 4 rewrites direct left recursion (e : e '+' e | INT) into a precedence-climbing rule, but rejects mutual (indirect) left recursion — a left-corner cycle through two or more rules — with error(119), even though the grammar is expressible in ANTLR syntax. This PR teaches our generator to accept the tractable subclass of those cycles, reducing them to direct left recursion on the model before the existing direct-recursion rewrite runs.

The motivating real-world case (from the issue) is dotnet/roslyn's CSharp.Generated.g4 — the C# compiler team's own grammar, whose only remaining blocker (once its six empty rules are corrected) is error(119) on four cycles: type/array_type/nullable_type/pointer_type, name/qualified_name, a 13-rule expression cycle, and pattern/binary_pattern.

How — left-corner substitution ("hub inlining")

For each left-corner cycle, one member is chosen as the hub; every satellite reachable in left-corner position from the hub has its alternatives inlined into the hub until the hub is directly left-recursive. Then:

• hub-only satellites are removed (binary_pattern, qualified_name, …);
• a satellite referenced from outside the cycle is retained unchanged — its body now calls the hub, which is the precedence rule (array_type, used by array_creation_expression);
• leading-optional recursion (C#'s range operator expression? '..' expression?) is split into expr '..' expr? | '..' expr? before substitution — a union-preserving rewrite ANTLR accepts.

The hub then flows into the unchanged rewrite_immediate_left_recursion, which produces the precedence-climbing primary (operator)* form.

Correctness by construction

The pass is gated: it commits a rewrite only when the rebuilt hub is a shape the direct-recursion classifier already accepts (every alternative Primary/Prefix/Binary/Suffix). Anything it cannot reduce — argument-bearing recursion, no token-consuming base case, non-convergent substitution — is left untouched and silent, so the existing ATN-level G4A005 detector reports it exactly as before. The pass therefore either emits a direct-recursion grammar the conformance-verified path accepts, or it changes nothing.

Because the grammar we emit is exactly the one we would hand to ANTLR, tree-equality is differentially testable against ANTLR's own runtime — which is how it was validated.

Validation

• Roslyn: our generator now accepts CSharp.Generated.g4 (previously error(119)/G4A005); the generated Rust compiles; parse trees are byte-identical to ANTLR 4.13.2's runtime on inputs exercising all four cycles (records, switch expressions, is not/and/or patterns, array & nullable types, dotted names, chained invocation/element access). The only grammar change needed is the "minimal lexer adjustment" the issue describes (supply the 9 lexer token rules; parser-only grammar).
• Conformance: full ANTLR runtime testsuite green at 357/357, zero skips. Direct-left-recursion handling is untouched; both existing indirect-LR fixtures still get their G4A005 diagnostic (the pass correctly declines them).
• New tests: unit tests with insta snapshots for each cycle shape (hub-and-spoke, two-rule, multi-alt satellite, leading-optional split, external-satellite retention, and each decline path), plus an end-to-end CLI regression fixture (MutualExpr.g4) that generates, compiles, and asserts byte-identical precedence trees.

Scope / acceptance criteria

This satisfies staged criteria 1 (detect+diagnose, already shipped via G4A005) and 2 (accept a proven subclass with a correct parser + full parity), and documents which shapes are supported vs declined (criterion 3) in docs/issue-151-mutual-left-recursion-plan.md.

Known non-goal (documented): inlined satellites lose their per-rule context node type (e.g. no BinaryExpressionContext) — the tree matches ANTLR's from the transformed grammar. Recovering typed satellite nodes needs synthesized alternative labels (ANTLR forbids mixing labeled/unlabeled alts per rule), deferred as a follow-up. A pre-existing runtime prediction difference on C# var local-declaration statements is unrelated to this change (reproduced with the hand-inlined grammar where this pass is a no-op) and is out of scope.

Summary by CodeRabbit

• New Features

  • Added a new preprocessing stage to collapse eligible mutual/indirect left-recursive grammar cycles into an equivalent direct-left-recursive form before the existing rewriting step.

• Bug Fixes

  • Improved safeguards so complex/unsafe recursion patterns are declined without altering the grammar, preserving the expected diagnostics and behavior.

• Tests

  • Added end-to-end coverage for mutual left recursion with new fixtures (hub/cycle collapsing, precedence/range behavior).
  • Added decline-path verification and capped large-source matching output for stability.

[github-actions]

Copy/Paste Detection

Found 2 duplication(s) across 4 changed Rust file(s) (threshold: 100 tokens).

Show duplications

Found a 22 line (132 tokens) duplication in the following files:

• Starting at line 1896 of tests/antlr4_rust_gen_cli.rs
• Starting at line 1996 of tests/antlr4_rust_gen_cli.rs

        "parser grammar Delegate;\ndelegated: {isTypeName()}? ID;\n",
    )
    .expect("delegate grammar should be writable");
    fs::write(&tokens, "lexer grammar Tokens;\nID: [a-z]+;\n")
        .expect("token grammar should be writable");

    let output = run_antlr4_rust_gen(&[
        root.as_os_str(),
        tokens.as_os_str(),
        OsStr::new("-I"),
        temp.path().as_os_str(),
        OsStr::new("--out-dir"),
        out.as_os_str(),
    ]);
    assert!(
        output.status.success(),
        "stdout: {}\nstderr: {}",
        utf8(&output.stdout),
        utf8(&output.stderr)
    );
    let parser = fs::read_to_string(out.join("root.rs")).expect("parser should be emitted");
    assert!(parser.contains("pub trait RootHooks"), "{parser}");
```rust

---

Found a 17 line (102 tokens) duplication in the following files:
* Starting at line 2978 of tests/antlr4_rust_gen_cli.rs
* Starting at line 3045 of tests/antlr4_rust_gen_cli.rs

```rust
        dir.join("L.g4").as_os_str(),
        OsStr::new("--sem-patterns"),
        dir.join("patterns.toml").as_os_str(),
        OsStr::new("--sem-unknown"),
        OsStr::new("error"),
        OsStr::new("--require-full-semantics"),
        OsStr::new("--out-dir"),
        out.as_os_str(),
    ]);
    assert!(
        output.status.success(),
        "stdout: {}\nstderr: {}",
        utf8(&output.stdout),
        utf8(&output.stderr)
    );

    let lexer = fs::read_to_string(out.join("l.rs")).expect("lexer should be emitted");

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

This PR adds a preprocessing pass that detects selected mutual left-recursion cycles, collapses them into direct-recursive hubs, and lets existing precedence rewriting process the result. Unsupported cycles remain unchanged for downstream diagnostics.

Changes

Mutual left-recursion support

Layer / File(s)	Summary
Registration and cycle analysis src/bin_support/grammar/mod.rs, src/bin_support/grammar/mutual_recursion.rs	Registers the pass, computes nullable left-corner cycles, selects hubs, and identifies removable satellites.
Hub rewriting and validation src/bin_support/grammar/mutual_recursion.rs	Splits leading optional recursive corners, inlines admissible satellite alternatives with bounded substitution, validates direct-recursion-compatible shapes, and records regenerated IDs and provenance.
Semantic pipeline integration src/bin_support/grammar/semantics.rs	Runs mutual-recursion elimination after basic diagnostics and before direct-left-recursion rewriting.
Grammar and generated-parser validation src/bin_support/grammar/mutual_recursion.rs, tests/fixtures/antlr4-rust-gen/mutual-left-recursion/*, tests/antlr4_rust_gen_cli.rs	Adds unit and end-to-end coverage for successful rewrites, declined cycles, generated rule structure, and precedence-preserving parse trees.

Estimated code review effort: 4 (Complex) | ~75 minutes

Sequence Diagram(s)

sequenceDiagram
  participant GrammarSource
  participant SemanticAnalysis
  participant MutualRecursionPass
  participant LeftCornerAnalysis
  participant DirectRecursionRewriter
  participant GeneratedParser
  GrammarSource->>SemanticAnalysis: provide integrated grammar units
  SemanticAnalysis->>MutualRecursionPass: eliminate mutual left recursion
  MutualRecursionPass->>LeftCornerAnalysis: compute nullable left-corner cycles
  LeftCornerAnalysis-->>MutualRecursionPass: return admissible cycles
  MutualRecursionPass-->>SemanticAnalysis: rewrite hubs or preserve declined cycles
  SemanticAnalysis->>DirectRecursionRewriter: rewrite direct left recursion
  DirectRecursionRewriter-->>GeneratedParser: emit precedence parser
  GeneratedParser-->>SemanticAnalysis: produce parse trees for fixture inputs

Loading

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Linked Issues check	❓ Inconclusive	Code changes and tests cover supported mutual-left-recursion rewriting and declined cases, but docs can't be verified because docs/*.md were excluded by !/docs/.	Please include the documentation files in review or provide an unfiltered diff so the supported and declined recursion shapes can be verified.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Out of Scope Changes check	✅ Passed	The changes stay focused on mutual-left-recursion support, semantics wiring, fixtures, and validation tests; no unrelated features are evident.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: adding support for mutual indirect left recursion via hub inlining.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch worktree-leftrec

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

📊 Source Code Metrics (this PR vs main)

File	Cyclomatic	Cognitive	Functions	LLOC	MI
src/bin_support/grammar/semantics.rs	654 ⚪	511 ⚪	124 ⚪	890 (main: 889) 🔴	0 ⚪
src/bin_support/grammar/mutual_recursion.rs	312 🆕	220 🆕	87 🆕	424 🆕	0 🆕
src/bin_support/grammar/mod.rs	1 ⚪	0 ⚪	0 ⚪	0 ⚪	47.27 (main: 47.67) 🔴

Generated by mehen v1.7.0 — the code quality watcher.

[github-actions]

Claude Code review skipped — usage limit reached.

You've hit your weekly limit · resets Jul 29, 8pm (UTC) Re-run the workflow once the quota resets.

[codecov]

Codecov Report

❌ Patch coverage is 91.24894% with 103 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/bin_support/grammar/mutual_recursion.rs	91.21%	103 Missing ⚠️

📢 Thoughts on this report? Let us know!

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f2cb374224

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[coderabbitai]

Actionable comments posted: 12

🤖 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 `@src/bin_support/grammar/mutual_recursion.rs`:
- Around line 296-328: The split logic around split_leading_optional must
re-examine produced alternatives until no leading optional cycle-member corner
remains. Update expand_leading_optionals to use a worklist or equivalent
fixpoint loop, reprocessing without_corner and any subsequent outputs before
adding them to expanded; do not allow residual optional recursive corners to
reach substitute_into_hub.
- Around line 125-152: Make the mutual-recursion rewrite decline path
side-effect free by buffering all fresh ID allocations and provenance records
produced during expand_leading_optionals and substitute_into_hub, rather than
mutating the live ids and provenance immediately. Commit the buffered
IDs/provenance only after hub_is_directly_rewritable and all budget checks pass,
before applying the rebuilt rules; ensure declined rewrites leave both the model
and ProvenanceIndex unchanged.
- Around line 383-391: Unify left-corner resolution between inline_satellite and
substitute_into_hub: in src/bin_support/grammar/mutual_recursion.rs:383-391,
consume the corner position returned by left_corner_target instead of selecting
the first RuleCall, and refuse inlining unless that element uses
Quantifier::One. In src/bin_support/grammar/mutual_recursion.rs:503-527, make
left_corner_target return both the resolved target and corner position, and
report or decline cycles when the walk crosses a nullable cycle-member corner;
remove the now-redundant skippable guard.
- Around line 343-363: Preserve source declaration order while collapsing
recursive alternatives in the worklist loop around left_corner_target and
inline_satellite. Replace FIFO appending with in-place expansion at the consumed
alternative’s position, or use equivalent depth-first splicing, so each
satellite expansion remains where its source alternative occurred and nested
inlining cannot reorder precedence.
- Around line 443-446: Update the first-recursive detection around
first_recursive to use first_significant_index, skipping leading Action,
Predicate, and Epsilon elements symmetrically with last_significant. In the
self-loop check, compare last_significant with the computed first_index instead
of literal 0, while preserving the existing None handling.
- Around line 573-582: Update record_hub_provenance to stop recording the hub
rule as its own origin and stop discarding cycle. Extend its inputs or defer the
provenance record until the cycle’s member/satellite owners can be represented,
then record the hub’s synthetic RuleBoundary origin against those real
contributing owners using the surrounding multi-owner provenance semantics.
- Around line 411-419: Update renumber_elements to recurse through
ElementKind::Block values, assigning fresh IDs to each nested block alternative
and recursively renumbering its elements before returning the cloned Alternative
in the inlining flow. Preserve all existing labels, options, commands, syntax,
and spans while ensuring every cloned nested block receives unique element IDs.
- Around line 977-986: Update the test
declines_cycle_without_a_token_consuming_operator so its before snapshot is
rendered from the parsed model before eliminate_mutual_left_recursion runs,
rather than from a separate run that already applies the pass. Keep the after
model and changed assertion, then compare the pre-pass rendering with the
post-pass rendering to verify the model remains untouched.
- Around line 503-527: Update left_corner_target to report nullable rule-call
targets as left-corner cycle members instead of skipping them during traversal.
Ensure substitute_into_hub or its hub_is_directly_rewritable validation declines
rewriting when substitution would leave a nullable cycle-member corner,
preserving the cycle unchanged rather than producing an indirectly
left-recursive model.

In `@src/bin_support/grammar/semantics.rs`:
- Around line 85-94: Update the eliminate_mutual_left_recursion call in the
integration flow to pass the existing root metadata and external-reference
information required by the rewrite. Ensure the transformation preserves
satellites that are declared parser roots or referenced outside rule bodies,
while leaving integrated.roots consistent with the retained entry rules.

In `@tests/antlr4_rust_gen_cli.rs`:
- Around line 2074-2081: Update the assertions in the parser validation test to
avoid interpolating the full parser into failure messages. When reporting
missing or unexpected functions, include only the matched line(s) or a bounded
excerpt of parser output while preserving the existing assertion conditions and
diagnostic context.

In `@tests/fixtures/antlr4-rust-gen/mutual-left-recursion/MutualExpr.g4`:
- Around line 1-31: The mutual-recursion fixture coverage only verifies accepted
cycles and lacks a declined-case contract test. Add a companion grammar fixture
containing an unsupported cycle, such as a three-rule cycle or argument-bearing
recursive call, and assert the CLI exits non-zero with diagnostic code G4A005
while preserving the original rule set.

🪄 Autofix (Beta)

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: ASSERTIVE

Plan: Pro Plus

Run ID: b05ab171-4563-4127-a2c2-52146e3d5c95

📥 Commits

Reviewing files that changed from the base of the PR and between b562375 and df3c356.

⛔ Files ignored due to path filters (5)

• docs/issue-151-mutual-left-recursion-plan.md is excluded by !**/docs/**
• docs/mutual-left-recursion-rfc.md is excluded by !**/docs/**
• src/bin_support/grammar/snapshots/antlr4_rust_gen__grammar__mutual_recursion__tests__external_satellite_retained.snap is excluded by !**/*.snap
• src/bin_support/grammar/snapshots/antlr4_rust_gen__grammar__mutual_recursion__tests__name_cycle_collapsed.snap is excluded by !**/*.snap
• src/bin_support/grammar/snapshots/antlr4_rust_gen__grammar__mutual_recursion__tests__optional_from_satellite.snap is excluded by !**/*.snap

📒 Files selected for processing (5)

• src/bin_support/grammar/mod.rs
• src/bin_support/grammar/mutual_recursion.rs
• src/bin_support/grammar/semantics.rs
• tests/antlr4_rust_gen_cli.rs
• tests/fixtures/antlr4-rust-gen/mutual-left-recursion/MutualExpr.g4

[tinovyatkin]

Independent validation: LR transform works; hit one unrelated pre-existing blocker

I tested this branch end-to-end against dotnet/roslyn's CSharp.Generated.g4 from a downstream consumer's perspective (mehen's C# analyzer). The mutual-left-recursion transform does what it claims. Details, including one blocker I filed separately and one downstream consequence worth flagging.

Setup note: the branch needed a rebase

worktree-leftrec was behind main and did not include the BOM fix (#215, merged). Since Roslyn's grammar literally begins with a UTF-8 BOM, testing it on the branch as-is fails immediately at G4F003. Rebasing onto main was clean (no conflicts) and produced 0.19.1 with both changes; everything below is from that rebased build. Worth rebasing the PR branch so CI exercises the combination.

The transform is correct

Reproducing the PR's claim on a minimal grammar with two interacting left-corner cycles (expression↔switch_expression and pattern↔binary_pattern, where constant_pattern : expression crosses between hubs — Roslyn's actual shape):

ANTLR 4.13.2:  error(119) ... [expression, binary_expression, switch_expression] and [pattern, binary_pattern]
ours (#221):   accepted, 0 diagnostics

and the trees are right — left-associativity preserved, both cycles resolved:

a + b + c
  (expression (expression (expression a) + (expression b)) + (expression c))

a switch { 1 or 2 => 1 }
  (expression (expression a) switch { (arm (pattern (pattern (constant_pattern (expression 1)))
    or (pattern (constant_pattern (expression 2)))) => (expression 1)) })

a + b switch { _ => 1 }
  (expression (expression (expression a) + (expression b)) switch { (arm (pattern _) => (expression 1)) })

On the real Roslyn grammar the generator accepts it with zero errors (only the five pre-existing G4A004 optional-block warnings), emits a 4.6 MB parser, and it compiles. All four cycles' constructs are reachable: RULE_RECORD_DECLARATION, RULE_UNARY_PATTERN, RULE_RELATIONAL_PATTERN, RULE_LIST_PATTERN, RULE_FILE_SCOPED_NAMESPACE_DECLARATION, RULE_COLLECTION_EXPRESSION are all present.

Blocker found (pre-existing, not from this PR): #224

Combined grammars allocate token types for inline string literals but emit no lexer rules for them. Roslyn's grammar is combined with ~275 inline literals and one named token, so the generated lexer had 18 tokens while the parser expected ~265 — nothing parsed, with no diagnostic. Filed as #224 with a 4-line repro; it reproduces on a trivial grammar with no left recursion, so it is orthogonal to this PR. ANTLR synthesizes T__n rules for these.

Worked around by mechanically converting to a split lexer/parser grammar with named tokens (265 literals → named tokens, tokenVocab). After that: 283 lexer tokens, and real parsing.

Downstream consequence of inlining — worth documenting more prominently

The PR notes satellites lose their context node type as a "known non-goal." On Roslyn that is 16 of 17 cycle members:

INLINED (gone):  switch_expression, binary_pattern, binary_expression, assignment_expression,
                 invocation_expression, member_access_expression, element_access_expression,
                 conditional_expression, is_pattern_expression, range_expression,
                 with_expression, postfix_unary_expression, conditional_access_expression,
                 nullable_type, pointer_type, qualified_name
kept:            array_type   (externally referenced, correctly retained as documented)

For a metrics/analysis consumer this is significant, because several semantically distinct constructs collapse into one RULE_EXPRESSION, and they carry different metric weights: invocation_expression is an ABC branch while member_access_expression is not; assignment_expression is an ABC assignment while binary_expression is a condition; conditional_expression and switch_expression each add cognitive nesting.

The good news: the flattened node still carries its distinguishing operator tokens as direct children, so classification by token probe remains possible —

a + b                 -> rule_index=0  direct tokens=["+"]
a switch { _ => 1 }   -> rule_index=0  direct tokens=["switch", "{", "}"]

That is exactly the strategy mehen's Java walker already uses for grammars-v4 Java's flat expression rule, so it is workable rather than fatal. But it does mean consumers must rewrite their walkers when moving a grammar onto this path — I'd suggest calling that out in the PR's fidelity section, since "loses its context node type" understates it when 16/17 satellites disappear.

Remaining failures are my harness, not the runtime

With the split grammar I got 12/13 modern-C# constructs parsing (is not, and/relational, or, records, file-scoped namespaces, nullable refs, ??=, list patterns, collection expressions, array/nullable types, dotted names). The residual failures trace to my hand-written flat lexer, not to codegen: Roslyn puts character-level rules in the parser (decimal_digit : '0'|'1'|…, hex digits, numeric suffixes 'L'/'U'/'M'), which become 1-char tokens that shadow DEC_INT/IDENTIFIER in any flat lexer. Getting this right needs a properly-moded lexer (and INTERP_TEXT/XML_TEXT genuinely need lexer modes, since they're only valid inside interpolated strings / doc comments). That's grammar-prep work, unrelated to #221.

Summary

• ✅ Mutual left recursion accepted, trees correct, including interacting hubs
• ✅ Roslyn grammar generates + compiles
• ⚠️ Needs rebase onto main for the BOM fix (fix(frontend): accept a UTF-8 BOM in grammar sources #215), else Roslyn fails at the first byte
• ⛔ Blocked in practice by bug(codegen): combined grammars allocate token types for inline literals but emit no lexer rules for them #224 (combined-grammar implicit tokens) until that lands
• 📝 Suggest documenting the satellite-inlining impact more strongly for analysis consumers

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 62f755ce3a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4ecbf73653

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8f236b7e7a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[coderabbitai]

Actionable comments posted: 6

🤖 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 `@src/bin_support/grammar/mutual_recursion.rs`:
- Around line 414-418: Update the mutual-recursion expansion logic around the
alternative construction to avoid propagating a hub alternative’s label,
options, and commands to every satellite expansion. Prefer declining the cycle
when any participating alternative is labeled; otherwise preserve the satellite
alternative’s own metadata and ensure generated alternatives cannot contain
duplicate labels.
- Around line 561-571: The substitution budget in substitution_budget only
limits accepted steps, while each step can materialize multiple alternatives.
Update the mutual-recursion guard to bound the combined result.len() +
worklist.len() as well as the existing steps count, preserving the current
budget calculation and ensuring the worklist cannot grow beyond the intended
cap.
- Around line 999-1007: Add a regression test alongside
declines_argument_bearing_recursion that constructs mutually recursive rules
with several satellite branches and alternatives, causing substitution to exceed
the pass’s budget or fail to converge. Run it through run and assert changed is
false, specifically covering the steps > budget bailout without relying on
argument-bearing recursion or ill-founded-cycle rejection.
- Around line 78-99: Wrap the cycle detection and elimination flow around
left_corner_cycles in a bounded outer fixpoint loop, recomputing names, nullable
rules, and cycles after each rewrite. Continue until no cycles remain or an
iteration makes no changes, while preserving the existing conservative gate and
preventing unbounded looping; ensure newly introduced or merged left-corner
cycles are revisited before returning failure.
- Around line 321-327: Update the split logic around the construction of
without_corner to decline the split when removing the corner leaves no
significant elements, including alternatives containing only actions or
predicates. Preserve the existing with_corner/without_corner behavior for
residual alternatives that still contain a significant element, and ensure the
caller treats the declined split as a cycle or otherwise avoids inlining an
epsilon-matching hub alternative.

In `@tests/antlr4_rust_gen_cli.rs`:
- Around line 2290-2304: Replace the handwritten tree_of assertions in the
generated test module with descriptive assert_snapshot! calls that capture the
full generated parse-tree dumps, storing snapshots in a sibling snapshots/
directory. Add #[allow(clippy::disallowed_methods)] to both the generated test
module and temporary generated crate, ensuring the configured insta
dev-dependency is available.

🪄 Autofix (Beta)

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: ASSERTIVE

Plan: Pro Plus

Run ID: f404a78f-df58-4dbd-9b77-8d7d65cd8d5b

📥 Commits

Reviewing files that changed from the base of the PR and between df3c356 and 8f236b7.

⛔ Files ignored due to path filters (5)

• docs/issue-151-mutual-left-recursion-plan.md is excluded by !**/docs/**
• docs/mutual-left-recursion-rfc.md is excluded by !**/docs/**
• src/bin_support/grammar/snapshots/antlr4_rust_gen__grammar__mutual_recursion__tests__external_satellite_retained.snap is excluded by !**/*.snap
• src/bin_support/grammar/snapshots/antlr4_rust_gen__grammar__mutual_recursion__tests__name_cycle_collapsed.snap is excluded by !**/*.snap
• src/bin_support/grammar/snapshots/antlr4_rust_gen__grammar__mutual_recursion__tests__optional_from_satellite.snap is excluded by !**/*.snap

📒 Files selected for processing (5)

• src/bin_support/grammar/mod.rs
• src/bin_support/grammar/mutual_recursion.rs
• src/bin_support/grammar/semantics.rs
• tests/antlr4_rust_gen_cli.rs
• tests/fixtures/antlr4-rust-gen/mutual-left-recursion/MutualExpr.g4

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: aa03b8f0ed

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[tinovyatkin]

Review round addressed — 33 threads, 5 confirmed defects, 1 spun out

Thanks both; this review found a genuine architectural mistake, not a list of nits. I reproduced every actionable claim before changing anything. Five were real, and they shared one root cause: the pass mutated the grammar and only then checked whether it could finish, and it derived which rule was the left corner separately from where that corner sat.

Confirmed defects (each reproduced, then fixed)

Reproducer	Was	Now
expr : power | ID; power : <assoc=right> expr '^' expr	wrong tree: a^b^c parsed left-nested; ANTLR gives right-nested	matches ANTLR byte-for-byte
a : b* 'x'; b : a 'b'	generated, deleted b, closure lost → language changed	declines (G4A005)
a : n b; b : a 'b'; n : ;	corner decided as b, spliced at n	declines (ambiguous corner)
e : x=s | ID; s : e '+' ID / s[int x] / s @init{} / s : … #Add	labels, args, rule-level actions silently dropped	declines
lexer grammar L; A : B 'a'; B : A 'b'	"unsupported embedded lexer action" naming an action the grammar never declared	not processed (parser-only)

The structural fix

The pass is now decide, then act:

• plan_cycle proves the rewrite admissible against the untouched model — allocates nothing, mutates nothing;
• apply_plan performs the planned splice and cannot fail.

So a declined cycle is left bit-for-bit unchanged (no IDs consumed, no provenance written), which declining_consumes_no_ids_and_writes_no_provenance asserts directly rather than claiming in a comment. There is now exactly one derivation of the corner — index and target together — which removes the mis-splice family as a class. Substitution splices in place, so declared alternative order (= precedence) is preserved; optional expansion runs to a fixpoint; spliced element trees are renumbered recursively.

Tests

20 unit tests, 13 of them decline paths, one per precondition. The old "model is untouched" assertion compared two post-pass renderings and could never fail (thanks @coderabbitai) — the helper now renders before invoking the pass, and all 13 use it. New CLI fixture DeclinedCycle.g4 + test asserting a declined cycle still reports G4A005 naming [a, b] and emits no parser. Failure messages print bounded matching lines instead of the whole generated parser.

Validation

• 357/357 conformance descriptors, zero skips
• Roslyn C#: 7/7 snippets byte-identical to ANTLR 4.13.2's runtime
• Roslyn VB: 4/4 byte-identical
• clippy -D warnings + full unit suite green

Spun out, not papered over

Skipping lexer units revealed that a left-recursive lexer rule stack-overflows the ATN builder — and does so on pristine origin/main with none of this PR's code present. The old confusing action error was masking a pre-existing crash. ANTLR rejects these outright (error(119)). Filed as #236.

Declined, with reasons

Two suggestions I did not take: snapshotting the generated Rust (it churns with unrelated codegen changes; I pin the parse tree instead, cross-validated against ANTLR, and snapshot the model-level rewrite separately), and a budget-exhaustion test (the loop's progress condition is now structural, so I cannot construct a non-convergent case — such a test would pin the bound, not the behaviour). Both are answered in-thread.

The net effect is that the subclass this PR accepts is narrower and provable rather than broad and occasionally wrong, which is what the issue asked for: never accept-and-miscompile.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@tinovyatkin Thanks for the thorough follow-up. The plan-then-apply split directly addresses the unsafe partial-mutation failure mode, and deriving the corner’s index and target together closes the splice ambiguity cleanly.

The strengthened decline-path coverage—especially asserting no ID/provenance changes and retaining G4A005 behavior—provides the right safety boundary for a deliberately conservative transformation. The targeted associativity and parser-only regressions also address the confirmed correctness issues well.

🐇 ✅

---

✏️ Learnings added

Learnt from: tinovyatkin
URL: https://github.com/ophi-dev/antlr-rust-runtime/pull/221

Timestamp: 2026-07-27T22:19:17.999Z
Learning: In `src/bin_support/grammar/mutual_recursion.rs`, mutual-left-recursion elimination must use a non-mutating planning phase followed by an infallible application phase. Any declined cycle must leave the grammar model bit-for-bit unchanged, including consuming no model IDs and writing no provenance.

---

Learnt from: tinovyatkin
URL: https://github.com/ophi-dev/antlr-rust-runtime/pull/221

Timestamp: 2026-07-27T22:19:17.999Z
Learning: The mutual-left-recursion pass applies only to parser grammar units. Left-recursive lexer rules are outside this pass and a pre-existing ATN-builder stack overflow for them is tracked separately in GitHub issue `#236`.

---

Learnt from: tinovyatkin
URL: https://github.com/ophi-dev/antlr-rust-runtime/pull/221

Timestamp: 2026-07-27T22:19:17.999Z
Learning: Mutual-left-recursion satellite inlining can remove satellite-specific parser context node types; downstream analysis consumers must classify flattened hub-rule parse trees through retained direct operator tokens when they need to distinguish constructs.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

---

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

[coderabbitai]

Actionable comments posted: 5

🤖 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 `@src/bin_support/grammar/mutual_recursion.rs`:
- Around line 383-404: Update split_optional to return
Option<Vec<PlannedAlternative>> and decline the split when the candidate carries
a label or other duplicated alternative attributes. Also reject the split when
removing the optional element leaves no significant grammar element, preventing
an epsilon-only branch from making a non-nullable hub appear nullable; update
its callers to handle the declined cycle.
- Around line 559-599: Remove the unused enumeration from the alternatives
iterator in the plan-building flow: update the closure around Alternative
construction to accept only each planned alternative, and delete the
corresponding let _ = index statement. Preserve all existing identity,
provenance, element, and source-attribute handling.
- Around line 694-726: Update removable_satellites so reachability includes
references from the hub’s planned alternatives, including verbatim and nested
block content, instead of unconditionally skipping hub_id. Traverse the planned
hub element lists alongside other surviving rules, while still excluding
removable satellite rules and preserving the existing fixpoint removal behavior.
- Around line 328-379: Update planned_corner so every substitutable corner
requires element.options.is_empty(), matching classify_corner’s bare-corner
predicate. Apply this condition to both the optional-split branch and the
regular satellite path, preventing options-bearing elements from being spliced
or having options discarded; preferably reuse the shared predicate if available.
- Around line 111-126: Ensure plan_cycle returns None when it produces no
substitutions or removable cycle members, rather than returning a no-op plan.
Update the plan construction around removable and the planned alternatives so at
least one rewrite is required before apply_plan is called; preserve existing
valid plans and let the outer loop exit when no progress is possible.

🪄 Autofix (Beta)

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: ASSERTIVE

Plan: Pro Plus

Run ID: e48fd967-8b8e-45f6-a7ea-60f22a94a6b1

📥 Commits

Reviewing files that changed from the base of the PR and between aa03b8f and 4afe111.

⛔ Files ignored due to path filters (6)

• src/bin_support/grammar/snapshots/antlr4_rust_gen__grammar__mutual_recursion__tests__assoc_right_preserved.snap is excluded by !**/*.snap
• src/bin_support/grammar/snapshots/antlr4_rust_gen__grammar__mutual_recursion__tests__consecutive_optionals.snap is excluded by !**/*.snap
• src/bin_support/grammar/snapshots/antlr4_rust_gen__grammar__mutual_recursion__tests__declared_order_preserved.snap is excluded by !**/*.snap
• src/bin_support/grammar/snapshots/antlr4_rust_gen__grammar__mutual_recursion__tests__external_satellite_retained.snap is excluded by !**/*.snap
• src/bin_support/grammar/snapshots/antlr4_rust_gen__grammar__mutual_recursion__tests__name_cycle_collapsed.snap is excluded by !**/*.snap
• src/bin_support/grammar/snapshots/antlr4_rust_gen__grammar__mutual_recursion__tests__optional_from_satellite.snap is excluded by !**/*.snap

📒 Files selected for processing (3)

• src/bin_support/grammar/mutual_recursion.rs
• tests/antlr4_rust_gen_cli.rs
• tests/fixtures/antlr4-rust-gen/mutual-left-recursion/DeclinedCycle.g4

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 793b3ae02e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6106e72040

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: eb69af7100

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b18ebff1e6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".