Replaced frame resizing with a simple check

src/errors.rs

@@ -1,12 +1,13 @@
 use crate::c_abi;
 use crate::FileMode;
+use crate::Frame;
 use std::path::{Path, PathBuf};
 
-#[derive(Debug, Clone, PartialEq)]
+#[derive(Debug, Clone, Copy, PartialEq)]
 /// The task being attempted when an error was returned
 pub enum ErrorTask {
     /// A file was being opened
-    OpenFile(PathBuf, FileMode),
+    OpenFile,
     /// The number of atoms was being read from a file
     ReadNumAtoms,
     /// A frame was being read from a file
@@ -16,13 +17,30 @@ pub enum ErrorTask {
     /// An file was being flushed to disk
     Flush,
     /// A path was being converted to a CString
-    ToCString(Option<std::ffi::NulError>),
+    ToCString,
+    /// Unknown task
+    UnknownTask,
+}
+
+impl std::fmt::Display for ErrorTask {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        use ErrorTask::*;
+        match &self {
+            OpenFile => write!(f, "Failed to open file as trajectory"),
+            ReadNumAtoms => write!(f, "Failed to read atom number from trajectory"),
+            Read => write!(f, "Failed to read trajectory"),
+            Write => write!(f, "Failed to write trajectory"),
+            Flush => write!(f, "Failed to flush trajectory"),
+            ToCString => write!(f, "Failed to convert to CString"),
+            UnknownTask => write!(f, "Task failed"),
+        }
+    }
 }
 
 #[derive(Debug, Clone, PartialEq)]
 /// Error type for the xdrfile library
 pub struct Error {
-    code: ErrorCode,
+    kind: ErrorKind,
     task: ErrorTask,
     source: Option<Box<Error>>,
 }
@@ -33,126 +51,148 @@ impl Error {
         &self.task
     }
 
+    /// Get the task being attempted when the error was returned
+    pub fn kind(&self) -> &ErrorKind {
+        &self.kind
+    }
+
     /// Get the error code returned by the C API, if any
