Improvements to handling of tsk_flags_t: (#227)

* Add RawFlags as typedef for tsk_flags_t
* Add From<RawFlags> for flags types.
  The implementation will remove invalid bits that are set,
  except for NodeFlags/IndividualFlags which allow
  client-defined flags.
* Flags types are now repr(transparent)
* Functions taking flags are now generic over Into<flags type>

Breaking changes:

* Added NodeFlags and InidividualFlags as first-class types.
  Functions previously returning, e.g, &[tsk_flags_t] now
  return &[NodeFlags], etc..  Due to repr(transparent),
  these return values have the same ABI.

Closes #226

Author: molpopgen

src/_macros.rs

@@ -498,6 +498,29 @@ macro_rules! impl_time_position_arithmetic {
     };
 }
 
+macro_rules! impl_from_for_flag_types {
+    ($flagstype: ty) => {
+        impl From<$crate::RawFlags> for $flagstype {
+            fn from(value: $crate::RawFlags) -> Self {
+                <$flagstype>::from_bits_truncate(value)
+            }
+        }
+    };
+}
+
+macro_rules! impl_flags {
+    ($flagstype: ty) => {
+        impl $flagstype {
+            /// We do not enforce valid flags in the library.
+            /// This function will return `true` if any bits
+            /// are set that do not correspond to allowed flags.
+            pub fn is_valid(&self) -> bool {
+                Self::from_bits(self.bits()).is_some()
+            }
+        }
+    };
+}
+
 /// Convenience macro to handle implementing
 /// [`crate::metadata::MetadataRoundtrip`]
 #[macro_export]

src/flags.rs

@@ -1,5 +1,5 @@
 use crate::bindings as ll_bindings;
