diff --git a/src/errors.rs b/src/errors.rs index f803a0f..2115bb3 100644 --- a/src/errors.rs +++ b/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), + 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>, } @@ -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 { - &self.code + pub fn code(&self) -> Option { + 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() - } - - /// Construct a new error during the ToCString task - pub(crate) fn from_convert() -> Self { - Self { - code: ErrorCode::NoCode, - task: ErrorTask::ToCString(None), - source: None, + use ErrorKind::*; + match self.kind { + ErrorCode(code) => code.is_eof(), + _ => false, } } - /// Construct a new error during the OpenFile task - pub(crate) fn from_open(path: impl AsRef, mode: FileMode) -> Self { + pub fn with_task(self, task: ErrorTask) -> Self { Self { - code: ErrorCode::NoCode, - 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) -> 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) -> 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) -> 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) -> 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, + kind: self.kind.clone(), 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(code: impl Into, value: T, task: ErrorTask) -> Result { + let code: ErrorCode = code.into(); + if let ErrorCode::ExdrOk = code { + Ok(value) + } else { + Err(Self { + kind: ErrorKind::from(code), + task, + source: None, + }) + } + } } -impl From for Error { - fn from(err: std::ffi::NulError) -> Self { +impl> From for Error { + fn from(kind: K) -> Self { Self { - code: ErrorCode::NoCode, - task: ErrorTask::ToCString(Some(err)), + task: ErrorTask::UnknownTask, + kind: kind.into(), source: None, } } } +impl From 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 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::*; - 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" - ), - } + write!(f, "{task}: {kind}", task = self.task, kind = self.kind) } } impl std::error::Error for Error { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - if let Some(source) = &self.source { - Some(source.as_ref()) - } else if self.code != ErrorCode::NoCode { - Some(&self.code) - } else if let ErrorTask::ToCString(Some(e)) = &self.task { - Some(e) + if let Some(err) = &self.source { + Some(err) } 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(code: impl Into, value: T) -> std::result::Result { - let code = code.into(); - if let Self::ExdrOk = code { - Ok(value) - } else { - Err(code) - } - } } impl From 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 = std::result::Result; @@ -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, - task: ErrorTask::Read, + kind: Code(ErrorCode::ExdrEndOfFile), + 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, - task: ErrorTask::OpenFile(PathBuf::from("not/a/file"), FileMode::Read), + kind: ErrorKind::CouldNotOpen { + path: PathBuf::from("not/a/file"), + mode: FileMode::Read, + }, + task: ErrorTask::OpenFile, source: None, }; assert!(!error.is_eof()); diff --git a/src/frame.rs b/src/frame.rs index d8e2650..805f8e7 100644 --- a/src/frame.rs +++ b/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)] diff --git a/src/lib.rs b/src/lib.rs index 9502d43..2db2839 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -101,8 +101,13 @@ impl FileMode { } fn path_to_cstring(path: impl AsRef) -> Result { - let s = path.as_ref().to_str().ok_or_else(Error::from_convert)?; - Ok(CString::new(s)?) + use ErrorKind::InvalidOsStr; + 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() - .map_err(|e| e.with_task(ErrorTask::Read))?, - ); + + let num_atoms = self + .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() - .map_err(|e| e.with_task(ErrorTask::Read))?, - ); + + let num_atoms = self + .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> { - 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> { + 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> { let result_invalid = path_to_cstring(PathBuf::from("invalid/\0path")); - assert_eq!( - result_invalid, - 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(_)"), - } + if let Err(err) = result_invalid { + assert!(err.task() == &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() { - ErrorTask::OpenFile(err_path, err_mode) => { - assert_eq!(path, err_path); - assert_eq!(FileMode::Read, *err_mode) - } - _ => panic!("Wrong Error type"), + if let ( + ErrorTask::OpenFile, + ErrorKind::CouldNotOpen { + path: err_path, + mode: err_mode, + }, + ) = (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"), }