Address some feedback

kaidokert · kaidokert · commit 2344e766dfbd · 2025-07-26T00:02:18.000-07:00
diff --git a/picojson/src/parse_error.rs b/picojson/src/parse_error.rs
@@ -17,8 +17,6 @@ pub enum ParseError {
     ScratchBufferFull,
     /// The input buffer is full.
     InputBufferFull,
-    /// A string slice was not valid UTF-8.
-    InvalidUtf8(core::str::Utf8Error),
     /// A number string could not be parsed.
     InvalidNumber,
     /// The parser entered an unexpected internal state.
@@ -67,7 +65,7 @@ impl From<stream_buffer::StreamBufferError> for ParseError {
 
 impl From<core::str::Utf8Error> for ParseError {
     fn from(err: core::str::Utf8Error) -> Self {
-        ParseError::InvalidUtf8(err)
+        ParseError::Utf8(err)
     }
 }
 
@@ -87,7 +85,7 @@ impl core::fmt::Display for ParseError {
     fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
         match self {
             ParseError::TokenizerError(e) => write!(f, "{e}"),
-            ParseError::InvalidUtf8(e) => write!(f, "Invalid UTF-8: {e}"),
+            ParseError::Utf8(e) => write!(f, "Invalid UTF-8: {e}"),
             _ => write!(f, "{self:?}"),
         }
     }
@@ -141,10 +139,10 @@ mod tests {
             Err(utf8_error) => {
                 let parse_error: ParseError = utf8_error.into();
                 match parse_error {
-                    ParseError::InvalidUtf8(_) => {
+                    ParseError::Utf8(_) => {
                         // Expected - conversion works correctly
                     }
-                    _ => panic!("Expected InvalidUtf8 error"),
+                    _ => panic!("Expected Utf8 error"),
                 }
             }
             Ok(_) => panic!("Expected UTF-8 validation to fail"),
diff --git a/picojson/src/push_parser.rs b/picojson/src/push_parser.rs
@@ -139,11 +139,11 @@ where
                                                         }
                                                     }
                                                 }
-                                                Err(_e) => {}
+                                                Err(e) => return Err(e.into()),
                                             }
                                         }
                                     }
-                                    Err(_e) => {}
+                                    Err(e) => return Err(e.into()),
                                 }
                             }
                             _ => {}
@@ -175,14 +175,23 @@ where
     where
         H: for<'a, 'b> PushParserHandler<'a, 'b, E>,
     {
-        // Handle any remaining content in the buffer
-        if self.state == ParserState::ParsingNumber {
-            let s = self.stream_buffer.get_unescaped_slice()?;
-            let num = JsonNumber::from_slice(s)?;
-            self.handler
-                .handle_event(Event::Number(num))
-                .map_err(PushParseError::Handler)?;
-            self.stream_buffer.clear_unescaped();
+        // Handle any remaining content in the buffer and check for incomplete parsing
+        match self.state {
+            ParserState::ParsingNumber => {
+                let s = self.stream_buffer.get_unescaped_slice()?;
+                let num = JsonNumber::from_slice(s)?;
+                self.handler
+                    .handle_event(Event::Number(num))
+                    .map_err(PushParseError::Handler)?;
+                self.stream_buffer.clear_unescaped();
+            }
+            ParserState::ParsingString => {
+                return Err(PushParseError::Parse(ParseError::EndOfData));
+            }
+            ParserState::ParsingKey => {
+                return Err(PushParseError::Parse(ParseError::EndOfData));
+            }
+            ParserState::Idle => {}
         }
 
         // Check that the JSON document is complete (all containers closed)
@@ -528,11 +537,11 @@ where
                                                 }
                                             }
                                         }
-                                        Err(_e) => {}
+                                        Err(e) => return Err(e.into()),
                                     }
                                 }
                             }
-                            Err(_e) => {}
+                            Err(e) => return Err(e.into()),
                         }
                     }
                 }
@@ -630,10 +639,10 @@ fn create_tokenizer_callback(
     event_storage: &mut [Option<(ujson::Event, usize)>; 2],
     position_offset: usize,
 ) -> impl FnMut(ujson::Event, usize) + '_ {
