Add parse-bench AST parity gate and fix Java > vs >> precpred#110
Conversation
Parse-bench can now dump Rule/Term/Err trees from rust-antlr and go-antlr and require byte-identical, error-free ASTs before timing (--ast-check). Fix left-recursive loop-enter so multi-token operator prefixes that shadow a lower-precedence single-token op (Java > vs >>) defer to StarLoopEntry adaptive predict instead of forcing enter and failing precpred. CI runs AST parity on kotlin, java, and trino. C# Mono fixture divergence is tracked in #109.
|
Warning Review limit reached
Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThe parser now classifies left-recursive operators by token-prefix length and defers multi-token operators when they shadow lower-precedence single-token operators. Benchmark runners for Rust and Go can emit structured parse-tree dumps. The benchmark script compares dumps, rejects error nodes and mismatches, and exposes this through Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
Copy/Paste DetectionFound 16 duplication(s) across 1 changed Rust file(s) (threshold: 100 tokens). Show duplicationsFound a 27 line (145 tokens) duplication in the following files:
fn generated_match_token_recovers_missing_token_from_context_follow() {
let atn = generated_match_recovery_atn();
let data = RecognizerData::new(
"Mini.g4",
Vocabulary::new(
[None, Some("'X'"), Some("'Y'")],
[None, Some("X"), Some("Y")],
[None::<&str>, None, None],
),
);
let mut parser = BaseParser::new(
CommonTokenStream::new(Source {
tokens: vec![TestToken::eof("parser-test", 3, 1, 3)],
index: 0,
}),
data,
);
parser.rule_context_stack = vec![
RuleContextFrame {
rule_index: 0,
invoking_state: 0,
},
RuleContextFrame {
rule_index: 1,
invoking_state: 1,
},
];Found a 27 line (127 tokens) duplication in the following files:
let mut atn = ParserAtnBuilder::new(2);
assert_eq!(
atn.add_state(AtnStateKind::RuleStart, Some(0))
.expect("state")
.index(),
0
);
assert_eq!(
atn.add_state(AtnStateKind::BlockStart, Some(0))
.expect("state")
.index(),
1
);
assert_eq!(
atn.add_state(AtnStateKind::Basic, Some(0))
.expect("state")
.index(),
2
);
assert_eq!(
atn.add_state(AtnStateKind::Basic, Some(0))
.expect("state")
.index(),
3
);
assert_eq!(
atn.add_state(AtnStateKind::BlockEnd, Some(0))Found a 33 line (126 tokens) duplication in the following files:
if self.fast_parser_predicate_matches(predicate_context, transition, index) {
let boundary = left_recursive_boundary(atn, state, target);
outcomes.extend(
self.recognize_state_fast(
atn,
FastRecognizeRequest {
state_number: target,
stop_state,
index,
rule_start_index,
decision_start_index: next_decision_start_index,
precedence,
depth: depth + 1,
recovery_symbols: Rc::clone(&epsilon_recovery_symbols),
recovery_state: epsilon_recovery_state,
},
FastRecognizeScratch {
predicate_context,
visiting,
memo,
expected,
},
)
.into_iter()
.map(|mut outcome| {
if let Some(rule_index) = boundary {
let boundary = self.arena_boundary_node(rule_index);
self.defer_fast_outcome_node(&mut outcome, boundary);
}
outcome
}),
);
} else {Found a 32 line (125 tokens) duplication in the following files:
let boundary = left_recursive_boundary(atn, state, target);
outcomes.extend(
self.recognize_state_fast(
atn,
FastRecognizeRequest {
state_number: target,
stop_state,
index,
rule_start_index,
decision_start_index: next_decision_start_index,
precedence,
depth: depth + 1,
recovery_symbols: Rc::clone(&epsilon_recovery_symbols),
recovery_state: epsilon_recovery_state,
},
FastRecognizeScratch {
predicate_context,
visiting,
memo,
expected,
},
)
.into_iter()
.map(|mut outcome| {
if let Some(rule_index) = boundary {
let boundary = self.arena_boundary_node(rule_index);
self.defer_fast_outcome_node(&mut outcome, boundary);
}
outcome
}),
);
}Found a 22 line (125 tokens) duplication in the following files:
atn.add_state(AtnStateKind::BlockEnd, Some(0))
.expect("state")
.index(),
4
);
assert_eq!(
atn.add_state(AtnStateKind::RuleStop, Some(0))
.expect("state")
.index(),
5
);
atn.set_rule_to_start_state(vec![0])
.expect("rule start states");
atn.set_rule_to_stop_state(vec![5])
.expect("rule stop states");
atn.add_decision_state(1).expect("decision state");
atn.add_transition(0, ParserTransitionSpec::Epsilon { target: 1 })
.expect("transition");
atn.add_transition(
1,
ParserTransitionSpec::Atom {
target: 2,Found a 33 line (115 tokens) duplication in the following files:
outcomes.extend(
self.recognize_state(
atn,
RecognizeRequest {
state_number: *target,
stop_state,
index,
rule_start_index,
decision_start_index: next_decision_start_index,
init_action_rules,
predicates,
semantics,
rule_args,
member_actions,
return_actions,
local_int_arg,
member_values: member_values.clone(),
return_values: return_values.clone(),
rule_alt_number: next_alt_number,
track_alt_numbers,
consumed_eof,
precedence,
depth: depth + 1,
recovery_symbols: epsilon_recovery_symbols.clone(),
recovery_state: epsilon_recovery_state,
},
visiting,
memo,
expected,
)
.into_iter()
.map(|mut outcome| {
prepend_decision(&mut outcome, decision);Found a 15 line (113 tokens) duplication in the following files:
fn generated_match_token_counts_single_token_deletion_recovery() {
let atn = generated_match_recovery_atn();
let data = RecognizerData::new(
"Mini.g4",
Vocabulary::new(
[None, Some("'X'"), Some("'Y'"), Some("'Z'")],
[None, Some("X"), Some("Y"), Some("Z")],
[None::<&str>, None, None, None],
),
);
let mut parser = BaseParser::new(
CommonTokenStream::new(Source {
tokens: vec![
TestToken::new(3).with_text("z"),
TestToken::new(2).with_text("y"),Found a 12 line (112 tokens) duplication in the following files:
let mut atn = ParserAtnBuilder::new(1);
for (state, kind, rule) in [
(0, AtnStateKind::RuleStart, 0),
(1, AtnStateKind::StarLoopEntry, 0),
(2, AtnStateKind::Basic, 0), // ops hub
(3, AtnStateKind::Basic, 0), // shift prec
(4, AtnStateKind::Basic, 0), // shift first >
(5, AtnStateKind::Basic, 0), // shift second >
(6, AtnStateKind::Basic, 0), // rel prec
(7, AtnStateKind::Basic, 0), // rel >
(8, AtnStateKind::LoopEnd, 0),
(9, AtnStateKind::RuleStop, 0),Found a 22 line (112 tokens) duplication in the following files:
fn predicate_after_token_atn() -> Atn {
let mut atn = ParserAtnBuilder::new(2);
assert_eq!(
atn.add_state(AtnStateKind::RuleStart, Some(0))
.expect("state")
.index(),
0
);
assert_eq!(
atn.add_state(AtnStateKind::Basic, Some(0))
.expect("state")
.index(),
1
);
assert_eq!(
atn.add_state(AtnStateKind::Basic, Some(0))
.expect("state")
.index(),
2
);
assert_eq!(
atn.add_state(AtnStateKind::Basic, Some(0))Found a 22 line (111 tokens) duplication in the following files:
fn left_recursive_loop_with_caller_follow_atn(caller_symbol: i32) -> Atn {
let mut atn = ParserAtnBuilder::new(2);
assert_eq!(
atn.add_state(AtnStateKind::RuleStart, Some(0))
.expect("state")
.index(),
0
);
assert_eq!(
atn.add_state(AtnStateKind::Basic, Some(0))
.expect("state")
.index(),
1
);
assert_eq!(
atn.add_state(AtnStateKind::Basic, Some(0))
.expect("state")
.index(),
2
);
assert_eq!(
atn.add_state(AtnStateKind::RuleStart, Some(1))Found a 14 line (110 tokens) duplication in the following files:
fn parser_matches_token_and_reports_mismatch() {
let source = Source {
tokens: vec![
TestToken::new(1).with_text("x"),
TestToken::eof("parser-test", 1, 1, 1),
],
index: 0,
};
let data = RecognizerData::new(
"Mini.g4",
Vocabulary::new([None, Some("'x'")], [None, Some("X")], [None::<&str>, None]),
);
let mut parser = BaseParser::new(CommonTokenStream::new(source), data);
let matched = parser.match_token(1).expect("token 1 should match");Found a 13 line (109 tokens) duplication in the following files:
fn parser_matches_token_and_reports_mismatch() {
let source = Source {
tokens: vec![
TestToken::new(1).with_text("x"),
TestToken::eof("parser-test", 1, 1, 1),
],
index: 0,
};
let data = RecognizerData::new(
"Mini.g4",
Vocabulary::new([None, Some("'x'")], [None, Some("X")], [None::<&str>, None]),
);
let mut parser = BaseParser::new(CommonTokenStream::new(source), data);Found a 22 line (108 tokens) duplication in the following files:
) -> Option<RecognizeOutcome> {
let (error_index, message) = self.expected_error_message(rule_index, start_index, expected);
let diagnostic = diagnostic_for_token(self.token_at(error_index), message);
let mut next_index = error_index;
loop {
let symbol = self.token_type_at(next_index);
if sync_symbols.contains(&symbol) {
if next_index == error_index {
return None;
}
break;
}
if symbol == TOKEN_EOF {
break;
}
let after = self.consume_index(next_index, symbol);
if after == next_index {
break;
}
next_index = after;
}
let mut nodes = NodeSeqId::EMPTY;Found a 15 line (108 tokens) duplication in the following files:
fn outcome_ties_keep_later_non_recursive_alternative() {
let arena = RecognitionArena::default();
let first = RecognizeOutcome {
index: 1,
consumed_eof: false,
alt_number: 0,
member_values: BTreeMap::new(),
return_values: BTreeMap::new(),
diagnostics: DiagnosticSeqId::EMPTY,
decisions: Vec::new(),
actions: vec![ParserAction::new(1, 0, 0, None)],
nodes: NodeSeqId::EMPTY,
};
let second = RecognizeOutcome {
actions: vec![ParserAction::new(2, 0, 0, None)],Found a 16 line (105 tokens) duplication in the following files:
let start_state = atn.rule_to_start_state().get(rule_index).ok_or_else(|| {
AntlrError::Unsupported(format!("rule {rule_index} has no start state"))
})?;
let stop_state = atn
.rule_to_stop_state()
.get(rule_index)
.filter(|state| *state != usize::MAX)
.ok_or_else(|| {
AntlrError::Unsupported(format!("rule {rule_index} has no stop state"))
})?;
let start_index = self.current_visible_index();
self.clear_prediction_diagnostics();
self.reset_per_parse_caches();
self.reset_recognition_arena();
let caller_follow_state = self.pending_invoking_follow_state(atn);Found a 13 line (100 tokens) duplication in the following files:
let mut expected = BTreeSet::new();
for index in (1..self.rule_context_stack.len()).rev() {
let invoking_state = self.rule_context_stack[index].invoking_state;
let Ok(state_number) = usize::try_from(invoking_state) else {
continue;
};
let Some(Transition::Rule { follow_state, .. }) = atn
.state(state_number)
.and_then(|state| state.transitions().first())
.map(ParserTransition::data)
else {
continue;
}; |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an AST parity verification mechanism between the Rust and Go ANTLR implementations in the parse benchmark tool via a new --ast-check flag. Additionally, it refactors the Rust parser's left-recursive loop enter prediction to correctly handle and defer multi-token prefixes that shadow lower-precedence single-token operators (such as Java's >> vs >). Feedback on the changes suggests simplifying the match arms in state_operator_token_prefix_complete by refactoring the Transition::Precedence arm to use boolean logic instead of a match guard, which also removes a redundant pattern from the catch-all arm.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| Transition::Precedence { | ||
| target, | ||
| precedence: transition_precedence, | ||
| } if *transition_precedence >= precedence => { | ||
| state_operator_token_prefix_complete(atn, *target, precedence, visited) | ||
| } | ||
| Transition::Predicate { target, .. } => { | ||
| state_operator_token_prefix_complete(atn, *target, precedence, visited) | ||
| } | ||
| Transition::Atom { .. } | ||
| | Transition::Range { .. } | ||
| | Transition::Set { .. } | ||
| | Transition::NotSet { .. } | ||
| | Transition::Wildcard { .. } | ||
| | Transition::Precedence { .. } => false, |
There was a problem hiding this comment.
The Transition::Precedence match arm can be simplified by removing the guard and using boolean logic inside the arm. This allows us to remove the redundant Transition::Precedence pattern from the catch-all arm, making the code more idiomatic and maintainable.
| Transition::Precedence { | |
| target, | |
| precedence: transition_precedence, | |
| } if *transition_precedence >= precedence => { | |
| state_operator_token_prefix_complete(atn, *target, precedence, visited) | |
| } | |
| Transition::Predicate { target, .. } => { | |
| state_operator_token_prefix_complete(atn, *target, precedence, visited) | |
| } | |
| Transition::Atom { .. } | |
| | Transition::Range { .. } | |
| | Transition::Set { .. } | |
| | Transition::NotSet { .. } | |
| | Transition::Wildcard { .. } | |
| | Transition::Precedence { .. } => false, | |
| Transition::Precedence { | |
| target, | |
| precedence: transition_precedence, | |
| } => { | |
| *transition_precedence >= precedence | |
| && state_operator_token_prefix_complete(atn, *target, precedence, visited) | |
| } | |
| Transition::Predicate { target, .. } => { | |
| state_operator_token_prefix_complete(atn, *target, precedence, visited) | |
| } | |
| Transition::Atom { .. } | |
| | Transition::Range { .. } | |
| | Transition::Set { .. } | |
| | Transition::NotSet { .. } | |
| | Transition::Wildcard { .. } => false, |
There was a problem hiding this comment.
Addressed in 8f09dfc: the precedence arm now uses the requested boolean condition and the redundant fallback pattern is removed. Validation passed with cargo +1.97.1 clippy --locked --all-targets --all-features -- -D warnings, cargo +1.97.1 test --locked, cargo test --locked --lib left_recursive_loop, and python3 tools/parse-bench/test_run.py.
|
@codex review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/parser.rs (1)
2864-2871: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy liftPreserve rule continuations and independent reachability flags.
Transition::Rule => truecan classify a token matched inside a helper rule as complete even when its caller still requires operator tokens. Predicates after the first token are also treated as unconditional, whilemaxdiscards predicate-dependent reachability when a multi-token path exists. These cases can incorrectly returnSome(true)and bypass the adaptive exit alternative.Carry rule-return continuations and represent single-token, multi-token, and predicate-dependent results as combinable flags. Add coverage for a rule-wrapped prefix and a predicate-gated single-token alternative sharing a prefix with a multi-token alternative.
As per coding guidelines, “Model behavior from ANTLR metadata and ATN structure instead” and keep runtime modules grammar-agnostic.
Also applies to: 3019-3063, 3085-3100, 3121-3122, 3187-3190, 3229-3236, 5371-5389
🤖 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/parser.rs` around lines 2864 - 2871, Update the operator reachability analysis around OperatorSymbolReachability and its Transition::Rule handling to preserve caller continuations, propagate predicate-dependent status beyond the first token, and combine independent single-token, multi-token, and predicate-dependent flags instead of letting max discard them. Base decisions on ANTLR metadata and ATN structure without grammar-specific logic, ensuring rule-wrapped prefixes and predicate-gated single-token alternatives sharing a multi-token prefix do not incorrectly return Some(true) or bypass the adaptive exit alternative; add coverage for both cases.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 `@tools/parse-bench/run.py`:
- Around line 575-583: Unify tree-dump escaping by making the Rust Terminal and
Error formatting paths use the same encoding contract as rustDebugStr(),
including Unicode cases such as leading combining marks. Update rustDebugStr()
or the Rust formatter as needed so equivalent trees produce byte-for-byte
identical output, and add coverage for these Unicode escape cases across both
dumpers.
- Around line 685-697: Update the generated dump-tree paths around
dump_tree_{spec.name} and the corresponding logic near the second reported
location to inspect lexer diagnostics as well as parser syntax errors. Capture
the lexer’s error count from the generated lexer or token stream, and return an
error whenever either lexer errors or parser syntax errors are nonzero,
preserving the existing diagnostic failure behavior.
---
Outside diff comments:
In `@src/parser.rs`:
- Around line 2864-2871: Update the operator reachability analysis around
OperatorSymbolReachability and its Transition::Rule handling to preserve caller
continuations, propagate predicate-dependent status beyond the first token, and
combine independent single-token, multi-token, and predicate-dependent flags
instead of letting max discard them. Base decisions on ANTLR metadata and ATN
structure without grammar-specific logic, ensuring rule-wrapped prefixes and
predicate-gated single-token alternatives sharing a multi-token prefix do not
incorrectly return Some(true) or bypass the adaptive exit alternative; add
coverage for both cases.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ed2eb61f-486d-4f4e-be3b-cd6b2b7b7d0b
📒 Files selected for processing (6)
.github/workflows/parse-bench.ymlREADME.mdsrc/parser.rstools/parse-bench/README.mdtools/parse-bench/run.pytools/parse-bench/test_run.py
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f09dfcb40
ℹ️ 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".
| if r < 0x20 || r == 0x7f {{ | ||
| fmt.Fprintf(&b, `\\u{{%x}}`, r) | ||
| }} else {{ | ||
| b.WriteRune(r) |
There was a problem hiding this comment.
When a fixture contains non-printing Unicode in token text, such as U+00A0 or U+2028 in a comment/string/identifier, the Rust dump uses {:?} and emits \u{...}, but this Go helper writes the raw rune because it only escapes C0 controls and DEL. That makes --ast-check report a byte-level Rust/Go AST diff even though the parse trees and token text are equivalent, so the Go formatter needs to mirror Rust's string debug escaping more completely.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 396465f. Rather than trying to track Rust debug-printability tables, both generated runners now encode every scalar as lowercase \u{...}, so valid Unicode including leading combining marks, NBSP, and U+2028 is byte-stable across languages. The generated-source regression and all 13 Kotlin/Java/Trino AST parity fixtures pass.
|
Addressed CodeRabbit’s outside-diff operator-reachability finding in The analysis now carries helper-rule return continuations after a matched prefix token, combines unconditional single-token, unconditional multi-token, and predicate-dependent reachability independently, and conservatively delegates recursive helper cycles to adaptive prediction. Added focused regressions for a rule-wrapped Validation on this head:
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 396465f4c5
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
--ast-check: before timing, dump each fixture’s parse tree fromrust-antlrandgo-antlrin the Kotlin-parityRule/Term/Errformat; require byte-identical dumps and no error nodes (Rust also rejects non-zeronumber_of_syntax_errors).precpred(_ctx, 11)failures on constructs likeListenableFuture<LookupResult>inside switch cases: left-recursive loop-enter no longer forces enter on multi-token operator prefixes that shadow a lower-precedence single-token op (Java>vs>>/>>>).kotlin,java,trino. C# Mono divergence tracked in parse-bench: investigate Rust vs Go AST divergence on C# Mono fixtures #109.Root cause (Java)
At RHS precedence of
<, one-token lookahead treated>as unconditional loop-enter via shift (>>). Operator selection then picked relational>,precpred(11)failed, and the parse reported syntax errors. Go’s StarLoopEntry adaptive predict exits instead, so the outer loop takes relational correctly.Fix keeps the single-token enter fast path (
+,*,., low-prec>, …) and only defers when the current precedence is multi-token-only for a symbol that is single-token at precedence 0.Test plan
cargo test --lib left_recursive_loop(includes new shadowing unit test)cargo test --lib parser::testspython3 tools/parse-bench/test_run.pybazel-sky-value-retriever.java: Rust dump matches Go, 0 error nodestools/parse-bench/fixtures/java/*.javadump clean under rust-antlrkotlindump still clean after the loop-enter changeFollow-up