refactor(serde_yml): 🎨 refactor modules by category and cleaning up

- Return `ErrorImpl::InvalidProgress` instead of using `unreachable!()` for `Progress::Iterable` and `Progress::Document`, avoiding unexpected panics. - Add duplicate anchor checks in `Scalar`, `SequenceStart`, and `MappingStart`, returning `ErrorImpl::DuplicateAnchor` and halting parsing for a fail-fast approach. - Enhance error handling consistency by ensuring parser is set to `None` after fatal errors (like unknown or duplicate anchors).
sebastienrousseau · Jan 1, 2025 · 8f1327f · 8f1327f
1 parent b362b97
commit 8f1327f
Show file tree

Hide file tree

Showing 6 changed files with 229 additions and 23 deletions.
diff --git a/src/libyml/safe_cstr.rs b/src/libyml/safe_cstr.rs
@@ -87,6 +87,7 @@ impl<'a> CStr<'a> {
     /// # Returns
     ///
     /// The length of the C-style string, not including the null terminator.
+    #[must_use]
     pub fn len(self) -> usize {
         let start = self.ptr.as_ptr();
         let mut end = start;
@@ -109,6 +110,7 @@ impl<'a> CStr<'a> {
     /// # Returns
     ///
     /// `true` if the C-style string is empty, `false` otherwise.
+    #[must_use]
     pub fn is_empty(self) -> bool {
         self.len() == 0
     }
@@ -118,6 +120,7 @@ impl<'a> CStr<'a> {
     /// # Returns
     ///
     /// A borrowed reference to the byte slice represented by the `CStr` instance.
+    #[must_use]
     pub fn to_bytes(self) -> &'a [u8] {
         let len = self.len();
         unsafe { slice::from_raw_parts(self.ptr.as_ptr(), len) }
@@ -180,6 +183,11 @@ fn display_lossy(
 /// # Panics
 ///
 /// This method will panic if the input `bytes` slice does not have a null terminator.
+///
+/// # Errors
+///
+/// This method will return a `CStrError` if the input `bytes` slice does not have a null terminator.
+///
 pub fn debug_lossy(
     mut bytes: &[u8],
     formatter: &mut fmt::Formatter<'_>,
@@ -214,14 +222,13 @@ pub fn debug_lossy(
         match from_utf8_result {
             Ok(_valid) => break,
             Err(utf8_error) => {
-                let end_of_broken =
-                    if let Some(error_len) = utf8_error.error_len() {
+                let end_of_broken = utf8_error
+                    .error_len()
+                    .map_or(bytes.len(), |error_len| {
                         valid.len() + error_len
-                    } else {
-                        bytes.len()
-                    };
+                    });
                 for b in &bytes[valid.len()..end_of_broken] {
-                    write!(formatter, "\\x{:02x}", b)?;
+                    write!(formatter, "\\x{b:02x}")?;
                 }
                 bytes = &bytes[end_of_broken..];
             }

diff --git a/src/libyml/tag.rs b/src/libyml/tag.rs
@@ -7,6 +7,7 @@ use std::{
 
 /// Custom error type for Tag operations.
 #[derive(Clone, Copy, Debug, Hash, Ord, PartialOrd, Eq, PartialEq)]
+#[must_use]
 pub struct TagFormatError;
 
 impl Display for TagFormatError {
@@ -80,8 +81,9 @@ impl Tag {
     /// # Returns
     ///
     /// Returns a `Tag` instance representing the specified tag string.
-    pub fn new(tag_str: &str) -> Tag {
-        Tag(Box::from(tag_str.as_bytes()))
+    #[must_use]
+    pub fn new(tag_str: &str) -> Self {
+        Self(Box::from(tag_str.as_bytes()))
     }
 }
 

diff --git a/src/libyml/util.rs b/src/libyml/util.rs
@@ -19,6 +19,7 @@ impl<T> Owned<T> {
     /// # Safety
     /// The created instance contains uninitialized memory, and should be properly
     /// initialized before use.
+    #[must_use]
     pub fn new_uninit() -> Owned<MaybeUninit<T>, T> {
         // Allocate memory for `T` but leave it uninitialized.
         let boxed = Box::new(MaybeUninit::<T>::uninit());
@@ -35,12 +36,13 @@ impl<T> Owned<T> {
     ///
     /// # Safety
     /// The caller must ensure that `definitely_init` is properly initialized.
-    pub unsafe fn assume_init(
+    #[must_use]
+    pub const unsafe fn assume_init(
         definitely_init: Owned<MaybeUninit<T>, T>,
     ) -> Owned<T> {
         let ptr = definitely_init.ptr;
         mem::forget(definitely_init);
-        Owned {
+        Self {
             ptr: ptr.cast(),
             marker: PhantomData,
         }

diff --git a/src/loader.rs b/src/loader.rs
@@ -101,7 +101,7 @@ impl<'input> Loader<'input> {
                 Cow::Owned(buffer)
             }
             Progress::Iterable(_) | Progress::Document(_) => {
-                unreachable!()
+                return Err(error::new(ErrorImpl::InvalidProgress));
             }
             Progress::Fail(err) => return Err(error::shared(err)),
         };
@@ -149,9 +149,9 @@ impl<'input> Loader<'input> {
         };
 
         let anchor_name = |anchor: &Anchor| {
-            format!("{:?}", anchor)
-                .trim_start_matches("\"")
-                .trim_end_matches("\"")
+            format!("{anchor:?}")
+                .trim_start_matches('"')
+                .trim_end_matches('"')
                 .to_owned()
         };
 
@@ -163,8 +163,8 @@ impl<'input> Loader<'input> {
                     return Some(document);
                 }
             };
+
             let event = match event {
-                YamlEvent::StreamStart => continue,
                 YamlEvent::StreamEnd => {
                     self.parser = None;
                     return if first {
@@ -176,21 +176,37 @@ impl<'input> Loader<'input> {
                         None
                     };
                 }
-                YamlEvent::DocumentStart => continue,
+                YamlEvent::DocumentStart | YamlEvent::StreamStart => {
+                    continue
+                }
                 YamlEvent::DocumentEnd => return Some(document),
 
-                YamlEvent::Alias(alias) => match anchors.get(&alias) {
-                    Some(id) => Event::Alias(*id),
-                    None => {
+                YamlEvent::Alias(alias) => {
+                    if let Some(id) = anchors.get(&alias) {
+                        Event::Alias(*id)
+                    } else {
                         document.error = Some(
                             error::new(ErrorImpl::UnknownAnchor(mark))
                                 .shared(),
                         );
                         return Some(document);
                     }
-                },
+                }
+
                 YamlEvent::Scalar(mut scalar) => {
+                    // NEW: Duplicate anchor check
                     if let Some(anchor) = scalar.anchor.take() {
+                        if anchors.contains_key(&anchor) {
+                            document.error = Some(
+                                error::new(ErrorImpl::DuplicateAnchor(
+                                    mark.to_string(),
+                                ))
+                                .shared(),
+                            );
+                            // If you want to end parsing entirely, uncomment:
+                            self.parser = None;
+                            return Some(document);
+                        }
                         let id = anchors.len();
                         document
                             .anchor_names
@@ -202,8 +218,20 @@ impl<'input> Loader<'input> {
                     }
                     Event::Scalar(scalar)
                 }
+
                 YamlEvent::SequenceStart(mut sequence_start) => {
+                    // NEW: Duplicate anchor check
                     if let Some(anchor) = sequence_start.anchor.take() {
+                        if anchors.contains_key(&anchor) {
+                            document.error = Some(
+                                error::new(ErrorImpl::DuplicateAnchor(
+                                    mark.to_string(),
+                                ))
+                                .shared(),
+                            );
+                            self.parser = None;
+                            return Some(document);
+                        }
                         let id = anchors.len();
                         document
                             .anchor_names
@@ -215,9 +243,22 @@ impl<'input> Loader<'input> {
                     }
                     Event::SequenceStart(sequence_start)
                 }
+
                 YamlEvent::SequenceEnd => Event::SequenceEnd,
+
                 YamlEvent::MappingStart(mut mapping_start) => {
+                    // NEW: Duplicate anchor check
                     if let Some(anchor) = mapping_start.anchor.take() {
+                        if anchors.contains_key(&anchor) {
+                            document.error = Some(
+                                error::new(ErrorImpl::DuplicateAnchor(
+                                    mark.to_string(),
+                                ))
+                                .shared(),
+                            );
+                            self.parser = None;
+                            return Some(document);
+                        }
                         let id = anchors.len();
                         document
                             .anchor_names
@@ -229,8 +270,10 @@ impl<'input> Loader<'input> {
                     }
                     Event::MappingStart(mapping_start)
                 }
+
                 YamlEvent::MappingEnd => Event::MappingEnd,
             };
+
             document.events.push((event, mark));
         }
     }

diff --git a/src/modules/error.rs b/src/modules/error.rs
@@ -32,17 +32,17 @@ pub struct Location {
 
 impl Location {
     /// Returns the byte index where the error occurred.
-    pub fn index(&self) -> usize {
+    pub const fn index(&self) -> usize {
         self.index
     }
 
     /// Returns the line number where the error occurred.
-    pub fn line(&self) -> usize {
+    pub const fn line(&self) -> usize {
         self.line
     }
 
     /// Returns the column number where the error occurred.
-    pub fn column(&self) -> usize {
+    pub const fn column(&self) -> usize {
         self.column
     }
 
@@ -73,10 +73,14 @@ pub type Result<T> = result::Result<T, Error>;
 /// including I/O errors, UTF-8 conversion errors, and errors originating from the `libyml` library.
 #[derive(Debug)]
 pub enum ErrorImpl {
+    /// An error indicating that an alias with the given name already exists.
+    DuplicateAnchor(String),
     /// A generic error message with an optional position.
     Message(String, Option<Pos>),
     /// An error originating from the `libyml` library.
     Libyml(libyml::Error),
+    /// An error indicating that the progress is invalid.
+    InvalidProgress,
     /// An I/O error.
     IoError(io::Error),
     /// An error encountered while converting a byte slice to a string using UTF-8 encoding.
@@ -114,8 +118,10 @@ pub enum ErrorImpl {
 impl Display for ErrorImpl {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self {
+            ErrorImpl::DuplicateAnchor(name) => write!(f, "Duplicate Anchor Error: An alias with the name '{}' already exists", name),
             ErrorImpl::Message(msg, _) => write!(f, "Error: {}", msg),
             ErrorImpl::Libyml(_) => write!(f, "Error: An error occurred in the Libyml library"),
+            ErrorImpl::InvalidProgress => write!(f, "Invalid Progress Error: The progress is invalid"),
             ErrorImpl::IoError(err) => write!(f, "I/O Error: {}", err),
             ErrorImpl::FromUtf8(err) => write!(f, "UTF-8 Conversion Error: {}", err),
             ErrorImpl::EndOfStream => write!(f, "Unexpected End of YAML Stream: The YAML stream ended unexpectedly while parsing a value"),
@@ -164,6 +170,19 @@ impl Error {
     }
 }
 
+impl std::ops::Deref for Error {
+    type Target = ErrorImpl;
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
+impl AsRef<ErrorImpl> for Error {
+    fn as_ref(&self) -> &ErrorImpl {
+        &self.0
+    }
+}
+
 /// Creates a new `Error` from the given `ErrorImpl`.
 pub fn new(inner: ErrorImpl) -> Error {
     Error(Box::new(inner))
@@ -263,6 +282,9 @@ impl ErrorImpl {
 
     fn message(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self {
+            ErrorImpl::DuplicateAnchor(name) => {
+                write!(f, "duplicate anchor '{}'", name)
+            }
             ErrorImpl::Message(description, None) => f.write_str(description),
             ErrorImpl::Message(description, Some(Pos { mark: _, path })) => {
                 if path != "." {
@@ -271,6 +293,7 @@ impl ErrorImpl {
                 f.write_str(description)
             }
             ErrorImpl::Libyml(_) => unreachable!(),
+            ErrorImpl::InvalidProgress => f.write_str("invalid progress"),
             ErrorImpl::IoError(err) => Display::fmt(err, f),
             ErrorImpl::FromUtf8(err) => Display::fmt(err, f),
             ErrorImpl::EndOfStream => f.write_str("EOF while parsing a value"),