-    move |event, _pos| {
+    move |event, pos| {
         for evt in event_storage.iter_mut() {
             if evt.is_none() {
-                *evt = Some((event, position_offset));
+                *evt = Some((event, position_offset + pos));
                 return;
             }
         }
diff --git a/picojson/src/stream_parser.rs b/picojson/src/stream_parser.rs
@@ -756,8 +756,7 @@ mod tests {
                     #[cfg(feature = "float")]
                     NumberResult::Float(f) => {
                         // This is expected in float-enabled build
-                        #[allow(clippy::approx_constant)]
-                        assert!((f - 3.14).abs() < 0.01);
+                        assert!((f - core::f64::consts::PI).abs() < 0.01);
                     }
                     #[cfg(feature = "float-skip")]
                     NumberResult::FloatSkipped => {
diff --git a/picojson/tests/json_checker_tests.rs b/picojson/tests/json_checker_tests.rs
@@ -39,7 +39,6 @@ mod json_checker_tests {
     // Test handler for PushParser conformance tests
     struct ConformanceTestHandler {
         event_count: usize,
-        error: Option<String>,
     }
 
     impl<'a, 'b> PushParserHandler<'a, 'b, String> for ConformanceTestHandler {
@@ -53,7 +52,6 @@ mod json_checker_tests {
         let mut buffer = [0u8; 2048]; // Larger buffer for pass1.json
         let handler = ConformanceTestHandler {
             event_count: 0,
-            error: None,
         };
         let mut parser = PushParser::<_, DefaultConfig>::new(handler, &mut buffer);
 
diff --git a/picojson/tests/push_parser.rs b/picojson/tests/push_parser.rs
@@ -582,41 +582,6 @@ mod tests {
         assert_eq!(parser.finish::<()>(), Ok(()));
     }
 
-    #[test]
-    fn test_unicode_escapes_not_yet_implemented() {
-        // Debug handler that captures strings and keys to test Unicode escape processing
-        struct UnicodeEscapeTestHandler {
-            events: Vec<std::string::String>,
-        }
-
-        impl<'a, 'b> PushParserHandler<'a, 'b, ()> for UnicodeEscapeTestHandler {
-            fn handle_event(&mut self, event: Event<'a, 'b>) -> Result<(), ()> {
-                let event_desc = match event {
-                    Event::StartObject => "StartObject".to_string(),
-                    Event::EndObject => "EndObject".to_string(),
-                    Event::Key(k) => format!("Key({})", k.as_ref()),
-                    Event::String(s) => format!("String({})", s.as_ref()),
-                    Event::EndDocument => "EndDocument".to_string(),
-                    _ => "Other".to_string(),
-                };
-                self.events.push(event_desc);
-                Ok(())
-            }
-        }
-
-        let mut buffer = [0u8; 256];
-        let handler = UnicodeEscapeTestHandler { events: Vec::new() };
-        let mut parser = PushParser::<_, DefaultConfig>::new(handler, &mut buffer);
-
-        // Test string with Unicode escape sequence (\u0041 should become 'A')
-        parser.write(br#"{"key": "\u0041"}"#).unwrap();
-        parser.finish::<()>().unwrap();
-        let handler = parser.destroy();
-
-        println!("Unicode escape events: {:?}", handler.events);
-
-        // This test verifies that Unicode escapes are working
-    }
 
     #[test]
     fn test_numbers() {
diff --git a/picojson/tests/push_parser_stress_test.rs b/picojson/tests/push_parser_stress_test.rs
@@ -9,15 +9,30 @@ use picojson::{
     DefaultConfig, Event, JsonNumber, NumberResult, PushParseError, PushParser, PushParserHandler,
 };
 
+/// Owned event representation for comparison
+#[derive(Debug, Clone, PartialEq)]
+enum OwnedEvent {
+    StartObject,
+    EndObject,
+    StartArray,
+    EndArray,
+    Key(String),
+    String(String),
+    Number(String),
+    Bool(bool),
+    Null,
+    EndDocument,
+}
+
 /// Handler that collects events for verification during stress testing
-struct StressTestHandler<'expected> {
-    events: Vec<Event<'static, 'static>>,
-    expected_events: &'expected [Event<'static, 'static>],
+struct StressTestHandler {
+    events: Vec<OwnedEvent>,
+    expected_events: Vec<OwnedEvent>,
     current_index: usize,
 }
 
-impl<'expected> StressTestHandler<'expected> {
-    fn new(expected_events: &'expected [Event<'static, 'static>]) -> Self {
+impl StressTestHandler {
+    fn new(expected_events: Vec<OwnedEvent>) -> Self {
         Self {
             events: Vec::new(),
             expected_events,
@@ -40,7 +55,7 @@ impl<'expected> StressTestHandler<'expected> {
             .zip(self.expected_events.iter())
             .enumerate()
         {
-            if !events_equal(actual, expected) {
+            if actual != expected {
                 return Err(format!(
                     "Event {} mismatch: expected {:?}, got {:?}",
                     i, expected, actual
@@ -52,30 +67,22 @@ impl<'expected> StressTestHandler<'expected> {
     }
 }
 
-impl<'input, 'scratch, 'expected> PushParserHandler<'input, 'scratch, String>
-    for StressTestHandler<'expected>
+impl<'input, 'scratch> PushParserHandler<'input, 'scratch, String>
+    for StressTestHandler
 {
     fn handle_event(&mut self, event: Event<'input, 'scratch>) -> Result<(), String> {
         // Convert to owned event for storage
         let owned_event = match event {
-            Event::StartObject => Event::StartObject,
-            Event::EndObject => Event::EndObject,
-            Event::StartArray => Event::StartArray,
-            Event::EndArray => Event::EndArray,
-            Event::Key(k) => Event::Key(picojson::String::Borrowed(k.as_ref().to_string().leak())),
-            Event::String(s) => {
-                Event::String(picojson::String::Borrowed(s.as_ref().to_string().leak()))
-            }
-            Event::Number(n) => Event::Number(JsonNumber::Borrowed {
-                raw: n.as_str().to_string().leak(),
-                parsed: n
-                    .as_int()
-                    .map(NumberResult::Integer)
-                    .unwrap_or(NumberResult::IntegerOverflow),
-            }),
-            Event::Bool(b) => Event::Bool(b),
-            Event::Null => Event::Null,
-            Event::EndDocument => Event::EndDocument,
+            Event::StartObject => OwnedEvent::StartObject,
+            Event::EndObject => OwnedEvent::EndObject,
+            Event::StartArray => OwnedEvent::StartArray,
+            Event::EndArray => OwnedEvent::EndArray,
+            Event::Key(k) => OwnedEvent::Key(k.as_ref().to_string()),
+            Event::String(s) => OwnedEvent::String(s.as_ref().to_string()),
+            Event::Number(n) => OwnedEvent::Number(n.as_str().to_string()),
+            Event::Bool(b) => OwnedEvent::Bool(b),
+            Event::Null => OwnedEvent::Null,
+            Event::EndDocument => OwnedEvent::EndDocument,
         };
 
         self.events.push(owned_event);
@@ -84,20 +91,21 @@ impl<'input, 'scratch, 'expected> PushParserHandler<'input, 'scratch, String>
     }
 }
 
-/// Compare events for equality, handling lifetime differences
-fn events_equal(a: &Event, b: &Event) -> bool {
-    match (a, b) {
-        (Event::StartObject, Event::StartObject) => true,
-        (Event::EndObject, Event::EndObject) => true,
-        (Event::StartArray, Event::StartArray) => true,
-        (Event::EndArray, Event::EndArray) => true,
-        (Event::Key(k1), Event::Key(k2)) => k1.as_ref() == k2.as_ref(),
-        (Event::String(s1), Event::String(s2)) => s1.as_ref() == s2.as_ref(),
-        (Event::Number(n1), Event::Number(n2)) => n1.as_str() == n2.as_str(),
-        (Event::Bool(b1), Event::Bool(b2)) => b1 == b2,
-        (Event::Null, Event::Null) => true,
-        (Event::EndDocument, Event::EndDocument) => true,
-        _ => false,
+impl OwnedEvent {
+    /// Convert from Event to OwnedEvent
+    fn from_event(event: &Event) -> Self {
+        match event {
+            Event::StartObject => OwnedEvent::StartObject,
+            Event::EndObject => OwnedEvent::EndObject,
+            Event::StartArray => OwnedEvent::StartArray,
+            Event::EndArray => OwnedEvent::EndArray,
+            Event::Key(k) => OwnedEvent::Key(k.as_ref().to_string()),
+            Event::String(s) => OwnedEvent::String(s.as_ref().to_string()),
+            Event::Number(n) => OwnedEvent::Number(n.as_str().to_string()),
+            Event::Bool(b) => OwnedEvent::Bool(*b),
+            Event::Null => OwnedEvent::Null,
+            Event::EndDocument => OwnedEvent::EndDocument,
+        }
     }
 }
 
@@ -299,7 +307,7 @@ fn test_push_parsing_with_config(
     chunk_pattern: &[usize],
 ) -> Result<(), String> {
     let mut buffer = vec![0u8; buffer_size];
-    let handler = StressTestHandler::new(&scenario.expected_events);
+    let handler = StressTestHandler::new(scenario.expected_events.iter().map(OwnedEvent::from_event).collect());
     let mut parser = PushParser::<_, DefaultConfig>::new(handler, &mut buffer);
 
     let mut writer = ChunkedWriter::new(scenario.json, chunk_pattern);
@@ -539,7 +547,7 @@ fn test_push_parser_stress_document_validation() {
 
         for &pattern in chunk_patterns {
             let mut buffer = vec![0u8; buffer_size];
-            let handler = StressTestHandler::new(&[]);
+            let handler = StressTestHandler::new(vec![]);
             let mut parser = PushParser::<_, DefaultConfig>::new(handler, &mut buffer);
             let mut writer = ChunkedWriter::new(json, pattern);
 

Original file line number	Diff line number	Diff line change
`@@ -17,8 +17,6 @@ pub enum ParseError {`
`17`	`17`	`ScratchBufferFull,`
`18`	`18`	`/// The input buffer is full.`
`19`	`19`	`InputBufferFull,`
`20`		`- /// A string slice was not valid UTF-8.`
`21`		`- InvalidUtf8(core::str::Utf8Error),`
`22`	`20`	`/// A number string could not be parsed.`
`23`	`21`	`InvalidNumber,`
`24`	`22`	`/// The parser entered an unexpected internal state.`
`@@ -67,7 +65,7 @@ impl From<stream_buffer::StreamBufferError> for ParseError {`
`67`	`65`
`68`	`66`	`impl From<core::str::Utf8Error> for ParseError {`
`69`	`67`	`fn from(err: core::str::Utf8Error) -> Self {`
`70`		`- ParseError::InvalidUtf8(err)`
	`68`	`+ ParseError::Utf8(err)`
`71`	`69`	`}`
`72`	`70`	`}`
`73`	`71`
`@@ -87,7 +85,7 @@ impl core::fmt::Display for ParseError {`
`87`	`85`	`fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {`
`88`	`86`	`match self {`
`89`	`87`	`ParseError::TokenizerError(e) => write!(f, "{e}"),`
`90`		`- ParseError::InvalidUtf8(e) => write!(f, "Invalid UTF-8: {e}"),`
	`88`	`+ ParseError::Utf8(e) => write!(f, "Invalid UTF-8: {e}"),`
`91`	`89`	`_ => write!(f, "{self:?}"),`
`92`	`90`	`}`
`93`	`91`	`}`
`@@ -141,10 +139,10 @@ mod tests {`
`141`	`139`	`Err(utf8_error) => {`
`142`	`140`	`let parse_error: ParseError = utf8_error.into();`
`143`	`141`	`match parse_error {`
`144`		`- ParseError::InvalidUtf8(_) => {`
	`142`	`+ ParseError::Utf8(_) => {`
`145`	`143`	`// Expected - conversion works correctly`
`146`	`144`	`}`
`147`		`- _ => panic!("Expected InvalidUtf8 error"),`
	`145`	`+ _ => panic!("Expected Utf8 error"),`
`148`	`146`	`}`
`149`	`147`	`}`
`150`	`148`	`Ok(_) => panic!("Expected UTF-8 validation to fail"),`
Original file line number	Diff line number	Diff line change
`@@ -139,11 +139,11 @@ where`
`139`	`139`	`}`
`140`	`140`	`}`
`141`	`141`	`}`
`142`		`- Err(_e) => {}`
	`142`	`+ Err(e) => return Err(e.into()),`
`143`	`143`	`}`
`144`	`144`	`}`
`145`	`145`	`}`
`146`		`- Err(_e) => {}`
	`146`	`+ Err(e) => return Err(e.into()),`
`147`	`147`	`}`
`148`	`148`	`}`
`149`	`149`	`_ => {}`
`@@ -175,14 +175,23 @@ where`
`175`	`175`	`where`
`176`	`176`	`H: for<'a, 'b> PushParserHandler<'a, 'b, E>,`
`177`	`177`	`{`
`178`		`- // Handle any remaining content in the buffer`
`179`		`- if self.state == ParserState::ParsingNumber {`
`180`		`- let s = self.stream_buffer.get_unescaped_slice()?;`
`181`		`- let num = JsonNumber::from_slice(s)?;`
`182`		`- self.handler`
`183`		`- .handle_event(Event::Number(num))`
`184`		`- .map_err(PushParseError::Handler)?;`
`185`		`- self.stream_buffer.clear_unescaped();`
	`178`	`+ // Handle any remaining content in the buffer and check for incomplete parsing`
	`179`	`+ match self.state {`
	`180`	`+ ParserState::ParsingNumber => {`
	`181`	`+ let s = self.stream_buffer.get_unescaped_slice()?;`
	`182`	`+ let num = JsonNumber::from_slice(s)?;`
	`183`	`+ self.handler`
	`184`	`+ .handle_event(Event::Number(num))`
	`185`	`+ .map_err(PushParseError::Handler)?;`
	`186`	`+ self.stream_buffer.clear_unescaped();`
	`187`	`+ }`
	`188`	`+ ParserState::ParsingString => {`
	`189`	`+ return Err(PushParseError::Parse(ParseError::EndOfData));`
	`190`	`+ }`
	`191`	`+ ParserState::ParsingKey => {`
	`192`	`+ return Err(PushParseError::Parse(ParseError::EndOfData));`
	`193`	`+ }`
	`194`	`+ ParserState::Idle => {}`
`186`	`195`	`}`
`187`	`196`
`188`	`197`	`// Check that the JSON document is complete (all containers closed)`
`@@ -528,11 +537,11 @@ where`
`528`	`537`	`}`
`529`	`538`	`}`
`530`	`539`	`}`
`531`		`- Err(_e) => {}`
	`540`	`+ Err(e) => return Err(e.into()),`
`532`	`541`	`}`
`533`	`542`	`}`
`534`	`543`	`}`
`535`		`- Err(_e) => {}`
	`544`	`+ Err(e) => return Err(e.into()),`
`536`	`545`	`}`
`537`	`546`	`}`
`538`	`547`	`}`
`@@ -630,10 +639,10 @@ fn create_tokenizer_callback(`
`630`	`639`	`event_storage: &mut [Option<(ujson::Event, usize)>; 2],`
`631`	`640`	`position_offset: usize,`
`632`	`641`	`) -> impl FnMut(ujson::Event, usize) + '_ {`
`633`		`- move \|event, _pos\| {`
	`642`	`+ move \|event, pos\| {`
`634`	`643`	`for evt in event_storage.iter_mut() {`
`635`	`644`	`if evt.is_none() {`
`636`		`- *evt = Some((event, position_offset));`
	`645`	`+ *evt = Some((event, position_offset + pos));`
`637`	`646`	`return;`
`638`	`647`	`}`
`639`	`648`	`}`