Skip to content

fix(codegen): union token sets for a group label shared across alternatives - #211

Merged
tinovyatkin merged 2 commits into
mainfrom
fix/205-multi-alt-label-union
Jul 26, 2026
Merged

fix(codegen): union token sets for a group label shared across alternatives#211
tinovyatkin merged 2 commits into
mainfrom
fix/205-multi-alt-label-union

Conversation

@tinovyatkin

@tinovyatkin tinovyatkin commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #205 — a token-group label used in multiple alternatives of the same rule got no accessor. In the CEL grammar (cel-go's CEL.g4):

calc
    : unary
    | calc op=('*'|'/'|'%') calc
    | calc op=('+'|'-') calc
    ;

RelationContext (label in one alternative) got op(), but CalcContext silently lost it: context_label_accessor required every declaration of a label to carry an identical token set (same_context_ref_target), and the two operator groups differ. Java ANTLR declares a single Token op field for this shape regardless of per-alternative grouping — parity target is accessor presence.

Fix

  • When every declaration of the label is token-backed, union the token sets into a synthetic reference ElementRef and thread it through the existing per-alternative selector checks (occurrence position, cardinality, no-following-match). Those checks still reject layouts where a single occurrence lookup cannot serve every alternative. Rule-reference labels keep requiring one shared target (the feat(codegen): emit typed accessors for labeled token groups and same-rule label mixes #201 same-rule label-mix shapes are out of scope here).
  • Added a shadowed_when_absent guard in context_label_selector: an optional labeled token that a later union-matching element could slide into when absent now drops the accessor instead of misresolving — closes the correctness hole the wider union would otherwise open.

Generated result for the CEL shape:

// CalcContext (previously missing entirely)
pub fn op(&self) -> Option<TerminalNode<'a>> {
    __token_children_matching(self.__node, &[18, 22, 23, 24, 25])  // '-' '+' '*' '/' '%'
        .nth(0)
        .map(TerminalNode::new)
}

Option (not Result) because the label is absent on the non-operator alternative.

Validation

  • New regression test token_group_label_shared_across_alternatives_unions_the_sets with a CEL-shaped relation/calc fixture: op() exists on both contexts; parsing a * b + c < d reads <, +, * through .op() at each tree level, None on the primary alternative.
  • End-to-end against the real cel-go CEL.g4: op() now emitted on Expr/Relation/Calc contexts; scratch crate parsing a * 2 + 1 < b passes. cel-rust's hand-rolled binary_operator_token() workaround becomes unnecessary.
  • cargo test --locked --features codegen: 1076 tests pass.
  • cargo clippy --locked --all-targets --all-features -- -D warnings: clean.
  • ANTLR runtime conformance sweep: 357/357 passed.

Summary by CodeRabbit

  • Bug Fixes

    • Improved labeled token accessors so label references are shared correctly across matching declarations and token groups are unioned across alternatives.
    • Ensured optional labeled occurrences don’t produce accessors when they would be shadowed by subsequent matches.
  • Tests

    • Added an end-to-end integration test validating token-group label propagation across multi-alternative expression structures.
    • Added a grammar fixture covering nested arithmetic/comparison operators and optional label shadowing scenarios.

…atives

