From cf7e0c7a6e3c97c87c7cbf496054695471fd82c3 Mon Sep 17 00:00:00 2001
From: molpopgen <krthornt@uci.edu>
Date: Thu, 24 Mar 2022 13:21:45 -0700
Subject: [PATCH] Fix several "pedantic" lints from clippy:

* Fix pedantic lints related to panics
* Fix lints due to possible integer wraparound issues
* Use safter pointer casts
---
 src/error.rs            |  5 +++++
 src/metadata.rs         |  9 ++++++---
 src/node_table.rs       |  2 +-
 src/table_collection.rs | 31 +++++++++++++++++++------------
 src/trees.rs            | 38 +++++++++++++++++++++++++++++---------
 5 files changed, 60 insertions(+), 25 deletions(-)

diff --git a/src/error.rs b/src/error.rs
index 603c26bb4..3e57ae04f 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -81,6 +81,11 @@ pub fn panic_on_tskit_error(code: i32) {
 /// let x = tskit::error::get_tskit_error_message(-207);
 /// assert_eq!(x, "Individual out of bounds");
 /// ```
+///
+/// # Panics
+///
+/// This function must allocate a C string, which may panic
+/// if the system runs out of memory.
 pub fn get_tskit_error_message(code: i32) -> String {
     let c_str = unsafe { std::ffi::CStr::from_ptr(crate::bindings::tsk_strerror(code)) };
     c_str.to_str().unwrap().to_owned()
diff --git a/src/metadata.rs b/src/metadata.rs
index 5843164ef..9d1f14dac 100644
--- a/src/metadata.rs
+++ b/src/metadata.rs
@@ -164,7 +164,7 @@
 //!   into `Python` via the `tskit` `Python API`.
 
 use crate::bindings::{tsk_id_t, tsk_size_t};
-use crate::SizeType;
+use crate::{SizeType, TskitError};
 use thiserror::Error;
 
 #[cfg(feature = "derive")]
@@ -230,7 +230,7 @@ impl EncodedMetadata {
         if self.encoded.is_empty() {
             std::ptr::null()
         } else {
-            self.encoded.as_ptr() as *const libc::c_char
+            self.encoded.as_ptr().cast::<i8>()
         }
     }
 
@@ -277,7 +277,10 @@ pub(crate) fn char_column_to_vector(
     }
     let mut buffer = vec![];
     for i in start..stop {
-        buffer.push(unsafe { *column.offset(i as isize) } as u8);
+        match isize::try_from(i) {
+            Ok(o) => buffer.push(unsafe { *column.offset(o) } as u8),
+            Err(_) => return Err(TskitError::RangeError("could not convert value to isize")),
+        };
     }
     Ok(Some(buffer))
 }
diff --git a/src/node_table.rs b/src/node_table.rs
index 66cbac501..9a54aa730 100644
--- a/src/node_table.rs
+++ b/src/node_table.rs
@@ -119,7 +119,7 @@ impl<'a> NodeTable<'a> {
     pub fn time_array_mut(&mut self) -> &mut [Time] {
         unsafe {
             std::slice::from_raw_parts_mut(
-                self.table_.time as *mut Time,
+                self.table_.time.cast::<Time>(),
                 self.table_.num_rows as usize,
             )
         }
diff --git a/src/table_collection.rs b/src/table_collection.rs
index db2950652..73a89b6dd 100644
--- a/src/table_collection.rs
+++ b/src/table_collection.rs
@@ -150,14 +150,17 @@ impl TableCollection {
     }
 
     /// Load a table collection from a file.
+    ///
+    /// # Panics
+    ///
+    /// This function allocates a `CString` to pass the file name to the C API.
+    /// A panic will occur if the system runs out of memory.
     pub fn new_from_file(filename: &str) -> Result<Self, TskitError> {
-        let tables = TableCollection::new(1.0); // Arbitrary sequence_length.
-        match tables {
-            Ok(_) => (),
+        // Arbitrary sequence_length.
+        let mut tables = match TableCollection::new(1.0) {
+            Ok(t) => (t),
             Err(e) => return Err(e),
-        }
-
-        let mut tables = tables.unwrap();
+        };
 
         let c_str = std::ffi::CString::new(filename).unwrap();
         let rv = unsafe {
@@ -241,9 +244,9 @@ impl TableCollection {
             ll_bindings::tsk_individual_table_add_row(
                 &mut (*self.as_mut_ptr()).individuals,
                 flags,
-                location.as_ptr() as *const f64,
+                location.as_ptr().cast::<f64>(),
                 location.len() as tsk_size_t,
-                parents.as_ptr() as *const tsk_id_t,
+                parents.as_ptr().cast::<tsk_id_t>(),
                 parents.len() as tsk_size_t,
                 std::ptr::null(),
                 0,
@@ -269,9 +272,9 @@ impl TableCollection {
             ll_bindings::tsk_individual_table_add_row(
                 &mut (*self.as_mut_ptr()).individuals,
                 flags,
-                location.as_ptr() as *const f64,
+                location.as_ptr().cast::<f64>(),
                 location.len() as tsk_size_t,
-                parents.as_ptr() as *const tsk_id_t,
+                parents.as_ptr().cast::<tsk_id_t>(),
                 parents.len() as tsk_size_t,
                 md.as_ptr(),
                 md.len().into(),
@@ -687,6 +690,10 @@ impl TableCollection {
 
     /// Dump the table collection to file.
     ///
+    /// # Panics
+    ///
+    /// This function allocates a `CString` to pass the file name to the C API.
+    /// A panic will occur if the system runs out of memory.
     pub fn dump(&self, filename: &str, options: TableOutputOptions) -> TskReturnValue {
         let c_str = std::ffi::CString::new(filename).unwrap();
         let rv = unsafe {
@@ -770,11 +777,11 @@ impl TableCollection {
         let rv = unsafe {
             ll_bindings::tsk_table_collection_simplify(
                 self.as_mut_ptr(),
-                samples.as_ptr() as *const tsk_id_t,
+                samples.as_ptr().cast::<tsk_id_t>(),
                 samples.len() as tsk_size_t,
                 options.bits(),
                 match idmap {
-                    true => output_node_map.as_mut_ptr() as *mut tsk_id_t,
+                    true => output_node_map.as_mut_ptr().cast::<tsk_id_t>(),
                     false => std::ptr::null_mut(),
                 },
             )
diff --git a/src/trees.rs b/src/trees.rs
index fa94e9615..3b8a17531 100644
--- a/src/trees.rs
+++ b/src/trees.rs
@@ -48,12 +48,12 @@ impl Tree {
     fn wrap(num_nodes: tsk_size_t, flags: TreeFlags) -> Self {
         let temp = unsafe {
             libc::malloc(std::mem::size_of::<ll_bindings::tsk_tree_t>())
-                as *mut ll_bindings::tsk_tree_t
+                .cast::<ll_bindings::tsk_tree_t>()
         };
         if temp.is_null() {
             panic!("out of memory");
         }
-        let mbox = unsafe { MBox::from_raw(temp as *mut ll_bindings::tsk_tree_t) };
+        let mbox = unsafe { MBox::from_raw(temp.cast::<ll_bindings::tsk_tree_t>()) };
         Self {
             inner: mbox,
             current_tree: 0,
@@ -410,6 +410,10 @@ impl Tree {
 
     /// Obtain the list of samples for the current tree/tree sequence
     /// as a vector.
+    ///
+    /// # Panics
+    ///
+    /// Will panic if the number of samples is too large to cast to a valid id.
     #[deprecated(since = "0.2.3", note = "Please use Tree::sample_nodes instead")]
     pub fn samples_to_vec(&self) -> Vec<NodeId> {
         let num_samples =
@@ -417,7 +421,10 @@ impl Tree {
         let mut rv = vec![];
 
         for i in 0..num_samples {
-            let u = unsafe { *(*(*self.inner).tree_sequence).samples.offset(i as isize) };
+            let u = match isize::try_from(i) {
+                Ok(o) => unsafe { *(*(*self.inner).tree_sequence).samples.offset(o) },
+                Err(e) => panic!("{e}"),
+            };
             rv.push(u.into());
         }
         rv
@@ -713,8 +720,8 @@ impl<'a> PostorderNodeIterator<'a> {
             ll_bindings::tsk_tree_postorder(
                 tree.as_ptr(),
                 NodeId::NULL.into(), // start from virtual root
-                nodes.as_mut_ptr() as *mut tsk_id_t,
-                ptr as *mut tsk_size_t,
+                nodes.as_mut_ptr().cast::<tsk_id_t>(),
+                ptr.cast::<tsk_size_t>(),
             )
         };
 
@@ -999,6 +1006,10 @@ impl TreeSequence {
     ///   This behavior may change in a future release, which could
     ///   break `API`.
     ///
+    /// # Panics
+    ///
+    /// This function allocates a `CString` to pass the file name to the C API.
+    /// A panic will occur if the system runs out of memory.
     pub fn dump(&self, filename: &str, options: TableOutputOptions) -> TskReturnValue {
         let c_str = std::ffi::CString::new(filename).unwrap();
         let rv =
@@ -1077,6 +1088,9 @@ impl TreeSequence {
     }
 
     /// Get the list of samples as a vector.
+    /// # Panics
+    ///
+    /// Will panic if the number of samples is too large to cast to a valid id.
     #[deprecated(
         since = "0.2.3",
         note = "Please use TreeSequence::sample_nodes instead"
@@ -1086,7 +1100,10 @@ impl TreeSequence {
         let mut rv = vec![];
 
         for i in 0..num_samples {
-            let u = NodeId::from(unsafe { *(*self.as_ptr()).samples.offset(i as isize) });
+            let u = match isize::try_from(i) {
+                Ok(o) => NodeId::from(unsafe { *(*self.as_ptr()).samples.offset(o) }),
+                Err(e) => panic!("{e}"),
+            };
             rv.push(u);
         }
         rv
@@ -1148,7 +1165,10 @@ impl TreeSequence {
         idmap: bool,
     ) -> Result<(Self, Option<Vec<NodeId>>), TskitError> {
         let mut tables = TableCollection::new(unsafe { (*(*self.inner).tables).sequence_length })?;
-        tables.build_index().unwrap();
+        match tables.build_index() {
+            Ok(_) => (),
+            Err(e) => return Err(e),
+        }
         let mut ts = tables.tree_sequence(TreeSequenceFlags::default())?;
         let mut output_node_map: Vec<NodeId> = vec![];
         if idmap {
@@ -1158,12 +1178,12 @@ impl TreeSequence {
             ll_bindings::tsk_treeseq_simplify(
                 self.as_ptr(),
                 // NOTE: casting away const-ness:
-                samples.as_ptr() as *mut tsk_id_t,
+                samples.as_ptr().cast::<tsk_id_t>(),
                 samples.len() as tsk_size_t,
                 options.bits(),
                 ts.as_mut_ptr(),
                 match idmap {
-                    true => output_node_map.as_mut_ptr() as *mut tsk_id_t,
+                    true => output_node_map.as_mut_ptr().cast::<tsk_id_t>(),
                     false => std::ptr::null_mut(),
                 },
             )