-use crate::tsk_flags_t;
+use crate::RawFlags;
 use bitflags::bitflags;
 
 bitflags! {
@@ -39,7 +39,8 @@ bitflags! {
     /// assert!(!flags.contains(SO::FILTER_POPULATIONS));
     /// ```
     #[derive(Default)]
-    pub struct SimplificationOptions: tsk_flags_t {
+    #[repr(transparent)]
+    pub struct SimplificationOptions: RawFlags {
         /// Default behavior
         const NONE = 0;
         const FILTER_SITES = ll_bindings::TSK_FILTER_SITES;
@@ -70,7 +71,8 @@ bitflags! {
 bitflags! {
     /// Modify behavior of [`crate::TableCollection::clear`].
     #[derive(Default)]
-    pub struct TableClearOptions : tsk_flags_t {
+    #[repr(transparent)]
+    pub struct TableClearOptions : RawFlags {
         /// Default behavior.
         const NONE = 0;
         const CLEAR_METADATA_SCHEMAS = ll_bindings::TSK_CLEAR_METADATA_SCHEMAS;
@@ -82,7 +84,8 @@ bitflags! {
 bitflags! {
     /// Modify behavior of [`crate::TableCollection::equals`].
     #[derive(Default)]
-    pub struct TableEqualityOptions : tsk_flags_t {
+    #[repr(transparent)]
+    pub struct TableEqualityOptions : RawFlags {
         /// Default behavior.
         const NONE = 0;
         const IGNORE_METADATA = ll_bindings::TSK_CMP_IGNORE_METADATA;
@@ -95,7 +98,8 @@ bitflags! {
 bitflags! {
     /// Modify behavior of [`crate::TableCollection::sort`].
     #[derive(Default)]
-    pub struct TableSortOptions : tsk_flags_t {
+    #[repr(transparent)]
+    pub struct TableSortOptions : RawFlags {
         /// Default behavior.
         const NONE = 0;
         /// Do not validate contents of edge table.
@@ -106,7 +110,8 @@ bitflags! {
 bitflags! {
     /// Modify behavior of [`crate::TableCollection::sort_individuals`].
     #[derive(Default)]
-    pub struct IndividualTableSortOptions : tsk_flags_t {
+    #[repr(transparent)]
+    pub struct IndividualTableSortOptions : RawFlags {
         /// Default behavior.
         const NONE = 0;
     }
@@ -116,7 +121,8 @@ bitflags! {
     /// Specify the behavior of iterating over [`Tree`] objects.
     /// See [`TreeSequence::tree_iterator`].
     #[derive(Default)]
-    pub struct TreeFlags: tsk_flags_t {
+    #[repr(transparent)]
+    pub struct TreeFlags: RawFlags {
         /// Default behavior.
         const NONE = 0;
         /// Update sample lists, enabling [`Tree::samples`].
@@ -140,7 +146,8 @@ bitflags! {
     /// call [`crate::TableCollection::build_index`] prior to calling
     /// [`crate::TableCollection::dump`].
     #[derive(Default)]
-    pub struct TableOutputOptions : tsk_flags_t {
+    #[repr(transparent)]
+    pub struct TableOutputOptions : RawFlags {
         const NONE = 0;
     }
 }
@@ -149,7 +156,8 @@ bitflags! {
     /// Modify behavior of [`crate::TableCollection::tree_sequence`]
     /// and [`crate::TreeSequence::new`].
     #[derive(Default)]
-    pub struct TreeSequenceFlags: tsk_flags_t {
+    #[repr(transparent)]
+    pub struct TreeSequenceFlags: RawFlags {
         /// Default behavior
         const NONE = 0;
         /// If used, then build table indexes if they are not present.
@@ -159,9 +167,10 @@ bitflags! {
 
 bitflags! {
     #[derive(Default)]
-    pub struct TableIntegrityCheckFlags: tsk_flags_t {
+    #[repr(transparent)]
+    pub struct TableIntegrityCheckFlags: RawFlags {
         /// Default behavior is a set of basic checks
-        const DEFAULT = 0;
+        const NONE = 0;
         /// Check that edges are ordered
         const CHECK_EDGE_ORDERING =ll_bindings::TSK_CHECK_EDGE_ORDERING;
         /// Check that sites are ordered
@@ -180,3 +189,81 @@ bitflags! {
         const CHECK_TREES=ll_bindings::TSK_CHECK_TREES;
     }
 }
+
+bitflags! {
+    #[derive(Default)]
+    #[repr(transparent)]
+    /// Node flags
+    pub struct NodeFlags : RawFlags {
+        /// Default (empty)
+        const NONE = 0;
+        /// Node is a sample
+        const IS_SAMPLE = ll_bindings::TSK_NODE_IS_SAMPLE;
+    }
+}
+
+bitflags! {
+    #[derive(Default)]
+    #[repr(transparent)]
+    /// Individual flags
+    pub struct IndividualFlags : RawFlags {
+        /// Default (empty)
+        const NONE = 0;
+    }
+}
+
+impl_flags!(SimplificationOptions);
+impl_flags!(TableClearOptions);
+impl_flags!(TableEqualityOptions);
+impl_flags!(TreeSequenceFlags);
+impl_flags!(TableSortOptions);
+impl_flags!(TreeFlags);
+impl_flags!(IndividualTableSortOptions);
+impl_flags!(TableIntegrityCheckFlags);
+impl_flags!(TableOutputOptions);
+
+impl_from_for_flag_types!(SimplificationOptions);
+impl_from_for_flag_types!(TableClearOptions);
+impl_from_for_flag_types!(TableEqualityOptions);
+impl_from_for_flag_types!(TreeSequenceFlags);
+impl_from_for_flag_types!(TableSortOptions);
+impl_from_for_flag_types!(TreeFlags);
+impl_from_for_flag_types!(IndividualTableSortOptions);
+impl_from_for_flag_types!(TableIntegrityCheckFlags);
+impl_from_for_flag_types!(TableOutputOptions);
+
+impl From<RawFlags> for NodeFlags {
+    fn from(flags: RawFlags) -> Self {
+        // Safety: node flags can contain user-defined values.
+        // It is an error on the user's part to define flags
+        // in the first 16 bits, as per the C API docs.
+        unsafe { Self::from_bits_unchecked(flags) }
+    }
+}
+
+impl From<RawFlags> for IndividualFlags {
+    fn from(flags: RawFlags) -> Self {
+        // Safety: node flags can contain user-defined values.
+        // It is an error on the user's part to define flags
+        // in the first 16 bits, as per the C API docs.
+        unsafe { Self::from_bits_unchecked(flags) }
+    }
+}
+
+impl NodeFlags {
+    /// We do not enforce valid flags in the library.
+    /// This function will return `true` if any bits
+    /// are set that do not correspond to allowed flags.
+    pub fn is_valid(&self) -> bool {
+        true
+    }
+}
+
+impl IndividualFlags {
+    /// We do not enforce valid flags in the library.
+    /// This function will return `true` if any bits
+    /// are set that do not correspond to allowed flags.
+    pub fn is_valid(&self) -> bool {
+        true
+    }
+}

src/individual_table.rs

@@ -1,13 +1,14 @@
 use crate::bindings as ll_bindings;
 use crate::metadata;
+use crate::IndividualFlags;
 use crate::IndividualId;
 use crate::Location;
-use crate::{tsk_flags_t, tsk_id_t, tsk_size_t, TskitError};
+use crate::{tsk_id_t, tsk_size_t, TskitError};
 
 /// Row of a [`IndividualTable`]
 pub struct IndividualTableRow {
     pub id: IndividualId,
-    pub flags: tsk_flags_t,
+    pub flags: IndividualFlags,
     pub location: Option<Vec<Location>>,
     pub parents: Option<Vec<IndividualId>>,
     pub metadata: Option<Vec<u8>>,
@@ -110,8 +111,14 @@ impl<'a> IndividualTable<'a> {
     /// # Errors
     ///
     /// * [`TskitError::IndexError`] if `row` is out of range.
-    pub fn flags<I: Into<IndividualId> + Copy>(&self, row: I) -> Result<tsk_flags_t, TskitError> {
-        unsafe_tsk_column_access!(row.into().0, 0, self.num_rows(), self.table_.flags)
+    pub fn flags<I: Into<IndividualId> + Copy>(
+        &self,
+        row: I,
+    ) -> Result<IndividualFlags, TskitError> {
+        match unsafe_tsk_column_access!(row.into().0, 0, self.num_rows(), self.table_.flags) {
+            Ok(f) => Ok(IndividualFlags::from(f)),
+            Err(e) => Err(e),
+        }
     }
 
     /// Return the locations for a given row.

src/lib.rs

@@ -449,6 +449,9 @@ impl_id_traits!(ProvenanceId);
 /// the error message is stored for diplay.
 pub type TskReturnValue = Result<i32, TskitError>;
 
+/// Alias for tsk_flags_t
+pub type RawFlags = crate::bindings::tsk_flags_t;
+
 /// Version of the rust crate.
 ///
 /// To get the C API version, see:

src/node_table.rs

@@ -1,15 +1,16 @@
 use crate::bindings as ll_bindings;
 use crate::metadata;
+use crate::NodeFlags;
 use crate::SizeType;
 use crate::Time;
-use crate::{tsk_flags_t, tsk_id_t, TskitError};
+use crate::{tsk_id_t, TskitError};
 use crate::{IndividualId, NodeId, PopulationId};
 
 /// Row of a [`NodeTable`]
 pub struct NodeTableRow {
     pub id: NodeId,
     pub time: Time,
-    pub flags: tsk_flags_t,
+    pub flags: NodeFlags,
     pub population: PopulationId,
     pub individual: IndividualId,
     pub metadata: Option<Vec<u8>>,
@@ -106,15 +107,18 @@ impl<'a> NodeTable<'a> {
     ///
     /// Will return [``IndexError``](crate::TskitError::IndexError)
     /// if ``row`` is out of range.
-    pub fn flags<N: Into<NodeId> + Copy>(&'a self, row: N) -> Result<tsk_flags_t, TskitError> {
-        unsafe_tsk_column_access!(row.into().0, 0, self.num_rows(), self.table_.flags)
+    pub fn flags<N: Into<NodeId> + Copy>(&'a self, row: N) -> Result<NodeFlags, TskitError> {
+        match unsafe_tsk_column_access!(row.into().0, 0, self.num_rows(), self.table_.flags) {
+            Ok(f) => Ok(NodeFlags::from(f)),
+            Err(e) => Err(e),
+        }
     }
 
     /// Mutable access to node flags.
-    pub fn flags_array_mut(&mut self) -> &mut [tsk_flags_t] {
+    pub fn flags_array_mut(&mut self) -> &mut [NodeFlags] {
         unsafe {
             std::slice::from_raw_parts_mut(
-                self.table_.flags,
+                self.table_.flags.cast::<NodeFlags>(),
                 crate::util::handle_u64_to_usize(self.table_.num_rows),
             )
         }
@@ -215,7 +219,7 @@ impl<'a> NodeTable<'a> {
     pub fn samples_as_vector(&self) -> Vec<NodeId> {
         let mut samples: Vec<NodeId> = vec![];
         for row in self.iter() {
-            if row.flags & crate::TSK_NODE_IS_SAMPLE > 0 {
+            if row.flags.contains(NodeFlags::IS_SAMPLE) {
                 samples.push(row.id);
             }
         }

src/prelude.rs

@@ -9,6 +9,6 @@ pub use streaming_iterator::DoubleEndedStreamingIterator;
 pub use streaming_iterator::StreamingIterator;
 pub use {
     crate::EdgeId, crate::IndividualId, crate::Location, crate::MigrationId, crate::MutationId,
-    crate::NodeId, crate::PopulationId, crate::Position, crate::SiteId, crate::SizeType,
-    crate::Time,
+    crate::NodeId, crate::PopulationId, crate::Position, crate::RawFlags, crate::SiteId,
+    crate::SizeType, crate::Time,
 };