Add JavaScript and TypeScript semantic hook support - #64
Conversation
WalkthroughThe change adds typed JavaScript and TypeScript lexer hooks, composed semantic dispatch, deduplicated diagnostics, parser token access, and token-emission callbacks. It defines helper mappings, generates hook-aware recognizers, and adds JavaScript and TypeScript parity harnesses that compare tokens and parse trees across fixtures using pinned CI tooling. Documentation covers the semantic design, build procedures, parity tests, and typed hook integration. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Copy/Paste DetectionFound 21 duplication(s) across 10 changed Rust file(s) (threshold: 100 tokens). Show duplicationsFound a 57 line (374 tokens) duplication in the following files:
use javascript_parser_base::JavaScriptParserBase;
fn dump_tree<S: AsRef<str>>(
out: &mut dyn Write,
tree: &ParseTree,
rule_names: &[S],
depth: usize,
) -> io::Result<()> {
let pad = " ".repeat(depth);
match tree {
ParseTree::Rule(rule) => {
let name = rule_names
.get(rule.context().rule_index())
.map_or("<?>", AsRef::as_ref);
writeln!(
out,
"{pad}Rule({name}, children={})",
rule.context().children().len()
)?;
for child in rule.context().children() {
dump_tree(out, child, rule_names, depth + 1)?;
}
}
ParseTree::Terminal(token) => writeln!(out, "{pad}Term({:?})", token.text())?,
ParseTree::Error(token) => writeln!(out, "{pad}Err({:?})", token.text())?,
}
Ok(())
}
fn main() -> ExitCode {
let mut args = env::args().skip(1);
let mut input: Option<PathBuf> = None;
let mut tokens_only = false;
while let Some(arg) = args.next() {
match arg.as_str() {
"--input" => input = args.next().map(PathBuf::from),
"--tokens" => tokens_only = true,
other => {
eprintln!("unknown argument: {other}");
return ExitCode::from(2);
}
}
}
let Some(input) = input else {
eprintln!("missing --input <path>");
return ExitCode::from(2);
};
let source = match fs::read_to_string(&input) {
Ok(source) => source,
Err(error) => {
eprintln!("failed to read {}: {error}", input.display());
return ExitCode::FAILURE;
}
};
if tokens_only {
let lexer = JavaScriptLexer::with_typed_hooks(Found a 39 line (262 tokens) duplication in the following files:
impl JavaScriptParserBase {
fn raw_token<S>(ctx: &mut ParserSemCtx<'_, S>, index: usize) -> Option<(i32, i32, String)>
where
S: TokenSource,
{
ctx.token_at(index)
.map(|token| (token.channel(), token.token_type(), token.text().to_owned()))
}
fn has_line_terminator_ahead<S>(ctx: &mut ParserSemCtx<'_, S>) -> bool
where
S: TokenSource,
{
let current = ctx.input_index();
let Some(previous) = current.checked_sub(1) else {
return false;
};
let Some((channel, mut token_type, mut text)) = Self::raw_token(ctx, previous) else {
return false;
};
if channel != HIDDEN_CHANNEL {
return false;
}
if token_type == LINE_TERMINATOR {
return true;
}
if token_type == WHITE_SPACES {
let Some(before_whitespace) = previous.checked_sub(1) else {
return false;
};
let Some((_, next_type, next_text)) = Self::raw_token(ctx, before_whitespace) else {
return false;
};
token_type = next_type;
text = next_text;
}
token_type == LINE_TERMINATOR
|| (token_type == MULTI_LINE_COMMENT && (text.contains('\r') || text.contains('\n')))
}Found a 34 line (179 tokens) duplication in the following files:
atn,
|lexer, action| {
if !generated_action(lexer, action)
&& !dispatch_lexer_action_hook(&hooks, lexer, action)
&& unknown_policy == UnknownSemanticPolicy::Error
&& let (Ok(rule), Ok(index)) = (
usize::try_from(action.rule_index()),
usize::try_from(action.action_index()),
)
{
lexer.record_semantic_error(true, rule, index);
}
},
|lexer, predicate| {
generated_predicate(lexer, predicate)
.or_else(|| dispatch_lexer_predicate_hook(&hooks, lexer, predicate))
.unwrap_or_else(|| match unknown_policy {
UnknownSemanticPolicy::AssumeTrue => true,
UnknownSemanticPolicy::AssumeFalse => false,
UnknownSemanticPolicy::Error => {
lexer.record_semantic_error(
false,
predicate.rule_index(),
predicate.pred_index(),
);
false
}
})
},
accept_adjuster,
);
hooks.borrow_mut().lexer_token_emitted(&token);
token
}Found a 38 line (153 tokens) duplication in the following files:
fn is_strict_mode<I, F>(&mut self, _ctx: &mut LexerSemCtx<'_, I, F>) -> bool
where
I: CharStream,
F: TokenFactory,
{
self.use_strict_current
}
fn is_regex_possible<I, F>(&mut self, _ctx: &mut LexerSemCtx<'_, I, F>) -> bool
where
I: CharStream,
F: TokenFactory,
{
!matches!(
self.last_token_type,
Some(
IDENTIFIER
| NULL_LITERAL
| BOOLEAN_LITERAL
| THIS
| CLOSE_BRACKET
| CLOSE_PAREN
| OCTAL_INTEGER_LITERAL
| DECIMAL_LITERAL
| HEX_INTEGER_LITERAL
| STRING_LITERAL
| PLUS_PLUS
| MINUS_MINUS
)
)
}
fn is_in_template_string<I, F>(&mut self, _ctx: &mut LexerSemCtx<'_, I, F>) -> bool
where
I: CharStream,
F: TokenFactory,
{
self.template_depth_stack.last().copied() == Some(self.current_depth)Found 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 20 line (125 tokens) duplication in the following files:
JavaScriptLexerBase::with_strict_default(false),
);
let mut stream = CommonTokenStream::new(lexer);
stream.fill();
let errors = stream.drain_source_errors();
if !errors.is_empty() {
for error in errors {
eprintln!("line {}:{} {}", error.line, error.column, error.message);
}
return ExitCode::FAILURE;
}
for token in stream.tokens() {
if token.token_type() != TOKEN_EOF {
println!("{}\t{}\t{:?}", token.token_type(), token.channel(), token.text());
}
}
return ExitCode::SUCCESS;
}
let lexer = JavaScriptLexer::with_typed_hooks(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 17 line (111 tokens) duplication in the following files:
atn: &Atn,
hooks: &mut H,
mut generated_action: A,
mut generated_predicate: P,
unknown_policy: UnknownSemanticPolicy,
accept_adjuster: E,
) -> CommonToken
where
I: CharStream,
F: TokenFactory,
H: SemanticHooks,
A: FnMut(&mut BaseLexer<I, F>, LexerCustomAction) -> bool,
P: FnMut(&BaseLexer<I, F>, LexerPredicate) -> Option<bool>,
E: FnMut(&mut BaseLexer<I, F>, i32, usize),
{
let hooks = RefCell::new(hooks);
let token = next_token_with_hooks(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, |
![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 target-action support for the unmodified official JavaScript grammar by introducing grammar-agnostic runtime hook plumbing, updating the generator to emit typed lexer and parser adapters, and adding JavaScript-specific Rust base modules and parity tests. Feedback on the changes suggests avoiding direct indexing on byte slices to prevent potential panics, unescaping single-quoted strings consistently, using the more idiomatic entry API for map insertions, and applying lint suppressions as outer attributes on the generated module.
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.
| let mut literals = Vec::new(); | ||
| while !body.is_empty() { | ||
| if body.starts_with('"') || body.starts_with('\'') { | ||
| let quote = body.as_bytes()[0]; |
There was a problem hiding this comment.
Avoid direct indexing on collections (e.g., body.as_bytes()[0]) as it can cause a runtime panic if the collection is empty. Instead, use safe accessors like .first() and handle the None case, or use pattern matching in conditional guards to allow safe fall-through.
| let quote = body.as_bytes()[0]; | |
| let Some("e) = body.as_bytes().first() else { return None; }; |
References
- Avoid direct indexing on collections (e.g., 'alts[0]') as it can cause a runtime panic if the collection is empty. Instead, use safe accessors like '.first()' and handle the 'None' case, or use pattern matching in conditional guards (e.g., 'let Some(&val) = alts.first()') to allow safe fall-through.
| let value = if quote == b'"' { | ||
| unescape_semantic_string(raw)? | ||
| } else { | ||
| raw.to_owned() | ||
| }; |
There was a problem hiding this comment.
Single-quoted string literals in JavaScript/Rust also support standard escape sequences (like \n, \t, \\, \'). Since unescape_semantic_string already handles both single and double quotes, we should unescape single-quoted strings as well to ensure consistent behavior.
let value = unescape_semantic_string(raw)?;| if let Some(existing) = signatures.insert(&mapping.method_name, signature.clone()) | ||
| && existing != signature | ||
| { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::InvalidData, | ||
| format!( | ||
| "typed semantic helper {} has conflicting literal signatures {existing:?} and {signature:?}", | ||
| mapping.call.name | ||
| ), | ||
| )); | ||
| } |
There was a problem hiding this comment.
Using the entry API is more idiomatic in Rust and avoids cloning the signature on every iteration when there is no conflict, as well as avoiding overwriting the existing entry.
match signatures.entry(&mapping.method_name) {
std::collections::btree_map::Entry::Occupied(entry) => {
if entry.get() != &signature {
return Err(io::Error::new(
io::ErrorKind::InvalidData,
format!(
"typed semantic helper {} has conflicting literal signatures {:?} and {:?}",
mapping.call.name,
entry.get(),
signature
),
));
}
}
std::collections::btree_map::Entry::Vacant(entry) => {
entry.insert(signature);
}
}| mod generated { | ||
| #![allow(dead_code, unused_imports, unreachable_pub, unused_qualifications)] |
There was a problem hiding this comment.
Apply lint suppressions as outer attributes on the dedicated wrapped module rather than using inner file attributes (#![...]), to ensure the module structure is clean and does not pollute the inner scope's attributes.
| mod generated { | |
| #![allow(dead_code, unused_imports, unreachable_pub, unused_qualifications)] | |
| #[allow(dead_code, unused_imports, unreachable_pub, unused_qualifications)] | |
| mod generated { |
References
- For generated Rust code, apply lint suppressions (such as
allow(warnings, missing_docs, ...)) and#[rustfmt::skip]as outer attributes on a dedicated wrapped module rather than using inner file attributes (#![...]), to ensure the generated file is include-safe and does not pollute the parent module's attributes.
![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: 29458fd68c
ℹ️ 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".
| let has_semantic_hooks = !lexer_typed_hook_mappings.is_empty() | ||
| || actions | ||
| .iter() | ||
| .any(|(_, template)| matches!(template, ActionTemplate::Hook(_))) | ||
| || predicates |
There was a problem hiding this comment.
When a lexer custom action is marked hooked only by --sem-unknown=hook (for example with no --grammar) or by a [[coordinate]] dispose = "hook" override, there is no ActionTemplate::Hook and no typed mapping in actions. This leaves has_semantic_hooks false, so the generated next_token can take the plain next_token_compiled path and never offer the action to SemanticHooks, even though the manifest reports it as hooked and --require-full-semantics accepts it. Include hook-disposed action coordinates in this condition so semantic dispatch is used for those lexers.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/bin/antlr4-rust-gen.rs (3)
9157-9167: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not bypass declared helper kind and argument constraints.
A call that fails declared matching—such as
n(true)forn(string), or a lexer-only helper used in a parser—still enters the generic typed-hook fallback becauseparsedisNone. Reject declaration mismatches; only use generic fallback when no declaration exists for that helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bin/antlr4-rust-gen.rs` around lines 9157 - 9167, The ParserPredicate handling around parse_semantic_helper_call must distinguish an undeclared helper from a declared helper whose kind or arguments fail validation. Reject declared mismatches instead of treating parsed.is_none() as a generic typed-hook fallback; allow that fallback only when no declaration exists, while preserving forced hooks and valid declared matches.
1905-1951: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInvoke
lexer_token_emittedfor every lexer created with hooks.Only the semantic-dispatch branch touches
self.hooks. A custom hook passed throughwith_hooksreceives no token callback when the grammar has no mapped semantics or uses only generated dispatch. Wrap the legacy branches and invoke the callback exactly once.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bin/antlr4-rust-gen.rs` around lines 1905 - 1951, Update the `next_token_call` construction so every hooks-enabled lexer invokes `self.hooks.lexer_token_emitted` exactly once per emitted token, including the `next_token_compiled` and `next_token_compiled_with_hooks` branches that currently bypass `self.hooks`. Preserve the existing semantic-dispatch behavior and wrap the legacy branch results without changing action, predicate, or accept-position handling.
1783-1797: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve hook-disposed lexer actions in the dispatch inventory.
Line 1785 removes every overridden action. A
dispose = "hook"action then disappears from typed mappings,has_semantic_hooks, andrun_action, so it may never reachH. Unsupported actions are also rejected before this override is applied. Apply coordinate overrides before rejection and retain an explicit hook-coordinate dispatch marker.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bin/antlr4-rust-gen.rs` around lines 1783 - 1797, Update the action-processing flow around coordinate_override so overrides are applied before unsupported-action rejection. Preserve entries whose coordinate override is dispose = "hook" by retaining an explicit hook dispatch marker in the action inventory, allowing typed mappings, has_semantic_hooks, and run_action to route them to H; continue rejecting genuinely unsupported actions.
🤖 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 @.github/workflows/javascript-parity.yml:
- Line 29: Pin the checkout, setup-java, and setup-python actions in the
workflow to their full immutable commit SHAs instead of mutable version tags.
Retain each action’s current version as an inline comment for maintainability,
updating the action references at the locations corresponding to uses:
actions/checkout, actions/setup-java, and actions/setup-python.
In `@docs/javascript-build.md`:
- Around line 16-18: Update the ANTLR download instructions in
docs/javascript-build.md to compute and validate the JAR’s SHA-256 checksum
using the same expected digest and validation approach as
.github/workflows/javascript-parity.yml, placing verification after curl and
before any Java invocation.
In `@src/bin/antlr4-rust-gen.rs`:
- Around line 9007-9015: Update the lexer typed-hook mapping flow around
validate_typed_hook_signatures and the mappings collection to validate
signatures before sorting and deduplicating. Key validation by normalized method
name and LexerTypedHookKind, and reject conflicting literal signatures so the
method map and generated dispatch arms cannot disagree. Apply the same
validation to the corresponding hook-processing block near the additional
referenced range.
In `@src/lexer.rs`:
- Around line 778-806: Clear semantic_error_coordinates at the start of each
token in begin_token so deduplication applies only within the current token
boundary. Leave record_semantic_error’s coordinate tracking unchanged and
preserve drain_errors behavior.
In `@tests/javascript-parity/dumper/.gitignore`:
- Around line 1-2: Update the tests/javascript-parity/dumper Cargo configuration
so its Cargo.lock is committed for reproducible locked builds: remove
/Cargo.lock from the relevant .gitignore, and generate or add the lockfile if it
is absent. Preserve the existing /target/ ignore rule.
In `@tests/javascript-parity/run.sh`:
- Around line 56-75: Add the --locked flag to both cargo run invocations for
antlr4-rust-gen and the cargo build invocation for the dumper, preserving their
existing arguments and command flow.
---
Outside diff comments:
In `@src/bin/antlr4-rust-gen.rs`:
- Around line 9157-9167: The ParserPredicate handling around
parse_semantic_helper_call must distinguish an undeclared helper from a declared
helper whose kind or arguments fail validation. Reject declared mismatches
instead of treating parsed.is_none() as a generic typed-hook fallback; allow
that fallback only when no declaration exists, while preserving forced hooks and
valid declared matches.
- Around line 1905-1951: Update the `next_token_call` construction so every
hooks-enabled lexer invokes `self.hooks.lexer_token_emitted` exactly once per
emitted token, including the `next_token_compiled` and
`next_token_compiled_with_hooks` branches that currently bypass `self.hooks`.
Preserve the existing semantic-dispatch behavior and wrap the legacy branch
results without changing action, predicate, or accept-position handling.
- Around line 1783-1797: Update the action-processing flow around
coordinate_override so overrides are applied before unsupported-action
rejection. Preserve entries whose coordinate override is dispose = "hook" by
retaining an explicit hook dispatch marker in the action inventory, allowing
typed mappings, has_semantic_hooks, and run_action to route them to H; continue
rejecting genuinely unsupported actions.
🪄 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: 47fb4124-b38e-4539-aedf-a13f4b1f02a3
⛔ Files ignored due to path filters (1)
tests/javascript-parity/dumper/src/generated/.gitignoreis excluded by!**/generated/**
📒 Files selected for processing (23)
.github/workflows/javascript-parity.ymlREADME.mddocs/issue-63-javascript-target-actions-plan.mddocs/javascript-build.mdpatterns/javascript.tomlsrc/atn/lexer.rssrc/bin/antlr4-rust-gen.rssrc/lexer.rssrc/parser.rstests/javascript-parity/README.mdtests/javascript-parity/dump_python.pytests/javascript-parity/dumper/.gitignoretests/javascript-parity/dumper/Cargo.tomltests/javascript-parity/dumper/src/javascript_lexer_base.rstests/javascript-parity/dumper/src/javascript_parser_base.rstests/javascript-parity/dumper/src/main.rstests/javascript-parity/run.shtests/javascript-parity/snippets/01-hashbang.jstests/javascript-parity/snippets/02-regex-vs-division.jstests/javascript-parity/snippets/03-strict-mode.jstests/javascript-parity/snippets/04-template-nesting.jstests/javascript-parity/snippets/05-line-terminators.jstests/javascript-parity/snippets/06-class-lookahead.js
tinovyatkin
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b0039cdc5
ℹ️ 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".
| let kind = fields | ||
| .remove("kind") | ||
| .map_or(Ok(SemanticsKind::ParserPredicate), |value| { |
There was a problem hiding this comment.
When an existing --sem-patterns file uses the pre-existing [[helper]] syntax without a kind, this default now scopes it only to parser predicates. Since helper matching was changed to require helper.kind == kind, those legacy helpers no longer match lexer predicates, so a lexer helper that previously lowered to hook or another template silently falls through to the unknown policy (often assume-true) instead of preserving the intended semantics. Treat omitted kind as the old wildcard behavior or otherwise keep lexer helpers compatible.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb4ade4af1
ℹ️ 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".
| parse_semantic_helper_call(body, kind) | ||
| .filter(|call| helper_call_matches(call, helper)) | ||
| .map(|_| helper) |
There was a problem hiding this comment.
When a semantic helper with a concrete lowering (for example lower = "bool(true)", bool(false), or a lookahead expression) is used as a negated predicate such as !this.foo(), parse_semantic_helper_call records call.negated but this path discards the call and applies the same helper.lower as for this.foo(). Hook adapters later account for call.negated, but non-hook helper lowerings now generate the un-negated result, so grammars using negated helper predicates can take the wrong lexer/parser alternative.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
README.md (1)
480-486: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winEscape the pipe in the table cell.
The
n("get"|"set")expression contains an unescaped|, so Markdown parses the row as three cells and breaks the table. Escape it asn("get"\|"set")or rewrite the text without a pipe.🤖 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 `@README.md` around lines 480 - 486, Update the README table cell containing the n("get"|"set") expression by escaping the pipe character or rewriting the expression without a pipe, ensuring Markdown continues to parse the row as a single cell.Source: Linters/SAST tools
tests/javascript-parity/run.sh (1)
11-19: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate option values before reading
$2.With
set -u, invoking the script with--antlr-jar,--grammars-v4,--work-dir, or--pythonwithout a value dereferences an unset$2and exits with a shell error rather than the intended usage error. Check"$#"before eachshift 2.Proposed fix
case "$1" in - --antlr-jar) ANTLR4_JAR="$2"; shift 2 ;; + --antlr-jar) + [ "$#" -ge 2 ] || { echo "--antlr-jar requires a value" >&2; exit 2; } + ANTLR4_JAR="$2"; shift 2 ;;Apply the same guard to the other value-taking options.
🤖 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 `@tests/javascript-parity/run.sh` around lines 11 - 19, Update the argument parsing case in the script’s option loop to validate that at least two arguments remain before reading “$2” or performing “shift 2” for ANTLR4_JAR, GRAMMARS_V4, WORK_DIR, and PYTHON. Route missing values through the existing intended usage-error behavior instead of allowing set -u to fail.
🤖 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 @.github/workflows/typescript-parity.yml:
- Around line 3-8: Add a top-level concurrency configuration to the
typescript-parity workflow, using a stable group keyed to the workflow and
relevant branch or pull-request reference, and set cancel-in-progress to true so
superseded runs are canceled while distinct refs remain isolated.
In `@tests/typescript-parity/dumper/.gitignore`:
- Line 1: Update the tests/typescript-parity/dumper .gitignore entries to
include src/generated/ alongside /target/, preventing generated recognizer
modules from being staged.
In `@tests/typescript-parity/dumper/src/typescript_lexer_base.rs`:
- Around line 90-99: Update process_close_brace to decrement braces_depth with
saturating subtraction instead of ordinary subtraction, preserving zero for
unmatched closing braces while leaving strict-mode scope restoration unchanged.
- Around line 132-138: Update decrease_template_depth to use saturating
subtraction when decrementing template_depth, matching the existing braces_depth
underflow protection while preserving normal depth reduction.
---
Outside diff comments:
In `@README.md`:
- Around line 480-486: Update the README table cell containing the
n("get"|"set") expression by escaping the pipe character or rewriting the
expression without a pipe, ensuring Markdown continues to parse the row as a
single cell.
In `@tests/javascript-parity/run.sh`:
- Around line 11-19: Update the argument parsing case in the script’s option
loop to validate that at least two arguments remain before reading “$2” or
performing “shift 2” for ANTLR4_JAR, GRAMMARS_V4, WORK_DIR, and PYTHON. Route
missing values through the existing intended usage-error behavior instead of
allowing set -u to fail.
🪄 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: c45d308f-c09e-4223-a9ad-7cd44a46115e
⛔ Files ignored due to path filters (3)
tests/javascript-parity/dumper/Cargo.lockis excluded by!**/*.locktests/typescript-parity/dumper/Cargo.lockis excluded by!**/*.locktests/typescript-parity/dumper/src/generated/.gitignoreis excluded by!**/generated/**
📒 Files selected for processing (25)
.github/workflows/javascript-parity.yml.github/workflows/typescript-parity.ymlREADME.mddocs/issue-63-javascript-target-actions-plan.mddocs/javascript-build.mddocs/typescript-build.mdpatterns/javascript.tomlsrc/bin/antlr4-rust-gen.rssrc/lexer.rstests/javascript-parity/dumper/.gitignoretests/javascript-parity/dumper/src/main.rstests/javascript-parity/run.shtests/typescript-parity/README.mdtests/typescript-parity/TypeScriptParityDumper.javatests/typescript-parity/dumper/.gitignoretests/typescript-parity/dumper/Cargo.tomltests/typescript-parity/dumper/src/main.rstests/typescript-parity/dumper/src/typescript_lexer_base.rstests/typescript-parity/dumper/src/typescript_parser_base.rstests/typescript-parity/run.shtests/typescript-parity/snippets/01-types.tstests/typescript-parity/snippets/02-contextual-helpers.tstests/typescript-parity/snippets/03-template-nesting.tstests/typescript-parity/snippets/04-line-terminators.tstests/typescript-parity/snippets/05-strict-mode.ts
💤 Files with no reviewable changes (1)
- tests/javascript-parity/dumper/.gitignore
| on: | ||
| pull_request: | ||
| push: | ||
| branches: | ||
| - main | ||
| workflow_dispatch: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a concurrency group to cancel superseded runs.
Without a concurrency setting, multiple pushes to the same PR can trigger parallel workflow runs that waste CI minutes. Adding a concurrency group with cancel-in-progress: true cancels outdated runs.
♻️ Suggested concurrency group
on:
pull_request:
push:
branches:
- main
workflow_dispatch:
+concurrency:
+ group: typescript-parity-${{ github.ref }}
+ cancel-in-progress: true
+
permissions:
contents: read📝 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.
| on: | |
| pull_request: | |
| push: | |
| branches: | |
| - main | |
| workflow_dispatch: | |
| on: | |
| pull_request: | |
| push: | |
| branches: | |
| - main | |
| workflow_dispatch: | |
| concurrency: | |
| group: typescript-parity-${{ github.ref }} | |
| cancel-in-progress: true |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 3-8: insufficient job-level concurrency limits (concurrency-limits): workflow is missing concurrency setting
(concurrency-limits)
🤖 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 @.github/workflows/typescript-parity.yml around lines 3 - 8, Add a top-level
concurrency configuration to the typescript-parity workflow, using a stable
group keyed to the workflow and relevant branch or pull-request reference, and
set cancel-in-progress to true so superseded runs are canceled while distinct
refs remain isolated.
Source: Linters/SAST tools
| @@ -0,0 +1 @@ | |||
| /target/ | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add src/generated/ to .gitignore.
The README states generated recognizer modules are "not committed," but src/generated/ is missing from .gitignore. An accidental git add . after running run.sh would stage generated files.
🛡️ Proposed fix
/target/
+src/generated/📝 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.
| /target/ | |
| /target/ | |
| src/generated/ |
🤖 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 `@tests/typescript-parity/dumper/.gitignore` at line 1, Update the
tests/typescript-parity/dumper .gitignore entries to include src/generated/
alongside /target/, preventing generated recognizer modules from being staged.
| fn process_close_brace<I, F>(&mut self, _ctx: &mut LexerSemCtx<'_, I, F>) | ||
| where | ||
| I: CharStream, | ||
| F: TokenFactory, | ||
| { | ||
| self.braces_depth -= 1; | ||
| self.use_strict_current = self | ||
| .pop_strict_mode_scope() | ||
| .unwrap_or(self.use_strict_default); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
Guard against braces_depth underflow in process_close_brace.
self.braces_depth -= 1 wraps silently in release mode for malformed input with unbalanced braces, which could cause is_in_template_string to return incorrect results. A saturating subtraction prevents this.
♻️ Proposed fix
fn process_close_brace<I, F>(&mut self, _ctx: &mut LexerSemCtx<'_, I, F>)
where
I: CharStream,
F: TokenFactory,
{
- self.braces_depth -= 1;
+ self.braces_depth = self.braces_depth.saturating_sub(1);
self.use_strict_current = self
.pop_strict_mode_scope()
.unwrap_or(self.use_strict_default);
}📝 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.
| fn process_close_brace<I, F>(&mut self, _ctx: &mut LexerSemCtx<'_, I, F>) | |
| where | |
| I: CharStream, | |
| F: TokenFactory, | |
| { | |
| self.braces_depth -= 1; | |
| self.use_strict_current = self | |
| .pop_strict_mode_scope() | |
| .unwrap_or(self.use_strict_default); | |
| } | |
| fn process_close_brace<I, F>(&mut self, _ctx: &mut LexerSemCtx<'_, I, F>) | |
| where | |
| I: CharStream, | |
| F: TokenFactory, | |
| { | |
| self.braces_depth = self.braces_depth.saturating_sub(1); | |
| self.use_strict_current = self | |
| .pop_strict_mode_scope() | |
| .unwrap_or(self.use_strict_default); | |
| } |
🤖 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 `@tests/typescript-parity/dumper/src/typescript_lexer_base.rs` around lines 90
- 99, Update process_close_brace to decrement braces_depth with saturating
subtraction instead of ordinary subtraction, preserving zero for unmatched
closing braces while leaving strict-mode scope restoration unchanged.
| fn decrease_template_depth<I, F>(&mut self, _ctx: &mut LexerSemCtx<'_, I, F>) | ||
| where | ||
| I: CharStream, | ||
| F: TokenFactory, | ||
| { | ||
| self.template_depth -= 1; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
Guard against template_depth underflow in decrease_template_depth.
Same underflow risk as braces_depth. Use saturating subtraction for consistency.
♻️ Proposed fix
fn decrease_template_depth<I, F>(&mut self, _ctx: &mut LexerSemCtx<'_, I, F>)
where
I: CharStream,
F: TokenFactory,
{
- self.template_depth -= 1;
+ self.template_depth = self.template_depth.saturating_sub(1);
}📝 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.
| fn decrease_template_depth<I, F>(&mut self, _ctx: &mut LexerSemCtx<'_, I, F>) | |
| where | |
| I: CharStream, | |
| F: TokenFactory, | |
| { | |
| self.template_depth -= 1; | |
| } | |
| fn decrease_template_depth<I, F>(&mut self, _ctx: &mut LexerSemCtx<'_, I, F>) | |
| where | |
| I: CharStream, | |
| F: TokenFactory, | |
| { | |
| self.template_depth = self.template_depth.saturating_sub(1); | |
| } |
🤖 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 `@tests/typescript-parity/dumper/src/typescript_lexer_base.rs` around lines 132
- 138, Update decrease_template_depth to use saturating subtraction when
decrementing template_depth, matching the existing braces_depth underflow
protection while preserving normal depth reduction.
1 participant
Summary
Root cause
The official JavaScript and TypeScript grammars rely on stateful target-specific members, actions, and semantic predicates. Generated lexers did not own a semantic-hook implementation, and the typed parser-hook path only supported zero-argument predicates, so calls such as
p("of")andn("get"|"set")could not be represented in a working Rust parser.TypeScript coverage
pandnStartTemplateString,IncreaseTemplateDepth, andDecreaseTemplateDepth--require-full-semanticsValidation
cargo test --lockedcargo clippy --locked --all-targets --all-features -- -D warningsCloses #63