A token-group label used in multiple alternatives of the same rule
(CEL's calc: 'calc op=('*'|'/'|'%') calc | calc op=('+'|'-') calc')
was silently dropped because context_label_accessor required every
declaration to carry an identical token set. ANTLR declares a single
Token op field for this shape, so union the per-alternative sets and
emit one accessor matching the union; per-alternative selector checks
still reject layouts where a single occurrence lookup cannot serve
every alternative, and an optional labeled occurrence that a later
union member could shadow now opts out instead of misresolving.

Fixes #205
@github-actions

Copy link
Copy Markdown

Copy/Paste Detection

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

Show duplications

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

  • Starting at line 1171 of tests/antlr4_rust_gen_cli.rs
  • Starting at line 1271 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

</details>

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: dddde4c3-99bc-440b-a871-95954c5a54c8

📥 Commits

Reviewing files that changed from the base of the PR and between c46ce69 and d079735.

⛔ Files ignored due to path filters (2)
  • src/bin/snapshots/antlr4_rust_gen__tests__multi_alternative_label_calc_context.snap is excluded by !**/*.snap
  • src/bin/snapshots/antlr4_rust_gen__tests__multi_alternative_label_shadowed_context.snap is excluded by !**/*.snap
📒 Files selected for processing (2)
  • src/bin/antlr4-rust-gen.rs
  • tests/fixtures/antlr4-rust-gen/multi-alternative-label/T.g4

📝 Walkthrough

Walkthrough

The generator now unions compatible token-backed labels across alternatives and suppresses shadowed accessors for absent optional labels. New grammar, unit, and CLI integration tests validate shared operator accessors and token selection.

Changes

Token-group accessor generation

Layer / File(s) Summary
Union token references and refine selectors
src/bin/antlr4-rust-gen.rs
Compatible token-backed labels now share unioned token metadata and reference targets, while optional selectors avoid shadowed accessors.
Validate shared labels across alternatives
tests/fixtures/antlr4-rust-gen/multi-alternative-label/T.g4, tests/antlr4_rust_gen_cli.rs
Adds an expression grammar and tests generated op() accessors across nested alternatives, including optional-label shadowing.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Grammar
  participant Generator
  participant GeneratedParser
  participant IntegrationTest
  Grammar->>Generator: generate lexer and parser
  Generator->>GeneratedParser: emit unioned op() accessors
  IntegrationTest->>GeneratedParser: parse expression input
  GeneratedParser->>IntegrationTest: return operator labels
Loading

Possibly related issues

  • Issue 201: Covers typed accessors for labeled token groups across alternatives, matching the updated unioning logic.
  • Issue 203: Addresses missing labeled operator accessors across repeated alternatives, which this change fixes and tests.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main codegen fix: unioning token sets for a shared label across alternatives.
Linked Issues check ✅ Passed The code and regression tests implement the requested unioned accessor behavior for a label shared across alternatives.
Out of Scope Changes check ✅ Passed The added grammar, integration test, and shadowed-optional guard are all directly related to validating the reported codegen bug.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ 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 fix/205-multi-alt-label-union

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

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

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown

Claude encountered an error after 7m 31s —— View job


Code review — PR #211

  • Gather context (diff vs origin/main, changed files)
  • Review generator changes in src/bin/antlr4-rust-gen.rs
  • Compare against upstream ANTLR v4.13.2 semantics
  • Review test coverage (fixture, CLI test, snapshots)
  • Validate candidate findings
  • Post consolidated review
    • branch fix/205-multi-alt-label-union

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@tests/antlr4_rust_gen_cli.rs`:
- Around line 461-530: Extend the fixture grammar used by
token_group_label_shared_across_alternatives_unions_the_sets with a rule such as
shadowed: x=IDENT? IDENT; and update the generated-output assertion to cover it.
Verify the generated parser omits the unsafe x() accessor for the optional
labeled occurrence, exercising the shadowed_when_absent guard while preserving
the existing alternative-label assertions.
🪄 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: bd07fbef-e6db-4293-b41a-96376271803a

📥 Commits

Reviewing files that changed from the base of the PR and between 7eb9307 and c46ce69.

📒 Files selected for processing (3)
  • src/bin/antlr4-rust-gen.rs
  • tests/antlr4_rust_gen_cli.rs
  • tests/fixtures/antlr4-rust-gen/multi-alternative-label/T.g4

Comment thread tests/antlr4_rust_gen_cli.rs
@github-actions

github-actions Bot commented Jul 26, 2026

Copy link
Copy Markdown

📊 Source Code Metrics (this PR vs main)

File Cyclomatic Cognitive Functions LLOC MI
src/bin/antlr4-rust-gen.rs 2257 (main: 2250) 🔴 1418 (main: 1415) 🔴 500 (main: 499) 🔴 3953 (main: 3944) 🔴 0 ⚪

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

@codecov

codecov Bot commented Jul 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c46ce694d3

ℹ️ 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".

Comment thread src/bin/antlr4-rust-gen.rs
Comment thread src/bin/antlr4-rust-gen.rs Outdated
…review

- Snapshot CalcContext (union accessor over both operator groups) and
  ShadowedContext (lead=PLUS? PLUS drops lead()) via the in-crate
  render_parser pattern, covering the shadowed_when_absent branch that
  also removes previously-emitted unsafe accessors for x=A? A shapes:
  with x absent, .nth(0) returned the unlabeled following token instead
  of None, so those accessors misresolved on every parse.
- Reword the union comment generically per AGENTS.md codegen boundary
  (no language-specific rule shapes in antlr4-rust-gen.rs).
- Drop the unreachable non-shared-target arm: target is only rendered
  for rule-backed labels, where the guard already forces one target.
@tinovyatkin

Copy link
Copy Markdown
Contributor Author

Review round addressed in d079735 + follow-up issues:

Fixed in d079735

  • Untested shadowed_when_absent guard (Claude Implement ANTLR v4 Rust runtime #1, CodeRabbit): added shadowed : lead=PLUS? PLUS unary ; to the fixture and pinned both behaviors with in-crate render_parser insta snapshots — multi_alternative_label_calc_context (union accessor over both operator groups) and multi_alternative_label_shadowed_context (no lead() emitted). The guard's behavior change vs main (removing the previously-emitted unsafe accessor for x=A? A shapes) is now spelled out in the commit body: with x absent, .nth(0) returned the unlabeled following token instead of None — every parse without the optional token misresolved, so this is a bug fix rather than a breaking API change (Claude Generated parser produces flat tree (no nested rule contexts) #2).
  • Language-specific comment (Codex P1): reworded to the generic r x=(A|B) r | r x=(C|D) r shape per the AGENTS.md codegen boundary.
  • Unobservable shared_target branch (Claude Cache per-decision look-1 to skip non-viable speculative paths #5): replaced with target: first.target.clone() + a comment stating the invariant (target is only rendered for rule-backed labels, where the earlier guard already forces one shared target).

Follow-up issues filed

No action

Local validation after the changes: cargo test --locked --features codegen 1076 green, clippy -D warnings clean, conformance sweep re-running now.

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.

bug(codegen): token-group label used in multiple alternatives loses its accessor (CalcContext::op)

1 participant