refactor: sys::TskBox<T> now requires that T implement a private trai…

…t. (#573)

This change prevents TskBox from being used outside of the crate,
which is a big safety improvement.
The new trait bound also simplifies several internal implementation
details.

src/sys/macros.rs

@@ -0,0 +1,11 @@
+#![macro_use]
+
+macro_rules! impl_tskteardown {
+    ($tsk: ty, $teardown: expr) => {
+        impl super::TskTeardown for $tsk {
+            unsafe fn teardown(&mut self) -> i32 {
+                $teardown(self as _)
+            }
+        }
+    };
+}

src/sys/mod.rs

@@ -1,5 +1,7 @@
 use thiserror::Error;
 
+mod macros;
+
 #[allow(dead_code)]
 #[allow(deref_nullptr)]
 #[allow(rustdoc::broken_intra_doc_links)]
@@ -8,8 +10,11 @@ pub mod bindings;
 pub mod flags;
 mod table_collection;
 mod tables;
+mod trait_impls;
+mod traits;
 mod tree;
 mod treeseq;
+mod tskbox;
 
 // tskit defines this via a type cast
 // in a macro. bindgen thus misses it.
@@ -22,7 +27,7 @@ pub use tables::*;
 pub use tree::LLTree;
 pub use treeseq::LLTreeSeq;
 
-mod tskbox;
+use traits::TskTeardown;
 
 #[non_exhaustive]
 #[derive(Error, Debug)]

src/sys/table_collection.rs

@@ -7,7 +7,6 @@ use super::bindings::tsk_population_table_t;
 #[cfg(feature = "provenance")]
 use super::bindings::tsk_provenance_table_t;
 use super::bindings::tsk_site_table_t;
-use super::bindings::tsk_table_collection_free;
 use super::bindings::tsk_table_collection_init;
 use super::bindings::tsk_table_collection_t;
 use super::tskbox::TskBox;
@@ -17,10 +16,9 @@ pub struct TableCollection(TskBox<tsk_table_collection_t>);
 
 impl TableCollection {
     pub fn new(sequence_length: f64) -> Result<Self, Error> {
-        let mut tsk = TskBox::new(
-            |tc: *mut tsk_table_collection_t| unsafe { tsk_table_collection_init(tc, 0) },
-            tsk_table_collection_free,
-        )?;
+        let mut tsk = TskBox::new(|tc: *mut tsk_table_collection_t| unsafe {
+            tsk_table_collection_init(tc, 0)
+        })?;
         tsk.as_mut().sequence_length = sequence_length;
         Ok(Self(tsk))
     }
@@ -30,14 +28,14 @@ impl TableCollection {
     // The returned value is uninitialized.
     // Using the object prior to initilization is likely to trigger UB.
     pub unsafe fn new_uninit() -> Self {
-        let tsk = unsafe { TskBox::new_uninit(tsk_table_collection_free) };
+        let tsk = unsafe { TskBox::new_uninit() };
         Self(tsk)
     }
 
     pub fn copy(&self) -> (i32, TableCollection) {
         // SAFETY: the C API requires that the destiniation of a copy be uninitalized.
         // Copying into it will initialize the object.
-        let mut dest = unsafe { TskBox::new_uninit(tsk_table_collection_free) };
+        let mut dest = unsafe { TskBox::new_uninit() };
         // SAFETY: self.as_ptr() is not null and dest matches the input
         // expectations of the C API.
         let rv = unsafe {

src/sys/tables.rs

@@ -39,18 +39,18 @@ basic_lltableref_impl!(LLIndividualTableRef, tsk_individual_table_t);
 basic_lltableref_impl!(LLProvenanceTableRef, tsk_provenance_table_t);
 
 macro_rules! basic_llowningtable_impl {
-    ($llowningtable: ident, $tsktable: ident, $init: ident, $free: ident, $clear: ident) => {
+    ($llowningtable: ident, $tsktable: ident, $init: ident, $clear: ident) => {
         #[repr(transparent)]
         #[derive(Debug)]
         pub struct $llowningtable(super::tskbox::TskBox<super::bindings::$tsktable>);
 
         impl $llowningtable {
             pub fn new() -> Self {
-                let table = super::tskbox::TskBox::new(
-                    |x: *mut super::bindings::$tsktable| unsafe { super::bindings::$init(x, 0) },
-                    super::bindings::$free,
-                )
-                .unwrap();
+                let table =
+                    super::tskbox::TskBox::new(|x: *mut super::bindings::$tsktable| unsafe {
+                        super::bindings::$init(x, 0)
+                    })
+                    .unwrap();
                 Self(table)
             }
 
@@ -76,49 +76,42 @@ basic_llowningtable_impl!(
     LLOwningEdgeTable,
     tsk_edge_table_t,
     tsk_edge_table_init,
-    tsk_edge_table_free,
     tsk_edge_table_clear
 );
 basic_llowningtable_impl!(
     LLOwningNodeTable,
     tsk_node_table_t,
     tsk_node_table_init,
-    tsk_node_table_free,
     tsk_node_table_clear
 );
 basic_llowningtable_impl!(
     LLOwningSiteTable,
     tsk_site_table_t,
     tsk_site_table_init,
-    tsk_site_table_free,
     tsk_site_table_clear
 );
 basic_llowningtable_impl!(
     LLOwningMutationTable,
     tsk_mutation_table_t,
     tsk_mutation_table_init,
-    tsk_mutation_table_free,
     tsk_mutation_table_clear
 );
 basic_llowningtable_impl!(
     LLOwningIndividualTable,
     tsk_individual_table_t,
     tsk_individual_table_init,
-    tsk_individual_table_free,
     tsk_individual_table_clear
 );
 basic_llowningtable_impl!(
     LLOwningMigrationTable,
     tsk_migration_table_t,
     tsk_migration_table_init,
-    tsk_migration_table_free,
     tsk_migration_table_clear
 );
 basic_llowningtable_impl!(
     LLOwningPopulationTable,
     tsk_population_table_t,
     tsk_population_table_init,
-    tsk_population_table_free,
     tsk_population_table_clear
 );
 
@@ -127,6 +120,5 @@ basic_llowningtable_impl!(
     LLOwningProvenanceTable,
     tsk_provenance_table_t,
     tsk_provenance_table_init,
-    tsk_provenance_table_free,
     tsk_provenance_table_clear
 );

src/sys/trait_impls.rs

@@ -0,0 +1,38 @@
+impl_tskteardown!(
+    super::bindings::tsk_table_collection_t,
+    super::bindings::tsk_table_collection_free
+);
+impl_tskteardown!(super::bindings::tsk_tree_t, super::bindings::tsk_tree_free);
+
+impl_tskteardown!(
+    super::bindings::tsk_edge_table_t,
+    super::bindings::tsk_edge_table_free
+);
+impl_tskteardown!(
+    super::bindings::tsk_node_table_t,
+    super::bindings::tsk_node_table_free
+);
+impl_tskteardown!(
+    super::bindings::tsk_site_table_t,
+    super::bindings::tsk_site_table_free
+);
+impl_tskteardown!(
+    super::bindings::tsk_mutation_table_t,
+    super::bindings::tsk_mutation_table_free
+);
+impl_tskteardown!(
+    super::bindings::tsk_individual_table_t,
+    super::bindings::tsk_individual_table_free
+);
+impl_tskteardown!(
+    super::bindings::tsk_population_table_t,
+    super::bindings::tsk_population_table_free
+);
+impl_tskteardown!(
+    super::bindings::tsk_provenance_table_t,
+    super::bindings::tsk_provenance_table_free
+);
+impl_tskteardown!(
+    super::bindings::tsk_migration_table_t,
+    super::bindings::tsk_migration_table_free
+);

src/sys/traits.rs

@@ -0,0 +1,13 @@
+/// For a type `tsk_foo_t`, this trait abstracts
+/// out the functionality of `tsk_foo_free`
+///
+/// # Note
+///
+/// This trait should NEVER be part of the public API.
+pub trait TskTeardown {
+    /// # Safety
+    ///
+    /// Implementations must abide by the expectations
+    /// of `tsk_foo_free` and C's `free`.
+    unsafe fn teardown(&mut self) -> i32;
+}

src/sys/tree.rs

@@ -16,12 +16,9 @@ pub struct LLTree<'treeseq> {
 
 impl<'treeseq> LLTree<'treeseq> {
     pub fn new(treeseq: &'treeseq LLTreeSeq, flags: TreeFlags) -> Result<Self, Error> {
-        let mut inner = TskBox::new(
-            |x: *mut super::bindings::tsk_tree_t| unsafe {
-                super::bindings::tsk_tree_init(x, treeseq.as_ref(), flags.bits())
-            },
-            super::bindings::tsk_tree_free,
-        )?;
+        let mut inner = TskBox::new(|x: *mut super::bindings::tsk_tree_t| unsafe {
+            super::bindings::tsk_tree_init(x, treeseq.as_ref(), flags.bits())
+        })?;
         // Gotta ask Jerome about this one--why isn't this handled in tsk_tree_init??
         if !flags.contains(TreeFlags::NO_SAMPLE_COUNTS) {
             // SAFETY: nobody is null here.