-    pub fn code(&self) -> &ErrorCode {
+    pub fn code(&self) -> Option<ErrorCode> {
-        &self.code
+        if let ErrorKind::ErrorCode(code) = self.kind {
+            Some(code)
+        } else {
+            None
+        }
     }
 
     /// True if the error is an end of file error, false otherwise
     pub fn is_eof(&self) -> bool {
-        self.code.is_eof()
+        use ErrorKind::*;
-    }
+        match self.kind {
-
+            ErrorCode(code) => code.is_eof(),
-    /// Construct a new error during the ToCString task
+            _ => false,
-    pub(crate) fn from_convert() -> Self {
-        Self {
-            code: ErrorCode::NoCode,
-            task: ErrorTask::ToCString(None),
-            source: None,
         }
     }
 
-    /// Construct a new error during the OpenFile task
+    pub fn with_task(self, task: ErrorTask) -> Self {
-    pub(crate) fn from_open(path: impl AsRef<Path>, mode: FileMode) -> Self {
         Self {
-            code: ErrorCode::NoCode,
+            kind: self.kind.clone(),
-            task: ErrorTask::OpenFile(path.as_ref().into(), mode),
-            source: None,
-        }
-    }
-
-    /// Construct a new error during the ReadNumAtoms task from a C error code
-    pub(crate) fn from_read_num_atoms(code: impl Into<ErrorCode>) -> Self {
-        Self {
-            code: code.into(),
-            task: ErrorTask::ReadNumAtoms,
-            source: None,
-        }
-    }
-
-    /// Construct a new error during the Read task from a C error code
-    pub(crate) fn from_read(code: impl Into<ErrorCode>) -> Self {
-        Self {
-            code: code.into(),
-            task: ErrorTask::Read,
-            source: None,
-        }
-    }
-
-    /// Construct a new error during the Write task from a C error code
-    pub(crate) fn from_write(code: impl Into<ErrorCode>) -> Self {
-        Self {
-            code: code.into(),
-            task: ErrorTask::Write,
-            source: None,
-        }
-    }
-
-    /// Construct a new error during the Flush task from a C error code
-    pub(crate) fn from_flush(code: impl Into<ErrorCode>) -> Self {
-        Self {
-            code: code.into(),
-            task: ErrorTask::Flush,
-            source: None,
-        }
-    }
-
-    /// Construct a new error during the ReadNumAtoms task from a C error code
-    pub(crate) fn with_task(self, task: ErrorTask) -> Self {
-        Self {
-            code: self.code,
             task,
             source: Some(Box::new(self)),
         }
     }
+
+    /// Convert an error code and output value from a C call to a Result
+    ///
+    /// `code` should be an integer return code returned from the C API. `value` should be the
+    /// function's output, which is generally either `()` or one of its arguments. If `code`
+    /// indicates the function returned successfully, the value is returned; otherwise, the
+    /// code is converted into the appropriate `Error`.
+    pub fn check_code<T>(code: impl Into<ErrorCode>, value: T, task: ErrorTask) -> Result<T, Self> {
+        let code: ErrorCode = code.into();
+        if let ErrorCode::ExdrOk = code {
+            Ok(value)
+        } else {
+            Err(Self {
+                kind: ErrorKind::from(code),
+                task,
+                source: None,
+            })
+        }
+    }
 }
 
-impl From<std::ffi::NulError> for Error {
+impl<K: Into<ErrorKind>> From<K> for Error {
-    fn from(err: std::ffi::NulError) -> Self {
+    fn from(kind: K) -> Self {
         Self {
-            code: ErrorCode::NoCode,
+            task: ErrorTask::UnknownTask,
-            task: ErrorTask::ToCString(Some(err)),
+            kind: kind.into(),
             source: None,
         }
     }
 }
 
+impl From<std::ffi::NulError> for ErrorKind {
+    fn from(err: std::ffi::NulError) -> Self {
+        Self::NullInStr(err)
+    }
+}
+
+impl From<(&Path, FileMode)> for ErrorKind {
+    fn from(value: (&Path, FileMode)) -> Self {
+        let (path, mode) = value;
+        ErrorKind::CouldNotOpen {
+            path: path.to_owned(),
+            mode,
+        }
+    }
+}
+
+impl From<(&Frame, usize)> for ErrorKind {
+    fn from(value: (&Frame, usize)) -> Self {
+        let (frame, num_atoms) = value;
+        ErrorKind::WrongSizeFrame {
+            expected: num_atoms,
+            found: frame.coords.len(),
+        }
+    }
+}
+
+impl From<ErrorCode> for ErrorKind {
+    fn from(code: ErrorCode) -> Self {
+        ErrorKind::ErrorCode(code)
+    }
+}
+
 impl std::fmt::Display for Error {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        use ErrorTask::*;
+        write!(f, "{task}: {kind}", task = self.task, kind = self.kind)
-        match &self.task {
-            OpenFile(path, mode) => write!(
-                f,
-                "Failed to open file at {path:?} with mode {mode:?}",
-                path = path,
-                mode = mode
-            ),
-            ReadNumAtoms => write!(f, "Failed to read atom number from trajectory"),
-            Read => write!(f, "Failed to read trajectory"),
-            Write => write!(f, "Failed to write trajectory"),
-            Flush => write!(f, "Failed to flush trajectory"),
-            ToCString(None) => write!(
-                f,
-                "Path cannot be converted to a C string because it was invalid Unicode"
-            ),
-            ToCString(Some(_)) => write!(
-                f,
-                "Path cannot be converted to a C string because it has a null byte"
-            ),
-        }
     }
 }
 
 impl std::error::Error for Error {
     fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
-        if let Some(source) = &self.source {
+        if let Some(err) = &self.source {
-            Some(source.as_ref())
+            Some(err)
-        } else if self.code != ErrorCode::NoCode {
-            Some(&self.code)
-        } else if let ErrorTask::ToCString(Some(e)) = &self.task {
-            Some(e)
         } else {
-            None
+            use ErrorKind::*;
+            match &self.kind {
+                NullInStr(err) => Some(err),
+                _ => None,
+            }
+        }
+    }
+}
+
+#[derive(Debug, Clone, PartialEq)]
+pub enum ErrorKind {
+    /// An error code from the C API
+    ErrorCode(ErrorCode),
+    /// Passed in a frame of the wrong size
+    WrongSizeFrame { expected: usize, found: usize },
+    /// C API failed to open a file (No return code provided)
+    CouldNotOpen { path: PathBuf, mode: FileMode },
+    /// &str could not be converted to &OsStr, probably because it is invalid unicode
+    InvalidOsStr,
+    /// &OsStr could not be converted to &CStr because it had a null byte
+    NullInStr(std::ffi::NulError),
+}
+
+impl std::fmt::Display for ErrorKind {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        use ErrorKind::*;
+        match &self {
+            ErrorCode(code) => code.fmt(f),
+            WrongSizeFrame { expected, found } => write!(
+                f,
+                "Expected frame of size {:?}, found {:?}",
+                expected, found
+            ),
+            CouldNotOpen { path, mode } => {
+                write!(f, "Could not open file at {:?} in mode {:?}", path, mode)
+            }
+            InvalidOsStr => write!(f, "CStr must be valid unicode on this platform"),
+            NullInStr(_err) => write!(f, "CStr cannot include null bytes"),
         }
     }
 }
@@ -190,8 +230,6 @@ pub enum ErrorCode {
     ExdrNr,
     /// Something unexpected happened
     UnmatchedCode(c_abi::xdrfile::BindgenTy1),
-    /// C returned no code at all
-    NoCode,
 }
 
 impl ErrorCode {
@@ -202,21 +240,6 @@ impl ErrorCode {
             _ => false,
         }
     }
-
-    /// Convert an error code and output value from a C call to a Result
-    ///
-    /// `code` should be an integer return code returned from the C API. `value` should be the
-    /// function's output, which is generally either `()` or one of its arguments. If `code`
-    /// indicates the function returned successfully, the value is returned; otherwise, the
-    /// code is converted into the appropriate `ErrorCode`.
-    pub fn check<T>(code: impl Into<Self>, value: T) -> std::result::Result<T, Self> {
-        let code = code.into();
-        if let Self::ExdrOk = code {
-            Ok(value)
-        } else {
-            Err(code)
-        }
-    }
 }
 
 impl From<c_abi::xdrfile::BindgenTy1> for ErrorCode {
@@ -251,8 +274,6 @@ impl std::fmt::Display for ErrorCode {
     }
 }
 
-impl std::error::Error for ErrorCode {}
-
 /// `Result` type for errors in the `xdrfile` crate
 pub type Result<T, E = Error> = std::result::Result<T, E>;
 
@@ -262,44 +283,48 @@ mod tests {
 
     #[test]
     fn test_is_eof() {
+        use ErrorKind::ErrorCode as Code;
         let error = Error {
-            code: c_abi::xdrfile::exdrENDOFFILE.into(),
+            kind: Code(c_abi::xdrfile::exdrENDOFFILE.into()),
             task: ErrorTask::Read,
             source: None,
         };
         assert!(error.is_eof());
 
         let error = Error {
-            code: ErrorCode::ExdrEndOfFile,
+            kind: Code(ErrorCode::ExdrEndOfFile),
-            task: ErrorTask::Read,
+            task: ErrorTask::ReadNumAtoms,
             source: None,
         };
         assert!(error.is_eof());
 
         let error = Error {
-            code: (c_abi::xdrfile::exdrENDOFFILE + 1).into(),
+            kind: Code((c_abi::xdrfile::exdrENDOFFILE + 1).into()),
             task: ErrorTask::Read,
             source: None,
         };
         assert!(!error.is_eof());
 
         let error = Error {
-            code: 0.into(),
+            kind: Code(0.into()),
             task: ErrorTask::Write,
             source: None,
         };
         assert!(!error.is_eof());
 
         let error = Error {
-            code: 255.into(),
+            kind: Code(255.into()),
             task: ErrorTask::Flush,
             source: None,
         };
         assert!(!error.is_eof());
 
         let error = Error {
-            code: ErrorCode::NoCode,
+            kind: ErrorKind::CouldNotOpen {
-            task: ErrorTask::OpenFile(PathBuf::from("not/a/file"), FileMode::Read),
+                path: PathBuf::from("not/a/file"),
+                mode: FileMode::Read,
+            },
+            task: ErrorTask::OpenFile,
             source: None,
         };
         assert!(!error.is_eof());

src/frame.rs

@@ -1,7 +1,7 @@
 use std::fmt;
 
 /// A frame represents a single step in a trajectory.
-#[derive(Clone, PartialEq)]
+#[derive(Clone)]
 pub struct Frame {
     /// Number of atoms in the frame
     pub num_atoms: u32,
@@ -13,10 +13,10 @@ pub struct Frame {
     pub time: f32,
 
     /// 3x3 box vector
-    pub box_vector: [[f32; 3]; 3],
+    pub box_vector: [[f32; 3usize]; 3usize],
 
     /// 3D coordinates for N atoms where N is num_atoms
-    pub coords: Vec<[f32; 3]>,
+    pub coords: Vec<[f32; 3usize]>,
 }
 
 impl Default for Frame {
@@ -76,12 +76,6 @@ impl Frame {
     pub fn len(self: &Frame) -> usize {
         self.num_atoms as usize
     }
-
-    /// Resizes the Frame in-place so that num_atoms is equal to new_size.
-    pub fn resize(&mut self, new_size: u32) {
-        self.num_atoms = new_size;
-        self.coords.resize(new_size as usize, [0.0; 3]);
-    }
 }
 
 #[cfg(test)]

src/lib.rs

@@ -101,8 +101,13 @@ impl FileMode {
 }
 
 fn path_to_cstring(path: impl AsRef<Path>) -> Result<CString> {
-    let s = path.as_ref().to_str().ok_or_else(Error::from_convert)?;
+    use ErrorKind::InvalidOsStr;
-    Ok(CString::new(s)?)
+    use ErrorTask::ToCString;
+    let s = path
+        .as_ref()
+        .to_str()
+        .ok_or_else(|| Error::from(InvalidOsStr).with_task(ToCString))?;
+    CString::new(s).map_err(|e| Error::from(e).with_task(ToCString))
 }
 
 /// A safe wrapper around the c implementation of an XDRFile
@@ -135,7 +140,7 @@ impl XDRFile {
                 })
             } else {
                 // Something went wrong. But the C api does not tell us what
-                Err(Error::from_open(path, filemode))
+                Err(Error::from((path, filemode)).with_task(ErrorTask::OpenFile))
             }
         }
     }
@@ -201,25 +206,29 @@ impl XTCTrajectory {
 impl Trajectory for XTCTrajectory {
     fn read(&mut self, frame: &mut Frame) -> Result<()> {
         let mut step: i32 = 0;
-        frame.resize(
+
-            self.get_num_atoms()
+        let num_atoms = self
-                .map_err(|e| e.with_task(ErrorTask::Read))?,
+            .get_num_atoms()
-        );
+            .map_err(|e| e.with_task(ErrorTask::Read))? as usize;
+        if num_atoms != frame.coords.len() {
+            return Err(Error::from((&*frame, num_atoms)).with_task(ErrorTask::Read));
+        }
+
         unsafe {
             // C lib requires an i32 to be passed, but step is exposed it as u32
             // (A step cannot be negative, can it?). So we need to create a step
             // variable to pass to read_xtc and cast it afterwards to u32
             let code = xdrfile_xtc::read_xtc(
                 self.handle.xdrfile,
-                frame.num_atoms as i32,
+                num_atoms as i32,
                 &mut step,
                 &mut frame.time,
                 &mut frame.box_vector,
-                frame.coords.as_ptr() as *mut [f32; 3],
+                frame.coords.as_mut_ptr(),
                 &mut self.precision.get(),
             ) as u32;
             frame.step = step as u32;
-            ErrorCode::check(code, ()).map_err(Error::from_read)
+            Error::check_code(code, (), ErrorTask::Read)
         }
     }
 
@@ -234,14 +243,14 @@ impl Trajectory for XTCTrajectory {
                 frame.coords[..].as_ptr() as *mut [f32; 3],
                 1000.0,
             ) as u32;
-            ErrorCode::check(code, ()).map_err(Error::from_write)
+            Error::check_code(code, (), ErrorTask::Write)
         }
     }
 
     fn flush(&mut self) -> Result<()> {
         unsafe {
             let code = xdr_seek::xdr_flush(self.handle.xdrfile) as u32;
-            ErrorCode::check(code, ()).map_err(Error::from_flush)
+            Error::check_code(code, (), ErrorTask::Flush)
         }
     }
 
@@ -258,7 +267,7 @@ impl Trajectory for XTCTrajectory {
                     // Reconstitute the CString so it is deallocated correctly
                     let _ = CString::from_raw(path_p);
 
-                    ErrorCode::check(code, num_atoms as u32).map_err(Error::from_read_num_atoms)
+                    Error::check_code(code, num_atoms as u32, ErrorTask::ReadNumAtoms)
                 }
             })
             .clone()
