Add flat indexed CST - #90
Conversation
|
Warning Review limit reached
Next review available in: 33 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThe runtime now builds flat, indexed CSTs with pooled child ranges and borrowing node views. Parser construction, semantic hooks, rollback, listeners, generated contexts, casting, walking, and parse helpers use the new storage model. Public exports and Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Copy/Paste DetectionFound 13 duplication(s) across 8 changed Rust file(s) (threshold: 100 tokens). Show duplicationsFound a 66 line (387 tokens) duplication in the following files:
use javascript_parser_base::JavaScriptParserBase;
fn dump_tree<S: AsRef<str>>(
out: &mut dyn Write,
tree: Node<'_>,
rule_names: &[S],
depth: usize,
) -> io::Result<()> {
let pad = " ".repeat(depth);
match tree.kind() {
NodeKind::Rule => {
let rule = tree.as_rule().expect("rule node kind checked");
let name = rule_names
.get(rule.rule_index())
.map_or("<?>", AsRef::as_ref);
writeln!(
out,
"{pad}Rule({name}, children={})",
rule.child_count()
)?;
for child in rule.children() {
dump_tree(out, child, rule_names, depth + 1)?;
}
}
NodeKind::Terminal => writeln!(
out,
"{pad}Term({:?})",
tree.as_terminal().expect("terminal node kind checked").text()
)?,
NodeKind::Error => writeln!(
out,
"{pad}Err({:?})",
tree.as_error().expect("error node kind checked").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 27 line (145 tokens) duplication in the following files:
fn generated_match_token_recovers_missing_token_from_context_follow() {
let atn = generated_match_recovery_atn();
let data = RecognizerData::new(
"Mini.g4",
Vocabulary::new(
[None, Some("'X'"), Some("'Y'")],
[None, Some("X"), Some("Y")],
[None::<&str>, None, None],
),
);
let mut parser = BaseParser::new(
CommonTokenStream::new(Source {
tokens: vec![TestToken::eof("parser-test", 3, 1, 3)],
index: 0,
}),
data,
);
parser.rule_context_stack = vec![
RuleContextFrame {
rule_index: 0,
invoking_state: 0,
},
RuleContextFrame {
rule_index: 1,
invoking_state: 1,
},
];Found a 25 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 29 line (123 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 {
let boundary = self.arena_boundary_node(rule_index);
self.arena_prepend(&mut outcome.nodes, boundary);
}
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 21 line (113 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();
self.reset_recognition_arena();
let caller_follow_state = self.pending_invoking_follow_state(atn);Found a 15 line (113 tokens) duplication in the following files:
fn generated_match_token_counts_single_token_deletion_recovery() {
let atn = generated_match_recovery_atn();
let data = RecognizerData::new(
"Mini.g4",
Vocabulary::new(
[None, Some("'X'"), Some("'Y'"), Some("'Z'")],
[None, Some("X"), Some("Y"), Some("Z")],
[None::<&str>, None, None, None],
),
);
let mut parser = BaseParser::new(
CommonTokenStream::new(Source {
tokens: vec![
TestToken::new(3).with_text("z"),
TestToken::new(2).with_text("y"),Found a 21 line (113 tokens) duplication in the following files:
dump_tree(out, child, rule_names, depth + 1)?;
}
}
NodeKind::Terminal => writeln!(
out,
"{pad}Term({:?})",
tree.as_terminal().expect("terminal node kind checked").text()
)?,
NodeKind::Error => writeln!(
out,
"{pad}Err({:?})",
tree.as_error().expect("error node kind checked").text()
)?,
}
Ok(())
}
fn main() -> ExitCode {
let mut args = env::args().skip(1);
let mut input: Option<PathBuf> = None;
let mut tokens_only = false;Found a 14 line (110 tokens) duplication in the following files:
fn parser_matches_token_and_reports_mismatch() {
let source = Source {
tokens: vec![
TestToken::new(1).with_text("x"),
TestToken::eof("parser-test", 1, 1, 1),
],
index: 0,
};
let data = RecognizerData::new(
"Mini.g4",
Vocabulary::new([None, Some("'x'")], [None, Some("X")], [None::<&str>, None]),
);
let mut parser = BaseParser::new(CommonTokenStream::new(source), data);
let matched = parser.match_token(1).expect("token 1 should match");Found a 13 line (109 tokens) duplication in the following files:
fn parser_matches_token_and_reports_mismatch() {
let source = Source {
tokens: vec![
TestToken::new(1).with_text("x"),
TestToken::eof("parser-test", 1, 1, 1),
],
index: 0,
};
let data = RecognizerData::new(
"Mini.g4",
Vocabulary::new([None, Some("'x'")], [None, Some("X")], [None::<&str>, None]),
);
let mut parser = BaseParser::new(CommonTokenStream::new(source), data);Found a 22 line (108 tokens) duplication in the following files:
) -> Option<RecognizeOutcome> {
let (error_index, message) = self.expected_error_message(rule_index, start_index, expected);
let diagnostic = diagnostic_for_token(self.token_at(error_index), message);
let mut next_index = error_index;
loop {
let symbol = self.token_type_at(next_index);
if sync_symbols.contains(&symbol) {
if next_index == error_index {
return None;
}
break;
}
if symbol == TOKEN_EOF {
break;
}
let after = self.consume_index(next_index, symbol);
if after == next_index {
break;
}
next_index = after;
}
let mut nodes = NodeSeqId::EMPTY;Found a 15 line (108 tokens) duplication in the following files:
fn outcome_ties_keep_later_non_recursive_alternative() {
let arena = RecognitionArena::default();
let first = RecognizeOutcome {
index: 1,
consumed_eof: false,
alt_number: 0,
member_values: BTreeMap::new(),
return_values: BTreeMap::new(),
diagnostics: DiagnosticSeqId::EMPTY,
decisions: Vec::new(),
actions: vec![ParserAction::new(1, 0, 0, None)],
nodes: NodeSeqId::EMPTY,
};
let second = RecognizeOutcome {
actions: vec![ParserAction::new(2, 0, 0, None)],Found a 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 replaces the recursive owning parse trees in the ANTLR Rust runtime with a flat, NodeId-addressed concrete syntax tree (CST) storage (ParseTreeStorage). This change significantly reduces allocation overhead and peak memory usage during parsing. The public tree access APIs have been migrated to borrowing views (Node, RuleNodeView, TerminalNodeView, and ErrorNodeView), and the generated parser and listener traversal logic have been updated accordingly. Feedback on the changes suggests using direct as casts instead of try_from with .expect() when storing rule indices, invoking states, and alternative numbers to avoid unnecessary panic branches and improve performance consistency.
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.
| payload_a: u32::try_from(context.rule_index).expect("rule index exceeds u32"), | ||
| payload_b: i32::try_from(context.invoking_state) | ||
| .expect("invoking state exceeds i32") | ||
| .cast_unsigned(), | ||
| start: context.start.map_or(NONE, |token| token.index() as u32), | ||
| stop: context.stop.map_or(NONE, |token| token.index() as u32), | ||
| alt_number: u32::try_from(context.alt_number).expect("alternative number exceeds u32"), |
There was a problem hiding this comment.
The rule index, invoking state, and alternative number are internally bounded values managed by the parser, which are guaranteed to fit within standard integer limits. Using try_from with .expect() here introduces unnecessary panic branches and runtime overhead. Since the retrieval side (e.g., rule_index(), invoking_state(), alt_number()) already uses direct as casts, we should use direct as casts on the storage side as well to keep the code consistent, clean, and performant.
| payload_a: u32::try_from(context.rule_index).expect("rule index exceeds u32"), | |
| payload_b: i32::try_from(context.invoking_state) | |
| .expect("invoking state exceeds i32") | |
| .cast_unsigned(), | |
| start: context.start.map_or(NONE, |token| token.index() as u32), | |
| stop: context.stop.map_or(NONE, |token| token.index() as u32), | |
| alt_number: u32::try_from(context.alt_number).expect("alternative number exceeds u32"), | |
| payload_a: context.rule_index as u32, | |
| payload_b: (context.invoking_state as i32).cast_unsigned(), | |
| start: context.start.map_or(NONE, |token| token.index() as u32), | |
| stop: context.stop.map_or(NONE, |token| token.index() as u32), | |
| alt_number: context.alt_number as u32, |
There was a problem hiding this comment.
Keeping the checked conversions here. These fields cross the compact u32/i32 storage boundary, so direct as casts would silently truncate oversized rule indices, invoking states, or alternative numbers and corrupt the indexed CST. The retrieval-side casts only widen already-validated stored values and therefore are not symmetric. This conversion runs once per completed rule, outside token matching and traversal hot paths. The other review fixes are pushed in 0ddf87667; full tests, strict clippy, and the 356-case runtime sweep pass.
263a742 to
1a42caf
Compare
1a42caf to
74394d3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/parser.rs (1)
4346-4368: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
build_parse_trees(false)no longer bounds flat-CST growth for the generated/direct-descent path.
match_token/match_set/match_not_set/match_wildcard/match_eof(and thematch_*_recoveringfamily) callself.terminal_tree(...)/self.error_tree(...)unconditionally, regardless ofself.build_parse_trees. In the new flatParseTreeStorage, "creating a node" is a permanent append across ~10 parallel arrays, not a cheap value that gets dropped — so every matched token still growsself.treeeven when parse-tree construction is disabled.add_parse_childonly gates whether the node is linked into its parent, not whether it was created at all.This is inconsistent with the rest of the diff:
DirectAdaptiveParser::consume_transitioncorrectly gates terminal creation withself.parser.build_parse_trees.then(|| self.parser.terminal_tree(token)), andparse_atn_rule_with_precedence/parse_atn_rule_with_runtime_options_and_precedencegate the arena→tree conversion behindif self.build_parse_trees. Since generated recursive-descent rule methods (the primary/fast codegen path this PR optimizes) callmatch_token/match_token_recoveringdirectly,Parser::set_build_parse_trees(false)no longer meaningfully bounds memory for that path on large inputs — defeating the documented purpose of the flag and undercutting this PR's own memory/allocation goals.🛡️ Direction for a fix
- pub fn match_token(&mut self, token_type: i32) -> Result<ParseTree, AntlrError> { + pub fn match_token(&mut self, token_type: i32) -> Result<ParseTree, AntlrError> { let current = self.input.lt_id(1).ok_or_else(|| AntlrError::ParserError { line: 0, column: 0, message: "missing current token".to_owned(), })?; let current_type = self.token_type_for_id(current); if current_type == token_type { self.consume(); - Ok(self.terminal_tree(current)) + Ok(if self.build_parse_trees { + self.terminal_tree(current) + } else { + self.placeholder_tree() + })
placeholder_tree()would need to return some cheap, reusable sentinelNodeIdthat is never dereferenced when trees are off (mirroring howDirectAdaptiveParser::consume_transitionreturnsNoneand lets the caller skip linking). The same gating would need to apply tomatch_set,match_not_set,match_wildcard,match_interval_condition, and thematch_*_recoveringfamily.Also applies to: 4590-4630, 5110-5125
🤖 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 4346 - 4368, Gate terminal and error node creation in Parser matching helpers on build_parse_trees, including match_token, match_set, match_not_set, match_wildcard, match_interval_condition, match_eof, and the match_*_recovering family. When disabled, return a cheap reusable placeholder NodeId without appending to ParseTreeStorage, while preserving existing matching, recovery, and linking behavior when enabled.src/bin/antlr4-rust-gen.rs (2)
6811-6849: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winForward error nodes through the embedded listener bridge.
__ListenerBridgeinherits the runtimevisit_error_nodeno-op, while the generated listener trait only exposesvisit_terminal, so recovery error nodes are dropped instead of reaching user code. Add an embedded-mode error-node callback and wire it through the bridge.🤖 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 6811 - 6849, The embedded listener bridge in __ListenerBridge currently drops recovery error nodes because it only forwards visit_terminal. Add visit_error_node to the generated listener trait and implement the corresponding ParseTreeListener callback in __ListenerBridge, converting the runtime error node to the generated listener-facing type and forwarding it to the user listener.
9402-9414: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRename the catch-all binding to
tree
The emittedrun_actionbody uses_treeinparser_action_hook(...), which triggersclippy::used_underscore_bindingin the generated parser. Update the snapshot test that asserts the old string.🤖 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 9402 - 9414, Rename the emitted run_action parameter from _tree to tree in render_parser_action_method, and pass tree to parser_action_hook while preserving the no-action signature behavior. Update the snapshot test expected string to match the new generated parameter and hook call.
🤖 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.
Outside diff comments:
In `@src/bin/antlr4-rust-gen.rs`:
- Around line 6811-6849: The embedded listener bridge in __ListenerBridge
currently drops recovery error nodes because it only forwards visit_terminal.
Add visit_error_node to the generated listener trait and implement the
corresponding ParseTreeListener callback in __ListenerBridge, converting the
runtime error node to the generated listener-facing type and forwarding it to
the user listener.
- Around line 9402-9414: Rename the emitted run_action parameter from _tree to
tree in render_parser_action_method, and pass tree to parser_action_hook while
preserving the no-action signature behavior. Update the snapshot test expected
string to match the new generated parameter and hook call.
In `@src/parser.rs`:
- Around line 4346-4368: Gate terminal and error node creation in Parser
matching helpers on build_parse_trees, including match_token, match_set,
match_not_set, match_wildcard, match_interval_condition, match_eof, and the
match_*_recovering family. When disabled, return a cheap reusable placeholder
NodeId without appending to ParseTreeStorage, while preserving existing
matching, recovery, and linking behavior when enabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6f4cebad-5a52-4301-b424-f80b17124e64
📒 Files selected for processing (12)
.conformance-review/Rust.test.stg.conformance-review/rust-test-stg-honest-reference-gap.mdCHANGELOG.mdREADME.mdsrc/bin/antlr4-rust-gen.rssrc/bin_support/embedded.rssrc/lib.rssrc/parser.rssrc/tree.rstests/javascript-parity/dumper/src/main.rstests/kotlin-parity/dumper/src/main.rstests/typescript-parity/dumper/src/main.rs
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74394d3fa2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if self.build_parse_trees { | ||
| context.add_child(child); | ||
| self.tree.add_child(context, child); | ||
| } else { | ||
| context.note_matched_child(); |
There was a problem hiding this comment.
When generated parsers run with set_build_parse_trees(false), token matches and child rule calls have already appended NodeIds to self.tree before add_parse_child is called; this branch only marks the context and never removes those orphan nodes. That means the no-tree mode still retains terminal/error/rule records in ParseTreeStorage, so memory grows with the full input even though the children are not linked. Please avoid creating these nodes while build_parse_trees is false or roll back the storage when taking this branch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0ddf87667. terminal_tree, error_tree, and rule_node now return one unresolvable sentinel while tree building is disabled, so generated token matches, recovery nodes, and completed child rules do not append to ParseTreeStorage. disabled_tree_building_does_not_grow_flat_storage covers repeated matches, an error node, and rule completion and asserts zero nodes/edges/extras/scratch allocation. Full tests, strict clippy, and the runtime sweep (356 passed, 0 failed) pass.
|
Addressed the three outside-diff CodeRabbit findings in
Validation passed:
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ddf876670
ℹ️ 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 _ = writeln!( | ||
| out, | ||
| "impl std::fmt::Display for {view_name}<'_> {{\n fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {{\n let chain: Vec<String> = self.__chain.iter().take(self.__chain.len().saturating_sub(1)).map(|state| state.to_string()).collect();\n write!(f, \"[{{}}]\", chain.join(\" \"))\n }}\n}}\n" | ||
| "impl std::fmt::Display for {view_name}<'_> {{\n fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {{\n let chain: Vec<String> = match &self.__node {{\n __GeneratedRuleContext::Stored(node) => node.invocation_states().map(|state| state.to_string()).collect(),\n __GeneratedRuleContext::Active {{ .. }} => Vec::new(),\n }};\n write!(f, \"[{{}}]\", chain.join(\" \"))\n }}\n}}\n" |
There was a problem hiding this comment.
When an embedded action casts an active $ctx through the new __active_context_view and formats the resulting typed context, this branch always renders []. Active rules have not been finished yet, so the flat tree parent links used by the stored case are not available; the previous generated views carried the invoking-state chain explicitly, and the comment above still promises Java RuleContext.toString parity. Actions/descriptors that print a cast active context will therefore lose the caller stack until the generated active view records the current invocation chain.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 537fc1a38. Active generated context views now capture BaseParser::active_invocation_states() and render the current-to-root invoking-state chain, excluding the root sentinel to match stored RuleNodeView and Java RuleContext.toString() behavior. Added parser-stack and generated-output regressions. Validation passed: cargo test --locked --all-targets --all-features, strict all-target/all-feature clippy, cargo fmt --all -- --check, focused LeftRecursion/MultipleAlternativesWithCommonLabel_3, and the full runtime sweep (356 passed, 0 failed, 1 known skip).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 537fc1a380
ℹ️ 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".
| std::iter::successors(Some(self), |rule| rule.node.parent()?.as_rule()) | ||
| .map(Self::invoking_state) | ||
| .take_while(|state| *state >= 0) |
There was a problem hiding this comment.
When a generated top-level rule finishes, its root context is created with the entry state as invoking_state rather than a -1 sentinel, so this iterator includes the root frame for stored child contexts. That differs from the old generated __chain[..len-1] behavior and Java RuleContext.toString(), causing listener/action code that formats child contexts (for example ctx.e_list(0)) to emit an extra root state and break parity output; exclude the final ancestor regardless of its sign.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1d029413b. RuleNodeView::invocation_states() now stops at the parentless root frame regardless of the root invoking-state value, while retaining the existing negative-state guard. The new invocation_states_exclude_a_nonnegative_root_frame regression builds a three-level stored tree with root state 4 and verifies [], [7], and [13, 7], matching the previous generated chain behavior and Java RuleContext.toString(). Validation passed: focused regression, cargo test --locked --all-targets --all-features, strict all-target/all-feature clippy, and cargo fmt --all -- --check.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d029413b9
ℹ️ 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 _ = writeln!( | ||
| accessors, | ||
| " pub fn {method}(&self, index: usize) -> Rc<{child_view}<'a>> {{ let node = self.__node.child_rules({child_index}).nth(index).expect(\"missing rule child\"); Rc::new({child_view}::__from_node_with_chain(node, self.__tokens, self.__chain.clone())) }}\n pub fn {method}_all(&self) -> Vec<Rc<{child_view}<'a>>> {{ self.__node.child_rules({child_index}).map(|node| Rc::new({child_view}::__from_node_with_chain(node, self.__tokens, self.__chain.clone()))).collect() }}" | ||
| " pub fn {method}(&self, index: usize) -> {child_view}<'a> {{\n let node = match &self.__node {{\n __GeneratedRuleContext::Stored(node) => node.child_rules({child_index}).nth(index),\n __GeneratedRuleContext::Active {{ context, storage, tokens, .. }} => context.child_rules(storage, tokens, {child_index}).nth(index),\n }}.expect(\"missing rule child\");\n {child_view}::__from_node(node)\n }}\n pub fn {method}_all(&self) -> Vec<{child_view}<'a>> {{\n let nodes: Vec<_> = match &self.__node {{\n __GeneratedRuleContext::Stored(node) => node.child_rules({child_index}).collect(),\n __GeneratedRuleContext::Active {{ context, storage, tokens, .. }} => context.child_rules(storage, tokens, {child_index}).collect(),\n }};\n nodes.into_iter().map({child_view}::__from_node).collect()\n }}" |
There was a problem hiding this comment.
When this generated accessor is called on an __GeneratedRuleContext::Active value, such as from an embedded @after action that casts $ctx and then reads ctx.child(0), it still wraps the child with __from_node(node). The enclosing rule has not reached finish_rule yet, and the flat storage only assigns parent links when that enclosing rule is finished, so the returned child view's Display/invocation_states() omits the active callers. The old implementation threaded self.__chain.clone() into child views; the active branch needs to preserve that invocation chain instead of treating the child as a fully stored subtree.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9018aab84. Generated typed views now own their effective invocation chain and child accessors prepend the child state to that chain, preserving active parent callers before flat-tree parent links exist. The embedded listener walker also has an active-chain mode seeded by BaseParser::active_invocation_states(), covering @after actions that walk a completed child subtree. This directly repairs the remote Listeners/LRWithLabels failure (1 [13] vs expected 1 [13 6]). Validation passed: focused generator regression, focused Listeners/LRWithLabels, full tests, strict clippy, formatting, and the full runtime sweep (356 passed, 0 failed, 1 known skip).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9018aab842
ℹ️ 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".
| } | ||
|
|
||
| #[must_use] | ||
| pub fn text(self) -> String { |
There was a problem hiding this comment.
With the flat CST API, Node::text now takes no token-store argument and public rule methods return NodeId, but the README quickstart snippets still call parsed.tree().text(parsed.tokens()), tree.text(&tokens), and tree.text(parser.token_store()). Anyone copying those examples after regenerating parsers will hit compile errors; update them to use parsed.tree().text() or resolve direct-rule IDs with parser.node(tree).text()/into_parsed_file(tree).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8d82ca3ad. The compact parse() example now uses parsed.tree().text(), the parse_with_parser example consumes the parser with into_parsed_file(root_id) before reading token count and text, and the manual entry-rule example resolves its returned NodeId through parser.node(root_id).text(). A repo search confirms no stale argument-taking text(...) calls remain in README.md; git diff --check and cargo test --locked --doc pass.
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! 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
Closes #84
Summary
NodeId-addressed, structure-of-arraysParseTreeStorageTokenId, and keep uncommon rule payloads in sparse side storageBreaking change
Generated parsers must be regenerated with the matching runtime/generator release. Direct rule methods now return
NodeId; callers resolve borrowing views throughparser.node(id)or consume the parser withinto_parsed_file(id).Performance
The final generated-only benchmark ran 20 measured parses after three warmups against
3bf4c598bacross 17 protected Kotlin, C#, Java, and Trino fixtures. Fourteen improved; changes ranged from-15.17%to+0.59%, with no regression approaching the 2% ceiling.On the OpenZeppelin Solidity fixtures, allocation calls fell by 4.23% to 11.61%, allocated bytes by 5.74% to 9.91%, peak live allocation by 40.63% to 63.03%, and median peak RSS by 4.20% to 14.18%. A 10,000-walk traversal of the 6,521-node Governor CST sustained about 326 million nodes/second.
Validation
cargo test --locked --all-targets --all-featurescargo clippy --locked --all-targets --all-features -- -D warningscargo fmt --all -- --check