From b4059ae15793e85d463e5e14a177df5f50b163f9 Mon Sep 17 00:00:00 2001 From: "Shane F. Carr" Date: Tue, 9 Apr 2024 14:26:32 -0700 Subject: [PATCH 01/23] Initial TryWriteable with refactoring --- utils/writeable/examples/writeable_message.rs | 2 +- utils/writeable/src/helpers.rs | 74 +++++++++++++++++ utils/writeable/src/lib.rs | 67 +++------------- utils/writeable/src/try_writeable.rs | 80 +++++++++++++++++++ 4 files changed, 168 insertions(+), 55 deletions(-) create mode 100644 utils/writeable/src/helpers.rs create mode 100644 utils/writeable/src/try_writeable.rs diff --git a/utils/writeable/examples/writeable_message.rs b/utils/writeable/examples/writeable_message.rs index b342fdb2618..d8640e172b5 100644 --- a/utils/writeable/examples/writeable_message.rs +++ b/utils/writeable/examples/writeable_message.rs @@ -47,7 +47,7 @@ impl fmt::Display for WriteableMessage { fn main() { icu_benchmark_macros::main_setup!(); - let (string, parts) = writeable_to_parts_for_test(&WriteableMessage("world")).unwrap(); + let (string, parts) = writeable::_internal::writeable_to_parts_for_test(&WriteableMessage("world")); assert_eq!(string, "Hello world 😅"); diff --git a/utils/writeable/src/helpers.rs b/utils/writeable/src/helpers.rs new file mode 100644 index 00000000000..cd57ec40f3d --- /dev/null +++ b/utils/writeable/src/helpers.rs @@ -0,0 +1,74 @@ +// This file is part of ICU4X. For terms of use, please see the file +// called LICENSE at the top level of the ICU4X source tree +// (online at: https://github.com/unicode-org/icu4x/blob/main/LICENSE ). + +use crate::*; +use alloc::string::String; +use alloc::vec::Vec; + +pub(crate) struct TestWriter { + pub(crate) string: String, + pub(crate) parts: Vec<(usize, usize, Part)>, +} + +impl TestWriter { + pub(crate) fn finish(mut self) -> (String, Vec<(usize, usize, Part)>) { + // Sort by first open and last closed + self + .parts + .sort_unstable_by_key(|(begin, end, _)| (*begin, end.wrapping_neg())); + (self.string, self.parts) + } +} + +impl fmt::Write for TestWriter { + fn write_str(&mut self, s: &str) -> fmt::Result { + self.string.write_str(s) + } + fn write_char(&mut self, c: char) -> fmt::Result { + self.string.write_char(c) + } +} + +impl PartsWrite for TestWriter { + type SubPartsWrite = Self; + fn with_part( + &mut self, + part: Part, + mut f: impl FnMut(&mut Self::SubPartsWrite) -> fmt::Result, + ) -> fmt::Result { + let start = self.string.len(); + f(self)?; + let end = self.string.len(); + if start < end { + self.parts.push((start, end, part)); + } + Ok(()) + } +} + +#[allow(clippy::type_complexity)] +pub fn writeable_to_parts_for_test( + writeable: &W, +) -> (String, Vec<(usize, usize, Part)>) { + let mut writer = helpers::TestWriter { + string: alloc::string::String::new(), + parts: Vec::new(), + }; + writeable.write_to_parts(&mut writer).expect("String writer infallible"); + writer.finish() +} + +#[doc(hidden)] +#[allow(clippy::type_complexity)] +pub fn try_writeable_to_parts_for_test( + writeable: &W, +) -> (String, Vec<(usize, usize, Part)>, Option) { + let mut writer = helpers::TestWriter { + string: alloc::string::String::new(), + parts: Vec::new(), + }; + let result = writeable.try_write_to_parts(&mut writer).expect("String writer infallible"); + let (actual_str, actual_parts) = writer.finish(); + (actual_str, actual_parts, result.err()) +} diff --git a/utils/writeable/src/lib.rs b/utils/writeable/src/lib.rs index 7d52288945a..add2bf8ac7b 100644 --- a/utils/writeable/src/lib.rs +++ b/utils/writeable/src/lib.rs @@ -69,14 +69,23 @@ extern crate alloc; mod cmp; #[cfg(feature = "either")] mod either; +mod helpers; mod impls; mod ops; +mod try_writeable; use alloc::borrow::Cow; use alloc::string::String; -use alloc::vec::Vec; use core::fmt; +pub use try_writeable::TryWriteable; + +#[doc(hidden)] +pub mod _internal { + pub use super::helpers::writeable_to_parts_for_test; + pub use super::helpers::try_writeable_to_parts_for_test; +} + /// A hint to help consumers of `Writeable` pre-allocate bytes before they call /// [`write_to`](Writeable::write_to). /// @@ -315,8 +324,7 @@ pub trait Writeable { /// ``` fn write_cmp_bytes(&self, other: &[u8]) -> core::cmp::Ordering { let mut wc = cmp::WriteComparator::new(other); - #[allow(clippy::unwrap_used)] // infallible impl - self.write_to(&mut wc).unwrap(); + let _ = self.write_to(&mut wc); wc.finish().reverse() } } @@ -396,7 +404,7 @@ macro_rules! assert_writeable_eq { }; ($actual_writeable:expr, $expected_str:expr, $($arg:tt)+) => {{ let actual_writeable = &$actual_writeable; - let (actual_str, _) = $crate::writeable_to_parts_for_test(actual_writeable).unwrap(); + let (actual_str, _) = $crate::_internal::writeable_to_parts_for_test(actual_writeable); assert_eq!(actual_str, $expected_str, $($arg)*); assert_eq!(actual_str, $crate::Writeable::write_to_string(actual_writeable), $($arg)+); let length_hint = $crate::Writeable::writeable_length_hint(actual_writeable); @@ -424,7 +432,7 @@ macro_rules! assert_writeable_parts_eq { }; ($actual_writeable:expr, $expected_str:expr, $expected_parts:expr, $($arg:tt)+) => {{ let actual_writeable = &$actual_writeable; - let (actual_str, actual_parts) = $crate::writeable_to_parts_for_test(actual_writeable).unwrap(); + let (actual_str, actual_parts) = $crate::_internal::writeable_to_parts_for_test(actual_writeable); assert_eq!(actual_str, $expected_str, $($arg)+); assert_eq!(actual_str, $crate::Writeable::write_to_string(actual_writeable), $($arg)+); assert_eq!(actual_parts, $expected_parts, $($arg)+); @@ -436,52 +444,3 @@ macro_rules! assert_writeable_parts_eq { assert_eq!(actual_writeable.to_string(), $expected_str); }}; } - -#[doc(hidden)] -#[allow(clippy::type_complexity)] -pub fn writeable_to_parts_for_test( - writeable: &W, -) -> Result<(String, Vec<(usize, usize, Part)>), fmt::Error> { - struct State { - string: alloc::string::String, - parts: Vec<(usize, usize, Part)>, - } - - impl fmt::Write for State { - fn write_str(&mut self, s: &str) -> fmt::Result { - self.string.write_str(s) - } - fn write_char(&mut self, c: char) -> fmt::Result { - self.string.write_char(c) - } - } - - impl PartsWrite for State { - type SubPartsWrite = Self; - fn with_part( - &mut self, - part: Part, - mut f: impl FnMut(&mut Self::SubPartsWrite) -> fmt::Result, - ) -> fmt::Result { - let start = self.string.len(); - f(self)?; - let end = self.string.len(); - if start < end { - self.parts.push((start, end, part)); - } - Ok(()) - } - } - - let mut state = State { - string: alloc::string::String::new(), - parts: Vec::new(), - }; - writeable.write_to_parts(&mut state)?; - - // Sort by first open and last closed - state - .parts - .sort_unstable_by_key(|(begin, end, _)| (*begin, end.wrapping_neg())); - Ok((state.string, state.parts)) -} diff --git a/utils/writeable/src/try_writeable.rs b/utils/writeable/src/try_writeable.rs new file mode 100644 index 00000000000..be6cbe42159 --- /dev/null +++ b/utils/writeable/src/try_writeable.rs @@ -0,0 +1,80 @@ +// This file is part of ICU4X. For terms of use, please see the file +// called LICENSE at the top level of the ICU4X source tree +// (online at: https://github.com/unicode-org/icu4x/blob/main/LICENSE ). + +use super::*; + +pub trait TryWriteable { + type Error; + + fn try_write_to(&self, sink: &mut W) -> Result, fmt::Error>; + + fn try_write_to_parts(&self, sink: &mut S) -> Result, fmt::Error>; + + fn try_writeable_length_hint(&self) -> LengthHint { + LengthHint::undefined() + } + + fn try_write_to_string(&self) -> Result, Self::Error> { + let hint = self.try_writeable_length_hint(); + if hint.is_zero() { + return Ok(Cow::Borrowed("")); + } + let mut output = String::with_capacity(hint.capacity()); + let result = match self.try_write_to(&mut output) { + Ok(result) => result, + Err(core::fmt::Error) => { + debug_assert!(false, "String infallible"); + Ok(()) + } + }; + result.map(|_| Cow::Owned(output)) + } + + fn try_write_cmp_bytes(&self, other: &[u8]) -> core::cmp::Ordering { + let mut wc = cmp::WriteComparator::new(other); + let _ = match self.try_write_to(&mut wc) { + Ok(result) => result, + Err(core::fmt::Error) => { + debug_assert!(false, "WriteComparator infallible"); + Ok(()) + } + }; + wc.finish().reverse() + } +} + +#[macro_export] +macro_rules! assert_try_writeable_eq { + ($actual_writeable:expr, $expected_str:expr, $expected_result:expr $(,)?) => { + $crate::assert_writeable_eq!($actual_writeable, $expected_result, $expected_str, ""); + }; + ($actual_writeable:expr, $expected_str:expr, $expected_result:expr, $($arg:tt)+) => {{ + let actual_writeable = &$actual_writeable; + let (actual_str, _, actual_result) = $crate::try_writeable_to_parts_for_test(actual_writeable); + assert_eq!(actual_str, $expected_str, $($arg)*); + assert_eq!(actual_result, $expected_result, $($arg)*); + let actual_result = match $crate::Writeable::try_write_to_string(actual_writeable) { + Ok(s) => { + assert_eq!(actual_cow_str, $expected_str, $($arg)+); + Ok(()) + } + Err(e) => Err(e) + }; + assert_eq!(actual_result, $expected_result, $($arg)*); + let length_hint = $crate::Writeable::writeable_length_hint(actual_writeable); + assert!( + length_hint.0 <= actual_str.len(), + "hint lower bound {} larger than actual length {}: {}", + length_hint.0, actual_str.len(), format!($($arg)*), + ); + if let Some(upper) = length_hint.1 { + assert!( + actual_str.len() <= upper, + "hint upper bound {} smaller than actual length {}: {}", + length_hint.0, actual_str.len(), format!($($arg)*), + ); + } + assert_eq!(actual_writeable.to_string(), $expected_str); + }}; +} From bfee8123c4c2ece4389199875421c8fa57936368 Mon Sep 17 00:00:00 2001 From: "Shane F. Carr" Date: Tue, 9 Apr 2024 15:03:22 -0700 Subject: [PATCH 02/23] Checkpoint --- utils/writeable/examples/writeable_message.rs | 3 +- utils/writeable/src/helpers.rs | 11 ++-- utils/writeable/src/lib.rs | 6 ++- utils/writeable/src/try_writeable.rs | 50 ++++++++++++++----- 4 files changed, 51 insertions(+), 19 deletions(-) diff --git a/utils/writeable/examples/writeable_message.rs b/utils/writeable/examples/writeable_message.rs index d8640e172b5..487290727a0 100644 --- a/utils/writeable/examples/writeable_message.rs +++ b/utils/writeable/examples/writeable_message.rs @@ -47,7 +47,8 @@ impl fmt::Display for WriteableMessage { fn main() { icu_benchmark_macros::main_setup!(); - let (string, parts) = writeable::_internal::writeable_to_parts_for_test(&WriteableMessage("world")); + let (string, parts) = + writeable::_internal::writeable_to_parts_for_test(&WriteableMessage("world")); assert_eq!(string, "Hello world 😅"); diff --git a/utils/writeable/src/helpers.rs b/utils/writeable/src/helpers.rs index cd57ec40f3d..df1dac96442 100644 --- a/utils/writeable/src/helpers.rs +++ b/utils/writeable/src/helpers.rs @@ -14,8 +14,7 @@ pub(crate) struct TestWriter { impl TestWriter { pub(crate) fn finish(mut self) -> (String, Vec<(usize, usize, Part)>) { // Sort by first open and last closed - self - .parts + self.parts .sort_unstable_by_key(|(begin, end, _)| (*begin, end.wrapping_neg())); (self.string, self.parts) } @@ -55,7 +54,9 @@ pub fn writeable_to_parts_for_test( string: alloc::string::String::new(), parts: Vec::new(), }; - writeable.write_to_parts(&mut writer).expect("String writer infallible"); + writeable + .write_to_parts(&mut writer) + .expect("String writer infallible"); writer.finish() } @@ -68,7 +69,9 @@ pub fn try_writeable_to_parts_for_test( string: alloc::string::String::new(), parts: Vec::new(), }; - let result = writeable.try_write_to_parts(&mut writer).expect("String writer infallible"); + let result = writeable + .try_write_to_parts(&mut writer) + .expect("String writer infallible"); let (actual_str, actual_parts) = writer.finish(); (actual_str, actual_parts, result.err()) } diff --git a/utils/writeable/src/lib.rs b/utils/writeable/src/lib.rs index add2bf8ac7b..4a1af5c0ad7 100644 --- a/utils/writeable/src/lib.rs +++ b/utils/writeable/src/lib.rs @@ -82,8 +82,8 @@ pub use try_writeable::TryWriteable; #[doc(hidden)] pub mod _internal { - pub use super::helpers::writeable_to_parts_for_test; pub use super::helpers::try_writeable_to_parts_for_test; + pub use super::helpers::writeable_to_parts_for_test; } /// A hint to help consumers of `Writeable` pre-allocate bytes before they call @@ -421,6 +421,8 @@ macro_rules! assert_writeable_eq { ); } assert_eq!(actual_writeable.to_string(), $expected_str); + let ordering = $crate::Writeable::write_cmp_bytes(actual_writeable, $expected_str.as_bytes()); + assert_eq!(ordering, core::cmp::Ordering::Equal, $($arg)*); }}; } @@ -442,5 +444,7 @@ macro_rules! assert_writeable_parts_eq { assert!(actual_str.len() <= upper, $($arg)+); } assert_eq!(actual_writeable.to_string(), $expected_str); + let ordering = $crate::Writeable::write_cmp_bytes(actual_writeable, $expected_str.as_bytes()); + assert_eq!(ordering, core::cmp::Ordering::Equal, $($arg)*); }}; } diff --git a/utils/writeable/src/try_writeable.rs b/utils/writeable/src/try_writeable.rs index be6cbe42159..324dbe23bd0 100644 --- a/utils/writeable/src/try_writeable.rs +++ b/utils/writeable/src/try_writeable.rs @@ -3,20 +3,28 @@ // (online at: https://github.com/unicode-org/icu4x/blob/main/LICENSE ). use super::*; +use core::cmp::Ordering; pub trait TryWriteable { type Error; - fn try_write_to(&self, sink: &mut W) -> Result, fmt::Error>; + fn try_write_to( + &self, + sink: &mut W, + ) -> Result, fmt::Error>; - fn try_write_to_parts(&self, sink: &mut S) -> Result, fmt::Error>; + fn try_write_to_parts( + &self, + sink: &mut S, + ) -> Result, fmt::Error>; - fn try_writeable_length_hint(&self) -> LengthHint { - LengthHint::undefined() - } + fn try_writeable_length_hint(&self) -> Result; fn try_write_to_string(&self) -> Result, Self::Error> { - let hint = self.try_writeable_length_hint(); + let (hint, has_error) = match self.try_writeable_length_hint() { + Ok(hint) => (hint, false), + Err((hint, _)) => (hint, true), + }; if hint.is_zero() { return Ok(Cow::Borrowed("")); } @@ -28,33 +36,39 @@ pub trait TryWriteable { Ok(()) } }; + debug_assert_eq!( + has_error, + result.is_err(), + "try_writeable_length_hint and try_write_to_string should have same error state" + ); result.map(|_| Cow::Owned(output)) } - fn try_write_cmp_bytes(&self, other: &[u8]) -> core::cmp::Ordering { + fn try_write_cmp_bytes(&self, other: &[u8]) -> Result { let mut wc = cmp::WriteComparator::new(other); - let _ = match self.try_write_to(&mut wc) { + let result = match self.try_write_to(&mut wc) { Ok(result) => result, Err(core::fmt::Error) => { debug_assert!(false, "WriteComparator infallible"); Ok(()) } }; - wc.finish().reverse() + let ordering = wc.finish().reverse(); + result.map(|_| ordering).map_err(|e| (ordering, e)) } } #[macro_export] macro_rules! assert_try_writeable_eq { - ($actual_writeable:expr, $expected_str:expr, $expected_result:expr $(,)?) => { - $crate::assert_writeable_eq!($actual_writeable, $expected_result, $expected_str, ""); + ($actual_writeable:expr, $expected_str:expr $(,)?) => { + $crate::assert_writeable_eq!($actual_writeable, $expected_str, Ok(()), ""); }; ($actual_writeable:expr, $expected_str:expr, $expected_result:expr, $($arg:tt)+) => {{ let actual_writeable = &$actual_writeable; let (actual_str, _, actual_result) = $crate::try_writeable_to_parts_for_test(actual_writeable); assert_eq!(actual_str, $expected_str, $($arg)*); assert_eq!(actual_result, $expected_result, $($arg)*); - let actual_result = match $crate::Writeable::try_write_to_string(actual_writeable) { + let actual_result = match $crate::TryWriteable::try_write_to_string(actual_writeable) { Ok(s) => { assert_eq!(actual_cow_str, $expected_str, $($arg)+); Ok(()) @@ -62,7 +76,10 @@ macro_rules! assert_try_writeable_eq { Err(e) => Err(e) }; assert_eq!(actual_result, $expected_result, $($arg)*); - let length_hint = $crate::Writeable::writeable_length_hint(actual_writeable); + let (length_hint, actual_result) = match $crate::TryWriteable::writeable_length_hint(actual_writeable) { + Ok(length_hint) => (length_hint, Ok(())), + Err((length_hint, e)) => (length_hint, Err(e)), + }; assert!( length_hint.0 <= actual_str.len(), "hint lower bound {} larger than actual length {}: {}", @@ -75,6 +92,13 @@ macro_rules! assert_try_writeable_eq { length_hint.0, actual_str.len(), format!($($arg)*), ); } + assert_eq!(actual_result, $expected_result, $($arg)*); assert_eq!(actual_writeable.to_string(), $expected_str); + let (ordering, actual_result) = match $crate::Writeable::try_write_cmp_bytes(actual_writeable, $expected_str.as_bytes()) { + Ok(ordering) => (ordering, Ok(())), + Err((ordering, e)) => (ordering, Err(e)), + }; + assert_eq!(ordering, core::cmp::Ordering::Equal, $($arg)+); + assert_eq!(actual_result, $expected_result, $($arg)*); }}; } From f8f4b896ba1dd2ec584f6f6af19b66e65aa6b897 Mon Sep 17 00:00:00 2001 From: "Shane F. Carr" Date: Tue, 9 Apr 2024 17:55:20 -0700 Subject: [PATCH 03/23] Add impl Writeable for [T] --- utils/writeable/src/impls.rs | 40 ++++++++++++++++++++++++++++++++++++ utils/writeable/src/lib.rs | 9 +++++++- 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/utils/writeable/src/impls.rs b/utils/writeable/src/impls.rs index ef075413e68..1f1c3e2548d 100644 --- a/utils/writeable/src/impls.rs +++ b/utils/writeable/src/impls.rs @@ -231,6 +231,46 @@ impl_write_smart_pointer!(alloc::boxed::Box); impl_write_smart_pointer!(alloc::rc::Rc); impl_write_smart_pointer!(alloc::sync::Arc); +/// Concatenates the elements of a slice. +/// +/// # Examples +/// +/// ``` +/// use writeable::Writeable; +/// use writeable::assert_writeable_eq; +/// +/// assert_writeable_eq!( +/// @skip display, +/// &["Hello", ", ", "world", "!"][..], +/// "Hello, world!" +/// ); +/// ``` +impl Writeable for [T] +where + T: Writeable, +{ + #[inline] + fn write_to(&self, sink: &mut W) -> fmt::Result { + self.iter().try_for_each(|t| t.write_to(sink)) + } + + #[inline] + fn write_to_parts(&self, sink: &mut W) -> fmt::Result { + self.iter().try_for_each(|t| t.write_to_parts(sink)) + } + + #[inline] + fn writeable_length_hint(&self) -> LengthHint { + self.iter().map(Writeable::writeable_length_hint).sum() + } + + // Use default write_to_string impl since it doesn't make sense to forward the inner impls. + // Note: We could optimize in the case of 1 element in the list. + + // Also use default write_cmp_bytes impl. Forwarding would require knowing how many bytes + // each writeable could consume, information we don't necesarily have. +} + #[test] fn test_string_impls() { fn check_writeable_slice(writeables: &[W]) { diff --git a/utils/writeable/src/lib.rs b/utils/writeable/src/lib.rs index 4a1af5c0ad7..abcd7c6657c 100644 --- a/utils/writeable/src/lib.rs +++ b/utils/writeable/src/lib.rs @@ -403,6 +403,14 @@ macro_rules! assert_writeable_eq { $crate::assert_writeable_eq!($actual_writeable, $expected_str, ""); }; ($actual_writeable:expr, $expected_str:expr, $($arg:tt)+) => {{ + let actual_writeable = &$actual_writeable; + $crate::assert_writeable_eq!(@skip display, $actual_writeable, $expected_str, $($arg)+); + assert_eq!(actual_writeable.to_string(), $expected_str); + }}; + (@skip display, $actual_writeable:expr, $expected_str:expr $(,)?) => { + $crate::assert_writeable_eq!(@skip display, $actual_writeable, $expected_str, ""); + }; + (@skip display, $actual_writeable:expr, $expected_str:expr, $($arg:tt)+) => {{ let actual_writeable = &$actual_writeable; let (actual_str, _) = $crate::_internal::writeable_to_parts_for_test(actual_writeable); assert_eq!(actual_str, $expected_str, $($arg)*); @@ -420,7 +428,6 @@ macro_rules! assert_writeable_eq { length_hint.0, actual_str.len(), format!($($arg)*), ); } - assert_eq!(actual_writeable.to_string(), $expected_str); let ordering = $crate::Writeable::write_cmp_bytes(actual_writeable, $expected_str.as_bytes()); assert_eq!(ordering, core::cmp::Ordering::Equal, $($arg)*); }}; From 67c280e6d72fb3c25652934492a4800ec6f785f7 Mon Sep 17 00:00:00 2001 From: "Shane F. Carr" Date: Tue, 9 Apr 2024 21:10:35 -0700 Subject: [PATCH 04/23] Checkpoint --- utils/writeable/src/helpers.rs | 27 ++++ utils/writeable/src/lib.rs | 41 +++--- utils/writeable/src/try_writeable.rs | 206 +++++++++++++++++++++++---- 3 files changed, 224 insertions(+), 50 deletions(-) diff --git a/utils/writeable/src/helpers.rs b/utils/writeable/src/helpers.rs index df1dac96442..e5e800543db 100644 --- a/utils/writeable/src/helpers.rs +++ b/utils/writeable/src/helpers.rs @@ -75,3 +75,30 @@ pub fn try_writeable_to_parts_for_test( let (actual_str, actual_parts) = writer.finish(); (actual_str, actual_parts, result.err()) } + +pub(crate) struct CoreWriteAsPartsWrite(pub W); + +impl fmt::Write for CoreWriteAsPartsWrite { + #[inline] + fn write_str(&mut self, s: &str) -> fmt::Result { + self.0.write_str(s) + } + + #[inline] + fn write_char(&mut self, c: char) -> fmt::Result { + self.0.write_char(c) + } +} + +impl PartsWrite for CoreWriteAsPartsWrite { + type SubPartsWrite = CoreWriteAsPartsWrite; + + #[inline] + fn with_part( + &mut self, + _part: Part, + mut f: impl FnMut(&mut Self::SubPartsWrite) -> fmt::Result, + ) -> fmt::Result { + f(self) + } +} diff --git a/utils/writeable/src/lib.rs b/utils/writeable/src/lib.rs index abcd7c6657c..b4b3fb7614c 100644 --- a/utils/writeable/src/lib.rs +++ b/utils/writeable/src/lib.rs @@ -177,6 +177,22 @@ pub struct Part { pub value: &'static str, } +impl Part { + /// A part that should annotate error segments in [`TryWriteable`] output. + /// + /// # Examples + /// + /// ``` + /// use writeable::TryWriteable; + /// + /// let try_writeable = + /// ``` + pub const ERROR: Part = Part { + category: "writeable", + value: "error", + }; +} + /// A sink that supports annotating parts of the string with `Part`s. pub trait PartsWrite: fmt::Write { type SubPartsWrite: PartsWrite + ?Sized; @@ -194,30 +210,7 @@ pub trait Writeable { /// The default implementation delegates to `write_to_parts`, and discards any /// `Part` annotations. fn write_to(&self, sink: &mut W) -> fmt::Result { - struct CoreWriteAsPartsWrite(W); - impl fmt::Write for CoreWriteAsPartsWrite { - fn write_str(&mut self, s: &str) -> fmt::Result { - self.0.write_str(s) - } - - fn write_char(&mut self, c: char) -> fmt::Result { - self.0.write_char(c) - } - } - - impl PartsWrite for CoreWriteAsPartsWrite { - type SubPartsWrite = CoreWriteAsPartsWrite; - - fn with_part( - &mut self, - _part: Part, - mut f: impl FnMut(&mut Self::SubPartsWrite) -> fmt::Result, - ) -> fmt::Result { - f(self) - } - } - - self.write_to_parts(&mut CoreWriteAsPartsWrite(sink)) + self.write_to_parts(&mut helpers::CoreWriteAsPartsWrite(sink)) } /// Write bytes and `Part` annotations to the given sink. Errors from the diff --git a/utils/writeable/src/try_writeable.rs b/utils/writeable/src/try_writeable.rs index 324dbe23bd0..cdf088b05db 100644 --- a/utils/writeable/src/try_writeable.rs +++ b/utils/writeable/src/try_writeable.rs @@ -5,26 +5,111 @@ use super::*; use core::cmp::Ordering; +/// A writeable object that can fail while writing. +/// +/// The default [`Writeable`] trait returns a [`fmt::Error`], which originates from the sink. +/// In contrast, this trait allows the _writeable itself_ to trigger an error. +/// +/// Implementations are expected to always make a _best attempt_ at writing to the sink +/// and should write replacement values in the error state. Therefore, the error can be +/// safely ignored to emulate a "lossy" mode. +/// +/// Any error substrings should be annotated with [`Part::ERROR`]. +/// +/// # Implementor Notes +/// +/// This trait requires that implementers make a _best attempt_ at writing to the sink, +/// _even in the error state_, such as with a placeholder or fallback string. +/// +/// In [`TryWriteable::try_write_to_parts()`], error substrings should be annotated with +/// [`Part::ERROR`]. Becuause of this, writing to parts is not default-implemented like +/// it is on [`Writeable`]. +/// +/// The trait is implemented on [`Result`] where `T` and `E` both implement [`Writeable`]; +/// In the `Ok` case, `T` is written, and in the `Err` case, `E` is written as a fallback value. +/// This impl, which writes [`Part::ERROR`], can be used as a basis for more advanced impls. +/// +/// # Examples +/// +/// Implementing on a custom type: +/// +/// ``` +/// use core::fmt; +/// use writeable::TryWriteable; +/// use writeable::PartsWrite; +/// use writeable::LengthHint; +/// +/// #[derive(Debug, PartialEq, Eq)] +/// enum MyWriteableError { +/// MissingName +/// } +/// +/// #[derive(Debug, PartialEq, Eq)] +/// struct MyWriteable { +/// pub name: Option<&'static str> +/// } +/// +/// impl TryWriteable for MyWriteable { +/// type Error = MyWriteableError; +/// +/// fn try_write_to_parts( +/// &self, +/// sink: &mut S, +/// ) -> Result, fmt::Error> { +/// sink.write_str("Hello, ")?; +/// // Use `impl TryWriteable for Result` to generate the error part: +/// let _ = self.name.ok_or("nobody").try_write_to_parts(sink)?; +/// sink.write_char('!')?; +/// if self.name.is_some() { +/// Ok(Ok(())) +/// } else { +/// Ok(Err(MyWriteableError::MissingName)) +/// } +/// } +/// +/// fn try_writeable_length_hint(&self) -> (LengthHint, Option) { +/// let (hint, e) = self.name.ok_or("nobody").try_writeable_length_hint(); +/// (hint + 8, e.map(|_| MyWriteableError::MissingName)) +/// } +/// } +/// +/// writeable::assert_try_writeable_eq!( +/// MyWriteable { +/// name: Some("Alice") +/// }, +/// "Hello, Alice!" +/// ); +/// +/// writeable::assert_try_writeable_eq!( +/// MyWriteable { +/// name: None +/// }, +/// "Hello, nobody!", +/// Err::<(), _>(MyWriteableError::MissingName), +/// ); +/// ``` pub trait TryWriteable { type Error; fn try_write_to( &self, sink: &mut W, - ) -> Result, fmt::Error>; + ) -> Result, fmt::Error> { + self.try_write_to_parts(&mut helpers::CoreWriteAsPartsWrite(sink)) + } fn try_write_to_parts( &self, sink: &mut S, ) -> Result, fmt::Error>; - fn try_writeable_length_hint(&self) -> Result; + fn try_writeable_length_hint(&self) -> (LengthHint, Option) { + // TODO: Discuss this function, its signature, and its behavior + (LengthHint::undefined(), None) + } fn try_write_to_string(&self) -> Result, Self::Error> { - let (hint, has_error) = match self.try_writeable_length_hint() { - Ok(hint) => (hint, false), - Err((hint, _)) => (hint, true), - }; + let (hint, hint_error) = self.try_writeable_length_hint(); if hint.is_zero() { return Ok(Cow::Borrowed("")); } @@ -37,14 +122,14 @@ pub trait TryWriteable { } }; debug_assert_eq!( - has_error, + hint_error.is_some(), result.is_err(), "try_writeable_length_hint and try_write_to_string should have same error state" ); result.map(|_| Cow::Owned(output)) } - fn try_write_cmp_bytes(&self, other: &[u8]) -> Result { + fn try_write_cmp_bytes(&self, other: &[u8]) -> (Ordering, Option) { let mut wc = cmp::WriteComparator::new(other); let result = match self.try_write_to(&mut wc) { Ok(result) => result, @@ -54,32 +139,90 @@ pub trait TryWriteable { } }; let ordering = wc.finish().reverse(); - result.map(|_| ordering).map_err(|e| (ordering, e)) + (ordering, result.err()) + } +} + +/// +impl TryWriteable for Result +where + T: Writeable, + E: Writeable + Clone, +{ + type Error = E; + + #[inline] + fn try_write_to( + &self, + sink: &mut W, + ) -> Result, fmt::Error> { + match self { + Ok(t) => t.write_to(sink).map(Ok), + Err(e) => e.write_to(sink).map(|_| Err(e.clone())), + } + } + + #[inline] + fn try_write_to_parts( + &self, + sink: &mut S, + ) -> Result, fmt::Error> { + match self { + Ok(t) => t.write_to_parts(sink).map(Ok), + Err(e) => sink + .with_part(Part::ERROR, |sink| e.write_to_parts(sink)) + .map(|_| Err(e.clone())), + } + } + + #[inline] + fn try_writeable_length_hint(&self) -> (LengthHint, Option) { + match self { + Ok(t) => (t.writeable_length_hint(), None), + Err(e) => (e.writeable_length_hint(), Some(e.clone())), + } + } + + #[inline] + fn try_write_to_string(&self) -> Result, Self::Error> { + match self { + Ok(t) => Ok(t.write_to_string()), + Err(e) => Err(e.clone()), + } + } + + #[inline] + fn try_write_cmp_bytes(&self, other: &[u8]) -> (Ordering, Option) { + match self { + Ok(t) => (t.write_cmp_bytes(other), None), + Err(e) => (e.write_cmp_bytes(other), Some(e.clone())), + } } } #[macro_export] macro_rules! assert_try_writeable_eq { ($actual_writeable:expr, $expected_str:expr $(,)?) => { - $crate::assert_writeable_eq!($actual_writeable, $expected_str, Ok(()), ""); + $crate::assert_try_writeable_eq!($actual_writeable, $expected_str, Ok(())); + }; + ($actual_writeable:expr, $expected_str:expr, $expected_result:expr $(,)?) => { + $crate::assert_try_writeable_eq!($actual_writeable, $expected_str, $expected_result, ""); }; ($actual_writeable:expr, $expected_str:expr, $expected_result:expr, $($arg:tt)+) => {{ + use $crate::TryWriteable; let actual_writeable = &$actual_writeable; - let (actual_str, _, actual_result) = $crate::try_writeable_to_parts_for_test(actual_writeable); + let (actual_str, _, actual_error) = $crate::_internal::try_writeable_to_parts_for_test(actual_writeable); assert_eq!(actual_str, $expected_str, $($arg)*); - assert_eq!(actual_result, $expected_result, $($arg)*); - let actual_result = match $crate::TryWriteable::try_write_to_string(actual_writeable) { - Ok(s) => { + assert_eq!(actual_error, $expected_result.err(), $($arg)*); + let actual_result = match actual_writeable.try_write_to_string() { + Ok(actual_cow_str) => { assert_eq!(actual_cow_str, $expected_str, $($arg)+); Ok(()) } Err(e) => Err(e) }; assert_eq!(actual_result, $expected_result, $($arg)*); - let (length_hint, actual_result) = match $crate::TryWriteable::writeable_length_hint(actual_writeable) { - Ok(length_hint) => (length_hint, Ok(())), - Err((length_hint, e)) => (length_hint, Err(e)), - }; + let (length_hint, hint_error) = actual_writeable.try_writeable_length_hint(); assert!( length_hint.0 <= actual_str.len(), "hint lower bound {} larger than actual length {}: {}", @@ -92,13 +235,24 @@ macro_rules! assert_try_writeable_eq { length_hint.0, actual_str.len(), format!($($arg)*), ); } - assert_eq!(actual_result, $expected_result, $($arg)*); - assert_eq!(actual_writeable.to_string(), $expected_str); - let (ordering, actual_result) = match $crate::Writeable::try_write_cmp_bytes(actual_writeable, $expected_str.as_bytes()) { - Ok(ordering) => (ordering, Ok(())), - Err((ordering, e)) => (ordering, Err(e)), - }; - assert_eq!(ordering, core::cmp::Ordering::Equal, $($arg)+); - assert_eq!(actual_result, $expected_result, $($arg)*); + assert_eq!(hint_error, $expected_result.err(), $($arg)*); + let (ordering, ordering_error) = actual_writeable.try_write_cmp_bytes($expected_str.as_bytes()); + assert_eq!(ordering, core::cmp::Ordering::Equal, $($arg)*); + assert_eq!(ordering_error, $expected_result.err(), $($arg)*); }}; } + +#[test] +fn test_result_try_writeable() { + let mut result: Result<&str, usize> = Ok("success"); + assert_try_writeable_eq!( + result, + "success" + ); + result = Err(44); + assert_try_writeable_eq!( + result, + "44", + Err::<(), _>(44) + ); +} From fb7c2bd1235ddece70c64f4c515a2ca79f919a58 Mon Sep 17 00:00:00 2001 From: "Shane F. Carr" Date: Tue, 9 Apr 2024 22:42:55 -0700 Subject: [PATCH 05/23] Docs and things --- utils/writeable/src/lib.rs | 30 ++-- utils/writeable/src/try_writeable.rs | 231 ++++++++++++++++++++++++--- 2 files changed, 231 insertions(+), 30 deletions(-) diff --git a/utils/writeable/src/lib.rs b/utils/writeable/src/lib.rs index b4b3fb7614c..391bf2511d8 100644 --- a/utils/writeable/src/lib.rs +++ b/utils/writeable/src/lib.rs @@ -180,13 +180,7 @@ pub struct Part { impl Part { /// A part that should annotate error segments in [`TryWriteable`] output. /// - /// # Examples - /// - /// ``` - /// use writeable::TryWriteable; - /// - /// let try_writeable = - /// ``` + /// For an example, see [`TryWriteable`]. pub const ERROR: Part = Part { category: "writeable", value: "error", @@ -341,12 +335,22 @@ macro_rules! impl_display_with_writeable { }; } -/// Testing macros for types implementing Writeable. The first argument should be a -/// `Writeable`, the second argument a string, and the third argument (*_parts_eq only) -/// a list of parts (`[(usize, usize, Part)]`). +/// Testing macros for types implementing [`Writeable`]. +/// +/// Arguments, in order: +/// +/// 1. The [`Writeable`] under test +/// 2. The expected string value +/// 3. [`*_parts_eq`] only: a list of parts (`[(start, end, Part)]`) +/// +/// Any remaining arguments get passed to `format!` /// -/// The macros tests for equality of string content, parts (*_parts_eq only), and -/// verify the size hint. +/// The macros tests the following: +/// +/// - Equality of string content +/// - Equality of parts ([`*_parts_eq`] only) +/// - Validity of size hint +/// - Basic validity of `cmp_bytes` /// /// # Examples /// @@ -390,6 +394,8 @@ macro_rules! impl_display_with_writeable { /// "Hello World" /// ); /// ``` +/// +/// [`*_parts_eq`]: assert_writeable_parts_eq #[macro_export] macro_rules! assert_writeable_eq { ($actual_writeable:expr, $expected_str:expr $(,)?) => { diff --git a/utils/writeable/src/try_writeable.rs b/utils/writeable/src/try_writeable.rs index cdf088b05db..4de4d142143 100644 --- a/utils/writeable/src/try_writeable.rs +++ b/utils/writeable/src/try_writeable.rs @@ -11,12 +11,12 @@ use core::cmp::Ordering; /// In contrast, this trait allows the _writeable itself_ to trigger an error. /// /// Implementations are expected to always make a _best attempt_ at writing to the sink -/// and should write replacement values in the error state. Therefore, the error can be -/// safely ignored to emulate a "lossy" mode. +/// and should write replacement values in the error state. Therefore, [`TryWriteable::Error`] +/// can be safely ignored to emulate a "lossy" mode. /// /// Any error substrings should be annotated with [`Part::ERROR`]. /// -/// # Implementor Notes +/// # Implementer Notes /// /// This trait requires that implementers make a _best attempt_ at writing to the sink, /// _even in the error state_, such as with a placeholder or fallback string. @@ -24,6 +24,9 @@ use core::cmp::Ordering; /// In [`TryWriteable::try_write_to_parts()`], error substrings should be annotated with /// [`Part::ERROR`]. Becuause of this, writing to parts is not default-implemented like /// it is on [`Writeable`]. +/// +/// Furthermore, [`TryWriteable::try_writeable_length_hint()`] is not default-implemented because +/// it has an invariant that the error state should be correctly propagated. /// /// The trait is implemented on [`Result`] where `T` and `E` both implement [`Writeable`]; /// In the `Ok` case, `T` is written, and in the `Err` case, `E` is written as a fallback value. @@ -45,11 +48,11 @@ use core::cmp::Ordering; /// } /// /// #[derive(Debug, PartialEq, Eq)] -/// struct MyWriteable { +/// struct HelloWorldWriteable { /// pub name: Option<&'static str> /// } /// -/// impl TryWriteable for MyWriteable { +/// impl TryWriteable for HelloWorldWriteable { /// type Error = MyWriteableError; /// /// fn try_write_to_parts( @@ -60,6 +63,9 @@ use core::cmp::Ordering; /// // Use `impl TryWriteable for Result` to generate the error part: /// let _ = self.name.ok_or("nobody").try_write_to_parts(sink)?; /// sink.write_char('!')?; +/// // Return a doubly-wrapped Result. +/// // The outer Result is for fmt::Error, handled by the `?`s above. +/// // The inner Result is for our own Self::Error. /// if self.name.is_some() { /// Ok(Ok(())) /// } else { @@ -73,24 +79,79 @@ use core::cmp::Ordering; /// } /// } /// +/// // Success case: /// writeable::assert_try_writeable_eq!( -/// MyWriteable { +/// HelloWorldWriteable { /// name: Some("Alice") /// }, /// "Hello, Alice!" /// ); /// -/// writeable::assert_try_writeable_eq!( -/// MyWriteable { +/// // Failure case, including the ERROR part: +/// writeable::assert_try_writeable_parts_eq!( +/// HelloWorldWriteable { /// name: None /// }, /// "Hello, nobody!", -/// Err::<(), _>(MyWriteableError::MissingName), +/// Err(MyWriteableError::MissingName), +/// [(7, 13, writeable::Part::ERROR)] /// ); /// ``` pub trait TryWriteable { type Error; + /// Writes the content of this writeable to a sink. + /// + /// If the sink hits an error, writing is abruptly ended and + /// `Err(`[`fmt::Error`]`)` is returned. + /// + /// If the writeable hits an error, writing is continued with a replacement value and then + /// `Ok(Err(`[`TryWriteable::Error`]`))` is returned. + /// + /// # Lossy Mode + /// + /// The [`fmt::Error`] should always be handled, but the [`TryWriteable::Error`] can be + /// ignored if a fallback string is desired instead of an error. + /// + /// To handle outer error but not the inner error, write: + /// + /// ``` + /// # use writeable::TryWriteable; + /// # let my_writeable: Result<&str, &str> = Ok(""); + /// # let mut sink = String::new(); + /// let _ = my_writeable.try_write_to(&mut sink)?; + /// # Ok::<(), core::fmt::Error>(()) + /// ``` + /// + /// # Examples + /// + /// The following examples use `Result<&str, usize>`, which implements [`TryWriteable`]. + /// + /// Success case: + /// + /// ``` + /// use writeable::TryWriteable; + /// + /// let w: Result<&str, usize> = Ok("success"); + /// let mut sink = String::new(); + /// let result = w.try_write_to(&mut sink); + /// + /// assert_eq!(result, Ok(Ok(()))); + /// assert_eq!(sink, "success"); + /// ``` + /// + /// Failure case: + /// + /// ``` + /// use writeable::TryWriteable; + /// + /// let w: Result<&str, usize> = Err(44); + /// let mut sink = String::new(); + /// let result = w.try_write_to(&mut sink); + /// + /// assert_eq!(result, Ok(Err(44))); + /// assert_eq!(sink, "44"); + /// ``` fn try_write_to( &self, sink: &mut W, @@ -98,16 +159,31 @@ pub trait TryWriteable { self.try_write_to_parts(&mut helpers::CoreWriteAsPartsWrite(sink)) } + /// Writes the content of this writeable to a sink with parts (annotations). + /// + /// For more information, see: + /// + /// - [`TryWriteable::try_write_to()`] for the general behavior. + /// - [`TryWriteable`] for an example with parts. + /// - [`Part`] for more about parts. fn try_write_to_parts( &self, sink: &mut S, ) -> Result, fmt::Error>; + /// Returns a hint for the number of UTF-8 bytes that will be written to the sink. + /// + /// This function returns `Some(error)` _if and only if_ [`TryWriteable::try_write_to()`] + /// returns `Err`. fn try_writeable_length_hint(&self) -> (LengthHint, Option) { // TODO: Discuss this function, its signature, and its behavior (LengthHint::undefined(), None) } + /// Writes the content of this writeable to a string. + /// + /// This function does not return a string in the failure case. If you need a replacement + /// string ("lossy mode"), use [`TryWriteable::try_write_to()`] instead. fn try_write_to_string(&self) -> Result, Self::Error> { let (hint, hint_error) = self.try_writeable_length_hint(); if hint.is_zero() { @@ -129,6 +205,83 @@ pub trait TryWriteable { result.map(|_| Cow::Owned(output)) } + /// Compares the content of this writeable to a byte slice. + /// + /// This function returns `Some(error)` _if and only if_ [`TryWriteable::try_write_to()`] + /// returns `Err`. + /// + /// For more information, see [`Writeable::write_cmp_bytes()`]. + /// + /// # Examples + /// + /// ``` + /// use core::cmp::Ordering; + /// use core::fmt; + /// use writeable::TryWriteable; + /// # use writeable::PartsWrite; + /// # use writeable::LengthHint; + /// + /// #[derive(Debug, PartialEq, Eq)] + /// enum MyWriteableError { + /// MissingName + /// } + /// + /// #[derive(Debug, PartialEq, Eq)] + /// struct HelloWorldWriteable { + /// pub name: Option<&'static str> + /// } + /// + /// impl TryWriteable for HelloWorldWriteable { + /// type Error = MyWriteableError; + /// // see impl in TryWriteable docs + /// # fn try_write_to_parts( + /// # &self, + /// # sink: &mut S, + /// # ) -> Result, fmt::Error> { + /// # sink.write_str("Hello, ")?; + /// # // Use `impl TryWriteable for Result` to generate the error part: + /// # let _ = self.name.ok_or("nobody").try_write_to_parts(sink)?; + /// # sink.write_char('!')?; + /// # // Return a doubly-wrapped Result. + /// # // The outer Result is for fmt::Error, handled by the `?`s above. + /// # // The inner Result is for our own Self::Error. + /// # if self.name.is_some() { + /// # Ok(Ok(())) + /// # } else { + /// # Ok(Err(MyWriteableError::MissingName)) + /// # } + /// # } + /// # fn try_writeable_length_hint(&self) -> (LengthHint, Option) { + /// # let (hint, e) = self.name.ok_or("nobody").try_writeable_length_hint(); + /// # (hint + 8, e.map(|_| MyWriteableError::MissingName)) + /// # } + /// } + /// + /// // Success case: + /// let writeable = HelloWorldWriteable { name: Some("Alice") }; + /// let writeable_str = writeable.try_write_to_string().expect("name is Some"); + /// + /// assert_eq!((Ordering::Equal, None), writeable.try_write_cmp_bytes(b"Hello, Alice!")); + /// + /// assert_eq!((Ordering::Greater, None), writeable.try_write_cmp_bytes(b"Alice!")); + /// assert_eq!(Ordering::Greater, (*writeable_str).cmp("Alice!")); + /// + /// assert_eq!((Ordering::Less, None), writeable.try_write_cmp_bytes(b"Hello, Bob!")); + /// assert_eq!(Ordering::Less, (*writeable_str).cmp("Hello, Bob!")); + /// + /// // Failure case: + /// let writeable = HelloWorldWriteable { name: None }; + /// let mut writeable_str = String::new(); + /// let _ = writeable.try_write_to(&mut writeable_str).expect("write to String is infallible"); + /// + /// assert_eq!((Ordering::Equal, Some(MyWriteableError::MissingName)), writeable.try_write_cmp_bytes(b"Hello, nobody!")); + /// + /// assert_eq!((Ordering::Greater, Some(MyWriteableError::MissingName)), writeable.try_write_cmp_bytes(b"Hello, alice!")); + /// assert_eq!(Ordering::Greater, (*writeable_str).cmp("Hello, alice!")); + /// + /// assert_eq!((Ordering::Less, Some(MyWriteableError::MissingName)), writeable.try_write_cmp_bytes(b"Hello, zero!")); + /// assert_eq!(Ordering::Less, (*writeable_str).cmp("Hello, zero!")); + /// ``` fn try_write_cmp_bytes(&self, other: &[u8]) -> (Ordering, Option) { let mut wc = cmp::WriteComparator::new(other); let result = match self.try_write_to(&mut wc) { @@ -143,7 +296,6 @@ pub trait TryWriteable { } } -/// impl TryWriteable for Result where T: Writeable, @@ -200,20 +352,41 @@ where } } +/// Testing macros for types implementing [`TryWriteable`]. +/// +/// Arguments, in order: +/// +/// 1. The [`TryWriteable`] under test +/// 2. The expected string value +/// 3. The expected result value, or `Ok(())` if omitted +/// 3. [`*_parts_eq`] only: a list of parts (`[(start, end, Part)]`) +/// +/// Any remaining arguments get passed to `format!` +/// +/// The macros tests the following: +/// +/// - Equality of string content +/// - Equality of parts ([`*_parts_eq`] only) +/// - Validity of size hint +/// - Basic validity of `cmp_bytes` +/// +/// For a usage example, see [`TryWriteable`]. +/// +/// [`*_parts_eq`]: assert_try_writeable_parts_eq #[macro_export] macro_rules! assert_try_writeable_eq { ($actual_writeable:expr, $expected_str:expr $(,)?) => { - $crate::assert_try_writeable_eq!($actual_writeable, $expected_str, Ok(())); + $crate::assert_try_writeable_eq!($actual_writeable, $expected_str, Ok(())) }; ($actual_writeable:expr, $expected_str:expr, $expected_result:expr $(,)?) => { - $crate::assert_try_writeable_eq!($actual_writeable, $expected_str, $expected_result, ""); + $crate::assert_try_writeable_eq!($actual_writeable, $expected_str, $expected_result, "") }; ($actual_writeable:expr, $expected_str:expr, $expected_result:expr, $($arg:tt)+) => {{ use $crate::TryWriteable; let actual_writeable = &$actual_writeable; - let (actual_str, _, actual_error) = $crate::_internal::try_writeable_to_parts_for_test(actual_writeable); + let (actual_str, actual_parts, actual_error) = $crate::_internal::try_writeable_to_parts_for_test(actual_writeable); assert_eq!(actual_str, $expected_str, $($arg)*); - assert_eq!(actual_error, $expected_result.err(), $($arg)*); + assert_eq!(actual_error, Result::<(), _>::from($expected_result).err(), $($arg)*); let actual_result = match actual_writeable.try_write_to_string() { Ok(actual_cow_str) => { assert_eq!(actual_cow_str, $expected_str, $($arg)+); @@ -221,7 +394,7 @@ macro_rules! assert_try_writeable_eq { } Err(e) => Err(e) }; - assert_eq!(actual_result, $expected_result, $($arg)*); + assert_eq!(actual_result, Result::<(), _>::from($expected_result), $($arg)*); let (length_hint, hint_error) = actual_writeable.try_writeable_length_hint(); assert!( length_hint.0 <= actual_str.len(), @@ -235,10 +408,26 @@ macro_rules! assert_try_writeable_eq { length_hint.0, actual_str.len(), format!($($arg)*), ); } - assert_eq!(hint_error, $expected_result.err(), $($arg)*); + assert_eq!(hint_error, Result::<(), _>::from($expected_result).err(), $($arg)*); let (ordering, ordering_error) = actual_writeable.try_write_cmp_bytes($expected_str.as_bytes()); assert_eq!(ordering, core::cmp::Ordering::Equal, $($arg)*); - assert_eq!(ordering_error, $expected_result.err(), $($arg)*); + assert_eq!(ordering_error, Result::<(), _>::from($expected_result).err(), $($arg)*); + actual_parts // return for assert_try_writeable_parts_eq + }}; +} + +/// See [`assert_try_writeable_eq`]. +#[macro_export] +macro_rules! assert_try_writeable_parts_eq { + ($actual_writeable:expr, $expected_str:expr, $expected_parts:expr $(,)?) => { + $crate::assert_try_writeable_parts_eq!($actual_writeable, $expected_str, Ok(()), $expected_parts) + }; + ($actual_writeable:expr, $expected_str:expr, $expected_result:expr, $expected_parts:expr $(,)?) => { + $crate::assert_try_writeable_parts_eq!($actual_writeable, $expected_str, $expected_result, $expected_parts, "") + }; + ($actual_writeable:expr, $expected_str:expr, $expected_result:expr, $expected_parts:expr, $($arg:tt)+) => {{ + let actual_parts = $crate::assert_try_writeable_eq!($actual_writeable, $expected_str, $expected_result, $($arg)*); + assert_eq!(actual_parts, $expected_parts, $($arg)+); }}; } @@ -253,6 +442,12 @@ fn test_result_try_writeable() { assert_try_writeable_eq!( result, "44", - Err::<(), _>(44) + Err(44) ); + assert_try_writeable_parts_eq!( + result, + "44", + Err(44), + [(0, 2, Part::ERROR)] + ) } From 8e63dfaaaeca1e496347f68614fdeb93f87d2ec1 Mon Sep 17 00:00:00 2001 From: "Shane F. Carr" Date: Tue, 9 Apr 2024 22:44:09 -0700 Subject: [PATCH 06/23] fmt --- utils/writeable/src/lib.rs | 10 +-- utils/writeable/src/try_writeable.rs | 105 ++++++++++++--------------- 2 files changed, 52 insertions(+), 63 deletions(-) diff --git a/utils/writeable/src/lib.rs b/utils/writeable/src/lib.rs index 391bf2511d8..b75c8f6842c 100644 --- a/utils/writeable/src/lib.rs +++ b/utils/writeable/src/lib.rs @@ -336,17 +336,17 @@ macro_rules! impl_display_with_writeable { } /// Testing macros for types implementing [`Writeable`]. -/// +/// /// Arguments, in order: -/// +/// /// 1. The [`Writeable`] under test /// 2. The expected string value /// 3. [`*_parts_eq`] only: a list of parts (`[(start, end, Part)]`) -/// +/// /// Any remaining arguments get passed to `format!` /// /// The macros tests the following: -/// +/// /// - Equality of string content /// - Equality of parts ([`*_parts_eq`] only) /// - Validity of size hint @@ -394,7 +394,7 @@ macro_rules! impl_display_with_writeable { /// "Hello World" /// ); /// ``` -/// +/// /// [`*_parts_eq`]: assert_writeable_parts_eq #[macro_export] macro_rules! assert_writeable_eq { diff --git a/utils/writeable/src/try_writeable.rs b/utils/writeable/src/try_writeable.rs index 4de4d142143..368f57abac7 100644 --- a/utils/writeable/src/try_writeable.rs +++ b/utils/writeable/src/try_writeable.rs @@ -9,22 +9,22 @@ use core::cmp::Ordering; /// /// The default [`Writeable`] trait returns a [`fmt::Error`], which originates from the sink. /// In contrast, this trait allows the _writeable itself_ to trigger an error. -/// +/// /// Implementations are expected to always make a _best attempt_ at writing to the sink /// and should write replacement values in the error state. Therefore, [`TryWriteable::Error`] /// can be safely ignored to emulate a "lossy" mode. -/// +/// /// Any error substrings should be annotated with [`Part::ERROR`]. -/// +/// /// # Implementer Notes /// /// This trait requires that implementers make a _best attempt_ at writing to the sink, /// _even in the error state_, such as with a placeholder or fallback string. -/// +/// /// In [`TryWriteable::try_write_to_parts()`], error substrings should be annotated with /// [`Part::ERROR`]. Becuause of this, writing to parts is not default-implemented like /// it is on [`Writeable`]. -/// +/// /// Furthermore, [`TryWriteable::try_writeable_length_hint()`] is not default-implemented because /// it has an invariant that the error state should be correctly propagated. /// @@ -38,18 +38,18 @@ use core::cmp::Ordering; /// /// ``` /// use core::fmt; -/// use writeable::TryWriteable; -/// use writeable::PartsWrite; /// use writeable::LengthHint; +/// use writeable::PartsWrite; +/// use writeable::TryWriteable; /// /// #[derive(Debug, PartialEq, Eq)] /// enum MyWriteableError { -/// MissingName +/// MissingName, /// } /// /// #[derive(Debug, PartialEq, Eq)] /// struct HelloWorldWriteable { -/// pub name: Option<&'static str> +/// pub name: Option<&'static str>, /// } /// /// impl TryWriteable for HelloWorldWriteable { @@ -73,8 +73,11 @@ use core::cmp::Ordering; /// } /// } /// -/// fn try_writeable_length_hint(&self) -> (LengthHint, Option) { -/// let (hint, e) = self.name.ok_or("nobody").try_writeable_length_hint(); +/// fn try_writeable_length_hint( +/// &self, +/// ) -> (LengthHint, Option) { +/// let (hint, e) = +/// self.name.ok_or("nobody").try_writeable_length_hint(); /// (hint + 8, e.map(|_| MyWriteableError::MissingName)) /// } /// } @@ -89,9 +92,7 @@ use core::cmp::Ordering; /// /// // Failure case, including the ERROR part: /// writeable::assert_try_writeable_parts_eq!( -/// HelloWorldWriteable { -/// name: None -/// }, +/// HelloWorldWriteable { name: None }, /// "Hello, nobody!", /// Err(MyWriteableError::MissingName), /// [(7, 13, writeable::Part::ERROR)] @@ -101,20 +102,20 @@ pub trait TryWriteable { type Error; /// Writes the content of this writeable to a sink. - /// + /// /// If the sink hits an error, writing is abruptly ended and /// `Err(`[`fmt::Error`]`)` is returned. - /// + /// /// If the writeable hits an error, writing is continued with a replacement value and then /// `Ok(Err(`[`TryWriteable::Error`]`))` is returned. - /// + /// /// # Lossy Mode - /// + /// /// The [`fmt::Error`] should always be handled, but the [`TryWriteable::Error`] can be /// ignored if a fallback string is desired instead of an error. - /// + /// /// To handle outer error but not the inner error, write: - /// + /// /// ``` /// # use writeable::TryWriteable; /// # let my_writeable: Result<&str, &str> = Ok(""); @@ -122,33 +123,33 @@ pub trait TryWriteable { /// let _ = my_writeable.try_write_to(&mut sink)?; /// # Ok::<(), core::fmt::Error>(()) /// ``` - /// + /// /// # Examples - /// + /// /// The following examples use `Result<&str, usize>`, which implements [`TryWriteable`]. - /// + /// /// Success case: - /// + /// /// ``` /// use writeable::TryWriteable; - /// + /// /// let w: Result<&str, usize> = Ok("success"); /// let mut sink = String::new(); /// let result = w.try_write_to(&mut sink); - /// + /// /// assert_eq!(result, Ok(Ok(()))); /// assert_eq!(sink, "success"); /// ``` - /// + /// /// Failure case: - /// + /// /// ``` /// use writeable::TryWriteable; - /// + /// /// let w: Result<&str, usize> = Err(44); /// let mut sink = String::new(); /// let result = w.try_write_to(&mut sink); - /// + /// /// assert_eq!(result, Ok(Err(44))); /// assert_eq!(sink, "44"); /// ``` @@ -160,9 +161,9 @@ pub trait TryWriteable { } /// Writes the content of this writeable to a sink with parts (annotations). - /// + /// /// For more information, see: - /// + /// /// - [`TryWriteable::try_write_to()`] for the general behavior. /// - [`TryWriteable`] for an example with parts. /// - [`Part`] for more about parts. @@ -172,7 +173,7 @@ pub trait TryWriteable { ) -> Result, fmt::Error>; /// Returns a hint for the number of UTF-8 bytes that will be written to the sink. - /// + /// /// This function returns `Some(error)` _if and only if_ [`TryWriteable::try_write_to()`] /// returns `Err`. fn try_writeable_length_hint(&self) -> (LengthHint, Option) { @@ -181,7 +182,7 @@ pub trait TryWriteable { } /// Writes the content of this writeable to a string. - /// + /// /// This function does not return a string in the failure case. If you need a replacement /// string ("lossy mode"), use [`TryWriteable::try_write_to()`] instead. fn try_write_to_string(&self) -> Result, Self::Error> { @@ -206,10 +207,10 @@ pub trait TryWriteable { } /// Compares the content of this writeable to a byte slice. - /// + /// /// This function returns `Some(error)` _if and only if_ [`TryWriteable::try_write_to()`] /// returns `Err`. - /// + /// /// For more information, see [`Writeable::write_cmp_bytes()`]. /// /// # Examples @@ -268,7 +269,7 @@ pub trait TryWriteable { /// /// assert_eq!((Ordering::Less, None), writeable.try_write_cmp_bytes(b"Hello, Bob!")); /// assert_eq!(Ordering::Less, (*writeable_str).cmp("Hello, Bob!")); - /// + /// /// // Failure case: /// let writeable = HelloWorldWriteable { name: None }; /// let mut writeable_str = String::new(); @@ -353,25 +354,25 @@ where } /// Testing macros for types implementing [`TryWriteable`]. -/// +/// /// Arguments, in order: -/// +/// /// 1. The [`TryWriteable`] under test /// 2. The expected string value /// 3. The expected result value, or `Ok(())` if omitted /// 3. [`*_parts_eq`] only: a list of parts (`[(start, end, Part)]`) -/// +/// /// Any remaining arguments get passed to `format!` /// /// The macros tests the following: -/// +/// /// - Equality of string content /// - Equality of parts ([`*_parts_eq`] only) /// - Validity of size hint /// - Basic validity of `cmp_bytes` -/// +/// /// For a usage example, see [`TryWriteable`]. -/// +/// /// [`*_parts_eq`]: assert_try_writeable_parts_eq #[macro_export] macro_rules! assert_try_writeable_eq { @@ -434,20 +435,8 @@ macro_rules! assert_try_writeable_parts_eq { #[test] fn test_result_try_writeable() { let mut result: Result<&str, usize> = Ok("success"); - assert_try_writeable_eq!( - result, - "success" - ); + assert_try_writeable_eq!(result, "success"); result = Err(44); - assert_try_writeable_eq!( - result, - "44", - Err(44) - ); - assert_try_writeable_parts_eq!( - result, - "44", - Err(44), - [(0, 2, Part::ERROR)] - ) + assert_try_writeable_eq!(result, "44", Err(44)); + assert_try_writeable_parts_eq!(result, "44", Err(44), [(0, 2, Part::ERROR)]) } From 4eddcb8b2f956abe8320c290b92638cf364e9ef6 Mon Sep 17 00:00:00 2001 From: "Shane F. Carr" Date: Tue, 9 Apr 2024 22:44:19 -0700 Subject: [PATCH 07/23] Revert "Add impl Writeable for [T]" This reverts commit f8f4b896ba1dd2ec584f6f6af19b66e65aa6b897. --- utils/writeable/src/impls.rs | 40 ------------------------------------ utils/writeable/src/lib.rs | 9 +------- 2 files changed, 1 insertion(+), 48 deletions(-) diff --git a/utils/writeable/src/impls.rs b/utils/writeable/src/impls.rs index 1f1c3e2548d..ef075413e68 100644 --- a/utils/writeable/src/impls.rs +++ b/utils/writeable/src/impls.rs @@ -231,46 +231,6 @@ impl_write_smart_pointer!(alloc::boxed::Box); impl_write_smart_pointer!(alloc::rc::Rc); impl_write_smart_pointer!(alloc::sync::Arc); -/// Concatenates the elements of a slice. -/// -/// # Examples -/// -/// ``` -/// use writeable::Writeable; -/// use writeable::assert_writeable_eq; -/// -/// assert_writeable_eq!( -/// @skip display, -/// &["Hello", ", ", "world", "!"][..], -/// "Hello, world!" -/// ); -/// ``` -impl Writeable for [T] -where - T: Writeable, -{ - #[inline] - fn write_to(&self, sink: &mut W) -> fmt::Result { - self.iter().try_for_each(|t| t.write_to(sink)) - } - - #[inline] - fn write_to_parts(&self, sink: &mut W) -> fmt::Result { - self.iter().try_for_each(|t| t.write_to_parts(sink)) - } - - #[inline] - fn writeable_length_hint(&self) -> LengthHint { - self.iter().map(Writeable::writeable_length_hint).sum() - } - - // Use default write_to_string impl since it doesn't make sense to forward the inner impls. - // Note: We could optimize in the case of 1 element in the list. - - // Also use default write_cmp_bytes impl. Forwarding would require knowing how many bytes - // each writeable could consume, information we don't necesarily have. -} - #[test] fn test_string_impls() { fn check_writeable_slice(writeables: &[W]) { diff --git a/utils/writeable/src/lib.rs b/utils/writeable/src/lib.rs index b75c8f6842c..f00d5d7df87 100644 --- a/utils/writeable/src/lib.rs +++ b/utils/writeable/src/lib.rs @@ -402,14 +402,6 @@ macro_rules! assert_writeable_eq { $crate::assert_writeable_eq!($actual_writeable, $expected_str, ""); }; ($actual_writeable:expr, $expected_str:expr, $($arg:tt)+) => {{ - let actual_writeable = &$actual_writeable; - $crate::assert_writeable_eq!(@skip display, $actual_writeable, $expected_str, $($arg)+); - assert_eq!(actual_writeable.to_string(), $expected_str); - }}; - (@skip display, $actual_writeable:expr, $expected_str:expr $(,)?) => { - $crate::assert_writeable_eq!(@skip display, $actual_writeable, $expected_str, ""); - }; - (@skip display, $actual_writeable:expr, $expected_str:expr, $($arg:tt)+) => {{ let actual_writeable = &$actual_writeable; let (actual_str, _) = $crate::_internal::writeable_to_parts_for_test(actual_writeable); assert_eq!(actual_str, $expected_str, $($arg)*); @@ -427,6 +419,7 @@ macro_rules! assert_writeable_eq { length_hint.0, actual_str.len(), format!($($arg)*), ); } + assert_eq!(actual_writeable.to_string(), $expected_str); let ordering = $crate::Writeable::write_cmp_bytes(actual_writeable, $expected_str.as_bytes()); assert_eq!(ordering, core::cmp::Ordering::Equal, $($arg)*); }}; From 7f1d730496cfc1b69a04f0d4eeff8f150547092d Mon Sep 17 00:00:00 2001 From: "Shane F. Carr" Date: Tue, 9 Apr 2024 22:45:58 -0700 Subject: [PATCH 08/23] clippy --- utils/writeable/src/helpers.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/utils/writeable/src/helpers.rs b/utils/writeable/src/helpers.rs index e5e800543db..06ac5f5c0c9 100644 --- a/utils/writeable/src/helpers.rs +++ b/utils/writeable/src/helpers.rs @@ -54,6 +54,7 @@ pub fn writeable_to_parts_for_test( string: alloc::string::String::new(), parts: Vec::new(), }; + #[allow(clippy::expect_used)] // for test code writeable .write_to_parts(&mut writer) .expect("String writer infallible"); @@ -69,6 +70,7 @@ pub fn try_writeable_to_parts_for_test( string: alloc::string::String::new(), parts: Vec::new(), }; + #[allow(clippy::expect_used)] // for test code let result = writeable .try_write_to_parts(&mut writer) .expect("String writer infallible"); From ef83866e1fb09ab4be586bea74fa95b3b896596f Mon Sep 17 00:00:00 2001 From: "Shane F. Carr" Date: Tue, 9 Apr 2024 22:52:26 -0700 Subject: [PATCH 09/23] Clean up assert_writeable_parts_eq to call assert_writeable_eq --- utils/writeable/src/lib.rs | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/utils/writeable/src/lib.rs b/utils/writeable/src/lib.rs index f00d5d7df87..f870d9436af 100644 --- a/utils/writeable/src/lib.rs +++ b/utils/writeable/src/lib.rs @@ -403,7 +403,7 @@ macro_rules! assert_writeable_eq { }; ($actual_writeable:expr, $expected_str:expr, $($arg:tt)+) => {{ let actual_writeable = &$actual_writeable; - let (actual_str, _) = $crate::_internal::writeable_to_parts_for_test(actual_writeable); + let (actual_str, actual_parts) = $crate::_internal::writeable_to_parts_for_test(actual_writeable); assert_eq!(actual_str, $expected_str, $($arg)*); assert_eq!(actual_str, $crate::Writeable::write_to_string(actual_writeable), $($arg)+); let length_hint = $crate::Writeable::writeable_length_hint(actual_writeable); @@ -422,6 +422,7 @@ macro_rules! assert_writeable_eq { assert_eq!(actual_writeable.to_string(), $expected_str); let ordering = $crate::Writeable::write_cmp_bytes(actual_writeable, $expected_str.as_bytes()); assert_eq!(ordering, core::cmp::Ordering::Equal, $($arg)*); + actual_parts // return for assert_writeable_parts_eq }}; } @@ -432,18 +433,7 @@ macro_rules! assert_writeable_parts_eq { $crate::assert_writeable_parts_eq!($actual_writeable, $expected_str, $expected_parts, ""); }; ($actual_writeable:expr, $expected_str:expr, $expected_parts:expr, $($arg:tt)+) => {{ - let actual_writeable = &$actual_writeable; - let (actual_str, actual_parts) = $crate::_internal::writeable_to_parts_for_test(actual_writeable); - assert_eq!(actual_str, $expected_str, $($arg)+); - assert_eq!(actual_str, $crate::Writeable::write_to_string(actual_writeable), $($arg)+); + let actual_parts = $crate::assert_writeable_eq!($actual_writeable, $expected_str, $($arg)*); assert_eq!(actual_parts, $expected_parts, $($arg)+); - let length_hint = $crate::Writeable::writeable_length_hint(actual_writeable); - assert!(length_hint.0 <= actual_str.len(), $($arg)+); - if let Some(upper) = length_hint.1 { - assert!(actual_str.len() <= upper, $($arg)+); - } - assert_eq!(actual_writeable.to_string(), $expected_str); - let ordering = $crate::Writeable::write_cmp_bytes(actual_writeable, $expected_str.as_bytes()); - assert_eq!(ordering, core::cmp::Ordering::Equal, $($arg)*); }}; } From 7658776b08e66ec7dd5da4be5872d60eaeaba037 Mon Sep 17 00:00:00 2001 From: "Shane F. Carr" Date: Tue, 9 Apr 2024 23:03:21 -0700 Subject: [PATCH 10/23] Oops, refactor is not completely non-breaking :( --- components/datetime/src/format/datetime.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/datetime/src/format/datetime.rs b/components/datetime/src/format/datetime.rs index 2aba106f406..7c228476768 100644 --- a/components/datetime/src/format/datetime.rs +++ b/components/datetime/src/format/datetime.rs @@ -618,7 +618,7 @@ mod tests { .into_japanese_date() .to_any(); - writeable::assert_writeable_eq!(dtf.format(&date).unwrap(), "Sep 1, 12 kansei-1789") + writeable::assert_writeable_eq!(dtf.format(&date).unwrap(), "Sep 1, 12 kansei-1789"); } #[test] From 36392e44e5dc60e246d8b45fd694117ad47716e0 Mon Sep 17 00:00:00 2001 From: "Shane F. Carr" Date: Tue, 9 Apr 2024 23:06:10 -0700 Subject: [PATCH 11/23] Changelog --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9819180749d..671cf7ed409 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,6 +59,9 @@ - Add `ZeroTrieSimpleAsciiCursor` for manual iteration (https://github.com/unicode-org/icu4x/pull/4383) - `zerovec` - Change `ZeroHashMap` to use `twox-hash` (https://github.com/unicode-org/icu4x/pull/4592) + - `writeable` + - Add `TryWriteable` for fallibility (https://github.com/unicode-org/icu4x/pull/4787) + - Add `write_cmp_bytes` for more efficient comparison (https://github.com/unicode-org/icu4x/pull/4402) ## icu4x 1.4.x From 06a25952b95a356139a8e51635028e4c6683030e Mon Sep 17 00:00:00 2001 From: "Shane F. Carr" Date: Wed, 10 Apr 2024 07:56:54 -0700 Subject: [PATCH 12/23] Apply suggestions from code review Co-authored-by: Robert Bastian <4706271+robertbastian@users.noreply.github.com> --- utils/writeable/src/lib.rs | 2 +- utils/writeable/src/try_writeable.rs | 20 ++++++++++---------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/utils/writeable/src/lib.rs b/utils/writeable/src/lib.rs index f870d9436af..1b7835f2b97 100644 --- a/utils/writeable/src/lib.rs +++ b/utils/writeable/src/lib.rs @@ -350,7 +350,7 @@ macro_rules! impl_display_with_writeable { /// - Equality of string content /// - Equality of parts ([`*_parts_eq`] only) /// - Validity of size hint -/// - Basic validity of `cmp_bytes` +/// - Reflexivity of `cmp_bytes` /// /// # Examples /// diff --git a/utils/writeable/src/try_writeable.rs b/utils/writeable/src/try_writeable.rs index 368f57abac7..e0aa120e593 100644 --- a/utils/writeable/src/try_writeable.rs +++ b/utils/writeable/src/try_writeable.rs @@ -8,10 +8,10 @@ use core::cmp::Ordering; /// A writeable object that can fail while writing. /// /// The default [`Writeable`] trait returns a [`fmt::Error`], which originates from the sink. -/// In contrast, this trait allows the _writeable itself_ to trigger an error. +/// In contrast, this trait allows the _writeable itself_ to trigger an error as well. /// /// Implementations are expected to always make a _best attempt_ at writing to the sink -/// and should write replacement values in the error state. Therefore, [`TryWriteable::Error`] +/// and should write replacement values in the error state. Therefore, the returned `Result` /// can be safely ignored to emulate a "lossy" mode. /// /// Any error substrings should be annotated with [`Part::ERROR`]. @@ -22,7 +22,7 @@ use core::cmp::Ordering; /// _even in the error state_, such as with a placeholder or fallback string. /// /// In [`TryWriteable::try_write_to_parts()`], error substrings should be annotated with -/// [`Part::ERROR`]. Becuause of this, writing to parts is not default-implemented like +/// [`Part::ERROR`]. Because of this, writing to parts is not default-implemented like /// it is on [`Writeable`]. /// /// Furthermore, [`TryWriteable::try_writeable_length_hint()`] is not default-implemented because @@ -43,7 +43,7 @@ use core::cmp::Ordering; /// use writeable::TryWriteable; /// /// #[derive(Debug, PartialEq, Eq)] -/// enum MyWriteableError { +/// enum HelloWorldWriteableError { /// MissingName, /// } /// @@ -103,18 +103,18 @@ pub trait TryWriteable { /// Writes the content of this writeable to a sink. /// - /// If the sink hits an error, writing is abruptly ended and - /// `Err(`[`fmt::Error`]`)` is returned. + /// If the sink hits an error, writing immediately ends, + /// `Err(`[`fmt::Error`]`)` is returned, and the sink does not contain valid output. /// - /// If the writeable hits an error, writing is continued with a replacement value and then - /// `Ok(Err(`[`TryWriteable::Error`]`))` is returned. + /// If the writeable hits an error, writing is continued with a replacement value, + /// `Ok(Err(`[`TryWriteable::Error`]`))` is returned, and the caller may continue using the sink. /// /// # Lossy Mode /// /// The [`fmt::Error`] should always be handled, but the [`TryWriteable::Error`] can be /// ignored if a fallback string is desired instead of an error. /// - /// To handle outer error but not the inner error, write: + /// To handle the sink error, but not the writeable error, write: /// /// ``` /// # use writeable::TryWriteable; @@ -126,7 +126,7 @@ pub trait TryWriteable { /// /// # Examples /// - /// The following examples use `Result<&str, usize>`, which implements [`TryWriteable`]. + /// The following examples use `Result<&str, usize>`, which implements [`TryWriteable`] because both `&str` and `usize` do. /// /// Success case: /// From 82721b0ae03f5f4670d85c0d67b02579110c2d5e Mon Sep 17 00:00:00 2001 From: "Shane F. Carr" Date: Wed, 10 Apr 2024 08:01:16 -0700 Subject: [PATCH 13/23] Docs fixes from code review fixes --- utils/writeable/src/try_writeable.rs | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/utils/writeable/src/try_writeable.rs b/utils/writeable/src/try_writeable.rs index e0aa120e593..9b3c13fb37b 100644 --- a/utils/writeable/src/try_writeable.rs +++ b/utils/writeable/src/try_writeable.rs @@ -53,7 +53,7 @@ use core::cmp::Ordering; /// } /// /// impl TryWriteable for HelloWorldWriteable { -/// type Error = MyWriteableError; +/// type Error = HelloWorldWriteableError; /// /// fn try_write_to_parts( /// &self, @@ -69,7 +69,7 @@ use core::cmp::Ordering; /// if self.name.is_some() { /// Ok(Ok(())) /// } else { -/// Ok(Err(MyWriteableError::MissingName)) +/// Ok(Err(HelloWorldWriteableError::MissingName)) /// } /// } /// @@ -78,7 +78,7 @@ use core::cmp::Ordering; /// ) -> (LengthHint, Option) { /// let (hint, e) = /// self.name.ok_or("nobody").try_writeable_length_hint(); -/// (hint + 8, e.map(|_| MyWriteableError::MissingName)) +/// (hint + 8, e.map(|_| HelloWorldWriteableError::MissingName)) /// } /// } /// @@ -94,7 +94,7 @@ use core::cmp::Ordering; /// writeable::assert_try_writeable_parts_eq!( /// HelloWorldWriteable { name: None }, /// "Hello, nobody!", -/// Err(MyWriteableError::MissingName), +/// Err(HelloWorldWriteableError::MissingName), /// [(7, 13, writeable::Part::ERROR)] /// ); /// ``` @@ -223,7 +223,7 @@ pub trait TryWriteable { /// # use writeable::LengthHint; /// /// #[derive(Debug, PartialEq, Eq)] - /// enum MyWriteableError { + /// enum HelloWorldWriteableError { /// MissingName /// } /// @@ -233,7 +233,7 @@ pub trait TryWriteable { /// } /// /// impl TryWriteable for HelloWorldWriteable { - /// type Error = MyWriteableError; + /// type Error = HelloWorldWriteableError; /// // see impl in TryWriteable docs /// # fn try_write_to_parts( /// # &self, @@ -249,12 +249,12 @@ pub trait TryWriteable { /// # if self.name.is_some() { /// # Ok(Ok(())) /// # } else { - /// # Ok(Err(MyWriteableError::MissingName)) + /// # Ok(Err(HelloWorldWriteableError::MissingName)) /// # } /// # } /// # fn try_writeable_length_hint(&self) -> (LengthHint, Option) { /// # let (hint, e) = self.name.ok_or("nobody").try_writeable_length_hint(); - /// # (hint + 8, e.map(|_| MyWriteableError::MissingName)) + /// # (hint + 8, e.map(|_| HelloWorldWriteableError::MissingName)) /// # } /// } /// @@ -275,12 +275,12 @@ pub trait TryWriteable { /// let mut writeable_str = String::new(); /// let _ = writeable.try_write_to(&mut writeable_str).expect("write to String is infallible"); /// - /// assert_eq!((Ordering::Equal, Some(MyWriteableError::MissingName)), writeable.try_write_cmp_bytes(b"Hello, nobody!")); + /// assert_eq!((Ordering::Equal, Some(HelloWorldWriteableError::MissingName)), writeable.try_write_cmp_bytes(b"Hello, nobody!")); /// - /// assert_eq!((Ordering::Greater, Some(MyWriteableError::MissingName)), writeable.try_write_cmp_bytes(b"Hello, alice!")); + /// assert_eq!((Ordering::Greater, Some(HelloWorldWriteableError::MissingName)), writeable.try_write_cmp_bytes(b"Hello, alice!")); /// assert_eq!(Ordering::Greater, (*writeable_str).cmp("Hello, alice!")); /// - /// assert_eq!((Ordering::Less, Some(MyWriteableError::MissingName)), writeable.try_write_cmp_bytes(b"Hello, zero!")); + /// assert_eq!((Ordering::Less, Some(HelloWorldWriteableError::MissingName)), writeable.try_write_cmp_bytes(b"Hello, zero!")); /// assert_eq!(Ordering::Less, (*writeable_str).cmp("Hello, zero!")); /// ``` fn try_write_cmp_bytes(&self, other: &[u8]) -> (Ordering, Option) { @@ -369,7 +369,7 @@ where /// - Equality of string content /// - Equality of parts ([`*_parts_eq`] only) /// - Validity of size hint -/// - Basic validity of `cmp_bytes` +/// - Reflexivity of `cmp_bytes` /// /// For a usage example, see [`TryWriteable`]. /// From 2d2a4370d502322856bf3ccbd263f4b514d719bb Mon Sep 17 00:00:00 2001 From: "Shane F. Carr" Date: Wed, 10 Apr 2024 08:35:23 -0700 Subject: [PATCH 14/23] Make assert_[try_]writeable_eq not return anything --- utils/writeable/src/lib.rs | 9 ++++++--- utils/writeable/src/try_writeable.rs | 5 ++++- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/utils/writeable/src/lib.rs b/utils/writeable/src/lib.rs index 1b7835f2b97..2581114658a 100644 --- a/utils/writeable/src/lib.rs +++ b/utils/writeable/src/lib.rs @@ -399,9 +399,12 @@ macro_rules! impl_display_with_writeable { #[macro_export] macro_rules! assert_writeable_eq { ($actual_writeable:expr, $expected_str:expr $(,)?) => { - $crate::assert_writeable_eq!($actual_writeable, $expected_str, ""); + $crate::assert_writeable_eq!($actual_writeable, $expected_str, "") }; ($actual_writeable:expr, $expected_str:expr, $($arg:tt)+) => {{ + $crate::assert_writeable_eq!(@internal, $actual_writeable, $expected_str, $($arg)*); + }}; + (@internal, $actual_writeable:expr, $expected_str:expr, $($arg:tt)+) => {{ let actual_writeable = &$actual_writeable; let (actual_str, actual_parts) = $crate::_internal::writeable_to_parts_for_test(actual_writeable); assert_eq!(actual_str, $expected_str, $($arg)*); @@ -430,10 +433,10 @@ macro_rules! assert_writeable_eq { #[macro_export] macro_rules! assert_writeable_parts_eq { ($actual_writeable:expr, $expected_str:expr, $expected_parts:expr $(,)?) => { - $crate::assert_writeable_parts_eq!($actual_writeable, $expected_str, $expected_parts, ""); + $crate::assert_writeable_parts_eq!($actual_writeable, $expected_str, $expected_parts, "") }; ($actual_writeable:expr, $expected_str:expr, $expected_parts:expr, $($arg:tt)+) => {{ - let actual_parts = $crate::assert_writeable_eq!($actual_writeable, $expected_str, $($arg)*); + let actual_parts = $crate::assert_writeable_eq!(@internal, $actual_writeable, $expected_str, $($arg)*); assert_eq!(actual_parts, $expected_parts, $($arg)+); }}; } diff --git a/utils/writeable/src/try_writeable.rs b/utils/writeable/src/try_writeable.rs index 9b3c13fb37b..7aa46af3745 100644 --- a/utils/writeable/src/try_writeable.rs +++ b/utils/writeable/src/try_writeable.rs @@ -383,6 +383,9 @@ macro_rules! assert_try_writeable_eq { $crate::assert_try_writeable_eq!($actual_writeable, $expected_str, $expected_result, "") }; ($actual_writeable:expr, $expected_str:expr, $expected_result:expr, $($arg:tt)+) => {{ + $crate::assert_try_writeable_eq!(@internal, $actual_writeable, $expected_str, $expected_result, $($arg)*); + }}; + (@internal, $actual_writeable:expr, $expected_str:expr, $expected_result:expr, $($arg:tt)+) => {{ use $crate::TryWriteable; let actual_writeable = &$actual_writeable; let (actual_str, actual_parts, actual_error) = $crate::_internal::try_writeable_to_parts_for_test(actual_writeable); @@ -427,7 +430,7 @@ macro_rules! assert_try_writeable_parts_eq { $crate::assert_try_writeable_parts_eq!($actual_writeable, $expected_str, $expected_result, $expected_parts, "") }; ($actual_writeable:expr, $expected_str:expr, $expected_result:expr, $expected_parts:expr, $($arg:tt)+) => {{ - let actual_parts = $crate::assert_try_writeable_eq!($actual_writeable, $expected_str, $expected_result, $($arg)*); + let actual_parts = $crate::assert_try_writeable_eq!(@internal, $actual_writeable, $expected_str, $expected_result, $($arg)*); assert_eq!(actual_parts, $expected_parts, $($arg)+); }}; } From 0745cf8a7fb410cd2c17624b3800bc59d2b0ecc3 Mon Sep 17 00:00:00 2001 From: "Shane F. Carr" Date: Wed, 10 Apr 2024 08:03:00 -0700 Subject: [PATCH 15/23] Revert "Oops, refactor is not completely non-breaking :(" This reverts commit 7658776b08e66ec7dd5da4be5872d60eaeaba037. --- components/datetime/src/format/datetime.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/datetime/src/format/datetime.rs b/components/datetime/src/format/datetime.rs index 7c228476768..2aba106f406 100644 --- a/components/datetime/src/format/datetime.rs +++ b/components/datetime/src/format/datetime.rs @@ -618,7 +618,7 @@ mod tests { .into_japanese_date() .to_any(); - writeable::assert_writeable_eq!(dtf.format(&date).unwrap(), "Sep 1, 12 kansei-1789"); + writeable::assert_writeable_eq!(dtf.format(&date).unwrap(), "Sep 1, 12 kansei-1789") } #[test] From 3e2107f6350e1ea9de4beb72431f8082b07a4f3f Mon Sep 17 00:00:00 2001 From: "Shane F. Carr" Date: Wed, 10 Apr 2024 08:36:42 -0700 Subject: [PATCH 16/23] Don't check the option value twice --- utils/writeable/src/try_writeable.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/utils/writeable/src/try_writeable.rs b/utils/writeable/src/try_writeable.rs index 7aa46af3745..895b44f454e 100644 --- a/utils/writeable/src/try_writeable.rs +++ b/utils/writeable/src/try_writeable.rs @@ -61,12 +61,12 @@ use core::cmp::Ordering; /// ) -> Result, fmt::Error> { /// sink.write_str("Hello, ")?; /// // Use `impl TryWriteable for Result` to generate the error part: -/// let _ = self.name.ok_or("nobody").try_write_to_parts(sink)?; +/// let err = self.name.ok_or("nobody").try_write_to_parts(sink)?.err(); /// sink.write_char('!')?; /// // Return a doubly-wrapped Result. /// // The outer Result is for fmt::Error, handled by the `?`s above. /// // The inner Result is for our own Self::Error. -/// if self.name.is_some() { +/// if err.is_none() { /// Ok(Ok(())) /// } else { /// Ok(Err(HelloWorldWriteableError::MissingName)) From 341a5599f065daf69646feff114001c5f7d0dafc Mon Sep 17 00:00:00 2001 From: "Shane F. Carr" Date: Wed, 10 Apr 2024 10:19:04 -0700 Subject: [PATCH 17/23] Implement option 2 --- utils/writeable/src/try_writeable.rs | 72 ++++++++++------------------ 1 file changed, 26 insertions(+), 46 deletions(-) diff --git a/utils/writeable/src/try_writeable.rs b/utils/writeable/src/try_writeable.rs index 895b44f454e..c7d8bdb2eed 100644 --- a/utils/writeable/src/try_writeable.rs +++ b/utils/writeable/src/try_writeable.rs @@ -25,9 +25,6 @@ use core::cmp::Ordering; /// [`Part::ERROR`]. Because of this, writing to parts is not default-implemented like /// it is on [`Writeable`]. /// -/// Furthermore, [`TryWriteable::try_writeable_length_hint()`] is not default-implemented because -/// it has an invariant that the error state should be correctly propagated. -/// /// The trait is implemented on [`Result`] where `T` and `E` both implement [`Writeable`]; /// In the `Ok` case, `T` is written, and in the `Err` case, `E` is written as a fallback value. /// This impl, which writes [`Part::ERROR`], can be used as a basis for more advanced impls. @@ -73,12 +70,8 @@ use core::cmp::Ordering; /// } /// } /// -/// fn try_writeable_length_hint( -/// &self, -/// ) -> (LengthHint, Option) { -/// let (hint, e) = -/// self.name.ok_or("nobody").try_writeable_length_hint(); -/// (hint + 8, e.map(|_| HelloWorldWriteableError::MissingName)) +/// fn writeable_length_hint(&self) -> LengthHint { +/// self.name.ok_or("nobody").writeable_length_hint() + 8 /// } /// } /// @@ -174,11 +167,10 @@ pub trait TryWriteable { /// Returns a hint for the number of UTF-8 bytes that will be written to the sink. /// - /// This function returns `Some(error)` _if and only if_ [`TryWriteable::try_write_to()`] - /// returns `Err`. - fn try_writeable_length_hint(&self) -> (LengthHint, Option) { - // TODO: Discuss this function, its signature, and its behavior - (LengthHint::undefined(), None) + /// This function returns the length of the "lossy mode" string; for more information, + /// see [`TryWriteable::try_write_to()`]. + fn writeable_length_hint(&self) -> LengthHint { + LengthHint::undefined() } /// Writes the content of this writeable to a string. @@ -186,7 +178,7 @@ pub trait TryWriteable { /// This function does not return a string in the failure case. If you need a replacement /// string ("lossy mode"), use [`TryWriteable::try_write_to()`] instead. fn try_write_to_string(&self) -> Result, Self::Error> { - let (hint, hint_error) = self.try_writeable_length_hint(); + let hint = self.writeable_length_hint(); if hint.is_zero() { return Ok(Cow::Borrowed("")); } @@ -198,18 +190,13 @@ pub trait TryWriteable { Ok(()) } }; - debug_assert_eq!( - hint_error.is_some(), - result.is_err(), - "try_writeable_length_hint and try_write_to_string should have same error state" - ); result.map(|_| Cow::Owned(output)) } /// Compares the content of this writeable to a byte slice. /// - /// This function returns `Some(error)` _if and only if_ [`TryWriteable::try_write_to()`] - /// returns `Err`. + /// This function compares the "lossy mode" string; for more information, + /// see [`TryWriteable::try_write_to()`]. /// /// For more information, see [`Writeable::write_cmp_bytes()`]. /// @@ -252,22 +239,18 @@ pub trait TryWriteable { /// # Ok(Err(HelloWorldWriteableError::MissingName)) /// # } /// # } - /// # fn try_writeable_length_hint(&self) -> (LengthHint, Option) { - /// # let (hint, e) = self.name.ok_or("nobody").try_writeable_length_hint(); - /// # (hint + 8, e.map(|_| HelloWorldWriteableError::MissingName)) - /// # } /// } /// /// // Success case: /// let writeable = HelloWorldWriteable { name: Some("Alice") }; /// let writeable_str = writeable.try_write_to_string().expect("name is Some"); /// - /// assert_eq!((Ordering::Equal, None), writeable.try_write_cmp_bytes(b"Hello, Alice!")); + /// assert_eq!(Ordering::Equal, writeable.write_cmp_bytes(b"Hello, Alice!")); /// - /// assert_eq!((Ordering::Greater, None), writeable.try_write_cmp_bytes(b"Alice!")); + /// assert_eq!(Ordering::Greater, writeable.write_cmp_bytes(b"Alice!")); /// assert_eq!(Ordering::Greater, (*writeable_str).cmp("Alice!")); /// - /// assert_eq!((Ordering::Less, None), writeable.try_write_cmp_bytes(b"Hello, Bob!")); + /// assert_eq!(Ordering::Less, writeable.write_cmp_bytes(b"Hello, Bob!")); /// assert_eq!(Ordering::Less, (*writeable_str).cmp("Hello, Bob!")); /// /// // Failure case: @@ -275,25 +258,24 @@ pub trait TryWriteable { /// let mut writeable_str = String::new(); /// let _ = writeable.try_write_to(&mut writeable_str).expect("write to String is infallible"); /// - /// assert_eq!((Ordering::Equal, Some(HelloWorldWriteableError::MissingName)), writeable.try_write_cmp_bytes(b"Hello, nobody!")); + /// assert_eq!(Ordering::Equal, writeable.write_cmp_bytes(b"Hello, nobody!")); /// - /// assert_eq!((Ordering::Greater, Some(HelloWorldWriteableError::MissingName)), writeable.try_write_cmp_bytes(b"Hello, alice!")); + /// assert_eq!(Ordering::Greater, writeable.write_cmp_bytes(b"Hello, alice!")); /// assert_eq!(Ordering::Greater, (*writeable_str).cmp("Hello, alice!")); /// - /// assert_eq!((Ordering::Less, Some(HelloWorldWriteableError::MissingName)), writeable.try_write_cmp_bytes(b"Hello, zero!")); + /// assert_eq!(Ordering::Less, writeable.write_cmp_bytes(b"Hello, zero!")); /// assert_eq!(Ordering::Less, (*writeable_str).cmp("Hello, zero!")); /// ``` - fn try_write_cmp_bytes(&self, other: &[u8]) -> (Ordering, Option) { + fn write_cmp_bytes(&self, other: &[u8]) -> Ordering { let mut wc = cmp::WriteComparator::new(other); - let result = match self.try_write_to(&mut wc) { + let _ = match self.try_write_to(&mut wc) { Ok(result) => result, Err(core::fmt::Error) => { debug_assert!(false, "WriteComparator infallible"); Ok(()) } }; - let ordering = wc.finish().reverse(); - (ordering, result.err()) + wc.finish().reverse() } } @@ -329,10 +311,10 @@ where } #[inline] - fn try_writeable_length_hint(&self) -> (LengthHint, Option) { + fn writeable_length_hint(&self) -> LengthHint { match self { - Ok(t) => (t.writeable_length_hint(), None), - Err(e) => (e.writeable_length_hint(), Some(e.clone())), + Ok(t) => t.writeable_length_hint(), + Err(e) => e.writeable_length_hint(), } } @@ -345,10 +327,10 @@ where } #[inline] - fn try_write_cmp_bytes(&self, other: &[u8]) -> (Ordering, Option) { + fn write_cmp_bytes(&self, other: &[u8]) -> Ordering { match self { - Ok(t) => (t.write_cmp_bytes(other), None), - Err(e) => (e.write_cmp_bytes(other), Some(e.clone())), + Ok(t) => t.write_cmp_bytes(other), + Err(e) => e.write_cmp_bytes(other), } } } @@ -399,7 +381,7 @@ macro_rules! assert_try_writeable_eq { Err(e) => Err(e) }; assert_eq!(actual_result, Result::<(), _>::from($expected_result), $($arg)*); - let (length_hint, hint_error) = actual_writeable.try_writeable_length_hint(); + let length_hint = actual_writeable.writeable_length_hint(); assert!( length_hint.0 <= actual_str.len(), "hint lower bound {} larger than actual length {}: {}", @@ -412,10 +394,8 @@ macro_rules! assert_try_writeable_eq { length_hint.0, actual_str.len(), format!($($arg)*), ); } - assert_eq!(hint_error, Result::<(), _>::from($expected_result).err(), $($arg)*); - let (ordering, ordering_error) = actual_writeable.try_write_cmp_bytes($expected_str.as_bytes()); + let ordering = actual_writeable.write_cmp_bytes($expected_str.as_bytes()); assert_eq!(ordering, core::cmp::Ordering::Equal, $($arg)*); - assert_eq!(ordering_error, Result::<(), _>::from($expected_result).err(), $($arg)*); actual_parts // return for assert_try_writeable_parts_eq }}; } From 2dcd8502e9a53991405f14cf7d50b35dbcc635bd Mon Sep 17 00:00:00 2001 From: "Shane F. Carr" Date: Wed, 10 Apr 2024 10:25:43 -0700 Subject: [PATCH 18/23] Apply suggestions from code review Co-authored-by: Robert Bastian <4706271+robertbastian@users.noreply.github.com> --- utils/writeable/src/try_writeable.rs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/utils/writeable/src/try_writeable.rs b/utils/writeable/src/try_writeable.rs index c7d8bdb2eed..dfaea28d045 100644 --- a/utils/writeable/src/try_writeable.rs +++ b/utils/writeable/src/try_writeable.rs @@ -183,13 +183,7 @@ pub trait TryWriteable { return Ok(Cow::Borrowed("")); } let mut output = String::with_capacity(hint.capacity()); - let result = match self.try_write_to(&mut output) { - Ok(result) => result, - Err(core::fmt::Error) => { - debug_assert!(false, "String infallible"); - Ok(()) - } - }; + let result = self.try_write_to(&mut output).unwrap_or_else(|core::fmt::Error| Ok(())); result.map(|_| Cow::Owned(output)) } From c6d71ad84cff40f99f57047947addb43c24b037c Mon Sep 17 00:00:00 2001 From: "Shane F. Carr" Date: Wed, 10 Apr 2024 10:24:35 -0700 Subject: [PATCH 19/23] Distribute code from helpers.rs --- utils/writeable/src/lib.rs | 9 +++--- utils/writeable/src/parts_write_adapter.rs | 32 +++++++++++++++++++ .../writeable/src/{helpers.rs => testing.rs} | 31 ++---------------- utils/writeable/src/try_writeable.rs | 3 +- 4 files changed, 41 insertions(+), 34 deletions(-) create mode 100644 utils/writeable/src/parts_write_adapter.rs rename utils/writeable/src/{helpers.rs => testing.rs} (74%) diff --git a/utils/writeable/src/lib.rs b/utils/writeable/src/lib.rs index 249f6515990..c4d98f30faa 100644 --- a/utils/writeable/src/lib.rs +++ b/utils/writeable/src/lib.rs @@ -69,9 +69,10 @@ extern crate alloc; mod cmp; #[cfg(feature = "either")] mod either; -mod helpers; mod impls; mod ops; +mod parts_write_adapter; +mod testing; mod try_writeable; use alloc::borrow::Cow; @@ -82,8 +83,8 @@ pub use try_writeable::TryWriteable; #[doc(hidden)] pub mod _internal { - pub use super::helpers::try_writeable_to_parts_for_test; - pub use super::helpers::writeable_to_parts_for_test; + pub use super::testing::try_writeable_to_parts_for_test; + pub use super::testing::writeable_to_parts_for_test; } /// A hint to help consumers of `Writeable` pre-allocate bytes before they call @@ -204,7 +205,7 @@ pub trait Writeable { /// The default implementation delegates to `write_to_parts`, and discards any /// `Part` annotations. fn write_to(&self, sink: &mut W) -> fmt::Result { - self.write_to_parts(&mut helpers::CoreWriteAsPartsWrite(sink)) + self.write_to_parts(&mut parts_write_adapter::CoreWriteAsPartsWrite(sink)) } /// Write bytes and `Part` annotations to the given sink. Errors from the diff --git a/utils/writeable/src/parts_write_adapter.rs b/utils/writeable/src/parts_write_adapter.rs new file mode 100644 index 00000000000..d20b634dcd1 --- /dev/null +++ b/utils/writeable/src/parts_write_adapter.rs @@ -0,0 +1,32 @@ +// This file is part of ICU4X. For terms of use, please see the file +// called LICENSE at the top level of the ICU4X source tree +// (online at: https://github.com/unicode-org/icu4x/blob/main/LICENSE ). + +use crate::*; + +pub(crate) struct CoreWriteAsPartsWrite(pub W); + +impl fmt::Write for CoreWriteAsPartsWrite { + #[inline] + fn write_str(&mut self, s: &str) -> fmt::Result { + self.0.write_str(s) + } + + #[inline] + fn write_char(&mut self, c: char) -> fmt::Result { + self.0.write_char(c) + } +} + +impl PartsWrite for CoreWriteAsPartsWrite { + type SubPartsWrite = CoreWriteAsPartsWrite; + + #[inline] + fn with_part( + &mut self, + _part: Part, + mut f: impl FnMut(&mut Self::SubPartsWrite) -> fmt::Result, + ) -> fmt::Result { + f(self) + } +} diff --git a/utils/writeable/src/helpers.rs b/utils/writeable/src/testing.rs similarity index 74% rename from utils/writeable/src/helpers.rs rename to utils/writeable/src/testing.rs index 06ac5f5c0c9..dcc44940881 100644 --- a/utils/writeable/src/helpers.rs +++ b/utils/writeable/src/testing.rs @@ -50,7 +50,7 @@ impl PartsWrite for TestWriter { pub fn writeable_to_parts_for_test( writeable: &W, ) -> (String, Vec<(usize, usize, Part)>) { - let mut writer = helpers::TestWriter { + let mut writer = TestWriter { string: alloc::string::String::new(), parts: Vec::new(), }; @@ -66,7 +66,7 @@ pub fn writeable_to_parts_for_test( pub fn try_writeable_to_parts_for_test( writeable: &W, ) -> (String, Vec<(usize, usize, Part)>, Option) { - let mut writer = helpers::TestWriter { + let mut writer = TestWriter { string: alloc::string::String::new(), parts: Vec::new(), }; @@ -77,30 +77,3 @@ pub fn try_writeable_to_parts_for_test( let (actual_str, actual_parts) = writer.finish(); (actual_str, actual_parts, result.err()) } - -pub(crate) struct CoreWriteAsPartsWrite(pub W); - -impl fmt::Write for CoreWriteAsPartsWrite { - #[inline] - fn write_str(&mut self, s: &str) -> fmt::Result { - self.0.write_str(s) - } - - #[inline] - fn write_char(&mut self, c: char) -> fmt::Result { - self.0.write_char(c) - } -} - -impl PartsWrite for CoreWriteAsPartsWrite { - type SubPartsWrite = CoreWriteAsPartsWrite; - - #[inline] - fn with_part( - &mut self, - _part: Part, - mut f: impl FnMut(&mut Self::SubPartsWrite) -> fmt::Result, - ) -> fmt::Result { - f(self) - } -} diff --git a/utils/writeable/src/try_writeable.rs b/utils/writeable/src/try_writeable.rs index dfaea28d045..c84b7c2e404 100644 --- a/utils/writeable/src/try_writeable.rs +++ b/utils/writeable/src/try_writeable.rs @@ -3,6 +3,7 @@ // (online at: https://github.com/unicode-org/icu4x/blob/main/LICENSE ). use super::*; +use crate::parts_write_adapter::CoreWriteAsPartsWrite; use core::cmp::Ordering; /// A writeable object that can fail while writing. @@ -150,7 +151,7 @@ pub trait TryWriteable { &self, sink: &mut W, ) -> Result, fmt::Error> { - self.try_write_to_parts(&mut helpers::CoreWriteAsPartsWrite(sink)) + self.try_write_to_parts(&mut CoreWriteAsPartsWrite(sink)) } /// Writes the content of this writeable to a sink with parts (annotations). From 89a227e5f529bc1f91bed6bc5d4e2e3f023e2121 Mon Sep 17 00:00:00 2001 From: "Shane F. Carr" Date: Wed, 10 Apr 2024 10:27:20 -0700 Subject: [PATCH 20/23] unwrap_or_else --- utils/writeable/src/try_writeable.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/utils/writeable/src/try_writeable.rs b/utils/writeable/src/try_writeable.rs index c84b7c2e404..a7eb4b27bfb 100644 --- a/utils/writeable/src/try_writeable.rs +++ b/utils/writeable/src/try_writeable.rs @@ -184,7 +184,9 @@ pub trait TryWriteable { return Ok(Cow::Borrowed("")); } let mut output = String::with_capacity(hint.capacity()); - let result = self.try_write_to(&mut output).unwrap_or_else(|core::fmt::Error| Ok(())); + let result = self + .try_write_to(&mut output) + .unwrap_or_else(|fmt::Error| Ok(())); result.map(|_| Cow::Owned(output)) } @@ -263,13 +265,9 @@ pub trait TryWriteable { /// ``` fn write_cmp_bytes(&self, other: &[u8]) -> Ordering { let mut wc = cmp::WriteComparator::new(other); - let _ = match self.try_write_to(&mut wc) { - Ok(result) => result, - Err(core::fmt::Error) => { - debug_assert!(false, "WriteComparator infallible"); - Ok(()) - } - }; + let _ = self + .try_write_to(&mut wc) + .unwrap_or_else(|fmt::Error| Ok(())); wc.finish().reverse() } } From d4a9d83836641b74db4fc1526a7c832a7e3f361b Mon Sep 17 00:00:00 2001 From: "Shane F. Carr" Date: Wed, 10 Apr 2024 10:58:22 -0700 Subject: [PATCH 21/23] Apply suggestions from code review Co-authored-by: Robert Bastian <4706271+robertbastian@users.noreply.github.com> --- utils/writeable/src/testing.rs | 1 - utils/writeable/src/try_writeable.rs | 8 ++++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/utils/writeable/src/testing.rs b/utils/writeable/src/testing.rs index dcc44940881..078ea9e2b54 100644 --- a/utils/writeable/src/testing.rs +++ b/utils/writeable/src/testing.rs @@ -61,7 +61,6 @@ pub fn writeable_to_parts_for_test( writer.finish() } -#[doc(hidden)] #[allow(clippy::type_complexity)] pub fn try_writeable_to_parts_for_test( writeable: &W, diff --git a/utils/writeable/src/try_writeable.rs b/utils/writeable/src/try_writeable.rs index a7eb4b27bfb..1c79435ec46 100644 --- a/utils/writeable/src/try_writeable.rs +++ b/utils/writeable/src/try_writeable.rs @@ -184,10 +184,10 @@ pub trait TryWriteable { return Ok(Cow::Borrowed("")); } let mut output = String::with_capacity(hint.capacity()); - let result = self + self .try_write_to(&mut output) - .unwrap_or_else(|fmt::Error| Ok(())); - result.map(|_| Cow::Owned(output)) + .unwrap_or_else(|fmt::Error| Ok(())) + .map(|()| Cow::Owned(output)) } /// Compares the content of this writeable to a byte slice. @@ -286,7 +286,7 @@ where ) -> Result, fmt::Error> { match self { Ok(t) => t.write_to(sink).map(Ok), - Err(e) => e.write_to(sink).map(|_| Err(e.clone())), + Err(e) => e.write_to(sink).map(|()| Err(e.clone())), } } From 60acec0b47adfa1e3551c473ee3c26f73bb1667f Mon Sep 17 00:00:00 2001 From: "Shane F. Carr" Date: Wed, 10 Apr 2024 11:00:10 -0700 Subject: [PATCH 22/23] fmt --- utils/writeable/src/try_writeable.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/utils/writeable/src/try_writeable.rs b/utils/writeable/src/try_writeable.rs index 1c79435ec46..0ba2b0778a6 100644 --- a/utils/writeable/src/try_writeable.rs +++ b/utils/writeable/src/try_writeable.rs @@ -184,8 +184,7 @@ pub trait TryWriteable { return Ok(Cow::Borrowed("")); } let mut output = String::with_capacity(hint.capacity()); - self - .try_write_to(&mut output) + self.try_write_to(&mut output) .unwrap_or_else(|fmt::Error| Ok(())) .map(|()| Cow::Owned(output)) } From aed2160e426be1775b5aed8e26785a9bbf561d60 Mon Sep 17 00:00:00 2001 From: "Shane F. Carr" Date: Wed, 10 Apr 2024 11:43:27 -0700 Subject: [PATCH 23/23] Replace one more instance of |_| with |()| --- utils/writeable/src/try_writeable.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/utils/writeable/src/try_writeable.rs b/utils/writeable/src/try_writeable.rs index 0ba2b0778a6..30b5f3e2c3d 100644 --- a/utils/writeable/src/try_writeable.rs +++ b/utils/writeable/src/try_writeable.rs @@ -298,7 +298,7 @@ where Ok(t) => t.write_to_parts(sink).map(Ok), Err(e) => sink .with_part(Part::ERROR, |sink| e.write_to_parts(sink)) - .map(|_| Err(e.clone())), + .map(|()| Err(e.clone())), } }