Skip to content

Commit 48d820a

Browse files
molpopgenmolpopgen
authored
refactor: return None for out-of-range row indexes. (#350)
This affects some, but not all, table getters: * functions to return metadata now return Option<Result<_, _>> instead of Result<Option<_>, _>. * Site ancestral state and mutation derived state now return Option<&[u8]> instead of Result<&[u8], TskitError> The justification for this is that the new behavior is more idiomatic. (Consider Vec::get, which returns None if the index is invalid.) BREAKING CHANGE: return values have changed
1 parent 094ec4f commit 48d820a

11 files changed

+165
-156
lines changed

‎src/_macros.rs

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -142,18 +142,13 @@ macro_rules! metadata_to_vector {
142142

143143
macro_rules! decode_metadata_row {
144144
($T: ty, $buffer: expr) => {
145-
match $buffer {
146-
None => Ok(None),
147-
Some(v) => Ok(Some(<$T as $crate::metadata::MetadataRoundtrip>::decode(
148-
&v,
149-
)?)),
150-
}
145+
<$T as $crate::metadata::MetadataRoundtrip>::decode($buffer)
151146
};
152147
}
153148

154149
macro_rules! table_row_decode_metadata {
155150
($owner: ident, $table: ident, $pos: ident) => {
156-
metadata_to_vector!($owner, $table, $pos).ok()?.map(|x| x)
151+
metadata_to_vector!($owner, $table, $pos)
157152
};
158153
}
159154

‎src/edge_table.rs

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -130,10 +130,10 @@ impl<'a> EdgeTable<'a> {
130130
pub fn metadata<T: metadata::MetadataRoundtrip>(
131131
&'a self,
132132
row: EdgeId,
133-
) -> Result<Option<T>, TskitError> {
133+
) -> Option<Result<T, TskitError>> {
134134
let table_ref = self.table_;
135135
let buffer = metadata_to_vector!(self, table_ref, row.0)?;
136-
decode_metadata_row!(T, buffer)
136+
Some(decode_metadata_row!(T, buffer).map_err(|e| e.into()))
137137
}
138138

139139
/// Return an iterator over rows of the table.
@@ -200,12 +200,13 @@ build_owned_table_type!(
200200
/// let rowid = edges.add_row_with_metadata(0., 1., 5, 10, &metadata).unwrap();
201201
/// assert_eq!(rowid, 0);
202202
///
203-
/// if let Some(decoded) = edges.metadata::<EdgeMetadata>(rowid).unwrap() {
204-
/// assert_eq!(decoded.value, 42);
205-
/// } else {
206-
/// panic!("hmm...we expected some metadata!");
203+
/// match edges.metadata::<EdgeMetadata>(rowid) {
204+
/// // rowid is in range, decoding succeeded
205+
/// Some(Ok(decoded)) => assert_eq!(decoded.value, 42),
206+
/// // rowid is in range, decoding failed
207+
/// Some(Err(e)) => panic!("error decoding metadata: {:?}", e),
208+
/// None => panic!("row id out of range")
207209
/// }
208-
///
209210
/// # }
210211
/// ```
211212
=> OwnedEdgeTable,

‎src/individual_table.rs

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -224,23 +224,23 @@ impl<'a> IndividualTable<'a> {
224224
/// .individuals()
225225
/// .metadata::<IndividualMetadata>(0.into())
226226
/// {
227-
/// Ok(metadata_option) => metadata_option,
228-
/// Err(e) => panic!("error: {:?}", e),
227+
/// Some(metadata_option) => metadata_option,
228+
/// None => panic!("expected metadata"),
229229
/// };
230230
/// // Now, check the contents of the Option
231231
/// match decoded_option {
232-
/// Some(metadata) => assert_eq!(metadata.x, 1),
233-
/// None => panic!("we expected Some(metadata)?"),
232+
/// Ok(metadata) => assert_eq!(metadata.x, 1),
233+
/// Err(e) => panic!("error decoding metadata: {:?}", e),
234234
/// }
235235
/// # }
236236
/// ```
237237
pub fn metadata<T: metadata::MetadataRoundtrip>(
238238
&'a self,
239239
row: IndividualId,
240-
) -> Result<Option<T>, TskitError> {
240+
) -> Option<Result<T, TskitError>> {
241241
let table_ref = self.table_;
242242
let buffer = metadata_to_vector!(self, table_ref, row.0)?;
243-
decode_metadata_row!(T, buffer)
243+
Some(decode_metadata_row!(T, buffer).map_err(|e| e.into()))
244244
}
245245

246246
/// Return an iterator over rows of the table.
@@ -288,6 +288,7 @@ build_owned_table_type!(
288288
/// An example with metadata.
289289
/// This requires the cargo feature `"derive"` for `tskit`.
290290
///
291+
///
291292
/// ```
292293
/// # #[cfg(any(feature="doc", feature="derive"))] {
293294
/// use tskit::OwnedIndividualTable;
@@ -307,10 +308,12 @@ build_owned_table_type!(
307308
/// let rowid = individuals.add_row_with_metadata(0, None, None, &metadata).unwrap();
308309
/// assert_eq!(rowid, 0);
309310
///
310-
/// if let Some(decoded) = individuals.metadata::<IndividualMetadata>(rowid).unwrap() {
311-
/// assert_eq!(decoded.value, 42);
312-
/// } else {
313-
/// panic!("hmm...we expected some metadata!");
311+
/// match individuals.metadata::<IndividualMetadata>(rowid) {
312+
/// // rowid is in range, decoding succeeded
313+
/// Some(Ok(decoded)) => assert_eq!(decoded.value, 42),
314+
/// // rowid is in range, decoding failed
315+
/// Some(Err(e)) => panic!("error decoding metadata: {:?}", e),
316+
/// None => panic!("row id out of range")
314317
/// }
315318
///
316319
/// # }

‎src/metadata.rs

Lines changed: 19 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,7 @@
164164
//! into `Python` via the `tskit` `Python API`.
165165
166166
use crate::bindings::{tsk_id_t, tsk_size_t};
167-
use crate::{SizeType, TskitError};
167+
use crate::SizeType;
168168
use thiserror::Error;
169169

170170
#[cfg(feature = "derive")]
@@ -257,25 +257,17 @@ pub(crate) fn char_column_to_slice<T: Sized>(
257257
row: tsk_id_t,
258258
num_rows: tsk_size_t,
259259
column_length: tsk_size_t,
260-
) -> Result<Option<&[u8]>, crate::TskitError> {
261-
let row = match tsk_size_t::try_from(row) {
262-
Ok(r) => r,
263-
Err(_) => return Err(TskitError::IndexError),
260+
) -> Option<&[u8]> {
261+
let row = match tsk_size_t::try_from(row).ok() {
262+
Some(r) if r < num_rows => r,
263+
_ => return None,
264264
};
265-
if row >= num_rows {
266-
return Err(crate::TskitError::IndexError {});
267-
}
268265
if column_length == 0 {
269-
return Ok(None);
266+
return None;
270267
}
271-
let row_isize = match isize::try_from(row) {
272-
Ok(r) => r,
273-
Err(_) => {
274-
return Err(TskitError::RangeError(format!(
275-
"could not convert u64 value {} to isize",
276-
stringify!(row)
277-
)))
278-
}
268+
let row_isize = match isize::try_from(row).ok() {
269+
Some(x) => x,
270+
None => return None,
279271
};
280272
let start = unsafe { *column_offset.offset(row_isize) };
281273
let stop = if (row as tsk_size_t) < num_rows {
@@ -284,32 +276,22 @@ pub(crate) fn char_column_to_slice<T: Sized>(
284276
column_length
285277
};
286278
if start >= stop {
287-
return Ok(None);
279+
return None;
288280
}
289281
if column_length == 0 {
290-
return Ok(None);
282+
return None;
291283
}
292-
let istart = match isize::try_from(start) {
293-
Ok(v) => v,
294-
Err(_) => {
295-
return Err(TskitError::RangeError(format!(
296-
"cauld not convert value {} to isize",
297-
stringify!(i)
298-
)));
299-
}
284+
let istart = match isize::try_from(start).ok() {
285+
Some(v) => v,
286+
None => return None,
300287
};
301-
let ustop = match usize::try_from(stop) {
302-
Ok(v) => v,
303-
Err(_) => {
304-
return Err(TskitError::RangeError(format!(
305-
"cauld not convert value {} to usize",
306-
stringify!(i)
307-
)));
308-
}
288+
let ustop = match usize::try_from(stop).ok() {
289+
Some(v) => v,
290+
None => return None,
309291
};
310-
Ok(Some(unsafe {
292+
Some(unsafe {
311293
std::slice::from_raw_parts(column.offset(istart) as *const u8, ustop - istart as usize)
312-
}))
294+
})
313295
}
314296

315297
#[cfg(test)]

‎src/migration_table.rs

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -176,10 +176,10 @@ impl<'a> MigrationTable<'a> {
176176
pub fn metadata<T: metadata::MetadataRoundtrip>(
177177
&'a self,
178178
row: MigrationId,
179-
) -> Result<Option<T>, TskitError> {
179+
) -> Option<Result<T, TskitError>> {
180180
let table_ref = self.table_;
181181
let buffer = metadata_to_vector!(self, table_ref, row.0)?;
182-
decode_metadata_row!(T, buffer)
182+
Some(decode_metadata_row!(T, buffer).map_err(|e| e.into()))
183183
}
184184

185185
/// Return an iterator over rows of the table.
@@ -242,10 +242,12 @@ build_owned_table_type!(
242242
/// let rowid = migrations.add_row_with_metadata((0., 1.), 1, (0, 1), 10.3, &metadata).unwrap();
243243
/// assert_eq!(rowid, 0);
244244
///
245-
/// if let Some(decoded) = migrations.metadata::<MigrationMetadata>(rowid).unwrap() {
246-
/// assert_eq!(decoded.value, 42);
247-
/// } else {
248-
/// panic!("hmm...we expected some metadata!");
245+
/// match migrations.metadata::<MigrationMetadata>(rowid) {
246+
/// // rowid is in range, decoding succeeded
247+
/// Some(Ok(decoded)) => assert_eq!(decoded.value, 42),
248+
/// // rowid is in range, decoding failed
249+
/// Some(Err(e)) => panic!("error decoding metadata: {:?}", e),
250+
/// None => panic!("row id out of range")
249251
/// }
250252
///
251253
/// # }

‎src/mutation_table.rs

Lines changed: 27 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -30,17 +30,25 @@ impl PartialEq for MutationTableRow {
3030
}
3131

3232
fn make_mutation_table_row(table: &MutationTable, pos: tsk_id_t) -> Option<MutationTableRow> {
33-
let table_ref = table.table_;
34-
Some(MutationTableRow {
35-
id: pos.into(),
36-
site: table.site(pos).ok()?,
37-
node: table.node(pos).ok()?,
38-
parent: table.parent(pos).ok()?,
39-
time: table.time(pos).ok()?,
40-
derived_state: table.derived_state(pos).ok()?.map(|s| s.to_vec()),
41-
metadata: table_row_decode_metadata!(table, table_ref, pos).map(|m| m.to_vec()),
42-
})
33+
let index = ll_bindings::tsk_size_t::try_from(pos).ok()?;
34+
match index {
35+
i if i < table.num_rows() => {
36+
let table_ref = table.table_;
37+
let derived_state = table.derived_state(pos).map(|s| s.to_vec());
38+
Some(MutationTableRow {
39+
id: pos.into(),
40+
site: table.site(pos).ok()?,
41+
node: table.node(pos).ok()?,
42+
parent: table.parent(pos).ok()?,
43+
time: table.time(pos).ok()?,
44+
derived_state,
45+
metadata: table_row_decode_metadata!(table, table_ref, pos).map(|m| m.to_vec()),
46+
})
47+
}
48+
_ => None,
49+
}
4350
}
51+
4452
pub(crate) type MutationTableRefIterator<'a> =
4553
crate::table_iterator::TableIterator<&'a MutationTable<'a>>;
4654
pub(crate) type MutationTableIterator<'a> = crate::table_iterator::TableIterator<MutationTable<'a>>;
@@ -143,10 +151,7 @@ impl<'a> MutationTable<'a> {
143151
///
144152
/// Will return [``IndexError``](crate::TskitError::IndexError)
145153
/// if ``row`` is out of range.
146-
pub fn derived_state<M: Into<MutationId>>(
147-
&'a self,
148-
row: M,
149-
) -> Result<Option<&[u8]>, TskitError> {
154+
pub fn derived_state<M: Into<MutationId>>(&'a self, row: M) -> Option<&[u8]> {
150155
metadata::char_column_to_slice(
151156
self,
152157
self.table_.derived_state,
@@ -160,10 +165,10 @@ impl<'a> MutationTable<'a> {
160165
pub fn metadata<T: metadata::MetadataRoundtrip>(
161166
&'a self,
162167
row: MutationId,
163-
) -> Result<Option<T>, TskitError> {
168+
) -> Option<Result<T, TskitError>> {
164169
let table_ref = self.table_;
165170
let buffer = metadata_to_vector!(self, table_ref, row.0)?;
166-
decode_metadata_row!(T, buffer)
171+
Some(decode_metadata_row!(T, buffer).map_err(|e| e.into()))
167172
}
168173

169174
/// Return an iterator over rows of the table.
@@ -226,12 +231,13 @@ build_owned_table_type!(
226231
/// let rowid = mutations.add_row_with_metadata(0, 1, 5, 10.0, None, &metadata).unwrap();
227232
/// assert_eq!(rowid, 0);
228233
///
229-
/// if let Some(decoded) = mutations.metadata::<MutationMetadata>(rowid).unwrap() {
230-
/// assert_eq!(decoded.value, 42);
231-
/// } else {
232-
/// panic!("hmm...we expected some metadata!");
234+
/// match mutations.metadata::<MutationMetadata>(rowid) {
235+
/// // rowid is in range, decoding succeeded
236+
/// Some(Ok(decoded)) => assert_eq!(decoded.value, 42),
237+
/// // rowid is in range, decoding failed
238+
/// Some(Err(e)) => panic!("error decoding metadata: {:?}", e),
239+
/// None => panic!("row id out of range")
233240
/// }
234-
///
235241
/// # }
236242
/// ```
237243
=> OwnedMutationTable,

‎src/node_table.rs

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -191,10 +191,10 @@ impl<'a> NodeTable<'a> {
191191
pub fn metadata<T: metadata::MetadataRoundtrip>(
192192
&'a self,
193193
row: NodeId,
194-
) -> Result<Option<T>, TskitError> {
194+
) -> Option<Result<T, TskitError>> {
195195
let table_ref = self.table_;
196196
let buffer = metadata_to_vector!(self, table_ref, row.0)?;
197-
decode_metadata_row!(T, buffer)
197+
Some(decode_metadata_row!(T, buffer).map_err(|e| e.into()))
198198
}
199199

200200
/// Return an iterator over rows of the table.
@@ -285,10 +285,12 @@ build_owned_table_type!(
285285
/// let rowid = nodes.add_row_with_metadata(0, 1., -1, -1, &metadata).unwrap();
286286
/// assert_eq!(rowid, 0);
287287
///
288-
/// if let Some(decoded) = nodes.metadata::<NodeMetadata>(rowid).unwrap() {
289-
/// assert_eq!(decoded.value, 42);
290-
/// } else {
291-
/// panic!("hmm...we expected some metadata!");
288+
/// match nodes.metadata::<NodeMetadata>(rowid) {
289+
/// // rowid is in range, decoding succeeded
290+
/// Some(Ok(decoded)) => assert_eq!(decoded.value, 42),
291+
/// // rowid is in range, decoding failed
292+
/// Some(Err(e)) => panic!("error decoding metadata: {:?}", e),
293+
/// None => panic!("row id out of range")
292294
/// }
293295
///
294296
/// # }

‎src/population_table.rs

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,18 @@ impl PartialEq for PopulationTableRow {
2121

2222
fn make_population_table_row(table: &PopulationTable, pos: tsk_id_t) -> Option<PopulationTableRow> {
2323
let table_ref = table.table_;
24-
Some(PopulationTableRow {
25-
id: pos.into(),
26-
metadata: table_row_decode_metadata!(table, table_ref, pos).map(|m| m.to_vec()),
27-
})
24+
let index = ll_bindings::tsk_size_t::try_from(pos).ok()?;
25+
26+
match index {
27+
i if i < table.num_rows() => {
28+
let metadata = table_row_decode_metadata!(table, table_ref, pos).map(|s| s.to_vec());
29+
Some(PopulationTableRow {
30+
id: pos.into(),
31+
metadata,
32+
})
33+
}
34+
_ => None,
35+
}
2836
}
2937

3038
pub(crate) type PopulationTableRefIterator<'a> =
@@ -75,10 +83,10 @@ impl<'a> PopulationTable<'a> {
7583
pub fn metadata<T: metadata::MetadataRoundtrip>(
7684
&'a self,
7785
row: PopulationId,
78-
) -> Result<Option<T>, TskitError> {
86+
) -> Option<Result<T, TskitError>> {
7987
let table_ref = self.table_;
8088
let buffer = metadata_to_vector!(self, table_ref, row.0)?;
81-
decode_metadata_row!(T, buffer)
89+
Some(decode_metadata_row!(T, buffer).map_err(TskitError::from))
8290
}
8391

8492
/// Return an iterator over rows of the table.
@@ -144,12 +152,13 @@ build_owned_table_type!(
144152
/// let rowid = populations.add_row_with_metadata(&metadata).unwrap();
145153
/// assert_eq!(rowid, 0);
146154
///
147-
/// if let Some(decoded) = populations.metadata::<PopulationMetadata>(rowid).unwrap() {
148-
/// assert_eq!(&decoded.name, "YRB");
149-
/// } else {
150-
/// panic!("hmm...we expected some metadata!");
155+
/// match populations.metadata::<PopulationMetadata>(rowid) {
156+
/// // rowid is in range, decoding succeeded
157+
/// Some(Ok(decoded)) => assert_eq!(&decoded.name, "YRB"),
158+
/// // rowid is in range, decoding failed
159+
/// Some(Err(e)) => panic!("error decoding metadata: {:?}", e),
160+
/// None => panic!("row id out of range")
151161
/// }
152-
///
153162
/// # }
154163
/// ```
155164
=> OwnedPopulationTable,

0 commit comments

Comments
 (0)