@@ -300,10 +309,14 @@ impl Trajectory for TRRTrajectory {
     fn read(&mut self, frame: &mut Frame) -> Result<()> {
         let mut step: i32 = 0;
         let mut lambda: f32 = 0.0;
-        frame.resize(
+
-            self.get_num_atoms()
+        let num_atoms = self
-                .map_err(|e| e.with_task(ErrorTask::Read))?,
+            .get_num_atoms()
-        );
+            .map_err(|e| e.with_task(ErrorTask::Read))? as usize;
+        if num_atoms != frame.coords.len() {
+            return Err(Error::from((&*frame, num_atoms)).with_task(ErrorTask::Read));
+        }
+
         unsafe {
             // C lib requires an i32 to be passed, but step is exposed it as u32
             // (A step cannot be negative, can it?). So we need to create a step
@@ -311,17 +324,17 @@ impl Trajectory for TRRTrajectory {
             // Similar for lambda.
             let code = xdrfile_trr::read_trr(
                 self.handle.xdrfile,
-                frame.num_atoms as i32,
+                num_atoms as i32,
                 &mut step,
                 &mut frame.time,
                 &mut lambda,
                 &mut frame.box_vector,
-                frame.coords.as_ptr() as *mut [f32; 3],
+                frame.coords.as_mut_ptr(),
                 std::ptr::null_mut(),
                 std::ptr::null_mut(),
             ) as u32;
             frame.step = step as u32;
-            ErrorCode::check(code, ()).map_err(Error::from_read)
+            Error::check_code(code, (), ErrorTask::Read)
         }
     }
 
@@ -338,14 +351,14 @@ impl Trajectory for TRRTrajectory {
                 std::ptr::null_mut(),
                 std::ptr::null_mut(),
             ) as u32;
-            ErrorCode::check(code, ()).map_err(Error::from_write)
+            Error::check_code(code, (), ErrorTask::Write)
         }
     }
 
     fn flush(&mut self) -> Result<()> {
         unsafe {
             let code = xdr_seek::xdr_flush(self.handle.xdrfile) as u32;
-            ErrorCode::check(code, ()).map_err(Error::from_flush)
+            Error::check_code(code, (), ErrorTask::Flush)
         }
     }
 
