fix: restore fast Java parsing with typed contexts - #175
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe fast recognizer now propagates alternative metadata through deferred trees and CST construction. Benchmark tooling labels runtime grammar variants, adds a Java parse fixture, and compares only matching baseline/current variants. ChangesParser alternative tracking
Benchmark grammar and comparison variants
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
@codex review |
Copy/Paste DetectionFound 14 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,
},
];
```rust
---
Found a 27 line (127 tokens) duplication in the following files:
* Starting at line 14316 of src/parser.rs
* Starting at line 14388 of src/parser.rs
```rust
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 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,
```rust
---
Found a 34 line (119 tokens) duplication in the following files:
* Starting at line 9748 of src/parser.rs
* Starting at line 9823 of src/parser.rs
```rust
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,
committed_decision: transition_committed,
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"),
```rust
---
Found a 12 line (112 tokens) duplication in the following files:
* Starting at line 13297 of src/parser.rs
* Starting at line 13380 of src/parser.rs
```rust
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))
```rust
---
Found a 22 line (111 tokens) duplication in the following files:
* Starting at line 13119 of src/parser.rs
* Starting at line 14753 of src/parser.rs
* Starting at line 14954 of src/parser.rs
```rust
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");
```rust
---
Found a 13 line (109 tokens) duplication in the following files:
* Starting at line 15087 of src/parser.rs
* Starting at line 17868 of src/parser.rs
```rust
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;
```rust
---
Found a 15 line (108 tokens) duplication in the following files:
* Starting at line 18134 of src/parser.rs
* Starting at line 18158 of src/parser.rs
```rust
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);
```rust
---
Found a 13 line (100 tokens) duplication in the following files:
* Starting at line 6414 of src/parser.rs
* Starting at line 6438 of src/parser.rs
```rust
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;
}; |
|
Claude finished @tinovyatkin's task in 14m 44s —— View job Code review — PR #175Verdict: no blocking issues found. I ran an independent review (2 bug-focused passes + 2 CLAUDE.md-compliance passes over Review checklist
✅ Previously-flagged issues — both verified fixed1. Operator alt markers past LR boundaries (Codex P2 on 2. Variant-only comparison should skip, not fail (Codex P2 on ✅ CLAUDE.md compliance — clean
🟡 One non-blocking observation (could not confirm as a defect)In the repetition path, the enter/exit loop-decision alternative is appended ( ℹ️ InformationalCodecov reports 92.07% patch coverage (28 uncovered lines in Nice, surgical fix — the eligibility gate was the right thing to relax, and the deferred-rope ordering is carefully done. |
📊 Source Code Metrics (this PR vs
|
| File | Cyclomatic | Cognitive | Functions | LLOC | MI |
|---|---|---|---|---|---|
| src/parser.rs | 2099 (main: 2067) 🔴 | 1400 (main: 1367) 🔴 | 654 (main: 645) 🔴 | 4583 (main: 4482) 🔴 | 0 ⚪ |
Generated by mehen v1.6.0 — the code quality watcher.
|
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". |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
tinovyatkin
changed the title
|
@codex review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@codex review |
![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: 45d9d7245c
ℹ️ 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. Keep it up! 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". |
…uting change The untouched-JavaParser.g4 lane previously ran the two predicate-bearing rules (and, via the caller cascade, most of the grammar) through the ATN interpreter. With untranslated predicates now lowering as generatable templates, that lane routes through generated rule bodies — a methodology change, not a runtime regression, and the per-fixture deltas are mixed by design (mojang 6.4->3.2 ms, google-closure 1.9->0.9 ms, but bazel-sky-value-retriever 10.4->19.0 ms on the CI runner, where the generated walker loses to the warmed interpreter DFA for that fixture's decision mix; the same shape exists on main between the portable and interpreted lanes). Bump JAVA_RUST_PREDICATE_VARIANT to v2 so the comparator skips the mismatched-variant rows and re-arms the 1.15x Java regression gate once both reports carry v2 — the same reset the v1 tag performed for the legacy-to-predicate transition in #175.
…known templates (#218) * perf(codegen): lower untranslated parser predicates as generatable Unknown templates An untranslated predicate body (e.g. a bare this.IsNotIdentifierAssign() helper call) previously produced no PredicateTemplate, leaving its coordinate out of the generated set. compile_generated_parser_transition then refused to compile the containing rule, and with require_generated_callees active the drop cascaded through drop_rules_calling_disabled_rules to every calling rule — the untouched grammars-v4 JavaParser.g4 kept only 15 of 129 generated rule bodies and routed everything else through the interpreter, 5-6x slower than the predicate-stripped portable grammar (issue #209). Lower such coordinates as a new PredicateTemplate::Unknown instead, mirroring UnknownWithFailMessage: SemIR PExpr::Hook(0), so evaluation keeps the documented hook -> unknown-policy chain. Typed/closure hooks stay consulted, --sem-unknown dispositions and --require-full-semantics behave unchanged, the manifest still reports disposition assume-true with template null, and a dispose="error" coordinate override still lowers to no SemIR entry. Untouched JavaParser.g4 now generates all 129 rule dispatch bodies; parse times match the lit-true ({ true }?) build within noise: mojang-data-result.java 14.7 -> 3.1 ms, google-closure-property.java 4.1 -> 0.9 ms (portable baseline 2.2 / 0.6 ms; the small residual is allow_semantic_context adaptive prediction at the two predicate-bearing decisions). Fixes #209 * test(codegen): snapshot the untranslated-predicate template collection Swap the hand-written assert_eq! in untranslated_parser_predicate_keeps_generated_rule for a named insta snapshot per the repository snapshot guidance (pinning a collection's full contents is a value test, not a property test). Addresses the Codex review comment on PR #218. * ci(parse-bench): bump the Java Rust benchmark variant for the #209 routing change The untouched-JavaParser.g4 lane previously ran the two predicate-bearing rules (and, via the caller cascade, most of the grammar) through the ATN interpreter. With untranslated predicates now lowering as generatable templates, that lane routes through generated rule bodies — a methodology change, not a runtime regression, and the per-fixture deltas are mixed by design (mojang 6.4->3.2 ms, google-closure 1.9->0.9 ms, but bazel-sky-value-retriever 10.4->19.0 ms on the CI runner, where the generated walker loses to the warmed interpreter DFA for that fixture's decision mix; the same shape exists on main between the portable and interpreted lanes). Bump JAVA_RUST_PREDICATE_VARIANT to v2 so the comparator skips the mismatched-variant rows and re-arms the 1.15x Java regression gate once both reports carry v2 — the same reset the v1 tag performed for the legacy-to-predicate transition in #175.
Fixes #174
Summary
JavaParser.g4; Python and Go retain their portable rewrite.Root cause
Typed traversal generation made Java request
track_context_alt_numbers: true. The runtime treated that private tree metadata as incompatible with fast recognition, routing Java expression rules through the exponentially slower general recognizer. The generated parser ATN is unchanged; the regression was the runtime eligibility gate introduced with typed traversal.The existing Java benchmark did not expose that route because it removed
JavaParserBaseand rewrote both semantic-predicate sites before Rust generation. The Rust lane now keeps the grammar unchanged, retaining those predicate coordinates and the interpreted-rule shape used by downstream consumers.Impact
On
class C { int m() { return 1; } }, currentmaintakes about 5.36 seconds under the corrected benchmark and then stack-overflows on the first larger Java fixture. This branch parses the minimal fixture in 0.08-0.19 ms and completes every existing Java fixture. Rust/Go parse-tree dumps remain byte-identical for all Java fixtures.Validation
cargo clippy --locked --all-targets --all-features -- -D warningscargo test --locked --all-targets --all-featurespython3 -m unittest tools/parse-bench/test_run.py(13 tests)origin/main: 5.36 seconds for the 50-byte issue fixture, then stack overflow on the first larger Java fixtureSummary by CodeRabbit
New Features
Bug Fixes
Benchmarking