Removed all fallible casts (except in tests)

src/errors.rs

@@ -1,6 +1,7 @@
 use crate::c_abi;
 use crate::FileMode;
 use crate::Frame;
+use std::convert::TryInto;
 use std::error::Error as StdError;
 use std::path::{Path, PathBuf};
 
@@ -27,6 +28,14 @@ pub enum Error {
     /// A path could not be converted to &CStr because it had a null byte
     NullInStr(std::ffi::NulError),
     CouldNotCheckNAtoms(Box<Error>),
+    /// Step was out of range for usize on this platform
+    StepSizeOutOfRange(i32),
+    /// A numeric cast from `value` failed during `task`
+    NumericCastFailed {
+        source: std::num::TryFromIntError,
+        task: ErrorTask,
+        value: usize,
+    },
 }
 
 impl Error {
@@ -64,6 +73,7 @@ impl std::error::Error for Error {
         match &self {
             NullInStr(err) => Some(err),
             CouldNotCheckNAtoms(err) => Some(err.as_ref()),
+            NumericCastFailed { source, .. } => Some(source),
             _ => None,
         }
     }
@@ -105,7 +115,7 @@ impl From<(&Frame, usize)> for Error {
 impl std::fmt::Display for Error {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         use Error::*;
-        match &self {
+        match self {
             CApiError { code, task } => write!(
                 f,
                 "Error while {task}: C API returned error code {code}",
@@ -122,10 +132,16 @@ impl std::fmt::Display for Error {
             }
             InvalidOsStr => write!(f, "Paths must be valid unicode on this platform"),
             NullInStr(_err) => write!(f, "Paths cannot include null bytes"),
-            CouldNotCheckNAtoms(_err) => write!(
+            CouldNotCheckNAtoms(_err) => {
+                write!(f, "Failed to read number of atoms in trajectory file")
+            }
+            StepSizeOutOfRange(n) => write!(f, "Step {} does not fit in usize on this platform", n),
+            NumericCastFailed { value, task, .. } => write!(
                 f,
-                "Failed to read number of atoms in trajectory file"
+                "Numeric cast from {value} failed while {task}",
-            )
+                value = value,
+                task = task
+            ),
         }
     }
 }
@@ -200,9 +216,13 @@ impl ErrorCode {
     }
 }
 
-impl From<c_abi::xdrfile::BindgenTy1> for ErrorCode {
+impl<T> From<T> for ErrorCode
-    fn from(code: c_abi::xdrfile::BindgenTy1) -> Self {
+where
-        match code {
+    T: TryInto<c_abi::xdrfile::BindgenTy1>,
+    <T as TryInto<c_abi::xdrfile::BindgenTy1>>::Error: std::fmt::Debug,
+{
+    fn from(code: T) -> Self {
+        match code.try_into().expect("C API return code was out of range") {
             c_abi::xdrfile::exdrOK => Self::ExdrOk,
             c_abi::xdrfile::exdrHEADER => Self::ExdrHeader,
             c_abi::xdrfile::exdrSTRING => Self::ExdrString,

src/frame.rs

@@ -28,9 +28,7 @@ impl Default for Frame {
 impl Frame {
     /// Creates an empty frame with a capacity of 0
     pub fn new() -> Frame {
-        Frame {
+        Default::default()
-            ..Default::default()
-        }
     }
 
     /// Creates a frame with the given capacity
@@ -55,6 +53,11 @@ impl Frame {
 
     /// Length of the frame (number of atoms)
     pub fn len(self: &Frame) -> usize {
+        self.num_atoms()
+    }
+
+    /// The number of atoms in the frame
+    pub fn num_atoms(self: &Frame) -> usize {
         self.coords.len()
     }
 

src/lib.rs

@@ -77,6 +77,7 @@ use c_abi::xdrfile_xtc;
 
 use lazy_init::Lazy;
 use std::cell::Cell;
+use std::convert::{TryFrom, TryInto};
 use std::ffi::CString;
 use std::io;
 use std::path::{Path, PathBuf};
@@ -108,6 +109,14 @@ fn path_to_cstring(path: impl AsRef<Path>) -> Result<CString> {
     Ok(CString::new(s)?)
 }
 
+fn to_i32(value: usize, task: ErrorTask) -> Result<i32> {
+    value.try_into().map_err(|e| Error::NumericCastFailed {
+        source: e,
+        value,
+        task,
+    })
+}
+
 /// Convert an error code from a C call to an Error
 ///
 /// `code` should be an integer return code returned from the C API.
@@ -250,7 +259,7 @@ impl Trajectory for XTCTrajectory {
             .map_err(|e| Error::CouldNotCheckNAtoms(Box::new(e)))?;
         if num_atoms != frame.coords.len() {
             Err((&*frame, num_atoms))?;
-        }
+        };
 
         unsafe {
             // C lib requires an i32 to be passed, but step is exposed it as u32
@@ -258,19 +267,18 @@ impl Trajectory for XTCTrajectory {
             // variable to pass to read_xtc and cast it afterwards to u32
             let code = xdrfile_xtc::read_xtc(
                 self.handle.xdrfile,
-                num_atoms as i32,
+                to_i32(num_atoms, ErrorTask::Read)?,
                 &mut step,
                 &mut frame.time,
                 &mut frame.box_vector,
                 frame.coords.as_mut_ptr(),
                 &mut self.precision.get(),
-            ) as u32;
+            );
-            frame.step = step as usize;
             if let Some(err) = check_code(code, ErrorTask::Read) {
-                Err(err)
+                return Err(err);
-            } else {
-                Ok(())
             }
+            frame.step = usize::try_from(step).map_err(|_| Error::StepSizeOutOfRange(step))?;
+            Ok(())
         }
     }
 
@@ -278,13 +286,13 @@ impl Trajectory for XTCTrajectory {
         unsafe {
             let code = xdrfile_xtc::write_xtc(
                 self.handle.xdrfile,
-                frame.len() as i32,
+                to_i32(frame.len(), ErrorTask::Write)?,
-                frame.step as i32,
+                to_i32(frame.step, ErrorTask::Write)?,
                 frame.time,
                 frame.box_vector.as_ptr() as *mut [[f32; 3]; 3],
                 frame.coords[..].as_ptr() as *mut [f32; 3],
                 1000.0,
-            ) as u32;
+            );
             if let Some(err) = check_code(code, ErrorTask::Write) {
                 Err(err)
             } else {
@@ -295,7 +303,7 @@ impl Trajectory for XTCTrajectory {
 
     fn flush(&mut self) -> Result<()> {
         unsafe {
-            let code = xdr_seek::xdr_flush(self.handle.xdrfile) as u32;
+            let code = xdr_seek::xdr_flush(self.handle.xdrfile);
             if let Some(err) = check_code(code, ErrorTask::Read) {
                 Err(err)
             } else {
@@ -312,15 +320,15 @@ impl Trajectory for XTCTrajectory {
                 unsafe {
                     let path = path_to_cstring(&self.handle.path)?;
                     let path_p = path.into_raw();
-                    let code =
+                    let code = xdrfile_xtc::read_xtc_natoms(path_p, &mut num_atoms as *const i32);
-                        xdrfile_xtc::read_xtc_natoms(path_p, &mut num_atoms as *const i32) as u32;
                     // Reconstitute the CString so it is deallocated correctly
                     let _ = CString::from_raw(path_p);
 
                     if let Some(err) = check_code(code, ErrorTask::ReadNumAtoms) {
                         Err(err)
                     } else {
-                        Ok(num_atoms as usize)
+                        Ok(usize::try_from(num_atoms)
+                            .expect("Number of atoms in file does not fit in usize"))
                     }
                 }
             })
@@ -391,7 +399,7 @@ impl Trajectory for TRRTrajectory {
             // Similar for lambda.
             let code = xdrfile_trr::read_trr(
                 self.handle.xdrfile,
-                num_atoms as i32,
+                to_i32(num_atoms, ErrorTask::Read)?,
                 &mut step,
                 &mut frame.time,
                 &mut lambda,
@@ -399,14 +407,12 @@ impl Trajectory for TRRTrajectory {
                 frame.coords.as_mut_ptr(),
                 std::ptr::null_mut(),
                 std::ptr::null_mut(),
-            ) as u32;
+            );
-
-            frame.step = step as usize;
             if let Some(err) = check_code(code, ErrorTask::Read) {
-                Err(err)
+                return Err(err);
-            } else {
-                Ok(())
             }
+            frame.step = usize::try_from(step).map_err(|_| Error::StepSizeOutOfRange(step))?;
+            Ok(())
         }
     }
 
@@ -414,15 +420,15 @@ impl Trajectory for TRRTrajectory {
         unsafe {
             let code = xdrfile_trr::write_trr(
                 self.handle.xdrfile,
-                frame.len() as i32,
+                to_i32(frame.len(), ErrorTask::Write)?,
-                frame.step as i32,
+                to_i32(frame.step, ErrorTask::Write)?,
                 frame.time,
                 0.0,
                 frame.box_vector.as_ptr() as *mut [[f32; 3]; 3],
                 frame.coords[..].as_ptr() as *mut [f32; 3],
                 std::ptr::null_mut(),
                 std::ptr::null_mut(),
-            ) as u32;
+            );
             if let Some(err) = check_code(code, ErrorTask::Write) {
                 Err(err)
             } else {
@@ -433,7 +439,7 @@ impl Trajectory for TRRTrajectory {
 
     fn flush(&mut self) -> Result<()> {
         unsafe {
-            let code = xdr_seek::xdr_flush(self.handle.xdrfile) as u32;
+            let code = xdr_seek::xdr_flush(self.handle.xdrfile);
             if let Some(err) = check_code(code, ErrorTask::Flush) {
                 Err(err)
             } else {
@@ -449,15 +455,15 @@ impl Trajectory for TRRTrajectory {
                 unsafe {
                     let path = path_to_cstring(&self.handle.path)?;
                     let path_p = path.into_raw();
-                    let code =
+                    let code = xdrfile_trr::read_trr_natoms(path_p, &mut num_atoms as *const i32);
-                        xdrfile_trr::read_trr_natoms(path_p, &mut num_atoms as *const i32) as u32;
                     // Reconstitute the CString so it is deallocated correctly
                     let _ = CString::from_raw(path_p);
 
                     if let Some(err) = check_code(code, ErrorTask::ReadNumAtoms) {
                         Err(err)
                     } else {
-                        Ok(num_atoms as usize)
+                        Ok(usize::try_from(num_atoms)
+                            .expect("Number of atoms in file does not fit in usize"))
                     }
                 }
             })
@@ -530,7 +536,7 @@ mod tests {
         let tempfile = NamedTempFile::new().expect("Could not create temporary file");
         let tmp_path = tempfile.path();
 
-        let natoms: u32 = 2;
+        let natoms = 2;
         let frame = Frame {
             step: 5,
             time: 2.0,
@@ -545,7 +551,7 @@ mod tests {
         }
         f.flush()?;
 
-        let mut new_frame = Frame::with_len(natoms as usize);
+        let mut new_frame = Frame::with_len(natoms);
         let mut f = TRRTrajectory::open_read(tmp_path)?;
         // let num_atoms = f.get_num_atoms()?;
         // assert_eq!(num_atoms, natoms);
@@ -744,7 +750,7 @@ mod tests {
         let tempfile = NamedTempFile::new()?;
         let tmp_path = tempfile.path();
 
-        let natoms: u32 = 2;
+        let natoms = 2;
         let frame = Frame {
             step: 5,
             time: 2.0,
@@ -755,7 +761,7 @@ mod tests {
         f.write(&frame)?;
         f.flush()?;
 
-        let mut new_frame = Frame::with_len(natoms as usize);
+        let mut new_frame = Frame::with_len(natoms);
         let mut f = XTCTrajectory::open_read(tmp_path)?;
 
         f.read(&mut new_frame)?;

tests/integration.rs

@@ -8,7 +8,7 @@ mod integration {
     fn test_use_library() -> Result<()> {
         let mut trj = XTCTrajectory::open_read("tests/1l2y.xtc")?;
         let num_atoms = trj.get_num_atoms()?;
-        let mut frame = Frame::with_len(num_atoms as usize);
+        let mut frame = Frame::with_len(num_atoms);
 
         trj.read(&mut frame)?;
         trj.read(&mut frame)?;
@@ -21,7 +21,7 @@ mod integration {
         let trj = XTCTrajectory::open_read("tests/1l2y.xtc")?;
         let frames: Result<Vec<Rc<Frame>>> = trj.into_iter().collect();
         for (idx, frame) in frames?.iter().enumerate() {
-            assert_eq!(frame.step as usize, idx + 1);
+            assert_eq!(frame.step, idx + 1);
         }
         Ok(())
     }