@@ -361,7 +374,7 @@ impl Trajectory for TRRTrajectory {
                     // Reconstitute the CString so it is deallocated correctly
                     let _ = CString::from_raw(path_p);
 
-                    ErrorCode::check(code, num_atoms as u32).map_err(Error::from_read_num_atoms)
+                    Error::check_code(code, num_atoms as u32, ErrorTask::ReadNumAtoms)
                 }
             })
             .clone()
@@ -456,10 +469,9 @@ mod tests {
 
     #[test]
     pub fn test_manual_loop() -> Result<(), Box<dyn std::error::Error>> {
-        let mut frame = Frame::new();
-
         let mut xtc_frames = Vec::new();
         let mut xtc_traj = XTCTrajectory::open_read("tests/1l2y.xtc")?;
+        let mut frame = Frame::with_capacity(xtc_traj.get_num_atoms()?);
 
         while let Ok(()) = xtc_traj.read(&mut frame) {
             xtc_frames.push(frame.clone());
@@ -486,27 +498,38 @@ mod tests {
         Ok(())
     }
 
+    #[test]
+    pub fn test_wrong_size_frame() -> Result<(), Box<dyn std::error::Error>> {
+        let mut xtc_traj = XTCTrajectory::open_read("tests/1l2y.xtc")?;
+        let mut frame = Frame::new();
+
+        let result = xtc_traj.read(&mut frame);
+        if let Err(e) = result {
+            assert_eq!(e.task(), &ErrorTask::Read);
+            assert!(if let ErrorKind::WrongSizeFrame { .. } = e.kind() {
+                true
+            } else {
+                false
+            });
+        } else {
+            panic!("A read with an incorrectly sized frame should not succeed")
+        }
+        Ok(())
+    }
+
     #[test]
     fn test_path_to_cstring() -> Result<(), Box<dyn std::error::Error>> {
         let result_invalid = path_to_cstring(PathBuf::from("invalid/\0path"));
 
-        assert_eq!(
+        if let Err(err) = result_invalid {
-            result_invalid,
+            assert!(err.task() == &ErrorTask::ToCString);
-            CString::new("invalid/\0path").map_err(Error::from)
-        );
-        assert!(!result_invalid.is_ok());
-        if let Err(e) = result_invalid {
-            match e.task() {
-                ErrorTask::ToCString(_) => (),
-                _ => panic!("path_to_cstring's errortask should be ErrorTask::ToCString(_)"),
-            }
         } else {
-            panic!("path_to_cstring on a NULL-containing string should return an error");
+            panic!("path_to_cstring should return Err if there are null bytes");
         }
 
         let result_valid = path_to_cstring("valid/path");
-
         assert_eq!(result_valid, Ok(CString::new("valid/path")?));
+
         Ok(())
     }
 
@@ -516,12 +539,18 @@ mod tests {
 
         let path = Path::new(&file_name);
         if let Err(e) = XDRFile::open(file_name, FileMode::Read) {
-            match e.task() {
+            if let (
-                ErrorTask::OpenFile(err_path, err_mode) => {
+                ErrorTask::OpenFile,
-                    assert_eq!(path, err_path);
+                ErrorKind::CouldNotOpen {
-                    assert_eq!(FileMode::Read, *err_mode)
+                    path: err_path,
-                }
+                    mode: err_mode,
-                _ => panic!("Wrong Error type"),
+                },
+            ) = (e.task(), e.kind())
+            {
+                assert_eq!(path, err_path);
+                assert_eq!(FileMode::Read, *err_mode)
+            } else {
+                panic!("Wrong Error type")
             }
         }
     }
@@ -533,7 +562,7 @@ mod tests {
         if let Err(e) = trr.get_num_atoms() {
             match e.task() {
                 ErrorTask::ReadNumAtoms => {
-                    assert_eq!(ErrorCode::ExdrMagic, *e.code());
+                    assert_eq!(Some(ErrorCode::ExdrMagic), e.code());
                 }
                 _ => panic!("Wrong Error type"),
             }
@@ -549,7 +578,7 @@ mod tests {
         if let Err(e) = trr.read(&mut frame) {
             match e.task() {
                 ErrorTask::Read => {
-                    assert_eq!(ErrorCode::ExdrMagic, *e.code());
+                    assert_eq!(Some(ErrorCode::ExdrMagic), e.code());
                 }
                 _ => panic!("Wrong Error type"),
             }