Implement semantic predicate accountability hooks#55
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
tinovyatkin has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Copy/Paste DetectionFound 16 duplication(s) across 8 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![CommonToken::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 23 line (133 tokens) duplication in the following files:
pub fn next_token_with_hooks<I, F, A, P, E>(
lexer: &mut BaseLexer<I, F>,
atn: &Atn,
mut custom_action: A,
mut semantic_predicate: P,
mut accept_adjuster: E,
) -> CommonToken
where
I: CharStream,
F: TokenFactory,
A: FnMut(&mut BaseLexer<I, F>, LexerCustomAction),
P: FnMut(&BaseLexer<I, F>, LexerPredicate) -> bool,
E: FnMut(&mut BaseLexer<I, F>, i32, usize),
{
next_token_with_hooks_impl(
lexer,
atn,
&mut custom_action,
&mut semantic_predicate,
&mut accept_adjuster,
LexerMatchStrategy {
compiled: None,
use_cache: false,Found a 22 line (130 tokens) duplication in the following files:
let Some(state) = atn.state(config.state) else {
continue;
};
for transition in &state.transitions {
if !transition.matches(symbol, MIN_CHAR_VALUE, MAX_CHAR_VALUE) {
continue;
}
let mut advanced = config.clone();
set_config_state(atn, &mut advanced, transition.target());
if symbol == EOF {
advanced.consumed_eof = true;
} else {
advanced.position += 1;
}
next.push(advanced);
}
}
let closure = epsilon_closure(atn, next, &mut |predicate| {
semantic_predicate(lexer, predicate)
});
let target_has_semantic_context = closure.has_semantic_context;Found a 19 line (120 tokens) duplication in the following files:
pub(crate) fn matching_action_brace(source: &str, mut index: usize) -> Option<usize> {
let mut nested = 0_usize;
let mut double_quoted = false;
let mut escaped = false;
while let Some(ch) = source[index..].chars().next() {
if escaped {
escaped = false;
index += ch.len_utf8();
continue;
}
match ch {
'\\' if double_quoted => escaped = true,
'"' => double_quoted = !double_quoted,
'\'' if !double_quoted => {
if let Some(next_index) = skip_char_literal(source, index) {
index = next_index;
continue;
}
}Found a 30 line (120 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,
},
visiting,
memo,
expected,
)
.into_iter()
.map(|mut outcome| {
if let Some(rule_index) = boundary {
outcome.nodes.prepend(Rc::new(
FastRecognizedNode::LeftRecursiveBoundary { rule_index },
));
}
outcome
}),
);
}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 (114 tokens) duplication in the following files:
fn parser_matches_token_and_reports_mismatch() {
let source = Source {
tokens: vec![
CommonToken::new(1).with_text("x"),
CommonToken::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);
assert_eq!(
parser.match_token(1).expect("token 1 should match").text(),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![
CommonToken::new(3).with_text("z"),
CommonToken::new(2).with_text("y"),Found a 20 line (111 tokens) duplication in the following files:
FastRecognizedNode::Rule {
rule_index,
invoking_state,
start_index,
stop_index,
children,
} => {
let mut context = ParserRuleContext::with_child_capacity(
*rule_index,
*invoking_state,
children.len(),
);
if let Some(token) = self.token_ref_at(*start_index) {
context.set_start_ref(token);
}
if let Some(token) = stop_index.and_then(|index| self.token_ref_at(index)) {
context.set_stop_ref(token);
}
if children.has_left_recursive_boundary() {
let folded = fold_fast_left_recursive_boundaries(children.to_vec());Found a 14 line (110 tokens) duplication in the following files:
fn outcome_ties_keep_later_non_recursive_alternative() {
let first = RecognizeOutcome {
index: 1,
consumed_eof: false,
alt_number: 0,
member_values: BTreeMap::new(),
return_values: BTreeMap::new(),
diagnostics: Vec::new(),
decisions: Vec::new(),
actions: vec![ParserAction::new(1, 0, 0, None)],
nodes: vec![RecognizedNode::Token { index: 0 }],
};
let second = RecognizeOutcome {
actions: vec![ParserAction::new(2, 0, 0, None)],Found a 13 line (109 tokens) duplication in the following files:
fn parser_matches_token_and_reports_mismatch() {
let source = Source {
tokens: vec![
CommonToken::new(1).with_text("x"),
CommonToken::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 20 line (107 tokens) duplication in the following files:
let start_state = atn
.rule_to_start_state()
.get(rule_index)
.copied()
.ok_or_else(|| {
AntlrError::Unsupported(format!("rule {rule_index} has no start state"))
})?;
let stop_state = atn
.rule_to_stop_state()
.get(rule_index)
.copied()
.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();
let caller_follow_state = self.pending_invoking_follow_state(atn);Found a 15 line (107 tokens) duplication in the following files:
FastRecognizedNode::MissingToken {
token_type,
at_index,
text,
} => {
let current = self.token_at(*at_index);
let token = CommonToken::new(*token_type)
.with_text(text.as_str())
.with_span(usize::MAX, usize::MAX)
.with_position(
current.as_ref().map(Token::line).unwrap_or_default(),
current.as_ref().map(Token::column).unwrap_or_default(),
);
Ok(ParseTree::Error(ErrorNode::new(token)))
}Found a 20 line (102 tokens) duplication in the following files:
atn: &Atn,
mut custom_action: A,
mut semantic_predicate: P,
mut accept_adjuster: E,
) -> CommonToken
where
I: CharStream,
F: TokenFactory,
A: FnMut(&mut BaseLexer<I, F>, LexerCustomAction),
P: FnMut(&BaseLexer<I, F>, LexerPredicate) -> bool,
E: FnMut(&mut BaseLexer<I, F>, i32, usize),
{
next_token_with_hooks_impl(
lexer,
atn,
&mut custom_action,
&mut semantic_predicate,
&mut accept_adjuster,
LexerMatchStrategy {
compiled: None,Found a 21 line (102 tokens) duplication in the following files:
) -> Option<RecognizeOutcome> {
let (error_index, message) = self.expected_error_message(rule_index, start_index, expected);
let token = self.token_at(error_index);
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;
}Found a 12 line (102 tokens) duplication in the following files:
atn.add_state(AtnState::new(4, AtnStateKind::BlockEnd).with_rule_index(0));
atn.add_state(AtnState::new(5, AtnStateKind::RuleStop).with_rule_index(0));
atn.set_rule_to_start_state(vec![0]);
atn.set_rule_to_stop_state(vec![5]);
atn.add_decision_state(1);
atn.state_mut(0)
.expect("state 0")
.add_transition(Transition::Epsilon { target: 1 });
atn.state_mut(1)
.expect("state 1")
.add_transition(Transition::Atom {
target: 2, |
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR adds semantic-predicate/action documentation, a new semantic IR module, and hook-aware parser and lexer runtime support. It also updates code generation to inventory semantic coordinates, emit 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 |
![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 implements the initial phases of the design for handling semantic predicates and actions (Issue #9). It introduces a new Semantic IR (semir module) to represent predicates and actions as data, adds support for user-defined SemanticHooks on the parser side, and implements a configurable policy (--sem-unknown) for handling unknown coordinates, complete with a semantics.json manifest output. The review feedback highlights two critical issues: a potential out-of-bounds panic in action_text when the start index exceeds the adjusted stop index, and a misalignment bug in parser_action_source_blocks that causes incorrect source spans and bodies to be recorded in the manifest for unsupported blocks.
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.
![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: b8ccf2e3b3
ℹ️ 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".
| .collect::<Vec<_>>(); | ||
| // A non-default policy must reach the interpreter through the emitted | ||
| // runtime options, so its literal forces the options-carrying call shape. | ||
| let unknown_policy_literal = match options.sem_unknown { | ||
| SemUnknownPolicy::AssumeTrue => None, |
There was a problem hiding this comment.
When --sem-unknown=assume-false or error is used on a parser whose ATN has only untranslated predicate transitions, unknown_policy_literal is set here but has_predicate_dispatch remains false, so the generated adaptive_direct_allowed gate can still take the ANTLR4_RUST_ADAPTIVE_DIRECT path. That helper falls back through parse_atn_rule(...) without the ParserRuntimeOptions emitted below, so the new unknown-predicate policy is lost and the predicate is treated as passing instead of failing/erroring. Please also disable the adaptive-direct path when a non-default unknown policy or unknown predicate coordinates are present.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3bcc2f2. adaptive_direct_allowed now also requires unknown_policy_literal.is_none(), so a non-default --sem-unknown policy disables the ANTLR4_RUST_ADAPTIVE_DIRECT shortcut. As you noted, that path runs parse_atn_rule_adaptive_or_fallback, which falls back through parse_atn_rule without the emitted ParserRuntimeOptions, dropping the policy. New test non_default_policy_disables_adaptive_direct_gate asserts the gate literal flips from && true && (default) to && false && under both assume-false and error, and first confirms the predicate-free fixture emits the enabled gate by default so the test can't silently pass.
![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 `@src/parser.rs`:
- Around line 424-444: In action_text(), the EOF fallback is using the wrong
boundary and can include hidden tokens before EOF. Update the stop calculation
to match text_interval() and $text by using the visible-token boundary for
TOKEN_EOF instead of blindly subtracting one, and keep the logic localized
around action.stop_index(), self.input.get(), and self.input.text() so the
returned action text excludes trailing whitespace.
🪄 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: bfd76bb4-9d5e-409c-b77e-b4357fa7ac13
📒 Files selected for processing (7)
CLAUDE.mdREADME.mddocs/issue-9-semantic-predicates-actions-design.mdsrc/bin/antlr4-rust-gen.rssrc/lib.rssrc/parser.rssrc/semir.rs
There was a problem hiding this comment.
tinovyatkin has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 568a824430
ℹ️ 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/parser.rs (2)
2382-2402: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAvoid replaying legacy and SemIR actions for the same coordinate.
When
semanticsis present, these paths still apply legacyParserMemberAction/ParserReturnActiontables first, then execute matching SemIR actions. If a generated parser passes both during migration,AddMemberside effects can double-apply. Prefer SemIR for a matched action coordinate, falling back to legacy only when no SemIR action exists.Also applies to: 2426-2444
🤖 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 2382 - 2402, The replay logic in the parser state handling is applying both legacy ParserMemberAction/ParserReturnAction entries and SemIR actions for the same coordinate, which can double-apply member updates. Update the code around the ParserTableSemCtx setup and the action replay loop in parser::... so that when semantics is present you first check for matching speculative SemIR actions and execute those instead, and only fall back to the legacy actions when no SemIR action exists for that source_state/coordinate. Make the same precedence change in the related block at the other referenced location so both paths stay consistent.
4953-4954: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winScope and restore the active unknown-predicate policy.
This mutates parser-level
unknown_predicate_policyand clearsunknown_predicate_hitsfor one runtime-options parse, but never restores the previous state on success or error. A later direct predicate check can inheritError/AssumeFalsefrom an earlier parse. Save the previous policy/hits and restore them before every return from this parse entry.Also applies to: 5013-5016
🤖 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 4953 - 4954, The runtime-options parse path is leaving parser state behind by mutating Parser::unknown_predicate_policy and clearing unknown_predicate_hits without restoring the prior values, so a later predicate check can inherit the wrong policy. In the parse entry that touches these fields, save the current unknown_predicate_policy and unknown_predicate_hits before changing them, and restore both on every exit path from the parse flow, including success and error returns.
🤖 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 `@docs/issue-9-semantic-predicates-actions-design.md`:
- Line 456: The markdownlint MD022 warning is caused by missing blank lines
around the Phase headings in the issue document. Update the markdown around the
`### Phase 3`, `### Phase 4`, and `### Phase 5` headings to insert the required
blank line separation before each heading, keeping the surrounding section text
unchanged. Use the heading markers themselves to locate the affected spots and
apply the same spacing fix consistently across those three sections.
In `@src/atn/lexer.rs`:
- Around line 229-274: The two semantic-hook entry points duplicate the same
action/predicate closure setup, so extract that wiring into a private helper.
Add a helper such as semantic_hook_closures that takes &RefCell<&mut H> and
returns the two closures used by next_token_with_semantic_hooks and
next_token_compiled_with_semantic_hooks, then have both functions create the
RefCell once and reuse the helper instead of inlining identical logic. Keep the
existing behavior in the closures (rule/action index conversion, LexerSemCtx
construction, and unwrap_or(true) default) unchanged.
In `@src/parser.rs`:
- Around line 2797-2811: The SemIR hook handling in `Parser::hook` is swallowing
missing `SemanticHooks::sempred` results by converting `None` into `false`,
which causes unregistered generated predicates to fail silently. Update
`Parser::hook` and the `SemanticHooks::sempred` call path to preserve the `None`
state for missing hooks and route it through the existing
unknown-coordinate/unsupported-hook handling instead of defaulting to rejection.
---
Outside diff comments:
In `@src/parser.rs`:
- Around line 2382-2402: The replay logic in the parser state handling is
applying both legacy ParserMemberAction/ParserReturnAction entries and SemIR
actions for the same coordinate, which can double-apply member updates. Update
the code around the ParserTableSemCtx setup and the action replay loop in
parser::... so that when semantics is present you first check for matching
speculative SemIR actions and execute those instead, and only fall back to the
legacy actions when no SemIR action exists for that source_state/coordinate.
Make the same precedence change in the related block at the other referenced
location so both paths stay consistent.
- Around line 4953-4954: The runtime-options parse path is leaving parser state
behind by mutating Parser::unknown_predicate_policy and clearing
unknown_predicate_hits without restoring the prior values, so a later predicate
check can inherit the wrong policy. In the parse entry that touches these
fields, save the current unknown_predicate_policy and unknown_predicate_hits
before changing them, and restore both on every exit path from the parse flow,
including success and error returns.
🪄 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: 100bf698-0678-4634-b1bc-c3e25ca0c070
📒 Files selected for processing (10)
CLAUDE.mdREADME.mddocs/issue-9-semantic-predicates-actions-design.mdpatterns/javascript.tomlsrc/atn/lexer.rssrc/bin/antlr4-rust-gen.rssrc/lexer.rssrc/lib.rssrc/parser.rssrc/semir.rs
Review pass over the SemIR delivery branch; validated with the full
conformance sweep (357/357), kotlin parity (9/9 trees match), unit
tests, and clippy -D warnings.
- Block walkers in bin_support/templates.rs now locate opening braces
through a shared GrammarSourceCursor that skips quoted literals,
comments, and charsets. Real grammars referencing brace tokens
('{' statementList? '}') previously desynchronized every predicate
span/hook pairing: on grammars-v4 JavaScriptParser the manifest had
0/16 predicate spans and 0 hook matches; now 16/16 with 11 routed to
the typed hook trait.
- Hook-routed lexer predicates fail codegen with a clear error instead
of panicking in the render path (generated lexers have no hook
plumbing yet).
- Helper-hooked coordinates report disposition "hooked" instead of
"translated" in semantics.json so users can tell which coordinates
still need a runtime hook implementation.
- Predicate IR evaluation no longer clones the speculative member map
and rule-name String on every evaluation inside the prediction loop;
the predicate context is now fully borrowed and read-only.
- patterns/javascript.toml aligned with grammars-v4 JavaScriptParser:
adds lineTerminatorAhead, drops the lexer-side isRegexPossible, and
documents the argument-taking n()/p() helper gap.
- Design doc Phase 4/5 statuses tempered to partially-implemented with
the concrete remaining gaps.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
tinovyatkin has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
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)
4300-4324: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftSurface unhandled parser actions instead of returning an ignored
bool.
parser_action_hookreturns whether the hook handled the committed action, but generated fallback code ignores that result. With default/no-op hooks, untranslated grammar actions can be skipped while parsing succeeds, which violates the fail-loud semantic-action boundary. Consider returning/propagatingResult<(), AntlrError>or recording unsupported action hits like unknown predicates.🤖 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 4300 - 4324, The parser_action_hook path currently returns a bool that can be silently ignored by generated fallback code, allowing unsupported grammar actions to pass without surfacing an error. Update parser_action_hook and its callers to propagate a failure signal instead of treating the return value as optional, ideally using a Result-based flow or equivalent error recording consistent with unknown predicate handling; reference parser_action_hook, ParserSemCtx, and semantic_hooks.action when wiring this through.
🤖 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 `@docs/issue-9-semantic-predicates-actions-design.md`:
- Line 482: The Phase 5 heading is missing the required blank line after it, so
update the markdown around the heading to ensure it is properly separated per
MD022. Locate the Phase 5 section in the document and add the missing empty line
immediately after the heading so the surrounding paragraph structure is correct.
---
Outside diff comments:
In `@src/parser.rs`:
- Around line 4300-4324: The parser_action_hook path currently returns a bool
that can be silently ignored by generated fallback code, allowing unsupported
grammar actions to pass without surfacing an error. Update parser_action_hook
and its callers to propagate a failure signal instead of treating the return
value as optional, ideally using a Result-based flow or equivalent error
recording consistent with unknown predicate handling; reference
parser_action_hook, ParserSemCtx, and semantic_hooks.action when wiring this
through.
🪄 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: 49417a6b-cbe4-4e51-9b30-b7492e852d9f
📒 Files selected for processing (6)
docs/issue-9-semantic-predicates-actions-design.mdpatterns/javascript.tomlsrc/bin/antlr4-rust-gen.rssrc/bin_support/templates.rssrc/parser.rssrc/recognizer.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6951933d2
ℹ️ 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".
Address PR #55 review findings, all in service of design goal G1 (never silently mis-parse): - Parser SemIR hook: route a declining user hook (`None`) through the configured `UnknownSemanticPolicy` instead of `unwrap_or(false)`, so a `PExpr::Hook` coordinate with no implementation no longer silently rejects its alternative. Extracts a shared `apply_unknown_predicate_policy` so the SemIR and legacy table paths dispatch identically (hook -> policy). (Codex + CodeRabbit) - Generator: disable the adaptive-direct shortcut whenever a non-default unknown-predicate policy is emitted; that path falls back through `parse_atn_rule` without the `ParserRuntimeOptions` carrying the policy, which would drop it. (Codex) - Generator: the lexer `run_predicate` catch-all arm now follows `--sem-unknown` (`assume-false` -> `_ => false`), so a mixed lexer's uncovered predicate is not left viable. (Codex) - `ParserSemCtx::action_text`: end the EOF interval at the previous *visible* token (matching `text_interval`/`$text`) instead of a blind `stop - 1`, excluding trailing hidden tokens. (Gemini + CodeRabbit) - Manifest: pair each action-block source span with its ATN state through the same offset used for templates, walking signature templates in lockstep so span/body provenance no longer drifts after a `returns [<...>]` template. (Gemini) - Deduplicate the two identical lexer semantic-hook closure bodies into `dispatch_lexer_action_hook` / `dispatch_lexer_predicate_hook`. (CodeRabbit + CPD) - Docs: blank lines around Phase headings (markdownlint MD022). (CodeRabbit) New regression tests cover the SemIR-hook policy fallthrough across all three policies, the adaptive-direct gate flip, the lexer default-arm flip, and action-slot/span alignment.
There was a problem hiding this comment.
tinovyatkin has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Pushed 3bcc2f2 addressing the open review threads. Summary: Fixed (fail-loud semantic boundary, G1):
Fixed (other):
Two "outside diff range" notes, addressed as no-change-needed:
Validation: @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3bcc2f2a1b
ℹ️ 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".
Address the two P1 findings from Codex's review of 3bcc2f2, both deeper instances of the fail-loud boundary (G1) in paths the first pass missed: - Generated recursive-descent predicate evaluation reads `BaseParser::unknown_predicate_policy`, but only the interpreter fallback installed it (via `ParserRuntimeOptions`); the generated-direct path left it at the `AssumeTrue` default, so a hook predicate returning `None` in a generated rule silently passed even under `--sem-unknown=error`. Add a public `set_unknown_predicate_policy` setter and have the generated parser constructor install a non-default policy, so both paths honor `--sem-unknown`. Also add `take_unknown_semantic_error` so the generated path can surface recorded `Error`-policy coordinates. - Under `--sem-unknown=hook`/`error`, an *uncovered* lexer predicate was marked `hooked` in the manifest and accepted by `--require-full-semantics`, yet generated lexers have no hook plumbing (a `hook`-lowered lexer predicate is already a codegen error) and no runtime coordinate recording, so `run_predicate`'s catch-all silently kept it viable. `render_lexer` now rejects uncovered lexer predicates under hook/error, mirroring the existing explicit-hook rejection. Tests: generated constructor installs the policy literal (codegen) and the generated-direct matcher honors it end-to-end (runtime, all three policies); uncovered lexer predicates are rejected under hook and error.
There was a problem hiding this comment.
tinovyatkin has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Pushed 88e9fbd addressing both P1 findings from the last review — both were deeper instances of the fail-loud boundary (G1) in paths the previous commit didn't reach:
Validation: @codex review |
There was a problem hiding this comment.
💡 Codex Review
antlr-rust-runtime/src/bin/antlr4-rust-gen.rs
Line 5376 in 88e9fbd
Surface generated unknown semantic hits before success
When a generated semantic decision evaluates a hook-lowered predicate and the hook returns None, parser_semantic_ir_predicate_matches_with_context_and_local records the coordinate under the Error fallback and returns false; the interpreter path checks this after recognition, but this generated success path returns the tree without draining take_unknown_semantic_error(). In that scenario the unknown predicate can merely prune an alternative and still produce a parse tree instead of the promised AntlrError::Unsupported, so add the same recorded-error check before returning success.
antlr-rust-runtime/src/bin/antlr4-rust-gen.rs
Line 3735 in 88e9fbd
Return the recorded unknown semantic error
For a hook-lowered predicate reached as an actual generated rule step, an unhandled hook under the Error fallback records the unsupported coordinate and returns false, but this branch converts it into a generic failed-predicate ParserError. That loses the coordinate and violates the fail-loud Unsupported behavior; check take_unknown_semantic_error() before falling back to failed_predicate_error here.
ℹ️ 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".
Address Codex P2 (review of 88e9fbd): the predicate-block scans, unlike the action collectors, never applied rule-name filtering. In a combined grammar (`--grammar` = one .g4 with both lexer and parser rules) this walked the other rule set's `{...}?` predicates into the positional pairing against this ATN's predicate transitions — mis-mapping a translatable predicate onto the wrong coordinate, or erroring with "no parser ATN predicate transition". `parser_predicate_templates` and the new `extract_supported_predicate_templates_filtered` now gate each predicate block through `rule_action_included` (the same filter the action collectors use), skipping blocks that belong to a different rule set so coordinate pairing stays aligned. `lexer_predicate_templates` passes the lexer's rule names; the parser scan passes the parser's. Tests: a translatable lexer-rule predicate preceding a parser rule is skipped (not mapped onto the parser coordinate), and the parser-rule predicate still maps to its correct coordinate.
There was a problem hiding this comment.
tinovyatkin has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
antlr-rust-runtime/src/bin/antlr4-rust-gen.rs
Lines 5841 to 5842 in bfee243
Let mixed lexer predicates use policy fallbacks
For a lexer with one translated predicate and one untranslated predicate under --sem-unknown=assume-false or assume-true, this equality check errors as soon as the template count differs, so the uncovered-coordinate logic and the generated catch-all arm never run. Fresh evidence is that render_lexer_predicate_method now has a policy-aware default arm for mixed lexers, but lexer_predicate_templates still rejects those mixed inputs before rendering.
ℹ️ 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".
The predicate rule-name filter added in bfee243 regressed SemPredEvalParser/ValidateInDFA: its grammar has `// ';' helps ...` above rule `a`, and the brace-counting `statement_rule_header` matches that `;` inside the comment as a rule terminator, so header resolution fails (`None`). `rule_action_included` treats an unresolvable header as "not in the filter set" and skipped rule `a`'s `{<False()>}?`/`{<True()>}?` predicates, dropping them from `parser_semantics()` — the guarded alternatives then went unguarded (`alt 1` printed instead of no-viable-alt). Introduce `predicate_block_included`, which excludes a predicate block only when its owning rule name *positively resolves* and is absent from the target rule set; an unresolvable header keeps the block (pre-filter behavior). Both predicate scans use it, so the combined-grammar lexer/parser separation still holds without dropping real predicates when the scraper is defeated by comments. Test: `parser_predicate_scan_keeps_predicate_when_header_unresolvable` pins the comment-with-semicolon case; SemPredEvalParser is back to 26/26.
There was a problem hiding this comment.
tinovyatkin has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
antlr-rust-runtime/src/bin/antlr4-rust-gen.rs
Line 5841 in ec7e495
Allow fallback for partially translated lexer predicates
When a lexer has a mix of one predicate that matches a built-in/pattern template and another raw predicate that should follow --sem-unknown=assume-true or assume-false, extract_supported_predicate_templates_filtered returns only the translated entries, so this count check rejects generation before the fallback arm rendered in run_predicate can apply. The all-unknown case is accepted via templates.is_empty(), so adding a single translatable predicate makes the same uncovered coordinate a hard codegen error instead of using the documented fallback policy.
antlr-rust-runtime/src/bin/antlr4-rust-gen.rs
Line 7697 in ec7e495
Preserve SetMember effects in interpreted rules
For parser rules that fall back to the ATN interpreter, supported member mutations are replayed speculatively through the SemIR action table, but this arm drops SetMember while only AddMember is collected. A rule with SetMember("i", "3") followed by a MemberEquals("i", "3") predicate will work on the generated-direct path (which inlines set_int_member) but the interpreted fallback leaves i at its previous value and can choose/reject the wrong alternative.
ℹ️ 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".
Address Codex's three findings on bfee243: - Generated top-level rule entry now calls `take_unknown_semantic_error` before returning `Ok`, so Error-policy coordinates the generated-direct predicate path recorded surface as `AntlrError::Unsupported` instead of a recovered `Ok` tree. Wires up the accessor added in 88e9fbd (previously only reachable from tests). Surfaced at the public entry (`allow_generated_fallback`) so nested rules accumulate and the outermost reports, mirroring the interpreter entry. (P1) - `parser_typed_hook_mappings` now applies the same `predicate_block_included` rule-name filter as `parser_predicate_templates`, so a combined grammar's lexer-rule helper predicate no longer consumes a parser coordinate and wires `MyParserTypedHooks` to the wrong method. (P2) - `enforce_sem_unknown` now fails codegen for any per-coordinate `dispose = "error"` override regardless of the global policy. Such an override lowers to no SemIR entry and does not escalate the runtime policy, so previously it silently fell back to the global default (e.g. AssumeTrue) instead of rejecting the coordinate. (P2) Tests: generated entry emits the surfacing call; typed-hook mapping skips a lexer-rule predicate preceding the parser helper; a per-coordinate error override fails even under assume-true.
Fixed in 053d7e1. Good follow-up — this was a gap my previous assume-* fix opened. Dropping the translated The two disposition classes now get different fallbacks: fn run_action(&mut self, action: antlr4_runtime::ParserAction, _tree: &antlr4_runtime::ParseTree) {
match action.source_state() {
3 => {} // assume-* override: silent no-op
_ => { let _ = self.base.parser_action_hook(action, _tree); } // hook / unknown
}
}The scan walks all ATN action states ( Verified end-to-end:
Test |
Addresses two Codex findings about `@init` member effects not being
visible to same-rule predicates/actions. Investigation showed both rest
on a deeper rule-name-resolution gap, so the fix is three connected parts:
1. Rule-name resolution (`named_action_rule_name`): a `@members` /
`@parser::members` block between the previous `;` and a rule's `@init`
no longer hides the rule name. The old code truncated the header at the
first `@` after the last `;`, so the rule name (which follows the
members block) was dropped and the rule's `@init` was silently
discarded on every path. Now `last_rule_header_identifier` skips
`@name {...}` blocks (tracking brace depth so a `{...}` inside a
`<...>` template body doesn't confuse the skip) and returns the
trailing rule identifier. Shared by `@init` and `@after`.
2. Mixed `@init` sequences (`init_action_member_writes`): a sequence like
`SetMember(x,1); writeln(...)` now hoists its member writes to rule
ENTRY (so same-rule predicates observe them) while the side effect
stays on the buffered exit-replay path. Previously the all-or-nothing
`init_action_mutates_members_only` gate excluded the whole sequence
from entry execution.
3. Together these make the generated-direct `@init`-before-body order
correct: verified end-to-end that a rule with
`@parser::members {<InitIntMember("i","0")>}` and
`s @init {<SetMember("i","1")>} : {<MemberEquals("i","1")>}? ...` now
parses without a failed-predicate diagnostic (the predicate observes
i==1), where it previously failed the predicate against the pre-init
value.
The interpreter-fallback variant of the finding is not reachable in
practice: a rule carrying `@init` + a same-rule predicate compiles
generated-direct (confirmed with wildcard and left-recursive shapes), so
the eager entry-write on the generated path is what governs correctness.
Tests: `init_action_rule_name_resolves_past_a_members_block`,
`init_member_write_after_members_block_runs_at_entry`,
`init_entry_extracts_member_writes_from_mixed_sequence`. clippy clean;
full suite green.
💡 Codex Reviewantlr-rust-runtime/src/bin/antlr4-rust-gen.rs Line 8078 in 36e5e83 Lower SetMember for interpreted semantic replay
When a rule falls back to the ATN interpreter, member-mutating actions are replayed speculatively through antlr-rust-runtime/src/bin/antlr4-rust-gen.rs Line 3942 in 36e5e83 Preserve child context for action hooks
For a generated child rule whose action state is routed to antlr-rust-runtime/src/bin/antlr4-rust-gen.rs Line 5537 in 36e5e83 Check action-hook failures before running @after
In the top-level interpreted path, ℹ️ 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". |
Fixed in 36e5e83 (both findings). Investigating these surfaced a deeper root cause, so the fix is three connected parts. Root cause (rule-name resolution).
Finding 2 (mixed Finding 1 (interpreter fallback). With the two fixes above, the generated-direct path now applies Verified end-to-end. A grammar with Tests: |
When a semantic decision's predicted alt has its leading predicate fail,
`render_semantic_alt_search` picks the first *other* alt whose candidate
condition holds. An alt with no locally-checkable guard — no leading
predicate and no computable lookahead, because its first consuming step
is a rule call / nested decision / loop — has candidate condition `"true"`,
so it shadowed a later alt with a concrete matching lookahead. A grammar
like `{p()}? 'a' | x | 'a'` (with `x : 'b'`) on input `a` therefore
selected the non-viable rule-call alt `x` after `p` failed, instead of
the viable `'a'` alt.
The fallback search now forces such an unresolved-guard alt to `false`
(new `semantic_alt_guard_is_unresolved`), so it is skipped rather than
shadowing a concretely-guarded alt. A genuine epsilon alt (no consuming
step) stays matchable.
This is scoped to the fallback search only: the shared
`semantic_alt_candidate_condition` is unchanged, so left-recursion
loop-entry (`render_generated_loop_semantic_prediction_filter`) and the
diagnostic path keep their behavior — verified that
`DuplicatedLeftRecursiveCall` / `PrefixAndOtherAlt` codegen is byte
-identical to before. The ATN-predicted alt is still honored via the
`_ => Some(__prediction.alt)` arm, so a rule-call alt that is itself the
predicted alt is unaffected.
Verified end-to-end: `{p()}? 'a' | x | 'a'` on input `a` (p false) now
parses `Ok` by selecting the `'a'` alt.
Tests: `semantic_alt_guard_classifies_unresolved_rule_call_alt`,
`semantic_alt_search_skips_unresolved_rule_call_alt`. clippy clean; full
suite green.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfe9cab220
ℹ️ 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".
Fixed in dfe9cab. Correct — an alt with an uncomputable FIRST set was being treated as an unconditional candidate.
The fix distinguishes the two: a new This only changes the fallback search triggered when another alt's predicate fails: the ATN-predicted alt is still honored via the I initially tried this in the shared Tests: |
|
@codex review All findings from your incremental reviews are now addressed through HEAD
Validation on |
There was a problem hiding this comment.
💡 Codex Review
antlr-rust-runtime/src/bin/antlr4-rust-gen.rs
Lines 6214 to 6216 in dfe9cab
Preserve fail-option messages for all predicates
When a grammar attaches <fail=...> to any translated predicate other than a constant false (for example a hook, lookahead, or member predicate that returns false at runtime), this match returns the original template and discards message. Later parser_semantic_ir_predicate_failure_message can only recover with ANTLR's fail text when the template became FalseWithMessage, so those predicates fall back to the generic failed/no-viable path despite the grammar supplying a fail option. Store the fail message alongside every predicate template, not only PredicateTemplate::False.
ℹ️ 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".
Follow-up to the semantic-alt fallback-search fix: forcing an
unresolved-FIRST alt to `if false` stopped it shadowing a later
concretely-guarded alt, but also made it unreachable when it was the only
remaining candidate — `{p()}? 'a' | x` with `p()` false and input in
`FIRST(x)` reported `NoViableAlt` instead of trying `x`.
`render_semantic_alt_search` now runs in two passes: resolved-guard alts
(predicate and/or lookahead) first, in order, then the unresolved alts in
order as a last resort (keeping their real condition, typically `true`).
So a resolved alt is never shadowed by an earlier unresolved rule-call
alt, and an unresolved alt is still selected when no resolved alt matched.
Verified end-to-end:
- `{p()}? 'a' | x` (x : 'b'), input `b`, p false -> selects `x` (was
NoViableAlt).
- `{p()}? 'a' | x | 'a'`, input `a`, p false -> selects the `'a'` alt (no
shadowing by rule-call `x`).
Still scoped to the fallback search; the shared
`semantic_alt_candidate_condition` and left-recursion loop-entry are
unchanged. Tests: `semantic_alt_search_orders_unresolved_alts_last`,
`semantic_alt_search_keeps_lone_unresolved_alt_reachable`. clippy clean;
full suite green.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e717e01aea
ℹ️ 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".
Fixed in e717e01. You're right — forcing the unresolved alt to
For if __semantic_la == 1 && <pred> { Some(1) } // pass 1: resolved
else if __semantic_la == 1 { Some(3) } // pass 1: resolved (token alt, not shadowed)
else if true { Some(2) } // pass 2: unresolved rule-call, last resort
else { None }Verified end-to-end:
Still scoped to the fallback search; the shared |
A grammar's `<fail='...'>` option was only preserved for a constant-false
predicate (folded into `FalseWithMessage`). For any other translated
predicate that can return false at runtime — a hook, lookahead, member,
or local-int predicate — `predicate_template_with_fail_message` discarded
the message, so `parser_semantic_ir_predicate_failure_message` had nothing
to surface and the parse fell back to the generic failed-predicate text
despite the grammar supplying a fail message.
Introduce a transparent `PredicateTemplate::WithFailMessage { inner,
message }` wrapper that carries the message alongside any template. It is
transparent to evaluation and codegen: `predicate_effective_template`
unwraps it wherever the inner template's behavior matters
(`render_lexer_predicate_expression`, `render_parser_semir_predicate_expr`,
`render_parser_predicate_array`, `can_generate_parser_predicate`,
`predicate_template_disposition`), and `predicate_template_fail_message`
exposes the message so the SemIR predicate builder emits `failure_message`
for every predicate that carries one, not only `FalseWithMessage`.
Verified end-to-end: `{<MemberEquals("i","1")>}?<fail='member not one'>`
with `i` defaulting to 0 now surfaces `rule s member not one` instead of
the generic failed-predicate diagnostic; the old generator emitted
`failure_message: None` for that coordinate.
Tests: `parses_predicate_fail_option_message` extended to cover the
wrapper (hook predicate), its transparency (effective template,
generatability, disposition = Hooked), the exposed message, and message
replacement on a second `<fail=...>`. clippy clean; full suite green.
`parse_sem_patterns` stripped everything after the first `#` on a line to
drop TOML comments, but a `#` inside a quoted string — e.g.
`match = "text == '#'"` or a `str("#")` lower expression — was treated as
a comment too, silently corrupting valid pattern files so the intended
pattern failed to match.
`strip_toml_comment` now scans for a comment `#` only outside quoted
strings (basic `"..."` and literal `'...'`), honoring `\"` escapes inside
a basic string so an escaped quote does not appear to close it.
Tests: `strip_toml_comment_respects_quoted_strings` (unit cases for
comment vs. in-string `#`, post-string comment, and escaped quote) and
`sem_patterns_keep_hash_inside_quoted_match` (end-to-end: a `#` inside a
quoted `match` body survives parsing and still lowers). clippy clean; full
suite green.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8c92e0334
ℹ️ 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".
| /// `true` when the action is handled so it satisfies a `hook`/`error` | ||
| /// unknown-semantic policy; the default no-op returns `false` (unhandled), | ||
| /// which fails loud under those policies. | ||
| fn custom_action<S>(&mut self, _ctx: &mut antlr4_runtime::ParserSemCtx<'_, S>, _action: antlr4_runtime::ParserAction) -> bool |
There was a problem hiding this comment.
When a grammar has a bare predicate helper whose Rust name normalizes to custom_action (for example customAction() or custom_action()), method_decls emits a required typed predicate method with that name and this default action hook method emits another custom_action in the same trait. Rust traits cannot overload methods by arity, so the generated parser fails to compile for an otherwise valid hook mapping; reserve/disambiguate this generated action-hook name.
Useful? React with 👍 / 👎.
`next_token_with_semantic_hooks` dispatched a custom lexer action through `LexerSemCtx` built from `&BaseLexer`, so a `SemanticHooks::lexer_action` hook could not change lexer state — a hook equivalent to `pushMode()`/`popMode()`/`setMode()` had no effect even when it returned handled, unlike the closure-based `custom_action` API which receives `&mut BaseLexer`. The accepted token would then be followed by lexing in the wrong mode. `LexerSemCtx` now holds a `LexerRef` that is either a shared borrow (for the speculative `lexer_sempred` path, where mutation is invalid) or a mutable borrow (for the committed `lexer_action` path). The action dispatch builds the mutable variant via `new_mut`, and the context exposes `set_mode` / `push_mode` / `pop_mode` that apply on the action path and are inert (returning `false`/`None`) on the predicate path. Read accessors work on both. Tests: `lexer_action_hook_context_can_change_mode` verifies mode push/pop/set apply through the mutable context and are inert through the shared one. clippy clean; full suite green.
Fixed in c8c92e0. Correct — the fail message was preserved only for a constant-false predicate (folded into Introduced a transparent let failure_message = predicate_template_fail_message(predicate).map_or_else(
|| "None".to_owned(),
|message| format!("Some(\"{}\")", rust_string(message)),
);Verified end-to-end with
I took care that the wrapper doesn't mis-classify: Test: |
Fixed in 19d1f85. Correct —
fn strip_toml_comment(line: &str) -> &str {
// ... track quote state; a `#` while `quote == None` starts the comment,
// a `#` inside a string is literal; `\` escapes the next byte in "..." ...
}Tests: |
Fixed in 0bc64e8. Correct — the hooks-based action dispatch built
These apply on the action path and are inert on the predicate path (returning Generated lexers don't route actions to Test: |
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/lexer.rs`:
- Around line 219-233: The documentation for `Lexer::set_mode` is inaccurate
because the `LexerRef::Shared(_)` branch does not emit any warning and only
returns `false`. Update the doc comment on `set_mode` to match the actual
behavior: it is applied only in the `LexerRef::Mut` path, and in the
speculative/predicate context it is silently ignored with a `false` return
value. Keep the wording aligned with the `set_mode` method and the
`LexerRef::Mut` / `LexerRef::Shared` match behavior.
🪄 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: c7d92712-4eb3-4072-9ee7-5399bb6147dc
📒 Files selected for processing (4)
src/atn/lexer.rssrc/bin/antlr4-rust-gen.rssrc/lexer.rssrc/parser.rs
| /// Sets the current lexer mode. Available only from a custom-action hook | ||
| /// (the mutable-borrow context); a no-op with a warning path for the | ||
| /// speculative predicate context, where mutating lexer state is invalid. | ||
| /// | ||
| /// Returns `true` if the mutation was applied (action context), `false` if | ||
| /// it was ignored (predicate context). | ||
| pub fn set_mode(&mut self, mode: i32) -> bool { | ||
| match &mut self.lexer { | ||
| LexerRef::Mut(lexer) => { | ||
| lexer.set_mode(mode); | ||
| true | ||
| } | ||
| LexerRef::Shared(_) => false, | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Doc claims a "warning path" that doesn't exist.
The comment says predicate-context calls are "a no-op with a warning path", but Shared(_) just returns false with no warning emitted. This can mislead hook implementers into expecting a diagnostic. Align the doc with the actual behavior (silent false return signals not-applied).
📝 Proposed doc fix
/// Sets the current lexer mode. Available only from a custom-action hook
- /// (the mutable-borrow context); a no-op with a warning path for the
- /// speculative predicate context, where mutating lexer state is invalid.
+ /// (the mutable-borrow context); a silent no-op for the speculative
+ /// predicate context, where mutating lexer state is invalid.
///
/// Returns `true` if the mutation was applied (action context), `false` if
/// it was ignored (predicate context).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Sets the current lexer mode. Available only from a custom-action hook | |
| /// (the mutable-borrow context); a no-op with a warning path for the | |
| /// speculative predicate context, where mutating lexer state is invalid. | |
| /// | |
| /// Returns `true` if the mutation was applied (action context), `false` if | |
| /// it was ignored (predicate context). | |
| pub fn set_mode(&mut self, mode: i32) -> bool { | |
| match &mut self.lexer { | |
| LexerRef::Mut(lexer) => { | |
| lexer.set_mode(mode); | |
| true | |
| } | |
| LexerRef::Shared(_) => false, | |
| } | |
| } | |
| /// Sets the current lexer mode. Available only from a custom-action hook | |
| /// (the mutable-borrow context); a silent no-op for the speculative | |
| /// predicate context, where mutating lexer state is invalid. | |
| /// | |
| /// Returns `true` if the mutation was applied (action context), `false` if | |
| /// it was ignored (predicate context). | |
| pub fn set_mode(&mut self, mode: i32) -> bool { | |
| match &mut self.lexer { | |
| LexerRef::Mut(lexer) => { | |
| lexer.set_mode(mode); | |
| true | |
| } | |
| LexerRef::Shared(_) => false, | |
| } | |
| } |
🤖 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/lexer.rs` around lines 219 - 233, The documentation for `Lexer::set_mode`
is inaccurate because the `LexerRef::Shared(_)` branch does not emit any warning
and only returns `false`. Update the doc comment on `set_mode` to match the
actual behavior: it is applied only in the `LexerRef::Mut` path, and in the
speculative/predicate context it is silently ignored with a `false` return
value. Keep the wording aligned with the `set_mode` method and the
`LexerRef::Mut` / `LexerRef::Shared` match behavior.
💡 Codex Reviewantlr-rust-runtime/src/bin/antlr4-rust-gen.rs Line 9630 in 0bc64e8 Disambiguate colliding typed hook methods
When two helper predicates normalize to the same Rust method name, this set collapses them into one trait method while the dispatch arms for both coordinates call that same method. For example, a grammar with ℹ️ 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". |
|
@codex review Addressed the latest batch through HEAD
Validation on |
The typed-hook trait declares a fixed `custom_action` method (the action
hook) alongside one method per bare predicate helper. A grammar helper
that normalizes to `custom_action` — e.g. `customAction()` or
`custom_action()` — collided with it, and since Rust traits cannot
overload by arity the generated parser failed to compile (two
`custom_action` methods, E0201) for an otherwise valid hook mapping.
`typed_hook_predicate_method_name` now suffixes a predicate-helper name
that equals the reserved `custom_action` with `_pred`, so the predicate
method and the action hook are distinct. The `sempred` dispatch arm uses
the same disambiguated name, so the mapping stays consistent.
Verified end-to-end: a grammar with `{customAction()}?` under
`--sem-unknown=hook` now generates a trait with both `custom_action_pred`
(predicate) and `custom_action` (action), and the generated crate
compiles (it previously failed with a duplicate-method error).
Tests: `typed_hook_predicate_method_name_avoids_action_hook_collision`
(unit) and `typed_hook_adapter_disambiguates_custom_action_helper`
(renders both distinct methods + the consistent dispatch arm). clippy
clean; full suite green.
Fixed in 07a9cbf. Correct — the typed-hook trait declares a fixed
const TYPED_HOOK_ACTION_METHOD: &str = "custom_action";
fn typed_hook_predicate_method_name(helper: &str) -> String {
let name = rust_function_name(helper);
if name == TYPED_HOOK_ACTION_METHOD { format!("{name}_pred") } else { name }
}Verified end-to-end: a grammar with Tests: |
|
@codex review HEAD is now
CI on |
💡 Codex Reviewantlr-rust-runtime/src/bin/antlr4-rust-gen.rs Lines 6164 to 6168 in 07a9cbf Apply lexer coordinate overrides before rejecting hook slots
When a lexer predicate body is matched by a helper/pattern that lowers to antlr-rust-runtime/src/bin/antlr4-rust-gen.rs Lines 1194 to 1198 in 07a9cbf Handle TOML literal strings in sem pattern files
ℹ️ 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". |
Two related `--sem-patterns` fixes: - Apply lexer predicate coordinate overrides before rejecting hook slots. `lexer_predicate_templates` rejected a slot that lowered to `Hook` (via a helper/pattern) before any per-coordinate override applied, so a `dispose = "assume-false"` / `"assume-true"` override on that lexer predicate — which the manifest reports as a documented fallback — still failed generation. It now applies the `LexerPredicate` coordinate override to each position-aligned slot first (mirroring the parser path), so an `assume-*` override turns the `Hook` slot into `False`/`True` and generation succeeds; a `hook` override still rejects (unsupported for generated lexers) and `error` leaves the coordinate uncovered. - Handle TOML literal (single-quoted) strings in `parse_toml_scalar`. `strip_toml_comment` already treats `'...'` as a quoted string, but the scalar parser only stripped double quotes, so `dispose = 'assume-false'`, `lower = 'hook'`, or a single-quoted `match` kept its quotes and was rejected or failed to match. Literal strings are now stripped verbatim (no escape processing, per TOML). Verified end-to-end: a lexer helper lowered to `hook` plus a single-quoted `dispose = 'assume-false'` lexer-predicate coordinate override now generates (previously errored on the hook slot), and the manifest reports the coordinate as `assume-false`. Tests: `parse_toml_scalar_strips_single_quoted_literals`, `sem_patterns_accept_single_quoted_dispose`. clippy clean; full suite green.
Both fixed in f72c6d9. Apply lexer coordinate overrides before rejecting hook slots (gen.rs:6164). Correct — for (slot, (rule_index, pred_index)) in slots.iter_mut().zip(predicates.iter().copied()) {
if let Some(override_template) = patterns.coordinate_predicate_template(
SemanticsKind::LexerPredicate, data.rule_names.get(rule_index).map(String::as_str), Some(pred_index),
) {
*slot = override_template; // assume-* -> Some(False/True); hook -> Some(Hook); error -> None
}
}So Handle TOML literal strings (gen.rs:1194). Correct, and a direct consistency gap with the Verified end-to-end: a lexer helper lowered to Tests: |
|
@codex review HEAD is now
Both are generator-only changes gated on |
|
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
ParserSemanticstable while keeping the existing legacy predicate/action table API as a compatibility adapter--sem-patterns,--require-full-semantics, explicit--sem-unknown=hook, coordinate overrides, exact pattern/helper lowering, and JavaScript helper hook data inpatterns/javascript.tomlLexerSemCtxValidation
cargo +1.95.0 check --locked --all-targetscargo +1.95.0 clippy --locked --all-targets --all-features -- -D warningscargo +1.95.0 test --locked --all-targetscargo +1.95.0 run --release --quiet --bin antlr4-runtime-testsuite->summary: 357 passed, 0 failed, 0 skipped, 357 runtests/kotlin-parity/run.sh --antlr-jar /tmp/antlr-cleanroom/tools/antlr-4.13.2-complete.jar --grammars-v4 /tmp/antlr-cleanroom/grammars-v4 --python /tmp/antlr-cleanroom/antlr-python/bin/python-> all 9 Kotlin/script snippets matchedCloses #9