From 8aa317df344c01761fa9eb27410cddeb5304fb43 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Tue, 24 Oct 2023 00:58:24 +0200 Subject: [PATCH] TEMP/NIGHTLY: Use `impl Trait` as return type for `next_string_reader` and `string_value_writer` --- rust-toolchain.toml | 2 +- src/lib.rs | 1 + src/reader/mod.rs | 3 +- src/reader/stream_reader.rs | 38 +++++++++++---- src/writer/mod.rs | 12 ++--- src/writer/stream_writer.rs | 94 ++++++++++++++++++++----------------- tests/custom_json_reader.rs | 8 ++-- tests/custom_json_writer.rs | 12 +++-- tests/reader_alloc_test.rs | 15 +++--- tests/writer_alloc_test.rs | 13 ++--- 10 files changed, 112 insertions(+), 86 deletions(-) diff --git a/rust-toolchain.toml b/rust-toolchain.toml index d636795..630707d 100644 --- a/rust-toolchain.toml +++ b/rust-toolchain.toml @@ -1,4 +1,4 @@ [toolchain] -channel = "1.73.0" +channel = "nightly" profile = "minimal" components = ["clippy", "rustfmt", "rust-docs"] diff --git a/src/lib.rs b/src/lib.rs index 5f6fa18..dca458e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -14,6 +14,7 @@ // When `docsrs` configuration flag is set enable banner for features in documentation // See https://stackoverflow.com/q/61417452 #![cfg_attr(docsrs, feature(doc_auto_cfg))] +#![feature(return_position_impl_trait_in_trait)] //! Struson is an [RFC 8259](https://www.rfc-editor.org/rfc/rfc8259.html) compliant streaming JSON reader and writer. //! diff --git a/src/reader/mod.rs b/src/reader/mod.rs index bcdb92d..9fe1163 100644 --- a/src/reader/mod.rs +++ b/src/reader/mod.rs @@ -1136,6 +1136,7 @@ pub trait JsonReader { /// # Examples /// ``` /// # use struson::reader::*; + /// # use std::io::Read; /// let mut json_reader = JsonStreamReader::new(r#"["hello"]"#.as_bytes()); /// json_reader.begin_array()?; /// @@ -1169,7 +1170,7 @@ pub trait JsonReader { /// when called after the top-level value has already been consumed and multiple top-level /// values are not enabled in the [`ReaderSettings`]. Both cases indicate incorrect /// usage by the user and are unrelated to the JSON data. - fn next_string_reader(&mut self) -> Result, ReaderError>; + fn next_string_reader(&mut self) -> Result; /// Consumes and returns a JSON number value /// diff --git a/src/reader/stream_reader.rs b/src/reader/stream_reader.rs index e49d47a..824e774 100644 --- a/src/reader/stream_reader.rs +++ b/src/reader/stream_reader.rs @@ -17,7 +17,7 @@ use super::json_path::json_path; use crate::{ json_number::{consume_json_number, NumberBytesProvider}, utf8, - writer::TransferredNumber, + writer::{StringValueWriter, TransferredNumber}, }; #[derive(PartialEq, Clone, Copy, strum::Display, Debug)] @@ -2095,16 +2095,16 @@ impl JsonReader for JsonStreamReader { Ok(str_bytes.get_str(self)) } - fn next_string_reader(&mut self) -> Result, ReaderError> { + fn next_string_reader(&mut self) -> Result { self.start_expected_value_type(ValueType::String)?; self.is_string_value_reader_active = true; - Ok(Box::new(StringValueReader { + Ok(StringValueReader { json_reader: self, utf8_buf: [0; STRING_VALUE_READER_BUF_SIZE], utf8_start_pos: 0, utf8_count: 0, reached_end: false, - })) + }) } fn next_number_as_string(&mut self) -> Result { @@ -2399,6 +2399,8 @@ impl<'j, R: Read> Read for StringValueReader<'j, R> { #[cfg(test)] mod tests { + use std::io::Write; + use super::*; use crate::writer::{ FiniteNumber, FloatingPointNumber, JsonNumberError, JsonStreamWriter, StringValueWriter, @@ -4029,12 +4031,32 @@ mod tests { #[test] fn transfer_to_writer_error() { - struct FailingJsonWriter; - fn err() -> IoError { IoError::new(ErrorKind::Other, "test error") } + struct FailingStringValueWriter; + impl Write for FailingStringValueWriter { + fn write(&mut self, _: &[u8]) -> std::io::Result { + Err(err()) + } + + fn flush(&mut self) -> std::io::Result<()> { + Err(err()) + } + } + impl StringValueWriter for FailingStringValueWriter { + fn write_str(&mut self, _: &str) -> Result<(), IoError> { + Err(err()) + } + + fn finish_value(self) -> Result<(), IoError> { + Err(err()) + } + } + + struct FailingJsonWriter; + // JsonWriter which always returns Err(...) // Note: If maintaining this becomes too cumbersome when adjusting JsonWriter API, can remove this test impl JsonWriter for FailingJsonWriter { @@ -4070,8 +4092,8 @@ mod tests { Err(err()) } - fn string_value_writer(&mut self) -> Result, IoError> { - Err(err()) + fn string_value_writer(&mut self) -> Result { + Err::(err()) } fn number_value_from_string(&mut self, _: &str) -> Result<(), JsonNumberError> { diff --git a/src/writer/mod.rs b/src/writer/mod.rs index c58d3f9..f708870 100644 --- a/src/writer/mod.rs +++ b/src/writer/mod.rs @@ -304,6 +304,7 @@ pub trait JsonWriter { /// # Examples /// ``` /// # use struson::writer::*; + /// # use std::io::Write; /// let mut writer = Vec::::new(); /// let mut json_writer = JsonStreamWriter::new(&mut writer); /// @@ -325,13 +326,7 @@ pub trait JsonWriter { /// when called after the top-level value has already been written and multiple top-level /// values are not enabled in the [`WriterSettings`]. Both cases indicate incorrect /// usage by the user. - /* - * TODO: Instead of Box should this directly declare struct as return type - * (and not have separate trait StringValueWriter)? - * But then users might not be able to implement JsonWriter themselves anymore easily; - * would also be inconsistent with JsonReader::next_string_reader - */ - fn string_value_writer(&mut self) -> Result, IoError>; + fn string_value_writer(&mut self) -> Result; /// Writes the string representation of a JSON number value /// @@ -555,6 +550,7 @@ pub trait StringValueWriter: Write { /// # Examples /// ``` /// # use struson::writer::*; + /// # use std::io::Write; /// let mut writer = Vec::::new(); /// let mut json_writer = JsonStreamWriter::new(&mut writer); /// @@ -581,7 +577,7 @@ pub trait StringValueWriter: Write { /// This method must be called when writing the string value is done to allow /// using the original JSON writer again. /* Consumes 'self' */ - fn finish_value(self: Box) -> Result<(), IoError>; + fn finish_value(self) -> Result<(), IoError>; } /// Sealed trait for finite number types such as `u32` diff --git a/src/writer/stream_writer.rs b/src/writer/stream_writer.rs index 26d9be4..b48e237 100644 --- a/src/writer/stream_writer.rs +++ b/src/writer/stream_writer.rs @@ -517,16 +517,16 @@ impl JsonWriter for JsonStreamWriter { self.flush() } - fn string_value_writer(&mut self) -> Result, IoError> { + fn string_value_writer(&mut self) -> Result { self.before_value()?; self.write_bytes(b"\"")?; self.is_string_value_writer_active = true; - Ok(Box::new(StringValueWriterImpl { + Ok(StringValueWriterImpl { json_writer: self, utf8_buf: [0; utf8::MAX_BYTES_PER_CHAR], utf8_pos: 0, utf8_expected_len: 0, - })) + }) } } @@ -732,7 +732,7 @@ impl<'j, W: Write> StringValueWriter for StringValueWriterImpl<'j, W> { Ok(()) } - fn finish_value(self: Box) -> Result<(), IoError> { + fn finish_value(self) -> Result<(), IoError> { if self.utf8_pos > 0 { return Err(IoError::new( ErrorKind::InvalidData, @@ -1054,53 +1054,61 @@ mod tests { #[test] fn string_writer_invalid() { - fn assert_invalid_utf8< - T, - F: FnOnce(Box) -> Result, - >( - f: F, - expected_custom_message: Option<&str>, - ) { - let mut writer = Vec::::new(); - let mut json_writer = JsonStreamWriter::new(&mut writer); - - match f(json_writer.string_value_writer().unwrap()) { - Ok(_) => panic!("Should have failed"), - Err(e) => { - assert_eq!(ErrorKind::InvalidData, e.kind()); - - match expected_custom_message { - // None if error message should not be compared, e.g. because it is created by Rust and might change - None => assert!( - e.get_ref().unwrap().is::(), - "Inner error is not Utf8Error" - ), - Some(message) => { - assert_eq!(message, e.to_string(), "Custom message does not match") + // Uses macro instead of function with FnOnce(Box<...>) as parameter to avoid issues with + // calling `StringValueWriter::finish_value` consuming `self`, see https://stackoverflow.com/q/46620790 + macro_rules! assert_invalid_utf8 { + ( + |$string_value_writer:ident| $writing_expr:expr, + $expected_custom_message:expr, // Option<&str> + ) => { + let mut writer = Vec::::new(); + let mut json_writer = JsonStreamWriter::new(&mut writer); + let mut $string_value_writer = json_writer.string_value_writer().unwrap(); + #[allow(unused_mut)] // only for some callers the closure has to be mutable + let mut writing_function = || -> Result<(), IoError> { + $writing_expr + }; + // Explicitly specify expression type to avoid callers having to specify it + let expected_custom_message: Option<&str> = $expected_custom_message; + + match writing_function() { + Ok(_) => panic!("Should have failed"), + Err(e) => { + assert_eq!(ErrorKind::InvalidData, e.kind()); + + match expected_custom_message { + // None if error message should not be compared, e.g. because it is created by Rust and might change + None => assert!( + e.get_ref().unwrap().is::(), + "Inner error is not Utf8Error" + ), + Some(message) => { + assert_eq!(message, e.to_string(), "Custom message does not match") + } } } } } } - assert_invalid_utf8( - |mut w| { + assert_invalid_utf8!( + |w| { // Invalid UTF-8 byte 1111_1000 w.write_all(b"\xF8") }, Some("invalid UTF-8 data"), ); - assert_invalid_utf8( - |mut w| { + assert_invalid_utf8!( + |w| { // Malformed UTF-8; high surrogate U+D800 encoded in UTF-8 (= invalid) w.write_all(b"\xED\xA0\x80") }, None, ); - assert_invalid_utf8( - |mut w| { + assert_invalid_utf8!( + |w| { // Greater than max code point U+10FFFF; split across multiple bytes w.write_all(b"\xF4")?; w.write_all(b"\x90")?; @@ -1110,16 +1118,16 @@ mod tests { None, ); - assert_invalid_utf8( - |mut w| { + assert_invalid_utf8!( + |w| { // Overlong encoding for two bytes w.write_all(b"\xC1\xBF") }, None, ); - assert_invalid_utf8( - |mut w| { + assert_invalid_utf8!( + |w| { // Incomplete four bytes w.write_all(b"\xF0")?; w.write_all(b"\x90")?; @@ -1129,8 +1137,8 @@ mod tests { Some("incomplete multi-byte UTF-8 data"), ); - assert_invalid_utf8( - |mut w| { + assert_invalid_utf8!( + |w| { // Leading continuation byte w.write_all(b"\x80")?; w.finish_value() @@ -1138,8 +1146,8 @@ mod tests { None, ); - assert_invalid_utf8( - |mut w| { + assert_invalid_utf8!( + |w| { // Too many continuation bytes w.write_all(b"\xC2")?; w.write_all(b"\x80")?; @@ -1149,8 +1157,8 @@ mod tests { None, ); - assert_invalid_utf8( - |mut w| { + assert_invalid_utf8!( + |w| { // Incomplete multi-byte followed by `write_str` w.write_all(b"\xF0")?; w.write_str("")?; // even empty string should trigger this error diff --git a/tests/custom_json_reader.rs b/tests/custom_json_reader.rs index 3f347e1..29944d0 100644 --- a/tests/custom_json_reader.rs +++ b/tests/custom_json_reader.rs @@ -1,6 +1,7 @@ // Allow `assert_eq!(true, ...)` because in some cases it is used to check a bool // value and not a 'flag' / 'state', and `assert_eq!` makes that more explicit #![allow(clippy::bool_assert_comparison)] +#![feature(return_position_impl_trait_in_trait)] //! Integration test for a custom `JsonReader` implementation //! @@ -13,6 +14,7 @@ use crate::custom_reader::JsonValueReader; use serde_json::json; +use std::io::Read; use struson::{ reader::{ json_path::{json_path, JsonPath}, @@ -293,15 +295,15 @@ mod custom_reader { self.next_str().map(str::to_owned) } - fn next_string_reader(&mut self) -> Result, ReaderError> { + fn next_string_reader(&mut self) -> Result { self.begin_value(ValueType::String)?; if let Some(Value::String(s)) = self.next_value.take() { self.is_string_value_reader_active = true; - Ok(Box::new(StringValueReader { + Ok(StringValueReader { delegate: s.as_bytes(), json_reader: self, reached_end: false, - })) + }) } else { unreachable!("begin_value should have verified that value is string") } diff --git a/tests/custom_json_writer.rs b/tests/custom_json_writer.rs index 77713a1..8ee22ab 100644 --- a/tests/custom_json_writer.rs +++ b/tests/custom_json_writer.rs @@ -1,6 +1,7 @@ // Allow `assert_eq!(true, ...)` because in some cases it is used to check a bool // value and not a 'flag' / 'state', and `assert_eq!` makes that more explicit #![allow(clippy::bool_assert_comparison)] +#![feature(return_position_impl_trait_in_trait)] //! Integration test for a custom `JsonWriter` implementation //! @@ -13,9 +14,10 @@ use custom_writer::JsonValueWriter; use serde_json::json; +use std::io::Write; use struson::{ reader::{JsonReader, JsonStreamReader}, - writer::{JsonNumberError, JsonWriter}, + writer::{JsonNumberError, JsonWriter, StringValueWriter}, }; mod custom_writer { @@ -168,13 +170,13 @@ mod custom_writer { Ok(()) } - fn string_value_writer(&mut self) -> Result, IoError> { + fn string_value_writer(&mut self) -> Result { self.check_before_value(); self.is_string_value_writer_active = true; - Ok(Box::new(StringValueWriterImpl { + Ok(StringValueWriterImpl { buf: Vec::new(), json_writer: self, - })) + }) } fn number_value_from_string(&mut self, value: &str) -> Result<(), JsonNumberError> { @@ -283,7 +285,7 @@ mod custom_writer { } } impl StringValueWriter for StringValueWriterImpl<'_, '_> { - fn finish_value(self: Box) -> Result<(), IoError> { + fn finish_value(self) -> Result<(), IoError> { let string = String::from_utf8(self.buf).map_err(|e| IoError::new(ErrorKind::InvalidData, e))?; self.json_writer.add_value(Value::String(string)); diff --git a/tests/reader_alloc_test.rs b/tests/reader_alloc_test.rs index 114c2e1..60e335b 100644 --- a/tests/reader_alloc_test.rs +++ b/tests/reader_alloc_test.rs @@ -2,7 +2,7 @@ // a 'flag' / 'state', and `assert_eq!` makes that more explicit #![allow(clippy::bool_assert_comparison)] -use std::error::Error; +use std::{error::Error, io::Read}; use assert_no_alloc::permit_alloc; // Only use import when creating debug builds, see also configuration below @@ -106,24 +106,21 @@ fn string_value_reader() -> Result<(), Box> { // Pre-allocate with expected size to avoid allocations during test execution let mut string_output = String::with_capacity(expected_string_value.len()); - // Obtain this here because return value is `Box` and therefore performs allocation - let mut string_value_reader = json_reader.next_string_reader()?; - assert_no_alloc(|| { + let mut string_value_reader = json_reader.next_string_reader()?; string_value_reader.read_to_string(&mut string_output)?; + drop(string_value_reader); + + // Use permit_alloc because assert_no_alloc currently also forbids dealloc + permit_alloc(|| json_reader.consume_trailing_whitespace())?; Ok(()) }); - drop(string_value_reader); - // TODO: Ideally would call this inside assert_no_alloc, but is not possible because `string_value_reader` - // is currently created outside of it and prevents moving `json_reader` into closure - json_reader.consume_trailing_whitespace()?; assert_eq!(expected_string_value, string_output); Ok(()) } #[test] -#[ignore = "transfer_to calls JsonWriter.string_value_writer and that returns a `Box`"] fn transfer_to() -> Result<(), Box> { let inner_json = r#"{"a":[{"b":1,"c":[[],[2,{"d":3,"e":"some string value"}]]}],"f":true}"#; let json = "{\"outer-ignored\": 1, \"outer\":[\"ignored\", ".to_owned() + inner_json + "]}"; diff --git a/tests/writer_alloc_test.rs b/tests/writer_alloc_test.rs index 155813d..35431c3 100644 --- a/tests/writer_alloc_test.rs +++ b/tests/writer_alloc_test.rs @@ -1,10 +1,10 @@ -use std::error::Error; +use std::{error::Error, io::Write}; use assert_no_alloc::permit_alloc; // Only use import when creating debug builds, see also configuration below #[cfg(debug_assertions)] use assert_no_alloc::AllocDisabler; -use struson::writer::{JsonStreamWriter, JsonWriter, WriterSettings}; +use struson::writer::{JsonStreamWriter, JsonWriter, StringValueWriter, WriterSettings}; // Only enable when creating debug builds #[cfg(debug_assertions)] @@ -106,22 +106,19 @@ fn string_value_writer() -> Result<(), Box> { let mut json_writer = JsonStreamWriter::new(&mut writer); let large_string = "abcd".repeat(500); - // Obtain this here because return value is `Box` and therefore performs allocation - let mut string_value_writer = json_writer.string_value_writer()?; assert_no_alloc(|| { + let mut string_value_writer = json_writer.string_value_writer()?; string_value_writer.write_all(b"a")?; string_value_writer.write_all(b"\0")?; string_value_writer.write_all(b"\n\t")?; string_value_writer.write_all(large_string.as_bytes())?; string_value_writer.write_all(b"test")?; + string_value_writer.finish_value()?; // Use permit_alloc because assert_no_alloc currently also forbids dealloc - permit_alloc(|| string_value_writer.finish_value())?; + permit_alloc(|| json_writer.finish_document())?; Ok(()) }); - // TODO: Ideally would call this inside assert_no_alloc, but is not possible because `string_value_writer` - // is currently created outside of it and prevents moving `json_writer` into closure - json_writer.finish_document()?; let expected_json = format!("\"a\\u0000\\n\\t{large_string}test\""); assert_eq!(expected_json, String::from_utf8(writer).unwrap());