[codex] Expose token byte offsets - #39
Conversation
|
Warning Review limit reached
More reviews will be available in 22 minutes and 53 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughAdds ChangesUTF-8 Byte-Span Support
Sequence Diagram(s)sequenceDiagram
participant Consumer
participant Lexer
participant CommonToken
rect rgba(70, 130, 180, 0.5)
note over Lexer,CommonToken: emit_with_stop path
Consumer->>Lexer: next_token()
Lexer->>Lexer: compute source_interval (text=None, stop valid)
Lexer->>Lexer: build TokenSourceText (Rc::clone, u32::try_from offsets)
Lexer->>CommonToken: factory.create(TokenSpec { source_text })
CommonToken-->>Lexer: token (byte_span may be set from source_text)
Lexer->>Lexer: check token.byte_span set?
alt byte_span not set
Lexer->>Lexer: token_byte_span(stop) via byte_offset_at
Lexer->>CommonToken: token.with_byte_span(start_byte, stop_byte)
end
Lexer-->>Consumer: token with byte_span
end
rect rgba(60, 179, 113, 0.5)
note over Lexer,CommonToken: eof_token path
Consumer->>Lexer: eof_token()
Lexer->>Lexer: eof_byte_offset() → byte_offset_at(input.index())
Lexer->>CommonToken: factory.create_eof(...)
Lexer->>CommonToken: token.with_byte_span(offset, offset)
Lexer-->>Consumer: EOF token with byte_span
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Copy/Paste DetectionNo duplications found in 2 changed Rust file(s) (threshold: 100 tokens). |
![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 UTF-8 byte span tracking for tokens by adding start_byte, stop_byte, and byte_span methods to the Token trait and implementing them for CommonToken. It also includes helper functions and unit tests to verify the new functionality. The review feedback points out a redundant implementation of byte_span in CommonToken that can be removed to simplify the code, as it matches the default trait implementation.
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.
|
|
||
| fn byte_span(&self) -> Range<usize> { | ||
| self.source_byte_span() | ||
| .unwrap_or_else(|| self.start_byte()..self.stop_byte()) | ||
| } |
There was a problem hiding this comment.
The byte_span method implementation in CommonToken is redundant because it is identical to the default implementation provided by the Token trait:
fn byte_span(&self) -> Range<usize> {
self.start_byte()..self.stop_byte()
}Removing this override simplifies the code and reduces duplication.
![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: 1ef55f6c2a
ℹ️ 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".
| self.source_byte_span() | ||
| .unwrap_or_else(|| self.start_byte()..self.stop_byte()) |
There was a problem hiding this comment.
When consumers call byte_span() on the EOF token produced by BaseLexer::eof_token after lexing non-ASCII input, this fallback branch is used because CommonToken::eof stores explicit <EOF> text and no source-backed byte span. That treats the Unicode scalar index as a byte offset; for input "β", EOF is at character index 1 but byte offset 2, so the span becomes 1..1 and &source[token.byte_span()] panics on a non-UTF-8 boundary instead of yielding the empty EOF slice.
Useful? React with 👍 / 👎.
1ef55f6 to
f49015f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f49015faba
ℹ️ 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".
| /// source when the token carries source byte bounds. The default | ||
| /// implementation treats character indices as byte offsets. | ||
| fn stop_byte(&self) -> usize { | ||
| default_stop_byte(self.start(), self.stop()) |
There was a problem hiding this comment.
When a lexer rule itself matches EOF after non-ASCII input, src/atn/lexer.rs:340 passes explicit <EOF> text into emit_with_stop instead of using BaseLexer::eof_token(), so the new methods here take this fallback and report the character index as a byte offset (for example, after β, 1..1 instead of 2..2). Fresh evidence is that EOF-rule path, which is not covered by the new eof_token() byte-span handling; consumers slicing by byte_span() can still hit a non-UTF-8 boundary for those tokens.
Useful? React with 👍 / 👎.
f49015f to
79ddeab
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 (1)
src/lexer.rs (1)
371-392:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply the computed byte span after custom factories run.
Line 388 skips
with_byte_spanjust becausesource_textwas passed intoTokenSpec, butF: TokenFactoryis customizable and may not copysource_textintoCommonToken.byte_span. For non-ASCII implicit tokens, that leavesbyte_span()falling back to character indices.🐛 Proposed fix
- let source_text_sets_byte_span = source_text.is_some(); + let source_byte_span = source_text + .as_ref() + .map(|source_text| (source_text.start_byte, source_text.stop_byte)); let text = text.or_else(|| { source_text .is_none() .then(|| self.input.text(TextInterval::new(self.token_start, stop))) }); @@ - if !source_text_sets_byte_span { - if let Some((start_byte, stop_byte)) = self.token_byte_span(stop) { - token = token.with_byte_span(start_byte, stop_byte); - } + if let Some((start_byte, stop_byte)) = + source_byte_span.or_else(|| self.token_byte_span(stop)) + { + token = token.with_byte_span(start_byte, stop_byte); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lexer.rs` around lines 371 - 392, The current logic skips applying the computed byte span via the with_byte_span method when source_text is provided, but the customizable TokenFactory may not actually use source_text to set byte_span on the token. Remove the source_text_sets_byte_span guard condition and always apply the computed byte span by calling with_byte_span after the factory creates the token in the self.factory.create call, ensuring byte_span is properly set regardless of whether the factory implementation uses the source_text parameter.
🧹 Nitpick comments (1)
src/lexer.rs (1)
659-700: ⚡ Quick winAdd a non-EOF implicit-text byte-span test.
The current tests cover EOF and explicit-text fallback paths, but not the new
source_textpath used whenemit(..., None)captures source text for a regular non-ASCII token.🧪 Suggested test coverage
+ #[test] + fn emit_implicit_text_uses_utf8_byte_span_for_non_ascii_input() { + let data = RecognizerData::new( + "T", + Vocabulary::new( + std::iter::empty::<Option<&str>>(), + std::iter::empty::<Option<&str>>(), + std::iter::empty::<Option<&str>>(), + ), + ); + let mut lexer = BaseLexer::new(InputStream::new("β"), data); + lexer.begin_token(); + lexer.consume_char(); + + let token = lexer.emit(1, DEFAULT_CHANNEL, None); + + assert_eq!(token.start(), 0); + assert_eq!(token.stop(), 0); + assert_eq!(token.text(), Some("β")); + assert_eq!(token.byte_span(), 0..2); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lexer.rs` around lines 659 - 700, Add a new test function that covers the implicit-text byte-span code path for non-EOF tokens. Create a test similar to eof_token_uses_utf8_byte_offset_after_non_ascii_input and eof_rule_token_uses_utf8_byte_offset_after_non_ascii_input, but instead of testing EOF tokens, create a regular token by calling emit with None as the text parameter (to trigger the source_text capture path) on non-ASCII input like "β", then assert that the token's byte_span reflects the correct UTF-8 byte offset rather than character offset. This will ensure the implicit-text path used when emit(..., None) is called properly handles UTF-8 byte offsets for regular tokens.
🤖 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/lexer.rs`:
- Around line 371-392: The current logic skips applying the computed byte span
via the with_byte_span method when source_text is provided, but the customizable
TokenFactory may not actually use source_text to set byte_span on the token.
Remove the source_text_sets_byte_span guard condition and always apply the
computed byte span by calling with_byte_span after the factory creates the token
in the self.factory.create call, ensuring byte_span is properly set regardless
of whether the factory implementation uses the source_text parameter.
---
Nitpick comments:
In `@src/lexer.rs`:
- Around line 659-700: Add a new test function that covers the implicit-text
byte-span code path for non-EOF tokens. Create a test similar to
eof_token_uses_utf8_byte_offset_after_non_ascii_input and
eof_rule_token_uses_utf8_byte_offset_after_non_ascii_input, but instead of
testing EOF tokens, create a regular token by calling emit with None as the text
parameter (to trigger the source_text capture path) on non-ASCII input like "β",
then assert that the token's byte_span reflects the correct UTF-8 byte offset
rather than character offset. This will ensure the implicit-text path used when
emit(..., None) is called properly handles UTF-8 byte offsets for regular
tokens.
79ddeab to
c506d61
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Summary
Token::start,Token::stop,Token::line, andToken::columnunitsToken::start_byte(),Token::stop_byte(), andToken::byte_span()with source-backedCommonTokenoffsetsCloses #23
Validation
cargo +1.95.0 test --lockedcargo +1.95.0 clippy --locked --all-targets --all-features -- -D warningsrustfmt --check src/token.rs src/lexer.rsgit diff --checkcargo +1.95.0 fmt --all --check(fails due pre-existing rustfmt drift insrc/atn/parser.rsandsrc/prediction.rs; touched files are rustfmt-clean)Summary by CodeRabbit
Release Notes
New Features
Tests