diff --git a/src/libyml/safe_cstr.rs b/src/libyml/safe_cstr.rs index 89772e5b..f983a266 100644 --- 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 index d9dd4b8c..8e5f4107 100644 --- 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 index 0a31dbe1..ff35f4fd 100644 --- a/src/libyml/util.rs +++ b/src/libyml/util.rs @@ -19,6 +19,7 @@ impl Owned { /// # Safety /// The created instance contains uninitialized memory, and should be properly /// initialized before use. + #[must_use] pub fn new_uninit() -> Owned, T> { // Allocate memory for `T` but leave it uninitialized. let boxed = Box::new(MaybeUninit::::uninit()); @@ -35,12 +36,13 @@ impl Owned { /// /// # 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, T>, ) -> Owned { 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 index 617f026b..545e48b5 100644 --- 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 index d6aa1bbd..55d2c523 100644 --- 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 = result::Result; /// 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), /// 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 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"), diff --git a/tests/test_loader.rs b/tests/test_loader.rs index 17699460..733e03cb 100644 --- a/tests/test_loader.rs +++ b/tests/test_loader.rs @@ -426,4 +426,133 @@ mod tests { assert!(document.error.is_none()); assert_eq!(document.anchor_event_map.len(), 0); } + + #[cfg(test)] + mod missing_coverage_tests { + use serde_yml::{ + de::Progress, + loader::{Document, Loader}, + modules::error::ErrorImpl, + }; + + /// Ensures that constructing a `Loader` with `Progress::Iterable` returns `InvalidProgress`. + #[test] + fn test_loader_invalid_progress_iterable() { + // Create a dummy Loader that we'll wrap in `Progress::Iterable`. + // The actual internals won't matter since we expect an immediate error. + let dummy_loader = Loader { + parser: None, + parsed_document_count: 0, + }; + + // Wrapping this in `Progress::Iterable` should trigger `InvalidProgress`. + let progress = Progress::Iterable(dummy_loader); + let loader_result = Loader::new(progress); + + assert!( + loader_result.is_err(), + "Expected an error (InvalidProgress) for Progress::Iterable" + ); + + if let Err(e) = loader_result { + match e.as_ref() { + ErrorImpl::InvalidProgress => { + // This is the exact variant we expect. + } + other => panic!( + "Expected ErrorImpl::InvalidProgress, got: {:?}", + other + ), + } + } else { + panic!("Expected Err(...), got Ok(...) instead."); + } + } + + /// Ensures that constructing a `Loader` with `Progress::Document` returns `InvalidProgress`. + #[test] + fn test_loader_invalid_progress_document() { + // Create a dummy Document that we’ll wrap in `Progress::Document`. + // Again, we expect an immediate error without parsing. + let dummy_document = Document { + events: vec![], + error: None, + anchor_event_map: Default::default(), + anchor_names: Default::default(), + }; + + // Wrapping this in `Progress::Document` should trigger `InvalidProgress`. + let progress = Progress::Document(dummy_document); + let loader_result = Loader::new(progress); + + assert!( + loader_result.is_err(), + "Expected an error (InvalidProgress) for Progress::Document" + ); + + if let Err(e) = loader_result { + match &*e { + ErrorImpl::InvalidProgress => { + // This is the exact variant we expect. + } + other => panic!( + "Expected ErrorImpl::InvalidProgress, got: {:?}", + other + ), + } + } else { + panic!("Expected Err(...), got Ok(...) instead."); + } + } + + /// Tests handling of a `DuplicateAnchor` error. + /// Creates YAML with two identical anchors to trigger the error. + #[test] + fn test_loader_duplicate_anchor() { + let yaml = r#"--- +key1: &dup_anchor something +key2: &dup_anchor something_else +"#; + let progress = Progress::Str(yaml); + let mut loader = + Loader::new(progress).expect("Loader creation failed"); + + let document = loader + .next_document() + .expect("Expected a document, even if erroneous"); + + // The presence of an error means the first anchor was fine, + // but the second identical anchor triggered `DuplicateAnchor`. + assert!( + document.error.is_some(), + "Expected a DuplicateAnchor error in the document" + ); + + if let Some(err_arc) = document.error { + match &*err_arc { + ErrorImpl::DuplicateAnchor(msg) => { + // Confirm we got the correct variant + assert!( + msg.contains("line"), + "DuplicateAnchor error message usually includes position info" + ); + } + other => panic!( + "Expected DuplicateAnchor error, got: {:?}", + other + ), + } + } else { + panic!("Document.error was None, expected DuplicateAnchor error"); + } + + // Because of the fatal anchor error, the loader’s parser is set to None, + // so subsequent calls to next_document() should yield None. + let next_doc = loader.next_document(); + assert!( + next_doc.is_none(), + "Expected the loader to stop parsing after DuplicateAnchor" + ); + } + } }