Content span - #88
Open
kaidokert wants to merge 2 commits into
Open
Conversation
Reviewer's GuideImplements chunk-aware ContentSpan events for simple JSON tokens by extending ParserCore to track token state, refactoring byte-accumulation into an optional callback, adding new event variants, and wiring these changes through PushParser and the content builders to extract unescaped spans in bulk. Sequence diagram for PushParser handling ContentSpan and PartialContentSpan eventssequenceDiagram
participant PushParser
participant ParserCore
participant PushContentBuilder
participant Handler
PushParser->>PushContentBuilder: set_chunk(data)
loop for each byte
PushParser->>ParserCore: next_event_impl(...)
ParserCore->>PushContentBuilder: get_next_byte()
alt Event::ContentSpan
ParserCore-->>PushParser: ContentSpan{kind, start, end, has_escapes}
PushParser->>PushContentBuilder: get_borrowed_slice(start, end)
PushParser->>Handler: handle_event(Event::String/Key/Number)
else Event::PartialContentSpanStart
ParserCore-->>PushParser: PartialContentSpanStart{kind, start, has_escapes_in_this_chunk}
PushParser->>PushContentBuilder: handle_partial_content_span_start(...)
else Event::PartialContentSpanEnd
ParserCore-->>PushParser: PartialContentSpanEnd{kind, end, has_escapes_in_this_chunk}
PushParser->>PushContentBuilder: get_borrowed_slice(0, relative_end)
PushParser->>Handler: handle_event(Event::String/Key/Number)
else Other events
ParserCore-->>PushParser: Event
PushParser->>Handler: handle_event(Event)
end
end
ER diagram for new ContentSpan event relationshipserDiagram
EVENT {
string id
ContentKind kind
int start
int end
bool has_escapes
}
CONTENT_KIND {
string name
}
EVENT ||--|| CONTENT_KIND : has_kind
Class diagram for new and updated event types (Event, ContentKind)classDiagram
class Event {
<<enum>>
+String
+Key
+Number
+Bool
+Null
+ContentSpan(kind: ContentKind, start: usize, end: usize, has_escapes: bool)
+PartialContentSpanStart(kind: ContentKind, start: usize, has_escapes_in_this_chunk: bool)
+PartialContentSpanEnd(kind: ContentKind, end: usize, has_escapes_in_this_chunk: bool)
+EndDocument
}
class ContentKind {
<<enum>>
+String
+Key
+Number
}
Event --> ContentKind : uses
Class diagram for ParserCore with ContentSpan trackingclassDiagram
class ParserCore {
-handles_chunked_input: bool
-current_content_kind: Option<ContentKind>
-current_content_start: usize
-current_content_has_escapes: bool
-continuing_from_previous_chunk: bool
-partial_span_start_emitted: bool
+next_event_impl(...)
+next_event_impl_with_flags(...)
+try_emit_content_span(...)
+try_emit_partial_content_span_start(...)
+track_content_spans(...)
+reset_content_tracking()
+reset_partial_span_start_flag()
}
ParserCore --> ContentKind : tracks
Class diagram for ContentExtractor trait and implementationsclassDiagram
class ContentExtractor {
<<trait>>
+get_next_byte() : Result<Option<u8>, ParseError>
+current_position() : usize
+validate_and_extract_string()
+validate_and_extract_key()
+validate_and_extract_number(...)
+process_begin_events(...)
+extract_string_content(...)
+extract_key_content(...)
+extract_number_content(...)
}
class PushContentBuilder {
+get_next_byte()
+current_chunk_len()
+position_offset()
+queue_unescaped_reset()
+extract_string_content(...)
+extract_key_content(...)
+extract_number_content(...)
}
class StreamContentBuilder {
+get_next_byte()
+extract_string_content(...)
+extract_key_content(...)
+extract_number_content(...)
}
class SliceContentBuilder {
+get_next_byte()
+extract_string_content(...)
+extract_key_content(...)
+extract_number_content(...)
}
ContentExtractor <|.. PushContentBuilder
ContentExtractor <|.. StreamContentBuilder
ContentExtractor <|.. SliceContentBuilder
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughIntroduces a new Changes
Sequence DiagramsequenceDiagram
participant User
participant PushParser
participant EventProcessor
participant ContentBuilder
participant DataSource
Note over PushParser,DataSource: ContentSpan Event Flow (Internal)
User->>PushParser: feed_slice(chunk)
PushParser->>EventProcessor: next_event_impl(content_extractor, None)
EventProcessor->>ContentBuilder: get_next_byte()
ContentBuilder->>DataSource: next_byte()
DataSource-->>ContentBuilder: Some(u8)
ContentBuilder-->>EventProcessor: Some(u8)
rect rgba(200, 220, 255, 0.3)
Note over EventProcessor: Content tracking<br/>(Begin event found)
EventProcessor->>EventProcessor: track_content_spans()
end
alt Content completes in chunk
EventProcessor->>EventProcessor: try_emit_content_span()
EventProcessor-->>PushParser: ContentSpan event
rect rgba(255, 200, 200, 0.3)
PushParser->>PushParser: Convert to String/Key/Number
PushParser-->>User: user_handler(String/Key/Number)
end
else Content spans chunks
EventProcessor-->>PushParser: PartialContentSpanStart
PushParser->>ContentBuilder: handle_partial_content_span_start()
ContentBuilder->>ContentBuilder: copy_partial_content_to_scratch()
PushParser-->>User: (await next chunk)
User->>PushParser: feed_slice(next_chunk)
EventProcessor->>EventProcessor: try_emit_partial_content_span_end()
EventProcessor-->>PushParser: PartialContentSpanEnd
PushParser->>PushParser: Assemble from scratch buffer
PushParser-->>User: user_handler(complete String/Key/Number)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
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 introduces a significant internal refactoring to the JSON parsing library, primarily focused on enhancing the 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
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- ParserCore::next_event_impl has grown very large and mixes multiple responsibilities; consider refactoring it into smaller, single‐purpose helper methods (e.g., separate ContentSpan emission, byte accumulation, and chunk boundary handling) to improve readability and maintainability.
- There’s an extensive use of debug logging in try_emit_content_span, track_content_spans, and other methods—consider reducing verbosity or gating these logs behind a dedicated feature flag so they don’t clutter normal output.
- The continuing_from_previous_chunk flag never appears to be set anywhere, so the PartialContentSpanEnd path may never be taken—make sure to update this flag at the right point when a token actually crosses a chunk boundary.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- ParserCore::next_event_impl has grown very large and mixes multiple responsibilities; consider refactoring it into smaller, single‐purpose helper methods (e.g., separate ContentSpan emission, byte accumulation, and chunk boundary handling) to improve readability and maintainability.
- There’s an extensive use of debug logging in try_emit_content_span, track_content_spans, and other methods—consider reducing verbosity or gating these logs behind a dedicated feature flag so they don’t clutter normal output.
- The continuing_from_previous_chunk flag never appears to be set anywhere, so the PartialContentSpanEnd path may never be taken—make sure to update this flag at the right point when a token actually crosses a chunk boundary.
## Individual Comments
### Comment 1
<location> `picojson/src/event_processor.rs:109-100` </location>
<code_context>
+ if let Some(ref mut accumulator) = byte_accumulator {
</code_context>
<issue_to_address>
**suggestion:** The logic for determining when to accumulate bytes is duplicated and could be unified.
Extract the `should_accumulate` logic into a helper to avoid duplication and simplify future updates.
</issue_to_address>
### Comment 2
<location> `picojson/src/shared.rs:8-9` </location>
<code_context>
use crate::{ujson, JsonNumber, String};
+/// Content type identification for ContentSpan events
+#[derive(Debug, PartialEq, Clone, Copy)]
+pub enum ContentKind {
+ String,
+ Key,
</code_context>
<issue_to_address>
**nitpick:** The ContentKind enum is missing documentation for each variant.
</issue_to_address>
### Comment 3
<location> `picojson/tests/push_parser_invalidslicebounds_repro.rs:65-66` </location>
<code_context>
println!("{}🏁 EndDocument", self.indent_str());
}
+ // ContentSpan events should not reach user code - they get converted by PushParser
+ Event::ContentSpan { .. }
+ | Event::PartialContentSpanStart { .. }
+ | Event::PartialContentSpanEnd { .. } => {
+ panic!("Internal ContentSpan events should not reach user handlers")
</code_context>
<issue_to_address>
**suggestion (testing):** No regression tests for invalid slice bounds with new ContentSpan logic.
Please add regression tests for invalid slice bounds and edge cases in the new ContentSpan logic to verify the parser handles these scenarios safely.
</issue_to_address>
### Comment 4
<location> `picojson/tests/push_parser_stress_test.rs:111-112` </location>
<code_context>
println!("{}🏁 EndDocument", self.indent_str());
}
+ // ContentSpan events should not reach user code - they get converted by PushParser
+ Event::ContentSpan { .. }
+ | Event::PartialContentSpanStart { .. }
+ | Event::PartialContentSpanEnd { .. } => {
+ panic!("Internal ContentSpan events should not reach user handlers")
</code_context>
<issue_to_address>
**suggestion (testing):** Stress tests do not cover new event types or chunk boundary edge cases.
Please add stress tests for ContentSpan and PartialContentSpan events, focusing on large, multi-chunk JSON inputs and escape sequences to ensure performance and correctness.
Suggested implementation:
```rust
use picojson::{PushParser, Event};
use std::str;
```
```rust
#[test]
fn stress_test_content_span_large_chunks() {
// Simulate a large JSON string split into multiple chunks
let json = "\"".to_owned() + &"a".repeat(10_000) + "\"";
let mut parser = PushParser::new();
let mut events = Vec::new();
for chunk in json.as_bytes().chunks(1024) {
for event in parser.push(chunk) {
events.push(event);
}
}
// Ensure no ContentSpan or PartialContentSpan events reach user code
for event in &events {
match event {
Event::ContentSpan { .. }
| Event::PartialContentSpanStart { .. }
| Event::PartialContentSpanEnd { .. } => {
panic!("Internal ContentSpan events should not reach user handlers");
}
_ => {}
}
}
// Check that the string was parsed correctly
assert!(events.iter().any(|e| matches!(e, Event::String(_))));
}
#[test]
fn stress_test_content_span_escape_sequences() {
// JSON string with escape sequences, split into chunks at escape boundaries
let json = "\"abc\\n\\t\\u1234def\"";
let mut parser = PushParser::new();
let mut events = Vec::new();
// Split at escape sequence boundaries
let chunks = vec![
&json.as_bytes()[..5], // "\"abc\"
&json.as_bytes()[5..8], // "n\"
&json.as_bytes()[8..11], // "t\"
&json.as_bytes()[11..17],// "u1234"
&json.as_bytes()[17..], // "def\""
];
for chunk in chunks {
for event in parser.push(chunk) {
events.push(event);
}
}
// Ensure no ContentSpan or PartialContentSpan events reach user code
for event in &events {
match event {
Event::ContentSpan { .. }
| Event::PartialContentSpanStart { .. }
| Event::PartialContentSpanEnd { .. } => {
panic!("Internal ContentSpan events should not reach user handlers");
}
_ => {}
}
}
// Check that the string was parsed correctly
assert!(events.iter().any(|e| matches!(e, Event::String(_))));
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| while !have_events(&self.parser_state.evts) { | ||
| if let Some(byte) = provider.next_byte()? { | ||
| if let Some(byte) = provider.get_next_byte()? { | ||
| { |
There was a problem hiding this comment.
suggestion: The logic for determining when to accumulate bytes is duplicated and could be unified.
Extract the should_accumulate logic into a helper to avoid duplication and simplify future updates.
Comment on lines
+8
to
+9
| #[derive(Debug, PartialEq, Clone, Copy)] | ||
| pub enum ContentKind { |
There was a problem hiding this comment.
nitpick: The ContentKind enum is missing documentation for each variant.
![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 ContentSpan tracking mechanism to optimize parsing of unescaped tokens in PushParser. The changes are extensive, refactoring content extraction and introducing new event types. The overall direction is good for performance. However, I've identified a critical bug in the new chunk-spanning logic related to incorrect position handling, which will cause parsing failures. There is also some dead code and opportunities for encapsulation that I've pointed out. Addressing these issues will make the new implementation robust.
Comment on lines
+131
to
+184
| // Handle end of content that spans chunk boundaries | ||
| // Create the content event in place to avoid borrowing issues | ||
|
|
||
| // Convert absolute position to relative position within current chunk | ||
| let position_offset = self.extractor.position_offset(); | ||
| let relative_end = end.saturating_sub(position_offset); | ||
| let chunk_len = self.extractor.current_chunk_len(); | ||
|
|
||
| log::debug!("PartialContentSpanEnd: kind={:?}, absolute_end={}, position_offset={}, relative_end={}, chunk_len={}", | ||
| kind, end, position_offset, relative_end, chunk_len); | ||
|
|
||
| // For content that continues from previous chunk, the content in this chunk | ||
| // starts at position 0 and ends at relative_end (but we need to exclude quotes) | ||
| let content_start = 0; | ||
| let content_end = match kind { | ||
| ContentKind::String | ContentKind::Key => { | ||
| // For strings and keys, relative_end points to the closing quote | ||
| // We want content up to (but not including) the closing quote | ||
| relative_end | ||
| } | ||
| ContentKind::Number => { | ||
| // For numbers, relative_end points after the last digit | ||
| relative_end | ||
| } | ||
| }; | ||
|
|
||
| log::debug!( | ||
| "PartialContentSpanEnd: extracting slice [{}, {})", | ||
| content_start, | ||
| content_end | ||
| ); | ||
|
|
||
| // Append the final part from this chunk to the scratch buffer | ||
| // First, get and copy the final slice data | ||
| let final_slice = self | ||
| .extractor | ||
| .get_borrowed_slice(content_start, content_end) | ||
| .map_err(PushParseError::Parse)?; | ||
|
|
||
| log::debug!( | ||
| "PartialContentSpanEnd: final_slice = {:?}", | ||
| core::str::from_utf8(final_slice).unwrap_or("[invalid utf8]") | ||
| ); | ||
|
|
||
| // Copy ALL data to local buffer to completely avoid borrowing conflicts | ||
| let mut final_data = alloc::vec::Vec::new(); | ||
| final_data.extend_from_slice(final_slice); | ||
|
|
||
| // Now append from local buffer - no more borrowing conflicts | ||
| for byte in final_data { | ||
| self.extractor | ||
| .append_unescaped_byte(byte) | ||
| .map_err(PushParseError::Parse)?; | ||
| } |
There was a problem hiding this comment.
There's a critical issue with position handling in this block. The get_borrowed_slice method of the extractor expects absolute stream positions, but it's being called with content_start and content_end, which are relative positions within the current chunk. This will lead to incorrect slicing and parsing failures, especially for multi-chunk data. A similar issue exists in the handle_partial_content_span_start function.
Additionally, the logic for copying the slice to the scratch buffer is inefficient, involving multiple intermediate copies to a Vec to work around the borrow checker.
I suggest refactoring this by:
- Adding a new method to
PushContentBuilderto get a slice using relative chunk positions. - Encapsulating the slice-to-scratch-buffer copy logic within
PushContentBuilderto improve readability and efficiency.
For example, you could add these methods to PushContentBuilder:
// In PushContentBuilder
pub fn get_relative_slice(&self, start: usize, end: usize) -> Result<&'input [u8], ParseError> {
if end > self.current_chunk.len() || start > end {
return Err(ParseError::Unexpected(
crate::shared::UnexpectedState::InvalidSliceBounds,
));
}
Ok(&self.current_chunk[start..end])
}
pub fn append_slice_to_scratch(&mut self, slice: &[u8]) -> Result<(), ParseError> {
for &byte in slice {
self.stream_buffer.append_unescaped_byte(byte)?;
}
Ok(())
}Then, this block could be simplified significantly, fixing the bug and improving clarity.
Comment on lines
+456
to
+505
| ujson::Event::End(EventToken::String) => { | ||
| if self.handles_chunked_input && self.continuing_from_previous_chunk { | ||
| // This is the end of content that was started in a previous chunk | ||
| let pos = provider.current_position(); | ||
| log::debug!( | ||
| "End String at pos={}, continuing_from_previous_chunk=true", | ||
| pos | ||
| ); | ||
|
|
||
| // Emit PartialContentSpanEnd event | ||
| let partial_end_event = Event::PartialContentSpanEnd { | ||
| kind: ContentKind::String, | ||
| end: pos, | ||
| has_escapes_in_this_chunk: self.current_content_has_escapes, | ||
| }; | ||
|
|
||
| // Reset tracking state now that content is complete | ||
| self.reset_content_tracking(); | ||
|
|
||
| Some(EventResult::Complete(partial_end_event)) | ||
| } else { | ||
| // Normal case - delegate to process_simple_events | ||
| None | ||
| } | ||
| } | ||
| ujson::Event::End(EventToken::Key) => { | ||
| if self.handles_chunked_input && self.continuing_from_previous_chunk { | ||
| // This is the end of content that was started in a previous chunk | ||
| let pos = provider.current_position(); | ||
| log::debug!( | ||
| "End Key at pos={}, continuing_from_previous_chunk=true", | ||
| pos | ||
| ); | ||
|
|
||
| // Emit PartialContentSpanEnd event | ||
| let partial_end_event = Event::PartialContentSpanEnd { | ||
| kind: ContentKind::Key, | ||
| end: pos, | ||
| has_escapes_in_this_chunk: self.current_content_has_escapes, | ||
| }; | ||
|
|
||
| // Reset tracking state now that content is complete | ||
| self.reset_content_tracking(); | ||
|
|
||
| Some(EventResult::Complete(partial_end_event)) | ||
| } else { | ||
| // Normal case - delegate to process_simple_events | ||
| None | ||
| } | ||
| } |
There was a problem hiding this comment.
The match arms for ujson::Event::End(EventToken::String) and ujson::Event::End(EventToken::Key) inside track_content_spans appear to be dead code. The process_simple_events function, which is called earlier in next_event_impl_with_flags, already handles these events and causes the function to return, preventing track_content_spans from being called for them. The logic to handle PartialContentSpanEnd is correctly located in try_emit_content_span.
This redundant code should be removed to improve clarity and avoid confusion.
Comment on lines
+322
to
+328
| let _start = 0; // Start of current chunk | ||
| let end = match kind { | ||
| ContentKind::String | ContentKind::Key => current_pos, // Current pos is at closing quote | ||
| ContentKind::Number => current_pos + 1, // Include the last digit | ||
| }; | ||
|
|
||
| let _has_escapes = self.current_content_has_escapes; |
There was a problem hiding this comment.
These variables are assigned but never used. They can be removed to improve code clarity.
Suggested change
| let _start = 0; // Start of current chunk | |
| let end = match kind { | |
| ContentKind::String | ContentKind::Key => current_pos, // Current pos is at closing quote | |
| ContentKind::Number => current_pos + 1, // Include the last digit | |
| }; | |
| let _has_escapes = self.current_content_has_escapes; | |
| let end = match kind { | |
| ContentKind::String | ContentKind::Key => current_pos, // Current pos is at closing quote | |
| ContentKind::Number => current_pos + 1, // Include the last digit | |
| }; | |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
picojson/examples/push_parser_demo.rs (1)
63-68: LGTM - Consistent ContentSpan guard.Same pattern as stream_parser_demo.rs - ensures internal events don't reach user handlers.
picojson/tests/push_parser_escapes.rs (1)
29-34: LGTM - Consistent ContentSpan guard.Same pattern applied in test handler.
picojson/tests/push_parser_invalidslicebounds_repro.rs (1)
64-69: LGTM - Consistent ContentSpan guard.Same pattern in event-to-string conversion.
picojson/tests/push_parser.rs (1)
483-488: LGTM - Consistent ContentSpan guard.Same pattern applied in test handler.
picojson/tests/push_parser_stress_test.rs (1)
110-115: LGTM - Consistent ContentSpan guard.Same pattern applied in OwnedEvent conversion function.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
picojson/Cargo.toml(1 hunks)picojson/examples/push_parser_demo.rs(1 hunks)picojson/examples/stream_parser_demo.rs(1 hunks)picojson/src/event_processor.rs(11 hunks)picojson/src/push_content_builder.rs(6 hunks)picojson/src/push_parser.rs(4 hunks)picojson/src/shared.rs(3 hunks)picojson/src/slice_content_builder.rs(2 hunks)picojson/src/slice_parser.rs(1 hunks)picojson/src/stream_content_builder.rs(2 hunks)picojson/tests/push_parser.rs(1 hunks)picojson/tests/push_parser_escapes.rs(1 hunks)picojson/tests/push_parser_invalidslicebounds_repro.rs(1 hunks)picojson/tests/push_parser_stress_test.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-07-13T05:10:01.847Z
Learnt from: kaidokert
Repo: kaidokert/picojson-rs PR: 55
File: picojson/src/slice_parser.rs:0-0
Timestamp: 2025-07-13T05:10:01.847Z
Learning: In picojson-rs event processing, the SliceParser uses an if/else pattern with process_simple_events() first, then process_begin_events() as fallback. Both branches use identical match statements for all EventResult variants for consistency and maintainability, even though process_begin_events() only returns Continue or None while process_simple_events() can return all variants.
Applied to files:
picojson/src/slice_parser.rspicojson/tests/push_parser_escapes.rspicojson/tests/push_parser.rspicojson/tests/push_parser_stress_test.rspicojson/tests/push_parser_invalidslicebounds_repro.rspicojson/src/shared.rspicojson/examples/stream_parser_demo.rspicojson/src/event_processor.rspicojson/examples/push_parser_demo.rspicojson/src/push_parser.rspicojson/src/push_content_builder.rs
📚 Learning: 2025-07-13T05:11:46.914Z
Learnt from: kaidokert
Repo: kaidokert/picojson-rs PR: 55
File: picojson/src/slice_parser.rs:273-286
Timestamp: 2025-07-13T05:11:46.914Z
Learning: In picojson-rs SliceParser, is_empty() and is_past_end() serve different purposes: is_empty() returns true when pos >= data.len() (at document boundary, all input consumed), while is_past_end() returns true when pos > data.len() (gone beyond input). For number parsing delimiter logic, is_empty() is correct because it detects when parsing the last token at document end, whereas is_past_end() would incorrectly indicate not at document end for standalone numbers.
Applied to files:
picojson/src/slice_parser.rs
📚 Learning: 2025-07-07T01:39:55.177Z
Learnt from: kaidokert
Repo: kaidokert/picojson-rs PR: 44
File: picojson/src/chunk_reader.rs:28-33
Timestamp: 2025-07-07T01:39:55.177Z
Learning: The PullParser trait in picojson-rs provides both next() and next_event() methods. The next() method is an iterator-like convenience method that returns Option<Result<Event, ParseError>>, returning None when EndDocument is reached. The next_event() method returns Result<Event, ParseError> directly. Both methods are valid and the choice depends on whether you want iterator-style usage (next) or direct result handling (next_event).
Applied to files:
picojson/src/slice_parser.rspicojson/src/stream_content_builder.rspicojson/src/slice_content_builder.rspicojson/src/shared.rspicojson/examples/stream_parser_demo.rspicojson/src/event_processor.rspicojson/examples/push_parser_demo.rspicojson/src/push_parser.rspicojson/src/push_content_builder.rs
📚 Learning: 2025-07-13T05:06:50.688Z
Learnt from: kaidokert
Repo: kaidokert/picojson-rs PR: 55
File: picojson/src/event_processor.rs:188-203
Timestamp: 2025-07-13T05:06:50.688Z
Learning: In picojson-rs event_processor module, the tokenizer callback intentionally uses a fixed-size array of 2 slots and silently drops events when full. This is deliberate design for embedded/constrained environments requiring: zero-allocation guarantee, panic-free operation, deterministic memory usage, and fixed memory footprint. The array size of 2 is sufficient for ujson's event generation patterns when processed in tight loops.
Applied to files:
picojson/src/slice_parser.rspicojson/tests/push_parser_escapes.rspicojson/tests/push_parser_stress_test.rspicojson/tests/push_parser_invalidslicebounds_repro.rspicojson/src/event_processor.rspicojson/examples/push_parser_demo.rspicojson/src/push_parser.rs
📚 Learning: 2025-06-28T18:12:30.015Z
Learnt from: kaidokert
Repo: kaidokert/picojson-rs PR: 1
File: stax/src/flex_parser.rs:0-0
Timestamp: 2025-06-28T18:12:30.015Z
Learning: In the stax JSON parser codebase, EscapeSequence event handlers exist in flex_parser.rs not because they're needed by that parser variant, but to avoid catch-all patterns in match statements. The flex parser doesn't need to process EscapeSequence events, but the other parser variant (direct parser) does need them.
Applied to files:
picojson/src/slice_parser.rspicojson/tests/push_parser_escapes.rspicojson/tests/push_parser.rspicojson/tests/push_parser_invalidslicebounds_repro.rspicojson/examples/stream_parser_demo.rspicojson/src/event_processor.rspicojson/examples/push_parser_demo.rspicojson/src/push_parser.rspicojson/src/push_content_builder.rs
📚 Learning: 2025-06-29T17:48:18.198Z
Learnt from: kaidokert
Repo: kaidokert/picojson-rs PR: 5
File: picojson/src/lib.rs:0-0
Timestamp: 2025-06-29T17:48:18.198Z
Learning: In the picojson-rs project, the `use tokenizer as ujson;` alias in lib.rs is a transitionary and fully internal private alias used during crate reorganization. Examples and external code no longer depend on this alias, making the private visibility appropriate.
Applied to files:
picojson/src/slice_parser.rspicojson/Cargo.tomlpicojson/src/event_processor.rspicojson/src/push_parser.rspicojson/src/push_content_builder.rs
📚 Learning: 2025-06-28T23:43:22.783Z
Learnt from: kaidokert
Repo: kaidokert/picojson-rs PR: 1
File: tokenizer/src/bitstack/mod.rs:0-0
Timestamp: 2025-06-28T23:43:22.783Z
Learning: In the picojson-rs project, the BitStack trait was redesigned to return bool instead of Option<bool> for pop() and top() methods. Empty stacks return false rather than None, which simplifies the API and avoids Option handling.
Applied to files:
picojson/src/slice_parser.rspicojson/src/stream_content_builder.rspicojson/src/push_content_builder.rs
📚 Learning: 2025-07-27T05:05:22.707Z
Learnt from: kaidokert
Repo: kaidokert/picojson-rs PR: 69
File: picojson/tests/json_checker_tests.rs:296-297
Timestamp: 2025-07-27T05:05:22.707Z
Learning: In JSON parsing tests for picojson-rs, when testing Unicode escape sequences, raw string literals use double backslashes (e.g., r#"\\uCAFE"#) to create JSON input containing single backslashes (\uCAFE) that the JSON parser processes. The double backslashes are not an error - they correctly represent the JSON input format that contains escape sequences for the parser to decode.
Applied to files:
picojson/src/event_processor.rs
🧬 Code graph analysis (6)
picojson/src/stream_content_builder.rs (4)
picojson/src/event_processor.rs (2)
get_next_byte(564-564)get_next_byte(902-904)picojson/src/push_content_builder.rs (2)
get_next_byte(120-122)next_byte(344-353)picojson/src/slice_content_builder.rs (2)
get_next_byte(47-49)next_byte(177-183)picojson/src/shared.rs (1)
next_byte(232-232)
picojson/src/slice_content_builder.rs (4)
picojson/src/event_processor.rs (2)
get_next_byte(564-564)get_next_byte(902-904)picojson/src/push_content_builder.rs (2)
get_next_byte(120-122)next_byte(344-353)picojson/src/stream_content_builder.rs (2)
get_next_byte(123-125)next_byte(295-314)picojson/src/shared.rs (1)
next_byte(232-232)
picojson/src/shared.rs (3)
picojson/src/push_content_builder.rs (1)
next_byte(344-353)picojson/src/slice_content_builder.rs (1)
next_byte(177-183)picojson/src/stream_content_builder.rs (1)
next_byte(295-314)
picojson/src/event_processor.rs (4)
picojson/src/slice_input_buffer.rs (1)
current_pos(43-45)picojson/src/push_content_builder.rs (1)
get_next_byte(120-122)picojson/src/slice_content_builder.rs (1)
get_next_byte(47-49)picojson/src/stream_content_builder.rs (1)
get_next_byte(123-125)
picojson/src/push_parser.rs (4)
picojson/src/shared.rs (2)
from_utf8(302-304)new(103-107)picojson/src/json_number.rs (1)
from_slice(80-98)picojson/src/push_content_builder.rs (2)
position_offset(114-116)new(53-68)picojson/src/event_processor.rs (2)
new(42-54)new(892-898)
picojson/src/push_content_builder.rs (4)
picojson/src/stream_content_builder.rs (3)
queue_unescaped_reset(117-119)get_next_byte(123-125)next_byte(295-314)picojson/src/event_processor.rs (2)
get_next_byte(564-564)get_next_byte(902-904)picojson/src/slice_content_builder.rs (2)
get_next_byte(47-49)next_byte(177-183)picojson/src/shared.rs (1)
next_byte(232-232)
🪛 GitHub Actions: Build and test
picojson/Cargo.toml
[error] 1-1: Command 'cargo test --no-default-features --features "int32,float-error"' exited with code 101 due to test failures in push_parser tests.
picojson/tests/push_parser.rs
[error] 269-269: test_consecutive_unicode_escapes failed with Parse(Unexpected(InvalidSliceBounds)). Step: cargo test --no-default-features --features "int32,float-truncate".
[error] 184-184: Assertion failed in test_simple_escapes: token sequence mismatch (expected String with more content). Step: cargo test --no-default-features --features "int32,float-truncate".
[error] 223-223: test_unicode_escapes failed with Parse(Unexpected(InvalidSliceBounds)). Step: cargo test --no-default-features --features "int32,float-truncate".
🪛 GitHub Actions: Coverage
picojson/tests/push_parser.rs
[error] 269-269: Test 'consecutive_unicode_escapes' failed. Parse(Unexpected(InvalidSliceBounds)).
[error] 184-184: Test 'test_simple_escapes' failed. Assertion mismatch: expected String(hello\nworld) but got String(hello\n).
[error] 223-223: Test 'unicode_escapes' failed. Parse(Unexpected(InvalidSliceBounds)).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: Sourcery review
- GitHub Check: AVR Panic Prevention (int32, pico-tiny, dev)
- GitHub Check: AVR Panic Prevention (int8, pico-huge, dev)
- GitHub Check: AVR Panic Prevention (int8, pico-huge, release)
- GitHub Check: AVR Panic Prevention (int32, pico-huge, dev)
- GitHub Check: AVR Panic Prevention (int32, pico-huge, release)
- GitHub Check: AVR Panic Prevention (int32, pico-tiny, release)
- GitHub Check: AVR Panic Prevention (int8, pico-tiny, release)
- GitHub Check: AVR Panic Prevention (int32, pico-tiny, dev)
- GitHub Check: AVR Panic Prevention (int32, pico-huge, release)
- GitHub Check: AVR Panic Prevention (int32, pico-huge, dev)
- GitHub Check: AVR Panic Prevention (int8, pico-tiny, dev)
- GitHub Check: AVR Panic Prevention (int8, pico-huge, dev)
- GitHub Check: AVR Panic Prevention (int8, pico-tiny, dev)
- GitHub Check: AVR Panic Prevention (int32, pico-tiny, release)
- GitHub Check: AVR Panic Prevention (int8, pico-huge, release)
- GitHub Check: AVR Panic Prevention (int8, pico-tiny, release)
- GitHub Check: Run AVR Test Suites
🔇 Additional comments (4)
picojson/examples/stream_parser_demo.rs (1)
58-63: LGTM - ContentSpan guard is consistent with broader changes.The panic guard for internal ContentSpan events ensures these implementation details don't leak to user handlers. The pattern is consistently applied across examples and tests.
Note: This panic guard pattern is duplicated across multiple files (push_parser_demo.rs, push_parser_escapes.rs, push_parser_invalidslicebounds_repro.rs, push_parser.rs, push_parser_stress_test.rs). Consider extracting to a shared test utility to reduce duplication, though the current approach ensures each example/test is self-contained.
picojson/src/slice_parser.rs (1)
153-153: LGTM - Clarifying comment for ContentSpan flow.The inline comment clarifies that SliceParser doesn't need byte accumulation in the ContentSpan-based parsing flow, improving code maintainability.
picojson/Cargo.toml (1)
54-55: Let me check the actual library declaration and purpose:No issues found—log dependency is appropriately configured for no-std use.
picojson is designed as a no_std compatible pull-parser for embedded systems. The
log = "0.4.27"dependency is correctly used:
- The log crate is no-std by default and contains ContentSpan functionality debug logging with negligible overhead.
- No explicit features are enabled in Cargo.toml, confirming the no-std variant is used.
- Log macros operate without recursion or heap allocations, maintaining embedded compatibility.
picojson/tests/push_parser.rs (1)
184-184: The review comment identifies legitimate critical issues with incomplete escape handling in ContentSpan implementation.The code reveals a TODO PLACEHOLDER in
push_parser.rs(lines 113-114) where escaped content is explicitly skipped without processing:} else { // For escaped content, fall back to the existing escape processing mechanism // This delegates to the byte_accumulator callback pattern for now // TODO: PLACEHOLDER - this will be replaced in Step 4 with proper PartialContentSpan handling continue; }This
continue;statement prevents any events from being emitted for escaped strings, causing the three failing tests to fail. The test handlers would receive incomplete event sequences:
- test_simple_escapes: No
String(hello\nworld)event emitted when\nescape is encountered- test_unicode_escapes: No
String(A)event when\u0041escape is encountered- test_consecutive_unicode_escapes: No
String(쫾몾)event when consecutive\uCAFE\uBABEescapes are encounteredAdditionally, the slice bounds validation in
push_content_builder.rs(lines 310-314, 372-376) can triggerInvalidSliceBoundserrors when ContentSpan positions reference chunks beyond the current data buffer, which aligns with the review comment's mention of "buffer boundary tracking" issues.However, I cannot execute the failing tests directly to confirm the exact error messages and stack traces, as Rust compilation is unavailable in this environment.
Comment on lines
+304
to
+322
| let content_slice = self | ||
| .extractor | ||
| .get_borrowed_slice(relative_start + 1, chunk_len) | ||
| .map_err(PushParseError::Parse)?; | ||
|
|
||
| log::debug!( | ||
| "handle_partial_content_span_start: content_slice = {:?}", | ||
| core::str::from_utf8(content_slice).unwrap_or("[invalid utf8]") | ||
| ); | ||
|
|
||
| // Copy ALL data to local buffer to completely avoid borrowing conflicts | ||
| let content_data = alloc::vec::Vec::from(content_slice); | ||
|
|
||
| // Now append from local buffer - no more borrowing conflicts | ||
| for byte in &content_data { | ||
| self.extractor | ||
| .append_unescaped_byte(*byte) | ||
| .map_err(PushParseError::Parse)?; | ||
| } |
There was a problem hiding this comment.
Don't drop the first byte when resuming a span.
When a token started in a previous chunk, absolute_start < position_offset, so relative_start saturates to 0. The current code then calls get_borrowed_slice(relative_start + 1, …), which lops off the very first byte of the new chunk (the first continuation character of the token). That corrupts every cross-chunk string/key/number without escapes. Please derive the slice start from whether the opener is present in this chunk, and only skip it when it truly exists.
- let content_slice = self
- .extractor
- .get_borrowed_slice(relative_start + 1, chunk_len)
+ let mut slice_start = if absolute_start >= position_offset {
+ match kind {
+ ContentKind::String | ContentKind::Key => relative_start + 1, // skip the quote in this chunk
+ ContentKind::Number => relative_start, // numbers have no delimiter to skip
+ }
+ } else {
+ 0 // token began in an earlier chunk; copy from the very first byte we have
+ };
+ slice_start = slice_start.min(chunk_len);
+
+ let content_slice = self
+ .extractor
+ .get_borrowed_slice(slice_start, chunk_len)📝 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.
Suggested change
| let content_slice = self | |
| .extractor | |
| .get_borrowed_slice(relative_start + 1, chunk_len) | |
| .map_err(PushParseError::Parse)?; | |
| log::debug!( | |
| "handle_partial_content_span_start: content_slice = {:?}", | |
| core::str::from_utf8(content_slice).unwrap_or("[invalid utf8]") | |
| ); | |
| // Copy ALL data to local buffer to completely avoid borrowing conflicts | |
| let content_data = alloc::vec::Vec::from(content_slice); | |
| // Now append from local buffer - no more borrowing conflicts | |
| for byte in &content_data { | |
| self.extractor | |
| .append_unescaped_byte(*byte) | |
| .map_err(PushParseError::Parse)?; | |
| } | |
| let mut slice_start = if absolute_start >= position_offset { | |
| match kind { | |
| ContentKind::String | ContentKind::Key => relative_start + 1, // skip the quote in this chunk | |
| ContentKind::Number => relative_start, // numbers have no delimiter to skip | |
| } | |
| } else { | |
| 0 // token began in an earlier chunk; copy from the very first byte we have | |
| }; | |
| slice_start = slice_start.min(chunk_len); | |
| let content_slice = self | |
| .extractor | |
| .get_borrowed_slice(slice_start, chunk_len) | |
| .map_err(PushParseError::Parse)?; | |
| log::debug!( | |
| "handle_partial_content_span_start: content_slice = {:?}", | |
| core::str::from_utf8(content_slice).unwrap_or("[invalid utf8]") | |
| ); | |
| // Copy ALL data to local buffer to completely avoid borrowing conflicts | |
| let content_data = alloc::vec::Vec::from(content_slice); | |
| // Now append from local buffer - no more borrowing conflicts | |
| for byte in &content_data { | |
| self.extractor | |
| .append_unescaped_byte(*byte) | |
| .map_err(PushParseError::Parse)?; | |
| } |
🤖 Prompt for AI Agents
picojson/src/push_parser.rs around lines 304 to 322: the code always skips the
first byte of the borrowed slice by using relative_start + 1, which drops the
first continuation byte when a token started in a previous chunk; compute the
slice start based on whether the opener is present in this chunk (i.e. check if
absolute_start < position_offset) and only add 1 when the opener is actually in
this chunk, then call get_borrowed_slice with that computed start and use that
slice to build the local buffer before appending bytes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Introduce ContentSpan tracking to ParserCore and push parser to efficiently handle string, key, and number tokens that fit entirely within or span across input chunks without escapes, while falling back to existing escape-based parsing for complex cases.
New Features:
Enhancements:
Build:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Chores