Add line/column to tokenizer errors - #89
Conversation
Tracks the line and column numbers in tokenizer, and propagates error source info properly. Note, size_t is used and can overflow in extreme contexts. Fixes #68
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThe tokenizer now records absolute byte position plus line and column in a new public Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Tokenizer as Tokenizer
participant ParseContext as ParseContext
participant ErrorFactory as Error
Client->>Tokenizer: parse_full / feed bytes
Tokenizer->>Tokenizer: advance bytes, update Position (pos,line,column)
Tokenizer->>ParseContext: enter/exit/check (with Position)
ParseContext-->>Tokenizer: Ok or Err(kind, Position)
Tokenizer->>ErrorFactory: Error::new(kind, char?, Position)
ErrorFactory-->>Client: return ParseError/Error containing Position
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Summary of ChangesHello @kaidokert, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the diagnostic capabilities of the JSON tokenizer by integrating line and column information into all generated error messages. This enhancement provides a more user-friendly experience when encountering parsing failures, as the exact location of the syntax error within the input document can now be precisely identified. The changes involve updating internal state management, error propagation, and output formatting, all while maintaining backward compatibility for existing test suites. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The repeated threading of
lineandcolumnthrough many internal methods (enter_*,exit_*,check_trailing_comma,start_token, etc.) makes the signature noise quite high; consider introducing a smallPosition { pos, line, column }helper or storing line/column inParseContextso callers only pass a single value or nothing at all. - Instead of using
line == 0as a sentinel to decide whichDisplay/Debugformat to print, it may be clearer and more type-safe to storeOption<(usize, usize)>for line/column and pattern match on that, which also avoids having to pass dummy0, 0values in tests.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The repeated threading of `line` and `column` through many internal methods (`enter_*`, `exit_*`, `check_trailing_comma`, `start_token`, etc.) makes the signature noise quite high; consider introducing a small `Position { pos, line, column }` helper or storing line/column in `ParseContext` so callers only pass a single value or nothing at all.
- Instead of using `line == 0` as a sentinel to decide which `Display`/`Debug` format to print, it may be clearer and more type-safe to store `Option<(usize, usize)>` for line/column and pattern match on that, which also avoids having to pass dummy `0, 0` values in tests.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request is a great enhancement, adding line and column numbers to tokenizer errors for better diagnostics. The implementation is solid, including the use of saturating_add to prevent overflows and adding comprehensive tests for the new functionality. The custom PartialEq for Error is a clever way to avoid breaking existing tests.
I have a couple of suggestions to further improve the accuracy of error reporting and the internal design of the Error struct. Please see my detailed comments.
8126fc2 to
9588376
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a significant improvement, adding line and column numbers to tokenizer errors to make debugging easier. The implementation is thorough, correctly tracking positions, propagating them to the Error type, and updating display formats. The use of saturating_add for position counters is a robust choice. This PR also implicitly fixes a bug where trailing comma errors reported an incorrect position, further enhancing error diagnostics. The new tests are comprehensive. I've provided a couple of suggestions for minor code cleanup and to make a test more idiomatic.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a great enhancement, adding line and column numbers to tokenizer errors, which will significantly improve the debugging experience. The implementation is thorough, including updates to the tokenizer's core logic, error structures, and extensive test coverage for the new position tracking.
I have a few suggestions to further improve robustness:
- Using
saturating_addfor byte position calculation to prevent overflow issues. - Revisiting the AVR-specific padding in the
Errorstruct to ensure it fully avoids the problematic size range. - Considering a more conventional handling of
\rand\r\nline endings for more intuitive column reporting.
Overall, this is a well-executed and valuable change.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a great enhancement, adding line and column numbers to tokenizer errors, which will significantly improve the debugging experience for parsing issues. The implementation is thorough, covering error propagation, comprehensive new tests, and robust handling of potential overflows using saturating_add. My feedback focuses on a few areas to further improve the code: ensuring a platform-specific workaround is effective and making some of the new tests more robust and idiomatic.
Tracks the line and column numbers in tokenizer, and propagates error source info properly. Note, size_t is used and can overflow in extreme contexts.
Fixes #68
Summary by Sourcery
Track line and column positions in the JSON tokenizer and surface them in error reporting while keeping existing error semantics.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.