feat(runtime): add_parse_listener — parse-time rule enter/exit events (ANTLR parity) - #204
Conversation
ErrorListener::syntax_error now receives offending: Option<TokenView> right after the recognizer, matching ANTLR's canonical syntaxError(recognizer, offendingSymbol, ...) contract. Every reference runtime passes the offending symbol; span-building error reporters (e.g. avdl's miette diagnostics with byte-offset underlines) need start_byte/stop_byte from the token, not just (line, column). ParserDiagnostic records the anchoring TokenId at each creation site (diagnostic_for_token already had the token in hand; the extraneous/ missing recovery paths record the current token) and dispatch resolves it to a TokenView from the token store. Lexer-originated diagnostics pass None, matching ANTLR's null offendingSymbol for lexer errors. Fixes #195
Review on #196 found the offending-token contract held only for extraneous-input/missing-token recovery and prediction diagnostics: generated parsers route ordinary mismatched-input, no-viable-alternative, failed-predicate, and sync errors through AntlrError::ParserError, whose diagnostic arm hard-coded offending: None — exactly the errors a real recognizer reports most. ParserError gains offending: Option<TokenId>, recorded where each error is built (recover_generated_match, generated sync, failed-predicate builders, recognition_error) rather than resolved at reporting time: prediction restores the input cursor, so lt(1) at dispatch can point at the decision start instead of the error index — no_viable_alternative_error_at now forwards the anchor its diagnostic already computed. Also converts the new listener test to a named insta snapshot per house style.
The depth-cap violation error now carries its offending token like every other ParserError built at a known input position.
ANTLR's addParseListener delivers enterEveryRule/exitEveryRule during recognition; cel-rust's RecursionListener (live expr-depth counting with parse abort) is built on it. Our runtime only had post-parse listeners, forcing ports to rework such listeners into post-parse tree walks. ParseListener (enter fallible for aborts, exit infallible) dispatches from generated rule bodies: enter before the body after the depth-cap probe, exit on every exit path, one simulated enter per left-recursive operator expansion (upstream Parser.pushNewRecursionContext fires triggerEnterRuleEvent) with matching exits as the rule unrolls — pairs always balance. A listener abort is sticky through rule-level recovery, drained at the top-level entry via take_parse_abort() (unified with the depth-cap violation, depth error preferred), and cleared at entry so instances never poison the next parse. Costs nothing when unused: dispatch sites gate on list emptiness (one predictable branch; Kotlin parse timings unchanged, ktor parse-bench fixture at baseline). When a listener is registered, generated dispatch routes ATN-preferred rules through their generated bodies so real grammars observe every rule; interpreter-only rules do not fire events (documented divergence, matching the depth cap). Emitted probes are plain if-let — generated output stays edition-2021 clean. e2e: cel-rust's RecursionListener ported verbatim in the deep-nesting fixture test — live counting, positioned abort error, LR expansion entries counted, clean reuse after abort. Closes #202
|
Warning Review limit reached
Next review available in: 21 seconds 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe runtime adds parse-time listener registration and rule callbacks, supports listener-triggered parse aborts, and propagates offending tokens through parser diagnostics and error listeners. Generated parsers expose the listener API and updated abort handling, with expanded unit and end-to-end coverage. ChangesParse listener and diagnostic plumbing
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant GeneratedParser
participant BaseParser
participant ParseListener
participant ErrorListener
GeneratedParser->>BaseParser: enter rule
BaseParser->>ParseListener: enter_every_rule
ParseListener-->>BaseParser: success or AntlrError
GeneratedParser->>BaseParser: execute and exit rule
BaseParser->>ParseListener: exit_every_rule
BaseParser->>ErrorListener: notify_error_listeners(offending token)
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
📊 Source Code Metrics (this PR vs
|
| File | Cyclomatic | Cognitive | Functions | LLOC | MI |
|---|---|---|---|---|---|
| src/parser.rs | 2147 (main: 2119) 🔴 | 1418 (main: 1409) 🔴 | 681 (main: 665) 🔴 | 4662 (main: 4623) 🔴 | 0 ⚪ |
| src/bin/antlr4-rust-gen.rs | 2250 (main: 2249) 🔴 | 1415 ⚪ | 499 (main: 498) 🔴 | 3944 (main: 3940) 🔴 | 0 ⚪ |
| src/lib.rs | 4 ⚪ | 3 ⚪ | 1 ⚪ | 7 ⚪ | 32.66 (main: 32.81) 🔴 |
Generated by mehen v1.7.0 — the code quality watcher.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![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: d259246bc1
ℹ️ 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 let Some(error) = self.base.parse_listener_enter_rule({index}) {{\n \ | ||
| return Err(GeneratedRuleError::Fatal(error));\n \ |
There was a problem hiding this comment.
When enter_every_rule mutates listener state before returning Err—as the recursion-depth listener added in this commit does—this immediate return bypasses the matching exit call below. BaseParser::reset clears the sticky abort but retains registered listeners, so reusing the parser without removing that listener leaves its depth elevated and can reject otherwise valid input; ensure callbacks invoked during the failed enter are appropriately unwound before propagating the error.
Useful? React with 👍 / 👎.
| for slot in &mut self.parse_listeners { | ||
| slot.0.exit_every_rule(rule_index); |
There was a problem hiding this comment.
When multiple listeners are registered, ANTLR unwinds them in reverse registration order: entries run A then B, while exits run B then A. Iterating forward here instead emits A then B on exit, which breaks the advertised ANTLR parity and produces incorrectly nested events for listeners sharing state; iterate the listener slots in reverse for exits.
Useful? React with 👍 / 👎.
| /// Receives committed rule enter/exit events during recognition, matching | ||
| /// ANTLR's `addParseListener` contract ([`Parser::add_parse_listener`]). |
There was a problem hiding this comment.
The new public documentation advertises Parser::add_parse_listener, but the Parser trait has no such associated method (the link is unresolved under cargo doc); registration exists only as an inherent method on BaseParser and generated concrete parsers. Consequently, code generic over P: Parser—the runtime's normal abstraction for controls such as prediction mode and maximum rule depth—cannot use this feature at all, so add registration/removal to the trait and forward them from generated implementations.
Useful? React with 👍 / 👎.
|
Local conformance sweep on this branch: 357 passed, 0 failed, 0 skipped. No descriptor registers a parse listener, so the sweep pins the unused-path invariant: emptiness-gated dispatch changes nothing when no listener is registered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@tests/antlr4_rust_gen_cli.rs`:
- Around line 1799-1817: Strengthen the left-recursive listener test around
RecursionListener by storing depth in a shared Arc<AtomicU16>, like high_water,
so it can be inspected after parsing. Add a successful operator-chain case that
remains below the recursion limit, then assert the shared depth returns to 0
after parser.s() completes; retain the existing over-limit abort assertion.
🪄 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: a45b8528-349f-40a9-b567-5f24025e9504
⛔ Files ignored due to path filters (6)
src/bin_support/grammar/generated/antlr_v4_lexer.rsis excluded by!**/generated/**src/snapshots/antlr4_runtime__parser__tests__generated_prediction_diagnostics_use_adaptive_context.snapis excluded by!**/*.snapsrc/snapshots/antlr4_runtime__parser__tests__parser_dispatches_recovery_diagnostics_through_registered_listeners.snapis excluded by!**/*.snapsrc/snapshots/antlr4_runtime__parser__tests__recovery_diagnostics_expose_the_offending_token_to_listeners.snapis excluded by!**/*.snapsrc/snapshots/antlr4_runtime__recognizer__tests__recognizers_replace_the_default_console_error_listener.snapis excluded by!**/*.snapsrc/xpath/generated/x_path_lexer.rsis excluded by!**/generated/**
📒 Files selected for processing (8)
src/bin/antlr4-runtime-testsuite.rssrc/bin/antlr4-rust-gen.rssrc/bin_support/grammar/frontend.rssrc/errors.rssrc/lib.rssrc/parser.rssrc/recognizer.rstests/antlr4_rust_gen_cli.rs
Copy/Paste DetectionFound 15 duplication(s) across 4 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 22 line (132 tokens) duplication in the following files:
* Starting at line 1100 of tests/antlr4_rust_gen_cli.rs
* Starting at line 1200 of tests/antlr4_rust_gen_cli.rs
```rust
"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}");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))
```rust
---
Found a 22 line (125 tokens) duplication in the following files:
* Starting at line 14853 of src/parser.rs
* Starting at line 14925 of src/parser.rs
```rust
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 34 line (119 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,
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);
```rust
---
Found a 15 line (113 tokens) duplication in the following files:
* Starting at line 15928 of src/parser.rs
* Starting at line 16206 of src/parser.rs
```rust
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),
```rust
---
Found a 22 line (112 tokens) duplication in the following files:
* Starting at line 15264 of src/parser.rs
* Starting at line 15465 of src/parser.rs
```rust
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))
```rust
---
Found a 14 line (110 tokens) duplication in the following files:
* Starting at line 15598 of src/parser.rs
* Starting at line 18367 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);
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);
```rust
---
Found a 22 line (108 tokens) duplication in the following files:
* Starting at line 7993 of src/parser.rs
* Starting at line 8384 of src/parser.rs
```rust
) -> 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)],
```rust
---
Found a 16 line (105 tokens) duplication in the following files:
* Starting at line 7130 of src/parser.rs
* Starting at line 7751 of src/parser.rs
```rust
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;
};
```rust
</details> |
1. Reserved-name collision: add_parse_listener/remove_parse_listeners join GENERATED_PARSER_RESERVED_RULE_METHODS so a grammar rule named addParseListener renames to add_parse_listener_rule instead of colliding with the facade (E0592). 2. Exit events now fire in reverse registration order, matching upstream Parser.triggerExitRuleEvent; new e2e test registers two tracing listeners and pins enter A,B / exit B,A plus pair balance on both the success and recovery paths. 3. Doc correction: the aborting enter_every_rule receives no matching exit (same as Java, where enterRule throws before try/finally); listener state shared across parses must be reset after an abort. 4. remove_parse_listeners clears the sticky abort and returns the boxed listeners, giving callers their accumulated state back without external shared handles. 5. enter_every_rule takes #[non_exhaustive] EnterRuleEvent so future fields extend the event without breaking implementors; the dispatch helper loses its #[cold] (it is the hot path once registered). 6. New coverage: multi-listener order, recovery balance, depth-cap + listener coexistence (either bound trips first and surfaces). Measured with-listener cost (finding 7): on the CEL grammar — the migration target, no ATN-preferred rules — a counting listener adds ~5% (0.0433 -> 0.0453 ms on cel-rust's criterion stress expression). On grammars with ATN-preferred rules the dominant cost is the routing override those rules take (Kotlin ktor fixture 10.2 -> 86 ms), shared byte-for-byte with the depth cap (cap-only run measures identically); listener dispatch on top of it is noise. Documented on the trait.
|
Review findings addressed in 4155572:
Local verification at 4155572: conformance 357/357, 1075 tests green, clippy |
1. Parser trait gains add_parse_listener(Box<dyn ParseListener>) / remove_parse_listeners() (mirroring set_max_rule_depth), fixing the broken intra-doc link and letting code generic over P: Parser reach listener registration. 2. LR-expansion aborts now match Java exactly: the listener enter probe fires AFTER push_new_recursion_context_with_previous (upstream assigns _ctx before triggerEnterRuleEvent), so an aborting expansion is already counted and the unroll fires its matching exit — Java parity via the finally-driven unrollRecursionContexts. Ordinary-rule aborts keep the documented no-exit behavior (also Java parity). Trait doc states both cases precisely. 3. Success-path LR coverage: a+a+a+a under a tracing listener pins 7 RULE_EXPR enters (1 dispatch + 3 expansions + 3 operands) and full enter/exit balance through the unroll loop. 4. Stale ATN-preferred routing comment updated for the listener guard.
|
Round-2 findings addressed in e87fb3a: (1) Local verification at e87fb3a: conformance 357/357 (note per earlier finding: the sweep exercises only the no-listener path — smoke crates never register one; the listener paths are covered by the e2e and generator unit tests), 1075 tests green, clippy clean, |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e87fb3a26c
ℹ️ 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".
| /// Registers a listener for committed rule enter/exit events during | ||
| /// recognition (ANTLR's `addParseListener`). See [`ParseListener`] for | ||
| /// the delivery contract. The default implementation drops the listener; | ||
| /// [`BaseParser`] and generated parsers deliver events. | ||
| fn add_parse_listener(&mut self, _listener: Box<dyn ParseListener>) {} |
There was a problem hiding this comment.
When registration is performed through a generic P: Parser, this default silently drops the listener (and the matching removal default returns an empty list). Fresh evidence beyond the earlier comment is that the current generated impl Parser for {type_name} in src/bin/antlr4-rust-gen.rs forwards settings only through set_max_rule_depth and still does not override either new listener method, so generated parsers inherit these no-ops despite the documentation saying they deliver events. Add generated forwarding implementations to self.base so generic callers receive the same behavior as the inherent facade.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
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)
175-188: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReconsider the
Sendbound onParseListener.
BaseParseris already!Send/!Syncby design (itsRc-backed caches), so requiring listener implementors to beSendbuys no actual thread-safety for the containing parser — it only forecloses lightweight listeners built onRc<RefCell<..>>, which fits this crate's stated single-threaded design better thanArc-based state.♻️ Suggested relaxation
-pub trait ParseListener: Send { +pub trait ParseListener {Based on learnings, "keep the ANTLR runtime intentionally single-threaded... do not recommend changing these Rc-backed fields to Arc," which suggests the crate's design should avoid unnecessary Send/thread-safety requirements where they don't correspond to a real cross-thread use case.
🤖 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 175 - 188, Remove the unnecessary Send supertrait from ParseListener so listeners can use single-threaded state such as Rc<RefCell<_>>. Keep the existing enter_every_rule and exit_every_rule method signatures and behavior unchanged.Source: Learnings
🤖 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/parser.rs`:
- Around line 5976-5985: Implement a blanket ParseListener implementation for
Box<T> where T: ParseListener + ?Sized, forwarding the trait methods to the
wrapped listener. This allows listeners returned by
BaseParser::remove_parse_listeners to be passed through the inherent generic
BaseParser::add_parse_listener method using normal dot-call syntax, without
changing either registration API.
---
Outside diff comments:
In `@src/parser.rs`:
- Around line 175-188: Remove the unnecessary Send supertrait from ParseListener
so listeners can use single-threaded state such as Rc<RefCell<_>>. Keep the
existing enter_every_rule and exit_every_rule method signatures and behavior
unchanged.
🪄 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: a6164992-1790-4471-915b-389594ebcfac
📒 Files selected for processing (4)
src/bin/antlr4-rust-gen.rssrc/lib.rssrc/parser.rstests/antlr4_rust_gen_cli.rs
CodeRabbit round 3: Box<dyn ParseListener> returned by remove_parse_listeners could not be passed back to the inherent add_parse_listener (Box did not implement ParseListener). Add the forwarding impl for Box<T: ParseListener + ?Sized> and pin the round-trip in the e2e test — a removed listener re-registers with its accumulated state and still enforces its limit. Also cover the left-recursive success path: one listener instance parses the same under-limit operator chain twice; identical high-water marks prove the live depth counter returned to zero after the first LR unroll (balanced enter/exit through expansions).
|
Round-3 findings addressed in 55cd624:
Local: unit tests green, clippy |
|
Conformance sweep on 55cd624 (round-3 fixes): 357 passed, 0 failed, 0 skipped. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55cd6245be
ℹ️ 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 let Some(error) = self.base.parse_listener_enter_rule({index}) {{\n \ | ||
| return Err(GeneratedRuleError::Fatal(error));\n \ |
There was a problem hiding this comment.
When enter_every_rule returns Err from a nested generated rule or left-recursive expansion, converting it to the same GeneratedRuleError::Fatal used for ordinary recognition failures lets the parent rule catch it and call recover_generated_rule. That recovery increments number_of_syntax_errors, may consume the remaining input and run recovery/after actions, and queues a false syntax diagnostic that report_generated_parser_diagnostics dispatches before the sticky abort is checked. A listener used as a resource bound therefore does not abort cleanly or promptly; propagate listener-requested aborts outside normal rule recovery.
Useful? React with 👍 / 👎.
Round-4 review built a Java oracle (ANTLR 4.13.2 on this PR's own Nest.g4) and caught that batching expansion exits at unroll diverges from upstream on live depth: recRuleSetPrevCtx fires triggerExitRuleEvent at the TOP of each operator-loop pass, so the outgoing iteration exits before the next expansion enters and flat chains never accumulate depth (a+a+...+a peaks at 2 in every ANTLR target; ours peaked at chain width). The motivating cel-rust RecursionListener would have rejected wide flat CEL expressions every other target accepts. The generated operator loop now exits the outgoing iteration first, and unroll_recursion_context drops the batched exits (upstream's unrollRecursionContexts walks exactly one link — the dispatch wrapper's single exit plays it). Event counts unchanged; timing now matches the oracle: 7/7 depth 2, 79/79 depth 2, bracket cases unchanged. The e2e that encoded the divergence now pins the Java-oracle depth of 2 for a 40-term chain. Also from review: generated impl Parser blocks forward add_parse_listener/remove_parse_listeners (they inherited the no-op trait defaults — generic registration silently dropped listeners and trait-removal left them firing with the sticky abort uncleared), and the trait doc gains the expansion-anchor note (EnterRuleEvent::current is the operator-side lookahead, not Java's whole-expression start).
|
Round-4 findings addressed in 3e4aaa9:
Local at 3e4aaa9: conformance 357/357, 1075 tests green, clippy clean. (The latest claude-review run died on an infra error at 3m26s — rerun triggered.) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e4aaa9db2
ℹ️ 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".
| // one-link walk when the rule finishes. | ||
| writeln!( | ||
| out, | ||
| "{pad} self.base.parse_listener_exit_rule({rule_index});" |
There was a problem hiding this comment.
With both a parse listener and set_max_rule_depth enabled, if the cap is first exceeded at a left-recursive operator expansion, this call exits the current listener context and the subsequent depth probe returns before a replacement enter occurs; the generated dispatch wrapper then emits its unconditional final exit, producing more exits than enters. Depth-counting listeners can therefore underflow or panic—for example, the Nest fixture on a+a with a cap of 3 exits RULE_EXPR twice after one enter—so this abort path must suppress the wrapper exit or otherwise retain balanced state.
Useful? React with 👍 / 👎.
A `.g4` file saved with a UTF-8 byte order mark failed to compile:
error[G4F003]: Hello.g4:1:0: mismatched input '\u{feff}grammar'
expecting {'lexer', 'parser', 'grammar'}
The cause is in the pinned meta-grammar rather than the runtime. antlr-ng
collapsed Java ANTLR's two trailing `NameStartChar` ranges
(`'ﷰ'..''` and `''..'�'`) into a single
`'ﷰ'..'�'`, which covers U+FEFF. That made the byte order mark a
legal identifier-start character, so the lexer produced one `ID` token
spanning the mark and the following keyword. A `UnicodeBOM` rule alone
cannot fix this, because `ID` still matches longer and the lexer takes the
longest match.
Restore the split so `NameStartChar` excludes U+FEFF, and add `` to
`WS` so the mark lands off the default channel.
Java spends a dedicated `UnicodeBOM : '' -> skip;` rule on this.
Folding it into `WS` instead is observationally identical and avoids
allocating a token type: a new rule renumbers every token after it
(`END_ARGUMENT` 75 -> 76, ...), which breaks the `frontend-snapshots.tsv`
oracles for the one corpus grammar that uses Argument-mode tokens. Those
oracles are recorded against antlr-ng, which has no such rule, so they
cannot be faithfully regenerated. Max token type stays 78.
The mark is skipped rather than stripped, so it keeps occupying a column
and every `SourceSpan` stays anchored to the real file offsets. Verified
against the 4.13.2 jar: a BOM'd source reports 1:18 where the unmarked
twin reports 1:17, a mid-file mark errors at 2:2, and a mark inside a
STRING_LITERAL or LEXER_CHAR_SET remains content.
`.tokens` vocabularies need a separate fix. They are generated sidecars
parsed line by line, so they never reach the grammar lexer, and U+FEFF is
not `char::is_whitespace`, so `trim` left the mark glued to the first token
name and generation failed with G4S029. Strip a leading mark there too.
Upstream's Java regex instead accepts `"ID"` as the token name and
silently imports a wrong vocabulary; failing that way is not worth
reproducing.
Also regenerates the self-hosted frontend, which had drifted: the
checked-in artifacts carried a `v0.15.2` header against crate 0.19.0 and
predated the `add_parse_listener` facade from #204, so
`update-stage0.sh --check` was already red before this change.
Verified with the full suite (1084 tests), CI clippy, the conformance
sweep (357 passed, 0 failed, 0 skipped), and
`tools/grammar-frontend/update-stage0.sh --check` (Stage 1 == Stage 2).
Closes #212
1 participant
Closes #202. Stacked on #196 (contains its commits; the listener's abort anchoring uses the offending-token field).
What
Parser::add_parse_listener/remove_parse_listeners— ANTLR'saddParseListenercontract, delivered during recognition:Parser.pushNewRecursionContext→triggerEnterRuleEvent), with matching exits as the rule unrolls — so listener-based depth counters see1+1+…+1exactly as Java's do.take_parse_abort()at the top-level entry, depth error preferred) and the parse fails even when recovery produced a tree. Reused parsers start clean.Why
The cel-rust migration (the
celcrate, ~400k downloads/90d) — itsRecursionListenercounts liveexprnesting and aborts pastmax_recursion_depth, built directly onadd_parse_listener. Without this API the port had to rework it into a post-parse tree walk: correct, but it means the migration PR says "we replaced your listener with a different mechanism" instead of "your listener ports verbatim". First impressions matter for a young runtime.Verification
RecursionListenershape verbatim against the deep-nesting fixture: live counting with balanced pairs (high-water mark asserted), positioned abort error past the limit, LR operator expansions counted as rule entries (40-terma+a+…chain rejected at expr-limit 8), clean reuse after abort.if let, verified bycargo checkin anedition = "2021"consumer crate).cargo test --features codegen1075 tests green; clippy-D warningsall-targets all-features clean.Summary by CodeRabbit
New Features
Bug Fixes