Dsgallups/file parsing bug

.github/workflows/ci.yaml

@@ -46,6 +46,7 @@ jobs:
       - uses: dtolnay/rust-toolchain@stable
         with:
           toolchain: ${{ env.RUST_TOOLCHAIN }}
+          components: clippy
       - name: Setup Rust cache
         uses: Swatinem/rust-cache@v2
       - name: Install alsa and udev
@@ -55,16 +56,17 @@ jobs:
         uses: actions-rs/cargo@v1
         with:
           command: clippy
+          args: --no-default-features
       - name: Run cargo clippy (std)
         uses: actions-rs/cargo@v1
         with:
           command: clippy
-          args: --features std
+          args: --no-default-features --features std
       - name: Run cargo clippy (bevy)
         uses: actions-rs/cargo@v1
         with:
           command: clippy
-          args: --features bevy
+          args: --features "bevy bevy_asset tracing serde"
   test:
     strategy:
       matrix:
@@ -90,7 +92,7 @@ jobs:
       - name: Build & run unit and e2e tests (no features)
         run: cargo test --lib --tests --no-default-features
       - name: Build & run unit and e2e tests (all features)
-        run: cargo test --lib --tests --features all
+        run: cargo test --lib --tests --features "std bevy bevy_asset tracing serde"
   all-doc-tests:
     runs-on: ubuntu-latest
     steps:
@@ -110,4 +112,4 @@ jobs:
       - name: Install alsa and udev
         run: sudo apt-get update; sudo apt-get install --no-install-recommends libasound2-dev libudev-dev
       - name: Run doc tests with all features (this also compiles README examples)
-        run: cargo test --doc --features bevy
+        run: cargo test --doc

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "midix"
-version = "4.0.0-alpha.5"
+version = "4.0.0-alpha.7"
 authors = ["dsgallups <dsgallups@protonmail.com>"]
 edition = "2024"
 description = "MIDI structures designed for humans"
@@ -24,7 +24,7 @@ serde = ["dep:serde"]
 
 
 [dependencies.bevy]
-version = "0.17.0-rc"
+version = "0.17"
 optional = true
 default-features = false
 

src/error/mod.rs

@@ -24,7 +24,7 @@ use thiserror::Error;
 // }
 
 /// All the ways parsing can go wrong
-#[derive(Debug, Error)]
+#[derive(Debug, Error, PartialEq, Eq)]
 pub enum ParseError {
     /// Invalid databyte (leading 1)
     #[error("Invalid Data Byte: {0:0X}")]
@@ -81,7 +81,7 @@ impl ParseError {
     }
 }
 /// Problems reading a file's header
