fix(codegen): emit label accessors nested in groups and mixed same-rule labels - #230
Conversation
Copy/Paste DetectionFound 11 duplication(s) across 4 changed Rust file(s) (threshold: 100 tokens). Show duplicationsFound a 26 line (160 tokens) duplication in the following files:
};
}
}
}
// Deepest choices first, so an inner result rolls up into its parent branch.
// The list is rebuilt from `per_branch` each pass, because folding an inner
// choice *creates* an entry for its parent that must then fold in turn.
let mut processed: BTreeSet<usize> = BTreeSet::new();
// Deepest unprocessed choice still holding entries. Folding one creates an
// entry for its parent, so the candidate set is re-examined every pass.
while let Some(choice) = per_branch
.keys()
.map(|(choice, _)| *choice)
.filter(|choice| !processed.contains(choice))
.max_by_key(|choice| depth_of_choice.get(choice).copied().unwrap_or(0))
{
processed.insert(choice);
let counts = per_branch
.iter()
.filter(|((candidate, _), _)| *candidate == choice)
.map(|((_, branch), count)| (*branch, *count))
.collect::<Vec<_>>();
if counts.is_empty() {
continue;
}
let agreed = if restricted_to_one_path {
```rust
---
Found a 23 line (148 tokens) duplication in the following files:
* Starting at line 2675 of src/bin_support/embedded.rs
* Starting at line 3282 of src/bin_support/embedded.rs
```rust
fn unscoped_reads_reject_alternatives_that_would_satisfy_them_unbound() {
let token_ref = |label: Option<&str>, target: &str, token_type| ElementRef {
label: label.map(ToOwned::to_owned),
target: target.to_owned(),
token_types: vec![token_type],
is_block: false,
is_list: false,
cardinality: ChildCardinality {
min: 1,
max: Some(1),
},
stable_accessor: true,
choice_branch: Vec::new(),
choice_arity: Vec::new(),
choice_spans: Vec::new(),
group_spans: Vec::new(),
branch_spans: Vec::new(),
leading_terminal: true,
span: None,
branch_local_cardinality: ChildCardinality::ONE,
group_local_cardinality: ChildCardinality::ONE,
};
let translate = |second: ElementRef| {Found a 22 line (132 tokens) duplication in the following files:
"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 22 line (121 tokens) duplication in the following files:
* Starting at line 2472 of src/bin_support/embedded.rs
* Starting at line 2624 of src/bin_support/embedded.rs
```rust
is_list,
cardinality: ChildCardinality {
min: 1,
max: Some(1),
},
stable_accessor: true,
choice_branch: Vec::new(),
choice_arity: Vec::new(),
choice_spans: Vec::new(),
group_spans: Vec::new(),
branch_spans: Vec::new(),
leading_terminal: true,
span: Some(span),
branch_local_cardinality: ChildCardinality::ONE,
group_local_cardinality: ChildCardinality::ONE,
};
let mut statement = rule("s");
statement.alts.push(AltModel {
label: None,
span: (0, 100),
// `xs+=A {action at 20} A`
refs: vec![Found a 14 line (120 tokens) duplication in the following files:
};
let parent = counts.first().and_then(|(branch, _)| {
ancestry.get(&(choice, *branch)).and_then(|chain| {
chain
.split_last()
.and_then(|(_, rest)| rest.last().copied())
})
});
for (branch, _) in &counts {
per_branch.remove(&(choice, *branch));
}
match parent {
Some(parent_key) => {
let slot = per_branch.entry(parent_key).or_insert(Some(0));
```rust
---
Found a 21 line (119 tokens) duplication in the following files:
* Starting at line 2250 of src/bin_support/embedded.rs
* Starting at line 2312 of src/bin_support/embedded.rs
```rust
let single_ref = ElementRef {
label: Some("name".to_owned()),
target: "e".to_owned(),
token_types: Vec::new(),
is_block: false,
is_list: false,
cardinality: ChildCardinality {
min: 1,
max: Some(1),
},
stable_accessor: true,
choice_branch: Vec::new(),
choice_arity: Vec::new(),
choice_spans: Vec::new(),
group_spans: Vec::new(),
branch_spans: Vec::new(),
leading_terminal: true,
span: None,
branch_local_cardinality: ChildCardinality::ONE,
group_local_cardinality: ChildCardinality::ONE,
};Found a 18 line (115 tokens) duplication in the following files:
let list_ref = || ElementRef {
label: Some("args".to_owned()),
target: "e".to_owned(),
token_types: Vec::new(),
is_block: false,
is_list: true,
cardinality: ChildCardinality { min: 1, max: None },
stable_accessor: true,
choice_branch: Vec::new(),
choice_arity: Vec::new(),
choice_spans: Vec::new(),
group_spans: Vec::new(),
branch_spans: Vec::new(),
leading_terminal: true,
span: None,
branch_local_cardinality: ChildCardinality::ONE,
group_local_cardinality: ChildCardinality::ONE,
};
```rust
---
Found a 17 line (104 tokens) duplication in the following files:
* Starting at line 2339 of src/bin_support/embedded.rs
* Starting at line 3462 of src/bin_support/embedded.rs
```rust
cardinality: ChildCardinality { min: 0, max: None },
stable_accessor: true,
choice_branch: Vec::new(),
choice_arity: Vec::new(),
choice_spans: Vec::new(),
group_spans: Vec::new(),
branch_spans: Vec::new(),
leading_terminal: true,
span: None,
branch_local_cardinality: ChildCardinality::ONE,
group_local_cardinality: ChildCardinality::ONE,
},
],
children: BTreeMap::new(),
leading_target: Some("e".to_owned()),
});
let m = model(vec![statement, rule("e")]);Found a 19 line (103 tokens) duplication in the following files:
target: "'a'".to_owned(),
token_types: vec![1],
is_block: false,
is_list: false,
cardinality: ChildCardinality {
min: 1,
max: Some(1),
},
stable_accessor: true,
choice_branch: Vec::new(),
choice_arity: Vec::new(),
choice_spans: Vec::new(),
group_spans: Vec::new(),
branch_spans: Vec::new(),
leading_terminal: true,
span: None,
branch_local_cardinality: ChildCardinality::ONE,
group_local_cardinality: ChildCardinality::ONE,
},
```rust
---
Found a 17 line (102 tokens) duplication in the following files:
* Starting at line 1990 of src/bin_support/embedded.rs
* Starting at line 2008 of src/bin_support/embedded.rs
```rust
label: Some("left".to_owned()),
target: "e".to_owned(),
token_types: Vec::new(),
is_block: false,
is_list: false,
cardinality: ChildCardinality::ONE,
stable_accessor: true,
choice_branch: Vec::new(),
choice_arity: Vec::new(),
choice_spans: Vec::new(),
group_spans: Vec::new(),
branch_spans: Vec::new(),
leading_terminal: true,
span: None,
branch_local_cardinality: ChildCardinality::ONE,
group_local_cardinality: ChildCardinality::ONE,
},Found a 17 line (102 tokens) duplication in the following files:
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");
```rust
</details> |
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe generator preserves labels inside unlabeled blocks, scopes positional references to alternatives, and rejects ambiguous embedded label resolution. New fixtures and tests cover grouped labels, mixed labels, optional branches, unbounded runs, and accessor omission cases. ChangesLabel resolution and accessor generation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GrammarFixture as multi-alternative-label/T.g4
participant Generator as antlr4-rust-gen
participant EmbeddedResolver as embedded label resolver
participant TypedContexts as generated typed contexts
participant Tests as Issue 201 regression tests
GrammarFixture->>Generator: provide grouped and alternative labels
Generator->>TypedContexts: emit branch-aware accessors
EmbeddedResolver->>TypedContexts: resolve embedded label positions
Tests->>TypedContexts: parse inputs and inspect accessors
TypedContexts-->>Tests: return labeled children or translation errors
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
📊 Source Code Metrics (this PR vs
|
| File | Cyclomatic | Cognitive | Functions | LLOC | MI |
|---|---|---|---|---|---|
| src/bin/antlr4-rust-gen.rs | 2506 (main: 2383) 🔴 | 1581 (main: 1486) 🔴 | 541 (main: 536) 🔴 | 4341 (main: 4191) 🔴 | 0 ⚪ |
| src/tree.rs | 340 (main: 330) 🔴 | 93 (main: 89) 🔴 | 176 (main: 174) 🔴 | 474 (main: 455) 🔴 | 0 ⚪ |
| src/bin_support/embedded.rs | 500 (main: 166) 🔴 | 327 (main: 105) 🔴 | 66 (main: 36) 🔴 | 739 (main: 248) 🔴 | 0 ⚪ |
Generated by mehen v1.7.0 — the code quality watcher.
|
Claude Code review skipped — usage limit reached.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f69861457c
ℹ️ 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".
…lation
Making choice-branch refs report `min: 0` (previous commit) exposed a latent
flaw in `TranslationCtx::resolve_label`: it counts same-target refs ahead of a
label to derive a positional CST index, but refs drawn from sibling branches of
a choice are mutually exclusive, so that count can exceed the children the parse
actually built.
For `r : (B | x=B) {$x.text}` the flattened refs are an unlabeled `B` followed by
the labeled one, so `$x` translated to `child_tokens(..).nth(1)` while the CST
holds a single `B` — the action silently observed a default value. Before the
descent change this grammar failed to translate outright, so the regression
traded a loud error for a wrong value.
`resolve_label` now tracks the running occurrence as `Option<usize>` and poisons
it at the first ref whose cardinality is not exact, reporting the label
unresolved. Callers already surface that as `cannot translate $x`, restoring the
loud failure. The accessor path is unaffected: it has its own
`exact_target_cardinality` guard and declines these layouts independently.
Also replaces the grammar-specific token names in the two new comments with
neutral placeholders, per the AGENTS.md codegen-boundaries rule.
Both findings reported by Codex on #230.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 742adfcc39
ℹ️ 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".
…token sets Three follow-ups from Codex's second pass on #230, each verified against the parent commit to separate regressions from pre-existing gaps. `structural_block_labels_inside` only inspected a block's direct elements, so a label under redundant grouping levels (`((x=A))?`) still lost its accessor. Extra grouping is syntactically inert; the check now descends nested blocks. This one predates the PR — the parent commit dropped the label too. The other two are regressions from making branch refs inexact: Token groups and wildcards carry an empty `target`, so they all shared one occurrence bucket in `resolve_label`. An optional disjoint group ahead of a labeled one (`r : (A | B)? x=(C | D) {$x.text}`) therefore poisoned a label it has nothing to do with, and a grammar that translated fine before now failed. Empty-target and list refs no longer participate in the buckets at all — their reads iterate or take the last terminal child and never consult the index, which is exactly why sharing a bucket was meaningless. An *optional* label with a following same-target child returned its saved index before the cardinality poisoning could apply. For `r : ({false}? x=A)? A {$x...}` that emitted `nth(0)`, reading the mandatory `A` as `x` when the optional group is skipped, where the parent commit had errored. `resolve_label` now applies the same shadowed-when-absent test the accessor path already uses.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
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 `@src/bin_support/embedded.rs`:
- Around line 384-395: The list handling in the alt.refs indexing loop must
poison occurrence_by_target for list elements with a real target instead of
continuing without updating the bucket. Keep empty-target token groups excluded
from indexing, but mark a list target as inexact/invalid so later same-target
labels remain unresolved rather than using occurrence 0; add coverage for the
list-before-single ordering such as errors+=unary followed by name=unary.
🪄 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: 96eb9291-2b63-4f43-98a4-141c29faec52
📒 Files selected for processing (3)
src/bin/antlr4-rust-gen.rssrc/bin_support/embedded.rstests/fixtures/antlr4-rust-gen/multi-alternative-label/T.g4
Exempting list refs from the occurrence accounting was too broad. A list ref
still contributes CST children, so a *later* same-target single label is indexed
against them: for `r : errors+=u name=u {$name.text}` the skip made the bucket
start at zero and emitted `nth(0)`, reading the first `errors` child as `name`.
Verified at runtime — on input `x y` the action printed `x` where `y` is correct.
The parent commit emitted `nth(1)` here, so this was a regression.
List refs now flow through the same cardinality accounting as any other ref,
which is both narrower and more accurate than skipping or blanket-poisoning:
* `errors+=u name=u` — the list contributes exactly one child, so the position
is still known and `nth(1)` is emitted, matching the parent commit;
* `errors+=u+ name=u` — the unbounded run leaves no fixed index, so the label
reports unresolved and the caller fails loudly. The parent commit silently
emitted a wrong `nth()` here, so this case improves on it.
A *labeled* list read is unaffected either way: it iterates every same-target
child and never consults the index, so it returns resolvable after the
accounting rather than before it.
Reported by CodeRabbit on #230, which also supplied the reverse-ordering repro
the existing fixtures missed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d64f014fe7
ℹ️ 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".
Three more shapes where `resolve_label` returned a read that silently selects the wrong element, plus the snapshot coverage the decline tests were missing. The incremental guards had grown into a pile of special cases that each answered "is this index countable?" while the real question is "does the read this label translates to select the label's own element and nothing else?". Every read `translate_element_read` emits is positional — `nth(i)` for a single label, all same-target children for a list, the last terminal child for a block — and none retain which branch built a child. `resolve_label` now asks that question directly, per label kind, in one place: * **list** — declines when any same-target element sits outside the label. Fixes `name=e ... errors+=e`, where `$errors` folded in `name`'s child. Pre-existing: the parent commit emitted the same wrong read. * **block** — declines when a following terminal would become the `last()` the read takes. Fixes `((x=(A | B))) C`, which read `C` as `x`. * **any kind** — declines a label declared twice when one read cannot serve both. Fixes `(x=A | x=B)`, which searched for an `A` child even on the `B` branch. The last two were regressions from exposing nested labels: the parent commit rejected both translations outright. The list case predates this PR. Countable shapes still resolve unchanged — `errors+=u name=u` keeps `nth(1)`, and `(A | B)? x=(C | D)` keeps its block read — so this narrows nothing that was already correct. Also snapshots the three declining contexts whole instead of probing them with `!contains`, per the AGENTS.md guidance that snapshots subsume negative guards: the absent accessor is now visible alongside everything the context does expose. Reported by Codex on #230.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bin/antlr4-rust-gen.rs (1)
14068-14132: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the required Insta Clippy allowance.
grouped_and_mixed_same_rule_labels_emit_accessors_without_crossing_branchesusesinsta::assert_snapshot!but does not include#[allow(clippy::disallowed_methods)]; bare test functions with Insta assertion macros need this allowance per the repository rules.Proposed fix
+ #[allow(clippy::disallowed_methods)] #[test] fn grouped_and_mixed_same_rule_labels_emit_accessors_without_crossing_branches() {🤖 Prompt for 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. In `@src/bin/antlr4-rust-gen.rs` around lines 14068 - 14132, Add #[allow(clippy::disallowed_methods)] to the grouped_and_mixed_same_rule_labels_emit_accessors_without_crossing_branches test function, directly alongside its existing #[test] attribute, without changing the snapshot assertions.Source: Coding guidelines
🤖 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/embedded.rs`:
- Around line 382-393: The resolve_label method must not return the first match
when body_offset is None. Collect matches from all alternatives, accept only
when they resolve to one provably identical read, and otherwise reject the
conflicting label; preserve the existing scoped lookup behavior when body_offset
selects a single alternative.
- Around line 426-441: Update the block-label handling in translate_element_read
so it does not emit a read when the action position cannot be compared with the
referenced block’s child positions. Carry element and action source spans
through the relevant model and restrict terminal lookup to children before the
action; otherwise reject the read instead of returning Some((element.clone(),
0)) for every empty target.
---
Outside diff comments:
In `@src/bin/antlr4-rust-gen.rs`:
- Around line 14068-14132: Add #[allow(clippy::disallowed_methods)] to the
grouped_and_mixed_same_rule_labels_emit_accessors_without_crossing_branches test
function, directly alongside its existing #[test] attribute, without changing
the snapshot 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: 5179fac2-fcf4-4f7a-8cd9-6eef4e6df2f4
⛔ Files ignored due to path filters (4)
src/bin/snapshots/antlr4_rust_gen__tests__multi_alternative_label_branch_hazard_context.snapis excluded by!**/*.snapsrc/bin/snapshots/antlr4_rust_gen__tests__multi_alternative_label_branch_rival_context.snapis excluded by!**/*.snapsrc/bin/snapshots/antlr4_rust_gen__tests__multi_alternative_label_mixed_unbounded_context.snapis excluded by!**/*.snapsrc/bin/snapshots/antlr4_rust_gen__tests__multi_alternative_label_nested_group_context.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
src/bin/antlr4-rust-gen.rssrc/bin_support/embedded.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3de4d48e21
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08748bb646
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/embedded.rs`:
- Around line 416-420: Update same_label_read to compare the read-mode fields
consumed by translate_element_read: include is_block and avoid using token_types
for block-read equivalence. Preserve comparisons for label, target, and list
mode, and add a test covering equivalent block labels across alternatives such
as x=(A | B) and x=(C | D).
- Around line 397-402: Update the alternative-resolution logic around
resolve_label_in_alt so nondeclaring alternatives are inspected for children
that can satisfy the selected positional, list, or block read instead of being
unconditionally skipped. Reject the unscoped read when such a child exists,
preserving unset-label behavior when no unlabelled alternative can satisfy it.
Add a regression case covering r: x=A | A with a subsequent $x.text lookup.
- Around line 437-454: Update the repeated-list-label resolution in the
element.is_list branch to require all declares_label(candidate) declarations to
use the same target and read mode as element before returning the resolved
iterator. Preserve the existing target-exclusion check, but return None when
declarations such as xs+=A and xs+=B differ instead of selecting only the first
target.
🪄 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: b0963be6-9a7a-4691-92af-803684dcbdb1
📒 Files selected for processing (1)
src/bin_support/embedded.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b773e8a1af
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfd07041e1
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/embedded.rs`:
- Around line 80-96: Retain the full enclosing choice ancestry in ElementRef
rather than only the innermost branch, and update can_coexist_with to report a
conflict when any shared choice has different alternatives. In
src/bin/antlr4-rust-gen.rs:2763-2767, append each current choice branch to
inherited ancestry. In src/bin_support/embedded.rs:1602-1677, add a
nested-choice regression covering both the safe inner action and a genuinely
sequential follower.
- Around line 444-460: Update alt_can_satisfy_read and the structural lowering
that builds AltModel.refs so unlabeled ElementKind::Range terminals are
preserved or otherwise considered for block reads. Ensure an unlabeled range
cannot satisfy an unscoped read when its target element is unset, while
retaining valid labeled terminal behavior, and add a regression test for r :
x=(A | B) | 'c'..'d'; with an unscoped $x read.
🪄 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: 78099ce7-be6e-49fe-aba5-e65e4dc5e475
⛔ Files ignored due to path filters (1)
src/bin/snapshots/antlr4_rust_gen__tests__left_recursive_label_alternatives.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
src/bin/antlr4-rust-gen.rssrc/bin_support/embedded.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f86861cde
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83e456b443
ℹ️ 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".
…inement
Seven findings from Codex, six roots.
**Enclosure recorded conflicting branches.** Collecting the tags of every ref
preceding the action also picks up an *earlier* sibling's — its refs precede the
action and share the choice's block span too. `(B | C x=A? A {…})` claimed both
branches, and the resulting `on_action_path` excluded the second branch's own
follower, so the trailing `A` was read as an absent `x`. For each enclosing choice
the code now selects the single branch containing the action: the last branch whose
refs have started before it.
**Sibling hazards applied even when the action cannot reach the sibling.** In
`(x=(A | B) {…} | C)` the `C` occupies the same terminal index, but the action only
ever runs on the labeled branch, so it was declining a valid read. The check is now
gated on branch confinement.
**A fixed index was accepted for an *optional* block label.** `x=(A | B)? C {…}`
has a fixed index 0, but when the block is absent `C` occupies it. The
optional-follower check now runs on the fixed-index path too, not only the
fallback.
**Read equivalence still demanded equal `is_block`.** `x=A | x='a'` resolves the
symbolic form in token mode and the literal in block mode, so the two were rejected
despite lowering to the same `child_tokens(A)` query. Token-backed resolutions are
compared by token type first.
**Satisfiability summed nested choices independently**, double-counting
`q x=q | ((q|b)|(q|c))` and rejecting a valid read. It now folds innermost-first
via a new `widest_child_count`, the max-analogue of `exact_child_count`.
**`can_coexist_with` does not by itself select one path.** A ref *after* a choice
coexists with every branch, so `exact_terminal_index` was summing mutually
exclusive branches and mis-indexing `A x=(B|C) | (y=A | D) E`. It now strips only
the tags of choices the element is inside, leaving the rest subject to cross-branch
agreement.
**A merged scalar label ignored repetition.** `(x=A+ | x=B+)` emitted `nth(0)`,
but ANTLR overwrites a scalar on each iteration, so repeated declarations merge as
`LastAfter`.
…ifier as met
Five findings from Codex, four roots.
**Merging compared occurrences in different units.** A block read indexes every
terminal child; a token read indexes only same-type children. `x='a' | A x=A`
reports occurrence 1 from both declarations, meaning different children, and the
merge silently picked one. Cross-mode merges are now accepted only at occurrence
zero — the single index where the two coordinate systems coincide — while
`x=A | x='a'` (both at zero) still merges as it should.
**An enclosing group is *taken* when an action inside it runs.** In
`(A x=A {$x.text})?` the action executes only if the group matched, so on that path
the preceding `A` is exactly-once; both `cardinality` and
`branch_local_cardinality` report `0..1` from the group's `?`, so neither could say
so. `ElementRef` gains `group_local_cardinality` (only the element's own EBNF
suffix) and `group_spans` (every enclosing block, single-alternative ones included),
and refs sharing a taken group with the action are judged by that figure.
**Merged declarations skipped the per-declaration hazards.** `(x=A? B | x=B)`
merged at occurrence zero, but the first branch's unlabeled `B` slides in when the
optional `x` is absent. Each declaration now faces the optional-follower check,
judged on its branch-local optionality so a `min: 0` that only reflects branch
membership does not count. Likewise a list merge required all mutually exclusive
declarations to share a start: `(A xs+=A | xs+=A)` needs skip 1 on one branch and
0 on the other, so it declines rather than returning an empty iterator on one path.
**A branch holding only an action emits no ref**, so attributing the action by ref
position picked a neighbouring branch and `x=A? (A | {$x.text})` failed.
`ElementRef` now records `branch_spans`, the enclosing alternative's own extent, so
the action is attributed to the branch whose text contains it — ref-free branches
included.
Fifteen rounds of review findings in this area produced a corpus of grammars that was living in a scratch directory and being re-checked by hand. Several of the fixes regressed an earlier case, and the only reason that surfaced was manual re-running — so the corpus is now a fixture directory and a test. `tests/fixtures/antlr4-rust-gen/label-resolution/` holds one grammar per shape whose *outcome* the resolver decides, and `label_resolution_corpus_matches_ expected_outcomes` asserts resolve-vs-decline for each with the reason recorded inline. The reason matters: most of these shapes have been wrong in both directions at some point, so "decline" and "resolve" are equally easy to break, and a bare boolean would not say which behaviour was intended. The signal differs by fixture and the test picks per case: a grammar whose action reads the label fails to render outright when resolution declines, while one relying on the typed accessor renders either way and the decision shows up as the method's presence. Verified by reverting the `@init` fix — the corpus names the case and prints the translation error.
…and sibling tests
Six findings from Codex, four roots.
**A closed inner group was treated as taken.** `on_taken_group` accepted a ref if
*any* enclosing group contained the action, so in `((q)? x=q {$x.text})?` the inner
`(q)?` counted as matched and the read became `nth(1)` — wrong whenever it is
absent. Every group the ref sits in must enclose the action; an inner one that
closed beforehand proves nothing.
**Deriving the read from the first declaration kept missing dimensions.** Guards
were added per property — token type, coexistence, occurrence, repetition — and
each round found another. Each declaration is now resolved on its own (recursing
with the other labels stripped) and the results must agree, so `(A x=A B | x=A C)`
declines on differing occurrence and `(x=A B | x=A+ C)` on differing repetition
without either needing its own check.
**A choice before the label was summed, not folded.** `can_coexist_with` keeps
every branch of a choice the label sits *outside* of, so claiming path-restriction
made `(A B | A C) x=A` count two prefix `A`s. Restriction is now derived: tags of
choices the label is inside are dropped, and only if none survive is the count
path-restricted.
**Two comparisons still keyed on spelling rather than token type.** A list read's
declarations (`xs+=A B | xs+='a' C`) and the sibling-shadow test both compared
source forms.
Also scoped the sibling-shadow exemption correctly: a sibling's child cannot
displace the label within a parse that bound it, but it remains a hazard when the
read may run with the label unset — which is exactly when the action is not
confined to the label's branch, `@after` bodies included.
`AliasDifferingOccurrenceInAlt` declines rather than merging. Codex asked for the
occurrences to be normalised into one system; a block read indexes every terminal
child while a token read indexes only same-type children, and reconciling them
needs a second traversal per declaration. Merging at occurrence zero — where the
two provably coincide — covers the real alias grammar (`x=A | x='a'` as top-level
alternatives, which resolves) and declines the rest rather than guessing.
All six added to the label-resolution corpus, now 21 cases.
Rebasing onto main brought in #235, which routes label accessors through `__labeled_token_children*` so deleted-token errors cannot consume the selected occurrence. The accessors this branch adds pick that up automatically; only the pinned snapshot text changes. Every diff is the iterator rename and nothing else — checked by filtering the snapshot diffs down to lines that are not that substitution, which left none. The two changes compose: #235 decides *which children a label read sees*, this branch decides *whether the read is emitted at all*.
558406f to
10edafd
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10edafd836
ℹ️ 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".
…choice
Seven findings from Codex, all in the embedded read path.
**A repeated block label emitted a first-match read.** `x=(A | B)+` is overwritten
each iteration, so ANTLR exposes the last match. The non-block path already
declined this; the block path now does too.
**Mixed-mode occurrence zero is not always the same child.** `B x=A | x='a'` has
the symbolic side at same-token occurrence 0 while sitting at terminal position 1,
so merging it with the literal's terminal 0 exposed `B`. `ElementRef` records
`leading_terminal` — whether no terminal can precede the element on its path — and
a mixed-mode merge now requires it on both sides, not merely occurrence zero.
**A branch-confined action resolved irrelevant sibling declarations.** In
`(x=A {$x} | x=A+ B)` the action's own read is unambiguous, but requiring agreement
with the other branch's repeated declaration rejected it. Declarations are filtered
to the action's branch first.
**Confinement to an outer branch does not restrict a nested choice inside it.**
`((a=A | b=B) x=(C | D) {…} | E)` summed the `A`/`B` prefixes and read `nth(2)`.
The block count now strips only the tags of choices the action is actually inside
and lets any survivor force cross-branch agreement — the same derivation the scalar
path uses.
**A single index cannot describe a repeated candidate.** `C x=(A | B) | (D | E)+`
reports the repeated group's start as 0 while it also covers index 1, where `x`
reads. `can_occupy_terminal_index` now tests the candidate's whole span.
**An inline action before the label counted children that had not matched.**
`{$x.text} A* x=A` reads an empty list whatever follows, yet the future `A*`
poisoned the count. The prefix filter now applies `matched_at_action`.
**A token-only choice holding an action collapsed away its branch spans**, so
`x=A? (A | B {$x.text})` could not tell that the action runs only where the sibling
`A` cannot shadow `x`. Such blocks stay expanded, like label-bearing ones.
All seven added to the corpus, now 28 cases.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a88072f5b
ℹ️ 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".
…ter match `LastAfter` applies to every branch, so merging repeated and non-repeated declarations onto it is only sound when no branch has a matching child *after* its declaration. In `(x=A A B | x=A+ C)` the first branch's trailing unlabeled `A` became the `last()`, so `x()` returned it instead of the bound first `A`. Fixing this exposed that the `merged_repeats` fixture I added two commits ago was itself unsound: `(last_pick = IDENT+ | last_pick = NUM+) NUM` puts a trailing `NUM` inside the accessor's `[IDENT, NUM]` union, so on the `NUM+` branch the read would return the trailing token rather than the bound one. The fixture now ends in `LPAREN`, outside the union, which is the shape where the merge is genuinely safe — and the new declining case is pinned separately as `RepeatedMergeFollowedByMatch`, bringing the corpus to 29. Reported by Codex on #230.
There was a problem hiding this comment.
💡 Codex Review
antlr-rust-runtime/src/bin/antlr4-rust-gen.rs
Lines 9229 to 9233 in 8616aad
For r : (x=A | x=B) EOF;, every parse binds x, yet the generated accessor is fn x(&self) -> Option<TerminalNode> rather than the required-child Result used for other mandatory labels. The first declaration's 0..1 cardinality reflects only that its individual choice branch may not be taken; using it for the entire top-level alternative ignores that every sibling branch has a compatible declaration. Fold all nested declarations by choice coverage before deriving the accessor cardinality.
AGENTS.md reference: AGENTS.md:L5-L9
ℹ️ 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".
| // Their cardinality on this path is the branch-local one. | ||
| element.cardinality = element.branch_local_cardinality; |
There was a problem hiding this comment.
Condition prefix counts on the label's enclosing group
For r : (A x=A)? EOF;, the generated RContext omits the valid x() accessor. On every parse where x is bound, the optional group is necessarily taken and the preceding A count is exactly one, while on the absent path child_tokens(A).nth(1) is safely empty; however, replacing the prefix cardinality with branch_local_cardinality retains the enclosing group's 0..1 quantifier, so the exactness check rejects it. Condition shared enclosing groups as taken when computing a declaration's prefix, analogous to the action-path handling.
AGENTS.md reference: AGENTS.md:L5-L9
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 058080f. r : (A x=A)? EOF; omitted a valid x(); it now emits it.
Your reasoning is what the fix encodes: "on every parse where x is bound, the optional group is necessarily taken and the preceding A count is exactly one, while on the absent path nth(1) is safely empty." Both cardinality and branch_local_cardinality reported 0..1 from the group's ?, so neither could express that.
The accessor path now does what the embedded path already did — when a ref shares every optional group with the label, those groups are taken wherever the label is bound, so the quantifier-free group_local_cardinality applies. One detail that cost me a cycle: exact_target_cardinality judges exactness from branch_local_cardinality, so overriding cardinality alone had no effect; both fields have to carry the on-path figure.
Pinned as OptionalGroupSharedWithLabel in the corpus.
…ndex as a sentinel
Two findings from Codex, both regressions from the previous two commits.
Requiring *every* enclosing group to contain the action was too strong. In
`((q) x=q {$x.text})?` the outer `?` is satisfied wherever the action runs, but the
inner `(q)` is mandatory and closes beforehand — so the check failed and the outer
`0..1` stayed on the preceding `q`, rejecting a read the parent emitted as `nth(1)`.
`group_spans` now carries a `GroupSpan { start, end, optional }` per enclosing
block, and only the groups that actually *relax* the lower bound have to enclose
the action. A mandatory group imposes nothing, so where it closes is irrelevant.
`usize::MAX` is the sentinel for "no fixed index, fall back to `last()`", but
`can_occupy_terminal_index` compared it as a position: in
`r @after {$x.text} : A? ((x=(B|C))) | D;` the `D` alternative was judged unable to
reach index `usize::MAX`, so the hazard check passed and input `d` reported `D` as
`x`. Any terminal can be the last child, so the sentinel now admits every
candidate — which makes the alternative a hazard and the label decline.
Both added to the corpus, now 31 cases.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e17fe88e4e
ℹ️ 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".
…clining
Five findings from Codex. Four fixed; the fifth is recorded as a known decline.
**`@init` emitted code that panics.** A scalar *rule* label lowers to
`.nth(i).expect("labeled rule child")`, so `r @init {$x.ctx} : x=q EOF;` generated
successfully and then panicked on every parse — no child exists at rule entry. Reads
that degrade gracefully (iterators, `.text`) still resolve; that one declines.
**Isolating one declaration reclassified its twin as an impostor.** Resolving each
declaration by *clearing* the other labels made `(x=A | x=A)` reject itself: the
sibling looked like an unlabeled shadow. They are now renamed rather than cleared,
so `declares_label` still exempts them, and the block-arm sibling check exempts
declarations too.
**`seen_terminal` scanned the shared refs vector**, so a sibling branch's terminals
made a later branch's element look non-leading. It now considers only refs on the
current branch's path.
**Two accessor-path over-rejections.** `(A x=A)? EOF` lost `x()` because the prefix
kept the enclosing group's `0..1`: when a ref shares every optional group with the
label, those groups are taken wherever the label is bound, so the quantifier-free
figure applies. And `(A | A A x=A)` lost `x()` because the sibling check rejected on
`max != 0` regardless of position — it now compares the sibling's furthest reachable
occurrence against the selected one.
`AliasDeclarationsInChoice` (`(x=A | x='a')` in one nested choice) still declines.
The same pair as *top-level alternatives* resolves, so this is a gap rather than a
miscompile: the nested form routes through the per-declaration merge, where the
literal's block-mode resolution and the symbolic's token-mode one are compared, and
several attempts at reconciling them each broke a verified case. It is recorded in
the corpus as a decline with that reason so the limitation is visible.
Corpus now 36 cases.
There was a problem hiding this comment.
💡 Codex Review
antlr-rust-runtime/src/bin/antlr4-rust-gen.rs
Lines 9246 to 9249 in 058080f
When one scalar label is declared in every branch of a nested exhaustive choice, taking only the first declaration's branch-relaxed cardinality incorrectly makes the label optional. For r : (x=q | x=q) EOF;, the generated context exposes x() -> Option<QContext> even though every parse assigns x; the corresponding required q() child uses Result<_, MissingChildError>. Aggregate the declarations through their recorded choice ancestry before computing the accessor cardinality so exhaustive labels retain required-child semantics.
AGENTS.md reference: AGENTS.md:L5-L9
ℹ️ 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".
A *forward* block label — `r : A {$x.text} B? x=(C|D) EOF;` — counted `B?` in its
prefix even though neither `B` nor the label has been matched when the action runs.
The optional `B` made the index inexact, so the read fell back to `last()`, which
returned the already-matched `A`.
The block prefix filtered on `on_action_path` (branch reachability) but not
`matched_at_action` (temporal ordering); the token-label path already applied both.
It now applies both, and the read is the correct `nth(1)`.
Pinned as `ForwardBlockLabel`, corpus now 37 cases.
Reported by Codex on #230.
There was a problem hiding this comment.
💡 Codex Review
antlr-rust-runtime/src/bin/antlr4-rust-gen.rs
Lines 9413 to 9418 in 28b7511
When repetition comes from an enclosing group and the scalar label is optional within each iteration, checking only source-level followers does not make .last() faithful. For r : (A x=A?)+ EOF;, parsing aaa binds x to the second token in the first iteration, then leaves it unchanged when the final iteration's optional x is absent; the generated skip(1).last() instead returns the third, unlabeled prefix token. Account for matching refs before the label that can recur in a later iteration, or decline this accessor.
ℹ️ 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".
…its read
A label re-assigned *after* an action still constrained that action's read:
`r : x=A {$x.text} x=A EOF;` rejected a read the parent emitted as `nth(0)`,
because the later declaration coexists with the first and so tripped the
"both present, different positions" guard. At the action's offset only the first
assignment exists.
The declaration filter now also requires `matched_at_action`, and — the part that
made the earlier attempt a no-op — it runs *before* the coexistence guard rather
than after it. Filtering afterwards left the guard reading the unfiltered set.
Pinned as `ReassignedAfterAction`, corpus now 38 cases.
Reported by Codex on #230.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e30862762
ℹ️ 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".
| choice_spans: choice_spans.to_vec(), | ||
| group_spans: group_spans.to_vec(), | ||
| branch_spans: branch_spans.to_vec(), | ||
| leading_terminal: !seen_terminal, |
There was a problem hiding this comment.
Update terminal state before continuing past collapsed blocks
Fresh evidence after the leading_terminal fix is A:'a'; B:'b'; r @after {$x.text}: (B) x=A | x='a';: the collapsed (B) ref is pushed with leading_terminal: true, but the immediate continue bypasses the loop's seen_terminal = true update, so x=A is also classified as leading. The mixed token/literal merge then accepts incompatible coordinate systems and retains terminal_children().nth(0); parsing ba reports b for x instead of a. Mark the collapsed block as terminal-bearing before continuing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in a962bbc.
Root cause was one missing line rather than the merge logic. A token-only group
collapses into a single ElementRef, and that arm continued past the loop's
seen_terminal = true update — so (B) never registered as a terminal and
every element after it was still tagged leading_terminal: true. x=A then
looked like a leading terminal, which made it compare equal-enough to x='a'
(a genuine leading terminal) under same_read_as_element, and the two got
merged despite indexing different coordinate systems: x=A wants
child_tokens(A).nth(i) (same-type index), x='a' wants the block-positional
one.
Fix is in collect_structural_context_refs_with_cardinality: the collapsed
group is a terminal child, so record it before skipping the rest of the body.
$x on your grammar now declines loudly instead of resolving.
Pinned as CollapsedBlockIsTerminal in the label-resolution corpus. Re-verified:
357/357 conformance, Avro IDL +20/−0, Kotlin byte-identical.
… repeated groups
Two findings from Codex, both about a *repeated* group making a count unfixed in a
way the existing checks did not model.
A same-target ref before a list label normally just shifts the `skip`, but when the
two share a repeated group it recurs on every iteration and interleaves with the
labeled children: `(A xs+=A)+` emitted `skip(1)`, so `aaaa` returned three tokens
where only two were bound. No single `skip` can separate the iterations, so this
layout declines. The exclusivity check previously scanned only the suffix, which is
why a *prefix* that recurs went unnoticed.
`GroupSpan` tracked whether a group *relaxes* the lower bound but not whether it can
run more than once. In `((A B)+ x=A {$x.text})?` the outer `?` is satisfied wherever
the action runs, yet the closed inner `+` still contributes a variable number of
preceding `A` children — so treating the prefix as exactly-once emitted `nth(1)` and
read the second prefix `a` instead of the bound token. A `repeated` flag now records
it, and a closed repeated group blocks the exactly-once treatment.
Both added to the corpus, now 40 cases.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39d8b47d23
ℹ️ 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".
…inement per choice
Three label reads could select a child the label never bound.
`(B) x=A | x='a'` — a token-only group collapses into a single ref, and that
arm `continue`d past the loop's `seen_terminal` update. Everything after it was
still marked `leading_terminal`, so the merge across the two declarations
compared a block-positional index against a same-type index and accepted them
as one read. A collapsed group *is* a terminal child; record it as one.
`({false}? x=A | A) (B {$x} | C)` — sibling-branch exclusion asked a rule-wide
`branch_confined` flag whether the action could run with the label unset. Here
the action is confined, but to an *unrelated later* choice, which says nothing
about the earlier one; the unlabeled `A` was exempted and read as `$x`.
Confinement has to be judged per separating choice: dismiss the sibling only
when the action sits inside the label's own branch of the choice that makes the
two mutually exclusive.
`D A x=(B | C) {$x}` on recovered input — a block read picks by position, and
the index comes from the grammar, which knows nothing about error recovery. A
deleted token still occupies a child slot, so `terminal_children()` shifted
every later position and the read returned the extraneous token. Route it
through a new `labeled_terminal_children`, which skips deleted-token errors and
keeps inserted missing ones — the same rule the generated labeled-token
accessors already use (#235), for the same reason: a conjured token is the value
ANTLR assigns the label, an extraneous one corresponds to no grammar element.
All three pinned in the label-resolution corpus. The runtime addition carries a
snapshot contrasting the raw and labeled iterators so the index shift is visible
rather than asserted.
… groups Two ways the accessor path's on-path restriction claimed more than it knew. Both are regressions this PR introduced — `origin/main` declines each shape, because it never resolved these labels at all. `((q | q | b) x=q | c)` — `choice_branch`, `choice_arity`, `choice_spans`, and `branch_spans` are indexed by position, so dropping the taken outer choice from one meant dropping the same position from all four. The code instead `retain`ed the first and `truncate`d the rest, which is only equivalent when the dropped entries form a suffix. Ancestry is stored outermost-first and the taken choices are the outer ones, so the surviving inner choice inherited the outer choice's arity of 2: a three-way choice read as an exhaustive two-way one, prefix count wrongly fixed at 1, and `.nth(1)` returns nothing on the `b q` path. Replaced both call sites — the accessor path and the equivalent one in action resolution — with `ElementRef::retain_choices`, which drops by mask across all four arrays so no call site can drift again. `((A B)+ x=A)?` — sharing every *optional* group with the label proves those groups ran, which is what lets `(A x=A)? EOF` keep its exact count of one. But a *repeated* group inside them ran an unknown number of times, so the count is not fixed and the accessor must decline. `on_taken_group` in the action path already carved this out; the accessor path now does too. Pinned as `closed_repeat_prefix` and `inner_choice_arity` in the multi-alternative-label fixture, snapshotted whole so the absent accessor is visible next to what the context does expose. `(q | q)` under the same shape still resolves, so the arity fix is precise rather than a blanket rejection.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a962bbc169
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d82b73ceb7
ℹ️ 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".
…lock terminals, decline mode-crossing list merges
Three more shapes where a resolved label read selected a child the label never
bound, plus one pre-existing case that emitted non-compiling Rust.
`(A | x=A) {$x}` — the shadow scan only looked at refs *after* the label, but an
impostor before it works just as well: on the first branch the unlabeled `A` is
the only child of its type, so `child_tokens(A).nth(0)` reports it. Now scans
both sides, restricted to non-coexisting refs on the `before` side — a ref the
label coexists with is a genuine prefix and is already folded into `occurrence`.
`(B y=C? | ) x=A | x='a'` — `leading_terminal` was recomputed from
`structural_block_token_types`, which is written for *collapsibility* and returns
empty for any block that is not one-element-per-branch. An expanded block that
did match a terminal therefore reported none, leaving the following element
falsely marked leading, which let the mixed token/literal merge through. Read the
state back off the refs the branches actually emitted instead. `leading_terminal`
is a claim that must hold on every path, so any branch that *can* match a
terminal falsifies it.
`xs+=A | xs+='a'` — the mixed-mode merge is sound because both sides lower to the
same scalar `nth(0)`. A *list* read has no such common form: token mode yields an
iterator, block mode a `String`. Merging them emitted `.collect()` on a `String`.
Restricted the merge to scalars.
That exposed a pre-existing hole on the same root cause: a list label whose
target names neither a rule nor a token type fell through to the block read and
produced the same broken `.collect()` — reachable on `origin/main` too, via
`xs+='a' | B xs+='a'`. It now declines. `xs+=A` forms are unaffected.
All four pinned in the label-resolution corpus.
…n_choices
The previous commit introduced `ElementRef::retain_choices` but converted only
the `action_branches`-keyed call site. Two more, both keying off
`element.choice_branch`, still `retain`ed one array and `truncate`d the rest —
correct only when the dropped entries form a suffix, which they do not when an
*outer* choice is the one dropped.
On `(((y=A | z=A | B) x=A {$x}) | C)` the surviving three-way inner choice
inherited the outer choice's arity of 2, so the `B` branch read as part of an
exhaustive two-way `A` choice, the prefix count was wrongly fixed at 1, and the
action emitted `child_tokens(A).nth(1)` — empty on `ba` even though `x` bound the
first `A`. (`origin/main` emitted `nth(0)` here, also wrong, so this closes a
pre-existing defect rather than a regression.)
Making the same choice genuinely exhaustive still resolves, at the correct
`nth(1)`: both cases pinned in the corpus so the fix reads the real arity rather
than giving up on inner choices.
parse-bench flake on 5dcbe9e — investigated, not a regressionThe first run failed on The runtime is additive only. Every non-generator file changed against The Java lane's generated parser has no parse-path change. It does differ from The failure signature is noise. Three parse-bench failures across 17 runs on
The java fixture is a 50-byte file — Not proposing a threshold change here, since that would be scope creep on a |
Fixes #201. Closes #233.
What the issue asked for vs. what was actually broken
The issue's two headline examples already worked at HEAD — PR #211 landed both
primitiveType.typeName()andmessageDeclaration.name(). Checking the realIdl.g4before writing any code is what found the actual gap: 20 labels across12 rules get no accessor, and neither of the issue's proposed fixes addresses
them, because the defects are in the structural ref model
(
collect_structural_context_refs_with_cardinality), not in the accessorderivation (
context_label_accessor) the issue points at.Cause 1 — group collapse swallowed labels sitting inside the block
A token-only block collapses into a single group ref. That collapse is what makes
x=(A | B)yield one labeled token child, so it has to stay. But it also firedwhen the block was unlabeled and the labels sat inside it:
messageDeclaration: (doc=DocComment)? ... (oneway=Oneway | Throws errors+=identifier ...)? ;Here the collapse produced one ref carrying the block's label (
None) andcontinued — never descending, sodoc,onewayanderrorswere simplydropped. Collapse now requires the label to be on the group itself; otherwise we
descend and let the inner refs carry their own labels.
Cause 2 —
stable_accessorwas masking a dishonest cardinalityRefs inside a multi-alternative block were blanket-marked
stable_accessor: falsewhile still carrying
min: 1— i.e. the model claimed a child is always presentwhen only the taken branch supplies it. The flag papered over the lie instead of
correcting it.
Branch refs now report
min: 0. That makes a sibling alternative matching thesame target read as inexact, so the pre-existing occurrence-lookup guards
(
exact_target_cardinality) reject exactly the layouts where positional accesscould resolve to another branch's child — no new guard logic needed. This is what
lets
name=identifieranderrors+=identifiercoexist on one rule:Follow-ups from review — the same honesty applied to action translation
Making branch refs inexact exposed a latent flaw one layer over, in
TranslationCtx::resolve_label. That function derives a positional CST index bycounting same-target refs ahead of a label, and several shapes make that count
meaningless. ~50 reviewer findings across ~25 rounds (Codex and CodeRabbit),
each verified against the parent commit to separate regressions this PR
introduced from pre-existing gaps it exposed:
r : (B | x=B) {$x}nth(1)on a 1-child CSTr : ((x=A))? EOFx()accessorx()emittedr : (A | B)? x=(C | D) {$x}cannot translate $xr : ({false}? x=A)? A {$x}nth(0)reads the mandatoryAr : errors+=u name=u {$name}nth(0)reads anerrorschildnth(1)r : errors+=u+ name=u {$name}nth(), silentlyr : name=e ... errors+=e {$errors}name's childr : (x=A | x=B) {$x}Aon theBbranchr : x=A | x=B+@after {$x}Alookup, defaults onBr : (xs+=(A | B))+ {$xs}.last()…collect()— does not compiler : (x=A)+ {$x}nth(0)pins the first, not the latestr : (x=A {$x} B | A C)cannot translate $xr : x=(A | B) | x=(C | D)+@aftercannot translate $xr : x=A | A+@after {$x}Ar : xs+=A xs+=B {$xs}A, drops everyBr : ({false}? x=A)? A? {$x}xr : ((x=e {$x} | f) | e)cannot translate $xr @after {$x} : (x=A | A) EOFAr : (xs+=A)? 'a' {$xs}(A : 'a';)xsr : xs+=A {$xs} A;cannot translate $xsr : (x=A e {$x} | x=A f {$x})cannot translate $xr @after {$x} : x=A | x='a';cannot translate $xr : (x=A | A) {$x} EOF;Ar : (a=A | b=A) x=A EOF;x()accessor droppedx()emittedr : (A | B) x=A {$x}r : (e | xs+=e {$xs}) EOF;cannot translate $xsr : ((x=(A | B))) A {$x}last()reads the trailingAnth(0)t=~'x' 'z' {$t.text}onyzlast()readszyr : (A | xs+=A) {$xs} EOF;r @init {$xs} : xs+=A A;cannot translate $xs((a=A | b=B) C | D) x=(E | F) {$x}nth(2), wrong onD Elast()r @after {$x} : (x='a') | B;basxr : (x=A | x=B) EOF;x()droppedx()emitted (merged read)r : (A x=A | B) EOF;x()droppedx()emittedr : (A x=A {$x} | B) EOF;cannot translate $xr : (a=A | ) x=A EOF;x()atnth(1), misses empty branchx()droppedr @after {$x} : A x=A | (A B? | A C?);cannot translate $xr @after {$x} : (B) x=A | x='a';r : ({false}? x=A | A) (B {$x} | C)r : D A x=(B | C) {$x}ondxabxbr : ((A B)+ x=A)? EOF;x()atnth(1), ignores the closed+x()droppedr : ((q | q | b) x=q | c) EOF;x()droppedr : (A | x=A) {$x}r : (B y=C? | ) x=A | x='a';r @after {$xs…collect()} : xs+=A | xs+='a';.collect()on aString— does not compiler @after {$xs…collect()} : xs+='a' | B xs+='a';.collect()(((y=A | z=A | B) x=A {$x}) | C)nth(0)/nth(1), wrong onbaThe unifying fix is that
resolve_labelnow carries the running occurrence asOption<usize>and stops guessing when the position isn't fixed:mutually-exclusive branch refs and unbounded list runs, without special-casing
either;
entirely, since their reads take the last terminal child and never consult an
index — sharing one
""bucket was what made unrelated groups interfere;coexist with them and slide into their position. Cardinality cannot express
this: a sequential optional follower may consume the label's only token, while a
sibling-branch ref reports the same
min: 0yet never coexists.ElementReftherefore carries
choice_branch: (choice id, branch index), socan_coexist_withanswers it structurally instead of by inference. The tag isthe full choice ancestry, and the exclusion applies only to mid-rule bodies —
an
@afterbody runs whichever branch the parse took, so a sibling match reallycan be the child present when its read executes;
choice_spans— the byte extent ofeach enclosing choice block — because ref order cannot distinguish an action
after a group from one inside its last branch;
@initruns before any child exists, so no child-based hazard applies;exact_child_count) that folds nestedchoices innermost-first and takes an explicit
restricted_to_one_pathflag —filtering to a path and demanding cross-branch agreement are contradictory;
token_typesrather than matching sourcespelling, since the read queries by type (
A : 'a';makesAand'a'onechild) — the same rule the typed-accessor path already uses;
to iterate; a repeated single label has no last-occurrence read) stay unresolved
instead of falling through to a different read — one of them was emitting Rust
that does not compile.
Roughly a third end up better than the parent commit rather than merely
restored — the unbounded-list, list-sharing-a-target, nested-grouping,
inner-choice-arity, and literal-list cases were already wrong before this PR, the
last of which generated Rust that does not compile.
Two further structural changes came out of the later rounds and are worth calling
out, because both replace a per-call-site convention with an enforced one:
ElementRef::retain_choices—choice_branch,choice_arity,choice_spans, andbranch_spansare indexed by position, so dropping a choicemust drop the same position from all four. Three call sites instead
retainedone and
truncated the rest, which is equivalent only when the dropped entriesform a suffix — and since ancestry is outermost-first while the dropped
choices are the outer ones, it was wrong in the common case. The invariant now
lives in one method.
ParserRuleContext::labeled_terminal_children(new public runtime API) — agrammar-derived index knows nothing about error recovery. A deleted token
occupies a child slot but corresponds to no grammar element, so counting it
shifts every later position; an inserted token is the value ANTLR binds to the
label and must stay. The new iterator keeps inserted and skips deleted,
discriminating on the synthetic
-1:-1span — the same rule fix(codegen): keep token labels stable through recovery #235's generated__labeled_token_childrenaccessors already apply, so the two label surfacesnow agree.
Correction: the block-label case is fixed, and #233 is closed
An earlier revision of this description said a block label reading across an
intervening terminal was unfixable here, because deciding it needed element spans
that
ElementRefdid not carry. That was wrong.grammar::model::Elementhas carriedspan: SourceSpanall along — it was simplynever propagated into
embedded::ElementRef. Once threaded through, the action'sbyte offset makes every remaining question in this area decidable, and the guesses
that stood in for it stopped trading one correct case for another.
The block-label read is now positional:
terminal_children().nth(i)whereiis the number of terminals matched ahead of the block on the action's parse path.
last()survives only as the fallback when that count is not fixed. So((x=(A | B))) C {$x}reads the token the label bound, andt=~'x' 'z' {$t.text}still works — the two are distinguished rather than traded.Worth noting how invisible the original bug was: the upstream descriptors use
input
zz, so both tokens arezandlast()passed by luck. I confirmed itwith
a : t=~'x' 'z' {$t.text}on inputyz— the action printedzbeforeand prints
ynow.Closes #233.
Design note: I did not take the issue's suggested approach
The issue floated "recording label ids on children in the tree, or deriving from
the alternative's shape at codegen." Recording label ids would grow every
node. Making the cardinality honest instead let the existing guards do the work:
both new shapes resolve with zero runtime cost and zero new tree state.
Verification
antlr4-runtime-testsuiteconformance sweepantlr4-python3-runtimeoracle)cargo clippy --locked --all-targets --all-features -- -D warningsThe accessor-set diff is the check I'd weight most. I built the generator at the
parent commit in a scratch
git worktree, generated the same grammars with bothbinaries, and diffed the emitted
pub fnset per context — proving nothing waslost matters more than counting what was gained. Presence alone is not
evidence of correctness. Both figures were re-confirmed after every commit — 24 of them.
The 357/357 sweep also does real work for the
resolve_labelguard: it exerciseshundreds of embedded actions, so it demonstrates the guard rejects no legitimate
translation.
I also confirmed the new end-to-end tests actually execute: the generated-project
tests run under
cargo testinside a temp crate, so I temporarily broke oneassertion and watched it fail. Without that they could have compiled-but-skipped
and looked green.
Two things that looked like bugs but are correct
docreadsNoneon the unmodifiedIdl.g4.DocCommentis-> channel(HIDDEN), so it never becomes a CST child —Noneis correct ANTLRbehaviour. Verified against a de-hidden copy of the grammar, where
docreadsSome("/** hello doc */").mixed_unbounded(unary* IN errors+=unary) still declines. A variablecount of the label's own target ahead of it leaves no fixed
.skip(N). Pinnedas an explicit negative test so a future change can't silently start emitting an
unsound accessor here.
Tests added
Extended
tests/fixtures/antlr4-rust-gen/multi-alternative-label/T.g4with bothissue shapes plus five hazard shapes, and covered them at three levels:
groupedandmixed(fixed shapes),nested_group(label under redundantgrouping), and the three declines
mixed_unbounded,branch_hazard, andbranch_rival. The declines are snapshotted whole rather than probed with!contains, per the AGENTS.md guidance; reviewing the recorded output is how Iconfirmed they drop only the label accessor and keep the plain
unary_children()/num_token()surface.tests/antlr4_rust_gen_cli.rs) parse real input through thegenerated parser and assert the label reads agree with the text — including both
branches of the choice and the all-optionals-absent case.
resolve_labelhazard separately,one per shape in the table above, including
list_refs_ahead_of_a_single_label_are_counted_then_poison_when_unbounded(covering both the countable and the unbounded ordering).
tests/fixtures/antlr4-rust-gen/label-resolution/,47 grammars) pins every outcome with its reason, as
(fixture, label, resolves, why)rows inlabel_resolution_corpus_matches_expected_outcomes. This is the artefact I'dpoint a future maintainer at: it makes each decline a documented decision rather
than an absence, and it caught two of my own over-corrections during the later
rounds. I mutation-tested it by reverting one fix and confirming it named the
case.
New rules and tokens are appended after the existing ones deliberately: adding
them mid-file renumbers rule/token indices and churns every pre-existing context
snapshot. That ordering keeps the snapshot diff to the two genuinely new files.
Reviewer notes
src/bin/antlr4-rust-gen.rsis the core change: ~15 lines in the block arm ofcollect_structural_context_refs_with_cardinality, plus the newstructural_block_labels_insidehelper.src/bin_support/embedded.rscarries theresolve_labelguard. The rest of the diff is tests, fixture, and snapshots.at lines 1507/1607 and 2589/2656 of
tests/antlr4_rust_gen_cli.rs, well outsidethis diff's 653–770 range. The bot scans whole changed files, so they surface here
without being introduced here.
pre-existing and unrelated, so I left it out of scope rather than widening the PR.
that unlabeled
ElementKind::Rangeelements be accounted for inalt_can_satisfy_read. Its repro (r : x=(A | B) | 'c'..'d';) is rejected byvalidation before reaching the resolver (
error[G4S009]), matching ANTLR's ownerror(181): token ranges not allowed in parser. There is no reachable inputthat needs the guard.
findings" when there were several. My checker fetched only the first page of
review comments and filtered to one bot. Both bugs are fixed; the current script
paginates and covers every bot, and reports 0 unreplied across all 96 threads.