refactor: mark functions to create borrowed tables as unsafe (#724)

Author: molpopgen

src/edge_table.rs

@@ -197,7 +197,14 @@ impl EdgeTable {
         Ok(Self { table_ })
     }
 
-    pub(crate) fn new_from_table(
+    // # Safety
+    //
+    // * this fn must NEVER by part of the public API
+    // * all returned values must only be visible to the public API
+    //   by REFERENCE (& or &mut)
+    // * the input ptr must not be NULL
+    // * the input ptr must point to an initialized table
+    pub(crate) unsafe fn new_from_table(
         edges: *mut ll_bindings::tsk_edge_table_t,
     ) -> Result<Self, TskitError> {
         let ptr = std::ptr::NonNull::new(edges).unwrap();

src/individual_table.rs

@@ -183,7 +183,14 @@ impl Iterator for IndividualTableIterator {
 }
 
 impl IndividualTable {
-    pub(crate) fn new_from_table(
+    // # Safety
+    //
+    // * this fn must NEVER by part of the public API
+    // * all returned values must only be visible to the public API
+    //   by REFERENCE (& or &mut)
+    // * the input ptr must not be NULL
+    // * the input ptr must point to an initialized table
+    pub(crate) unsafe fn new_from_table(
         individuals: *mut ll_bindings::tsk_individual_table_t,
     ) -> Result<Self, TskitError> {
         let ptr = std::ptr::NonNull::new(individuals).unwrap();

src/migration_table.rs

@@ -211,7 +211,14 @@ pub struct MigrationTable {
 }
 
 impl MigrationTable {
-    pub(crate) fn new_from_table(
+    // # Safety
+    //
+    // * this fn must NEVER by part of the public API
+    // * all returned values must only be visible to the public API
+    //   by REFERENCE (& or &mut)
+    // * the input ptr must not be NULL
+    // * the input ptr must point to an initialized table
+    pub(crate) unsafe fn new_from_table(
         migrations: *mut ll_bindings::tsk_migration_table_t,
     ) -> Result<Self, TskitError> {
         let ptr = std::ptr::NonNull::new(migrations).unwrap();

src/mutation_table.rs

@@ -207,7 +207,14 @@ pub struct MutationTable {
 }
 
 impl MutationTable {
-    pub(crate) fn new_from_table(
+    // # Safety
+    //
+    // * this fn must NEVER by part of the public API
+    // * all returned values must only be visible to the public API
+    //   by REFERENCE (& or &mut)
+    // * the input ptr must not be NULL
+    // * the input ptr must point to an initialized table
+    pub(crate) unsafe fn new_from_table(
         mutations: *mut ll_bindings::tsk_mutation_table_t,
     ) -> Result<Self, TskitError> {
         let ptr = std::ptr::NonNull::new(mutations).unwrap();

src/node_table.rs

@@ -439,7 +439,14 @@ impl NodeTable {
         Ok(Self { table_ })
     }
 
-    pub(crate) fn new_from_table(
+    // # Safety
+    //
+    // * this fn must NEVER by part of the public API
+    // * all returned values must only be visible to the public API
+    //   by REFERENCE (& or &mut)
+    // * the input ptr must not be NULL
+    // * the input ptr must point to an initialized table
+    pub(crate) unsafe fn new_from_table(
         nodes: *mut ll_bindings::tsk_node_table_t,
     ) -> Result<Self, TskitError> {
         let ptr = std::ptr::NonNull::new(nodes).unwrap();

src/population_table.rs

@@ -157,7 +157,14 @@ pub struct PopulationTable {
 }
 
 impl PopulationTable {
-    pub(crate) fn new_from_table(
+    // # Safety
+    //
+    // * this fn must NEVER by part of the public API
+    // * all returned values must only be visible to the public API
+    //   by REFERENCE (& or &mut)
+    // * the input ptr must not be NULL
+    // * the input ptr must point to an initialized table
+    pub(crate) unsafe fn new_from_table(
         populations: *mut ll_bindings::tsk_population_table_t,
     ) -> Result<Self, TskitError> {
         let ptr = std::ptr::NonNull::new(populations).unwrap();

src/provenance.rs

@@ -154,7 +154,14 @@ pub struct ProvenanceTable {
 }
 
 impl ProvenanceTable {
-    pub(crate) fn new_from_table(
+    // # Safety
+    //
+    // * this fn must NEVER by part of the public API
+    // * all returned values must only be visible to the public API
+    //   by REFERENCE (& or &mut)
+    // * the input ptr must not be NULL
+    // * the input ptr must point to an initialized table
+    pub(crate) unsafe fn new_from_table(
         provenances: *mut ll_bindings::tsk_provenance_table_t,
     ) -> Result<Self, crate::TskitError> {
         let ptr = std::ptr::NonNull::new(provenances).unwrap();

src/site_table.rs

@@ -176,7 +176,14 @@ pub struct SiteTable {
 }
 
 impl SiteTable {
-    pub(crate) fn new_from_table(
+    // # Safety
+    //
+    // * this fn must NEVER by part of the public API
+    // * all returned values must only be visible to the public API
+    //   by REFERENCE (& or &mut)
+    // * the input ptr must not be NULL
+    // * the input ptr must point to an initialized table
+    pub(crate) unsafe fn new_from_table(
         sites: *mut ll_bindings::tsk_site_table_t,
     ) -> Result<Self, TskitError> {
         let ptr = std::ptr::NonNull::new(sites).unwrap();

src/table_collection.rs

@@ -100,16 +100,24 @@ impl TableCollection {
     /// ```
     pub fn new<P: Into<Position>>(sequence_length: P) -> Result<Self, TskitError> {
         let mut inner = LLTableCollection::new(sequence_length.into().into())?;
-        let edges = crate::EdgeTable::new_from_table(inner.edges_mut())?;
-        let nodes = crate::NodeTable::new_from_table(inner.nodes_mut())?;
-        let sites = crate::SiteTable::new_from_table(inner.sites_mut())?;
-        let mutations = crate::MutationTable::new_from_table(inner.mutations_mut())?;
-        let individuals = crate::IndividualTable::new_from_table(inner.individuals_mut())?;
-        let populations = crate::PopulationTable::new_from_table(inner.populations_mut())?;
-        let migrations = crate::MigrationTable::new_from_table(inner.migrations_mut())?;
+        // SAFETY: all the casts to *mut Foo are coming in via an implicit
+        // cast from &mut Foo, which means that the ptr cannot be NULL.
+        // Further, successful creation of LLTableCollection means
+        // that tables are initialized.
+        // Finally, none of these variables will be pub directly other than
+        // by reference.
+        let edges = unsafe { crate::EdgeTable::new_from_table(inner.edges_mut())? };
+        let nodes = unsafe { crate::NodeTable::new_from_table(inner.nodes_mut())? };
+        let sites = unsafe { crate::SiteTable::new_from_table(inner.sites_mut())? };
+        let mutations = unsafe { crate::MutationTable::new_from_table(inner.mutations_mut())? };
+        let individuals =
+            unsafe { crate::IndividualTable::new_from_table(inner.individuals_mut())? };
+        let populations =
+            unsafe { crate::PopulationTable::new_from_table(inner.populations_mut())? };
+        let migrations = unsafe { crate::MigrationTable::new_from_table(inner.migrations_mut())? };
         #[cfg(feature = "provenance")]
         let provenances =
-            crate::provenance::ProvenanceTable::new_from_table(inner.provenances_mut())?;
+            unsafe { crate::provenance::ProvenanceTable::new_from_table(inner.provenances_mut())? };
         Ok(Self {
             inner,
             idmap: vec![],
@@ -127,16 +135,24 @@ impl TableCollection {
 
     pub(crate) fn new_from_ll(lltables: LLTableCollection) -> Result<Self, TskitError> {
         let mut inner = lltables;
-        let edges = crate::EdgeTable::new_from_table(inner.edges_mut())?;
-        let nodes = crate::NodeTable::new_from_table(inner.nodes_mut())?;
-        let sites = crate::SiteTable::new_from_table(inner.sites_mut())?;
-        let mutations = crate::MutationTable::new_from_table(inner.mutations_mut())?;
-        let individuals = crate::IndividualTable::new_from_table(inner.individuals_mut())?;
-        let populations = crate::PopulationTable::new_from_table(inner.populations_mut())?;
-        let migrations = crate::MigrationTable::new_from_table(inner.migrations_mut())?;
+        // SAFETY: all the casts to *mut Foo are coming in via an implicit
+        // cast from &mut Foo, which means that the ptr cannot be NULL.
+        // Further, successful creation of LLTableCollection means
+        // that tables are initialized.
+        // Finally, none of these variables will be pub directly other than
+        // by reference.
+        let edges = unsafe { crate::EdgeTable::new_from_table(inner.edges_mut())? };
+        let nodes = unsafe { crate::NodeTable::new_from_table(inner.nodes_mut())? };
+        let sites = unsafe { crate::SiteTable::new_from_table(inner.sites_mut())? };
+        let mutations = unsafe { crate::MutationTable::new_from_table(inner.mutations_mut())? };
+        let individuals =
+            unsafe { crate::IndividualTable::new_from_table(inner.individuals_mut())? };
+        let populations =
+            unsafe { crate::PopulationTable::new_from_table(inner.populations_mut())? };
+        let migrations = unsafe { crate::MigrationTable::new_from_table(inner.migrations_mut())? };
         #[cfg(feature = "provenance")]
         let provenances =
-            crate::provenance::ProvenanceTable::new_from_table(inner.provenances_mut())?;
+            unsafe { crate::provenance::ProvenanceTable::new_from_table(inner.provenances_mut())? };
         Ok(Self {
             inner,
             idmap: vec![],
@@ -1740,10 +1756,14 @@ impl TableCollection {
         }
 
         // convert sys version of tables to non-sys version of tables
-        let new_edges = EdgeTable::new_from_table(new_edges.as_mut())?;
-        let new_migrations = MigrationTable::new_from_table(new_migrations.as_mut())?;
-        let new_mutations = MutationTable::new_from_table(new_mutations.as_mut())?;
-        let new_sites = SiteTable::new_from_table(new_sites.as_mut())?;
+        // SAFETY: all the casts to *mut Foo are coming in via an implicit
+        // cast from &mut Foo, which means that the ptr cannot be NULL.
+        // Further, all input tables are initialized.
+        // Finally, none of these variables will be every be pub.
+        let new_edges = unsafe { EdgeTable::new_from_table(new_edges.as_mut())? };
+        let new_migrations = unsafe { MigrationTable::new_from_table(new_migrations.as_mut())? };
+        let new_mutations = unsafe { MutationTable::new_from_table(new_mutations.as_mut())? };
+        let new_sites = unsafe { SiteTable::new_from_table(new_sites.as_mut())? };
 
         // replace old tables with new tables
         tables.set_edges(&new_edges).map(|_| ())?;