-#[derive(Debug, Error)]
+#[derive(Debug, Error, PartialEq, Eq)]
 pub enum HeaderError {
     /// Type 0 MIDI Format (SingleMultiChannel) defines multiple tracks
     #[error("Type 0 MIDI Format (SingleMultiChannel) defined multiple tracks")]
@@ -107,7 +107,7 @@ impl From<HeaderError> for ParseError {
 }
 
 /// Problems reading a file's metamessages
-#[derive(Debug, Error)]
+#[derive(Debug, Error, PartialEq, Eq)]
 pub enum MetaMessageError {
     /// contains varlen inner, should be 1.
     #[error("varlen for channel count was {0}. Expected 1.")]
@@ -129,7 +129,7 @@ impl From<MetaMessageError> for ParseError {
 }
 
 /// problems reading a track
-#[derive(Debug, Error)]
+#[derive(Debug, Error, PartialEq, Eq)]
 pub enum TrackError {
     /// Invalid event
     #[error("Invalid event found: {0:0X}")]
@@ -143,7 +143,7 @@ impl From<TrackError> for ParseError {
 }
 
 /// Problems reading a chunk
-#[derive(Debug, Error)]
+#[derive(Debug, Error, PartialEq, Eq)]
 pub enum ChunkError {
     /// Finding more than one header for a chunk
     #[error("Found more than one header for this chunk.")]
@@ -157,7 +157,7 @@ pub enum ChunkError {
 }
 
 /// Problems with the file after reading it through
-#[derive(Debug, Error)]
+#[derive(Debug, Error, PartialEq, Eq)]
 pub enum FileError {
     /// No format was found
     #[error("The file's format couldn't be determined")]

src/file/builder/event/mod.rs

@@ -19,7 +19,7 @@ This type is yielded by [`Reader::read_event`] and will be consumed by a Writer
 
 # Overview
 
-Except [`FileEvent::Eof`] Events can be placed into two categories
+Events can be placed into two categories:
 
 ## Chunk Events
 
@@ -71,9 +71,6 @@ pub enum FileEvent<'a> {
     ///
     /// See [`TrackEvent`] for a detailed breakdown
     TrackEvent(TrackEvent<'a>),
-
-    /// Yielded when no more bytes can be read
-    Eof,
 }
 
 impl From<RawHeaderChunk> for FileEvent<'_> {

src/file/builder/mod.rs

@@ -117,7 +117,7 @@ impl<'a> MidiFileBuilder<'a> {
                 self.unknown_chunks.push(data);
                 Ok(())
             }
-            Eof => Err(ReaderErrorKind::OutOfBounds),
+            Eof => Err(ReaderErrorKind::Eof),
         }
     }
     /// Attempts to finish the midifile from the provided chunks.

src/file/meta/smpte_offset.rs

@@ -90,9 +90,9 @@ impl SmpteOffset {
     /// # Returns
     /// The time offset in microseconds as a floating-point value
     pub const fn as_micros_with_override(&self, fps: SmpteFps) -> f64 {
-        ((((self.hour as u32 * 3600) + (self.minute as u32) * 60 + self.second as u32) * 1_000_000)
+        ((((self.hour as u64 * 3600) + (self.minute as u64) * 60 + self.second as u64) * 1_000_000)
             as f64)
-            + ((self.frame as u32) * 1_000_000) as f64 / fps.as_f64()
+            + ((self.frame as u64) * 1_000_000) as f64 / fps.as_f64()
             + ((self.subframe as u32) * 10_000) as f64 / fps.as_f64()
     }
     /// Convert this SMPTE offset to microseconds.
@@ -102,9 +102,9 @@ impl SmpteOffset {
     /// minutes, seconds, frames, and subframes to provide a precise
     /// microsecond value.
     pub const fn as_micros(&self) -> f64 {
-        ((((self.hour as u32 * 3600) + (self.minute as u32) * 60 + self.second as u32) * 1_000_000)
+        ((((self.hour as u64 * 3600) + (self.minute as u64) * 60 + self.second as u64) * 1_000_000)
             as f64)
-            + ((self.frame as u32) * 1_000_000) as f64 / self.fps.as_f64()
+            + ((self.frame as u64) * 1_000_000) as f64 / self.fps.as_f64()
             + ((self.subframe as u32) * 10_000) as f64 / self.fps.as_f64()
     }
 
@@ -145,7 +145,7 @@ impl SmpteOffset {
             v => return Err(SmpteError::TrackFrame(v)),
         };
         let hour = data[0] & 0b0001_1111;
-        if hour > 24 {
+        if hour > 23 {
             return Err(SmpteError::HourOffset(hour));
         }
         let minute = data[1];

src/message/channel/voice.rs

@@ -195,7 +195,7 @@ impl FromLiveEventBytes for ChannelVoiceMessage {
                 let mut temp = Reader::from_byte_slice(data);
                 let c = Controller::read(&mut temp).map_err(|e| match e.kind {
                     ReaderErrorKind::ParseError(p) => p,
-                    ReaderErrorKind::OutOfBounds => ParseError::MissingData,
+                    ReaderErrorKind::OutOfBounds | ReaderErrorKind::Eof => ParseError::MissingData,
                 })?;
                 VoiceEvent::ControlChange(c)
             }

src/micros.rs

@@ -6,7 +6,7 @@ pub trait DurationExt {
     fn to_micros(&self) -> UMicros;
 }
 
-impl DurationExt for std::time::Duration {
+impl DurationExt for core::time::Duration {
     fn to_micros(&self) -> UMicros {
         UMicros(self.as_micros() as u64)
     }

src/reader/error.rs

@@ -13,14 +13,17 @@ pub struct ReaderError {
 }
 
 /// A kind of error that a reader can produce
-#[derive(Debug, Error)]
+#[derive(Debug, Error, PartialEq, Eq)]
 pub enum ReaderErrorKind {
     /// Parsing errors
     #[error("Parsing {0}")]
     ParseError(#[from] ParseError),
-    /// Reading out of bounds.
+    /// Reading out of bounds. If the read is passed the length of the file, then [`ReaderErrorKind::Eof`] is returned.
     #[error("Read out of bounds!")]
     OutOfBounds,
+    /// The end of the file has been reached.
+    #[error("End of file read!")]
+    Eof,
 }
 
 impl ReaderErrorKind {
@@ -34,10 +37,11 @@ impl ReaderError {
     pub const fn new(position: usize, kind: ReaderErrorKind) -> Self {
         Self { position, kind }
     }
-    /// True if out of bounds or unexpected end of file
+    /// True if out of bounds
     pub const fn is_out_of_bounds(&self) -> bool {
         matches!(self.kind, ReaderErrorKind::OutOfBounds)
     }
+
     /// Returns the error kind of the reader.
     pub fn error_kind(&self) -> &ReaderErrorKind {
         &self.kind
@@ -54,14 +58,6 @@ impl ReaderError {
             kind: ReaderErrorKind::ParseError(error),
         }
     }
-
-    /// Create a new out of bounds error
-    pub const fn oob(position: usize) -> Self {
-        Self {
-            position,
-            kind: ReaderErrorKind::OutOfBounds,
-        }
-    }
 }
 
 /// The Read Result type (see [`ReaderError`])

src/reader/mod.rs

@@ -153,20 +153,28 @@ impl<'slc> Reader<Cow<'slc, [u8]>> {
 
 //internal implementations
 impl<'slc, R: MidiSource<'slc>> Reader<R> {
-    // Returns None if there's no bytes left to read
+    /// Reads the exact amount of bytes given the length.
+    ///
+    /// Errors with OutOfBounds if the result is beyond the total length of the source.
     pub(super) fn read_exact<'slf>(&'slf mut self, bytes: usize) -> ReadResult<Cow<'slc, [u8]>>
     where
         'slc: 'slf,
     {
         if self.buffer_position() > self.reader.max_len() {
-            return Err(ReaderError::oob(self.buffer_position()));
+            return Err(ReaderError::new(
+                self.buffer_position(),
+                ReaderErrorKind::OutOfBounds,
+            ));
         }
         let start = self.buffer_position();
 
         let end = start + bytes;
 
         if end > self.reader.max_len() {
-            return Err(ReaderError::oob(self.buffer_position()));
+            return Err(ReaderError::new(
+                self.buffer_position(),
+                ReaderErrorKind::OutOfBounds,
+            ));
         }
 
         self.state.increment_offset(bytes);
@@ -185,7 +193,10 @@ impl<'slc, R: MidiSource<'slc>> Reader<R> {
             let me = unsafe { &*ptr };
             Ok(*me)
         } else {
-            Err(ReaderError::oob(self.buffer_position()))
+            Err(ReaderError::new(
+                self.buffer_position(),
+                ReaderErrorKind::OutOfBounds,
+            ))
         }
     }
 
@@ -196,7 +207,10 @@ impl<'slc, R: MidiSource<'slc>> Reader<R> {
         let res = self
             .reader
             .get_byte(self.buffer_position())
-            .ok_or(ReaderError::oob(self.buffer_position()))?;
+            .ok_or(ReaderError::new(
+                self.buffer_position(),
+                ReaderErrorKind::OutOfBounds,
+            ))?;
         self.state.increment_offset(1);
 
         Ok(res)
@@ -209,7 +223,10 @@ impl<'slc, R: MidiSource<'slc>> Reader<R> {
         let res = self
             .reader
             .get_byte(self.buffer_position())
-            .ok_or(ReaderError::oob(self.buffer_position()))?;
+            .ok_or(ReaderError::new(
+                self.buffer_position(),
+                ReaderErrorKind::OutOfBounds,
+            ))?;
         self.state.increment_offset(1);
         DataByte::new(res).map_err(|e| ReaderError::parse_error(self.buffer_position(), e))
     }
@@ -274,14 +291,15 @@ impl<'slc, R: MidiSource<'slc>> Reader<R> {
                 ParseState::InsideMidi => {
                     // expect only a header or track chunk
                     let chunk = match self.read_exact(4) {
-                        Ok(c) => c,
-                        Err(e) => {
-                            if e.is_out_of_bounds() {
-                                return Ok(FileEvent::Eof);
-                            } else {
+                        Ok(chunk) => chunk,
+                        Err(e) => match e.error_kind() {
+                            ReaderErrorKind::OutOfBounds => {
+                                return Err(ReaderError::new(e.position(), ReaderErrorKind::Eof));
+                            }
+                            _ => {
                                 return Err(e);
                             }
-                        }
+                        },
                     };
 
                     match chunk.as_ref() {
@@ -322,7 +340,12 @@ impl<'slc, R: MidiSource<'slc>> Reader<R> {
                     let ev = TrackEvent::read(self, &mut running_status)?;
                     break FileEvent::TrackEvent(ev);
                 }
-                ParseState::Done => break FileEvent::Eof,
+                ParseState::Done => {
+                    return Err(ReaderError::new(
+                        self.buffer_position(),
+                        ReaderErrorKind::Eof,
+                    ));
+                }
             }
         };
 

tests/read_all.rs

@@ -1,16 +1,13 @@
-use midix::{file::builder::event::FileEvent, reader::Reader};
+use midix::reader::{Reader, ReaderErrorKind};
 
 fn loop_through(bytes: &[u8]) {
     let mut reader = Reader::from_byte_slice(bytes);
 
     loop {
-        match reader.read_event() {
-            Ok(e) => {
-                if e == FileEvent::Eof {
-                    break;
-                }
-            }
-            Err(e) => {
+        if let Err(e) = reader.read_event() {
+            if matches!(e.error_kind(), ReaderErrorKind::Eof) {
+                break;
+            } else {
                 panic!("Error at {}, {:?}", reader.buffer_position(), e);
             }
         }
@@ -47,9 +44,13 @@ fn read_pi_damaged() {
     let bytes = include_bytes!("../test-asset/PiDamaged.mid");
     let mut reader = Reader::from_byte_slice(bytes);
 
-    while let Ok(e) = reader.read_event() {
-        if e == FileEvent::Eof {
-            panic!("Corrupted file should not have yielded an eof event")
+    loop {
+        if let Err(e) = reader.read_event() {
+            if matches!(e.error_kind(), ReaderErrorKind::Eof) {
+                panic!("Corrupted file should not have yielded an eof event")
+            } else {
+                return;
+            }
         }
     }
 }

tests/smpte_offset.rs

@@ -355,7 +355,7 @@ fn test_multiple_tracks_with_different_offsets() {
     let mut offsets = Vec::new();
 
     while let Ok(event) = reader.read_event() {
-        println!("ok");
+        println!("ok: {event:?}");
         if let FileEvent::TrackEvent(track_event) = event
             && let TrackMessage::Meta(MetaMessage::SmpteOffset(offset)) = track_event.event()
         {