store buffered tokens once by TokenId - #88
Conversation
|
Warning Review limit reached
Next review available in: 27 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 replaces owned buffered tokens with a canonical 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 21 duplication(s) across 19 changed Rust file(s) (threshold: 100 tokens). Show duplicationsFound a 58 line (383 tokens) duplication in the following files:
use javascript_parser_base::JavaScriptParserBase;
fn dump_tree<S: AsRef<str>>(
out: &mut dyn Write,
tree: &ParseTree,
tokens: &TokenStore,
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, tokens, rule_names, depth + 1)?;
}
}
ParseTree::Terminal(token) => writeln!(out, "{pad}Term({:?})", token.text(tokens))?,
ParseTree::Error(token) => writeln!(out, "{pad}Err({:?})", token.text(tokens))?,
}
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 38 line (198 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,
);
let token = token?;
hooks.borrow_mut().lexer_token_emitted(
sink.view(token)
.expect("lexer hook token should be present in its sink"),
);
Ok(token)
}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 24 line (135 tokens) duplication in the following files:
pub fn next_token_with_hooks<I, A, P, E>(
lexer: &mut BaseLexer<I>,
sink: &mut TokenSink<'_>,
atn: &Atn,
mut custom_action: A,
mut semantic_predicate: P,
mut accept_adjuster: E,
) -> Result<TokenId, TokenStoreError>
where
I: CharStream,
A: FnMut(&mut BaseLexer<I>, LexerCustomAction),
P: FnMut(&BaseLexer<I>, LexerPredicate) -> bool,
E: FnMut(&mut BaseLexer<I>, i32, usize),
{
next_token_with_hooks_impl(
lexer,
sink,
atn,
&mut custom_action,
&mut semantic_predicate,
&mut accept_adjuster,
LexerMatchStrategy {
compiled: None,
use_cache: false,Found a 24 line (134 tokens) duplication in the following files:
let atn = AtnDeserializer::new(&SerializedAtn::from_i32(&[
4, 0, 2, // version, lexer, max token type
9, // states
6, -1, // 0 token start
2, 0, // 1 rule 0 start
1, 0, // 2
1, 0, // 3
7, 0, // 4 rule 0 stop
2, 1, // 5 rule 1 start
1, 1, // 6
1, 1, // 7
7, 1, // 8 rule 1 stop
0, // non-greedy
0, // precedence
2, // rules
1, 1, // rule 0 starts at 1, token type 1
5, 2, // rule 1 starts at 5, token type 2
1, // modes
0, // default mode starts at 0
0, // sets
8, // edges
0, 1, 1, 0, 0, 0, // start -> rule 0
0, 5, 1, 0, 0, 0, // start -> rule 1
1, 2, 5, 'a' as i32, 0, 0, 2, 3, 5, 'b' as i32, 0, 0, 3, 4, 1, 0, 0, 0, 5, 6, 5,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 35 line (129 tokens) duplication in the following files:
fn is_strict_mode<I>(&mut self, _ctx: &mut LexerSemCtx<'_, I>) -> bool
where
I: CharStream,
{
self.use_strict_current
}
fn is_regex_possible<I>(&mut self, _ctx: &mut LexerSemCtx<'_, I>) -> bool
where
I: CharStream,
{
!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>(&mut self, _ctx: &mut LexerSemCtx<'_, I>) -> bool
where
I: CharStream,
{
self.template_depth_stack.last().copied() == Some(self.current_depth)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 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 20 line (119 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_id_at(*start_index) {
self.set_context_start(&mut context, token);
}
if let Some(token) = stop_index.and_then(|index| self.token_id_at(index)) {
self.set_context_stop(&mut context, token);
}
if children.has_left_recursive_boundary() {
let folded = fold_fast_left_recursive_boundaries(children.to_vec());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 (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 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 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![
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 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 21 line (107 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;
}Found a 16 line (106 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,
) -> Result<TokenId, TokenStoreError>
where
I: CharStream,
H: SemanticHooks,
A: FnMut(&mut BaseLexer<I>, LexerCustomAction) -> bool,
P: FnMut(&BaseLexer<I>, LexerPredicate) -> Option<bool>,
E: FnMut(&mut BaseLexer<I>, i32, usize),
{
let hooks = RefCell::new(hooks);
let token = next_token_with_hooks(Found a 11 line (106 tokens) duplication in the following files:
fn finds_first_rule_depth_first() {
let mut tokens = TokenStore::new(None, "");
let mut nested = ParserRuleContext::new(1, -1);
nested.add_child(ParseTree::Terminal(terminal(
&mut tokens,
TreeToken::new(1).with_text("x"),
)));
let mut root = ParserRuleContext::new(0, -1);
root.add_child(ParseTree::Rule(RuleNode::new(nested)));
let tree = ParseTree::Rule(RuleNode::new(root));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, |
tinovyatkin
changed the title
![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 introduces a major breaking change to the ANTLR4 Rust runtime by migrating from pointer-owned tokens to a compact, flat TokenStore where tokens are addressed by a lightweight TokenId and accessed via borrowing TokenView values. CommonTokenStream now directly owns the TokenStore, and parse-tree nodes store only TokenIds, requiring a reference to the TokenStore for token-dependent tree APIs. The code generator and parity tests have been updated accordingly. The review feedback points out two excellent optimization opportunities in src/token_stream.rs where mapping directly to &str instead of calling .to_owned() on each token inside text and text_all avoids unnecessary temporary String allocations during collection.
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.
| pub fn text(&self, start: usize, stop: usize) -> String { | ||
| if start > stop || start >= self.source_token_count { | ||
| return String::new(); | ||
| } | ||
| // Java's `BufferedTokenStream.getText(Interval)` stops at the first | ||
| // EOF token, so an interval whose stop index lands on EOF renders | ||
| // without a trailing `<EOF>` (diagnostics rely on this). | ||
| self.tokens[start..=stop.min(self.tokens.len().saturating_sub(1))] | ||
| .iter() | ||
| (start..=stop.min(self.source_token_count.saturating_sub(1))) | ||
| .filter_map(|index| self.get(index)) | ||
| .take_while(|token| token.token_type() != TOKEN_EOF) | ||
| .map(|token| token.text()) | ||
| .collect::<Vec<_>>() | ||
| .join("") | ||
| } | ||
|
|
||
| /// Concatenated text of every buffered token except EOF — ANTLR's | ||
| /// `TokenStream.getText()`, the shape generated test actions read through | ||
| /// `self.input().text()`. | ||
| pub fn text_all(&mut self) -> String { | ||
| self.fill(); | ||
| self.tokens | ||
| .iter() | ||
| .map(|token| token.text().to_owned()) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
In text, calling .to_owned() on each token's text creates a temporary String allocation for every single token in the range, which is then collected. Since TokenView::text returns a &str, you can map directly to &str and let collect::<String>() handle the concatenation. This avoids all temporary String allocations and significantly improves performance.
| pub fn text(&self, start: usize, stop: usize) -> String { | |
| if start > stop || start >= self.source_token_count { | |
| return String::new(); | |
| } | |
| // Java's `BufferedTokenStream.getText(Interval)` stops at the first | |
| // EOF token, so an interval whose stop index lands on EOF renders | |
| // without a trailing `<EOF>` (diagnostics rely on this). | |
| self.tokens[start..=stop.min(self.tokens.len().saturating_sub(1))] | |
| .iter() | |
| (start..=stop.min(self.source_token_count.saturating_sub(1))) | |
| .filter_map(|index| self.get(index)) | |
| .take_while(|token| token.token_type() != TOKEN_EOF) | |
| .map(|token| token.text()) | |
| .collect::<Vec<_>>() | |
| .join("") | |
| } | |
| /// Concatenated text of every buffered token except EOF — ANTLR's | |
| /// `TokenStream.getText()`, the shape generated test actions read through | |
| /// `self.input().text()`. | |
| pub fn text_all(&mut self) -> String { | |
| self.fill(); | |
| self.tokens | |
| .iter() | |
| .map(|token| token.text().to_owned()) | |
| .collect() | |
| } | |
| pub fn text(&self, start: usize, stop: usize) -> String { | |
| if start > stop || start >= self.source_token_count { | |
| return String::new(); | |
| } | |
| (start..=stop.min(self.source_token_count.saturating_sub(1))) | |
| .filter_map(|index| self.get(index)) | |
| .take_while(|token| token.token_type() != TOKEN_EOF) | |
| .map(|token| token.text()) | |
| .collect() | |
| } |
There was a problem hiding this comment.
Fixed in f41543c. TokenView::text() now exposes the token-store lifetime, so CommonTokenStream::text() maps borrowed &str values directly into the output String without per-token temporary allocations. Added text_concatenates_borrowed_token_text. Validated with cargo test --locked --all-targets --all-features and cargo clippy --locked --all-targets --all-features -- -D warnings.
| pub fn text_all(&self) -> String { | ||
| self.tokens() | ||
| .filter(|token| token.token_type() != TOKEN_EOF) | ||
| .map(|token| token.text()) | ||
| .map(|token| token.text().to_owned()) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
In text_all, calling .to_owned() on each token's text creates a temporary String allocation for every single token in the stream. Mapping directly to &str avoids these allocations and is much more efficient.
| pub fn text_all(&self) -> String { | |
| self.tokens() | |
| .filter(|token| token.token_type() != TOKEN_EOF) | |
| .map(|token| token.text()) | |
| .map(|token| token.text().to_owned()) | |
| .collect() | |
| } | |
| pub fn text_all(&self) -> String { | |
| self.tokens() | |
| .filter(|token| token.token_type() != TOKEN_EOF) | |
| .map(|token| token.text()) | |
| .collect() | |
| } |
There was a problem hiding this comment.
Fixed in f41543c. text_all() now collects borrowed token text directly, with the same store-lifetime correction and focused concatenation regression. Validated with cargo test --locked --all-targets --all-features and cargo clippy --locked --all-targets --all-features -- -D warnings.
![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: b82e097173
ℹ️ 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".
| loop { | ||
| let mut sink = TokenSink::new(&mut store); | ||
| let id = source.next_token(&mut sink)?; | ||
| source_errors.extend(source.drain_errors()); |
There was a problem hiding this comment.
For entry rules that intentionally accept only a prefix of the stream, this eager loop now lexes through EOF during CommonTokenStream construction and accumulates every lexer diagnostic up front. Later report_generated_parser_diagnostics()/drain_source_errors() will report errors from tokens the parser never requested (e.g. rule: ID; on x y @ can surface the @ error even though parsing stopped after the first token and only needed at most the next lookahead), whereas the previous lazy stream only surfaced lexer errors as lookahead was fetched. Consider keeping per-token diagnostics gated by the highest requested token or preserving lazy fetching semantics for diagnostics.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f41543c. Eager buffering now tags each lexer diagnostic with the token whose fetch produced it, while the stream tracks the highest token index actually requested. drain_source_errors() releases only diagnostics in that requested prefix; fill() and whole-stream iteration intentionally release all. Added source_errors_remain_hidden_until_their_token_is_requested, covering an error attached to EOF after a requested prefix. Validated with the full all-target/all-feature tests and strict Clippy.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/token_stream.rs`:
- Around line 49-55: Update the token-fetch loop around TokenSink::new and
source.next_token to record the store length before each call, then validate
that exactly one token was appended and that the returned id refers to that
newly appended token; reject stale or otherwise invalid IDs instead of accepting
sink.view(id). Add a regression test covering a stale non-EOF ID and ensuring
eager buffering terminates.
In `@src/token.rs`:
- Around line 311-321: Update the source-backed span validation in the token
construction path around spec.start_byte and spec.stop_byte to reject offsets
that are not UTF-8 character boundaries, using source.is_char_boundary for both
endpoints before accepting the span. Preserve the existing range and overflow
errors, and add a test covering a span that splits a multibyte code point to
ensure malformed Unicode spans are rejected rather than producing empty
TokenView::text() output.
In `@src/tree.rs`:
- Around line 710-736: Make the tokens and tree fields of ParsedFile private
while retaining new, tokens(), tree(), and into_parts() as the public access
API. Ensure callers can no longer replace either component independently,
preserving the association between the TokenStore and parsed tree.
🪄 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: d9752d7b-625b-41c1-b036-7a85cc37b56d
📒 Files selected for processing (22)
.conformance-review/Rust.test.stgCHANGELOG.mdREADME.mdsrc/atn/lexer.rssrc/atn/lexer_dfa.rssrc/bin/antlr4-rust-gen.rssrc/bin_support/embedded.rssrc/char_stream.rssrc/lexer.rssrc/lib.rssrc/parser.rssrc/semir.rssrc/token.rssrc/token_stream.rssrc/tree.rstests/javascript-parity/dumper/src/javascript_lexer_base.rstests/javascript-parity/dumper/src/javascript_parser_base.rstests/javascript-parity/dumper/src/main.rstests/kotlin-parity/dumper/src/main.rstests/typescript-parity/dumper/src/main.rstests/typescript-parity/dumper/src/typescript_lexer_base.rstests/typescript-parity/dumper/src/typescript_parser_base.rs
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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
CommonToken/TokenRefbuffering with one compact, index-addressedTokenStoreTokenSink, while parser, prediction, and CST paths carryTokenIdRc<RefCell<TokenStore>>handles; parse-tree nodes now store IDs and token-dependent APIs accept an explicit&TokenStoreParsedFile<R>so completed trees retain their canonical token storeWhy
The previous stream representation stored each logical token more than once and paid per-token allocation, reference-counting, and cache-footprint costs before parsing began. This change makes the stream-owned store the only canonical representation and keeps text/source payloads sparse or stream-wide.
Breaking changes
Generated lexers and parsers must be regenerated with the matching runtime/generator release. Custom token sources now append
TokenSpecvalues throughTokenSink. Tree token/text APIs require the owning store, and generatedparse()helpers returnParsedFile<R>.Measurements
The 10x Solidity
governor.solallocation corpus improved as follows:A same-machine generated-parser comparison covered 12 Kotlin, C#, and Java fixtures. Eleven were faster and the remaining Kotlin fixture was +1.0%; the regression gate passed for all results.
Validation
cargo fmt --checkcargo test --locked --all-targets --all-featurescargo clippy --locked --all-targets --all-features -- -D warnings356 passed, 0 failed, 1 skippedParsedFiletraversal/token-resolution regression preserving the PR [codex] add parse tree traversal helpers #69 traversal helpersFixes #82