Add strong ID type infrastructure + NodeID

[molpopgen]

This PR starts a discussion about strengthening the ID types in tskit-rust.
See #128 for background.

[molpopgen]

So, it is not quite API-preserving:

• A perhaps key issue is that we have to break the return type, TskReturnValue, defined in src/lib.rs.
• It'd be nice to be able to have as casts to, say, usize, rather than needing usize::from.
• Comparisons to NULL either need a new trait (which then needs importing), use TSK_NULL.into(), which is a bit inelegant (and API-breaking), or a new, generic, "check if the ID is null function", which is fine in the long run imposes short-term API pain.

cc @MomoLangenstein @hyanwong

[molpopgen]

Clippy is failing due to rust-lang/rust-bindgen#2049 and rust-lang/rust-bindgen#1651

[molpopgen]

A quick performance check suggests no loss of performance in the example forward sim.

[molpopgen]

The much more difficult part is to update src/node_table.rs to take the right kind of ID. Much of the API makes use of macros to generate code for patterns common to all table types. This design makes it tedious to update one table at a time without breaking others.

[juntyr]

So, it is not quite API-preserving:

• A perhaps key issue is that we have to break the return type, TskReturnValue, defined in src/lib.rs.

That is actually a good change in my opinion. I personally dislike when Results are hidden behind type aliases. I would either remove the alias or make it generic (a popular solution in other crates):

type TskReturnValue<T> = Result<T, TskitError>;

• It'd be nice to be able to have as casts to, say, usize, rather than needing usize::from.

From what I know, that is impossible. as casts are a language feature only for casting between primitive types.

• Comparisons to NULL either need a new trait (which then needs importing), use TSK_NULL.into(), which is a bit inelegant (and API-breaking), or a new, generic, "check if the ID is null function", which is fine in the long run imposes short-term API pain.

You could add this new trait, add a new function, or implement PartialEq<tsk_id_t> and PartialOrd<tsk_id_t> for the new types. Then, you can compare them directly with the const.

[juntyr]

Clippy is failing due to rust-lang/rust-bindgen#2049 and rust-lang/rust-bindgen#1651

I thought you disabled clippy checks for the auto generated code? Right now, chippy seems to fail on actual compilation errors. Anyways, there are much stronger checks that you could enable for this project. If you’d like, I could open a PR for that after we have finished this one.

[molpopgen]

I thought you disabled clippy checks for the auto generated code? Right now, chippy seems to fail on actual compilation errors. Anyways, there are much stronger checks that you could enable for this project. If you’d like, I could open a PR for that after we have finished this one.

No, clippy hits everything. This issue with bindgen is new to stable/beta. As for a PR w/more checks, that can happen any time. There's no guarantee that this one is getting merged. And, if it does, it won't be anytime soon. There's a lot of nuance here. And even if this does work out, there are 3-4 more downstream PRs needed for all the other ID types.

[juntyr]

I thought you disabled clippy checks for the auto generated code? Right now, chippy seems to fail on actual compilation errors. Anyways, there are much stronger checks that you could enable for this project. If you’d like, I could open a PR for that after we have finished this one.

No, clippy hits everything. This issue with bindgen is new to stable/beta. As for a PR w/more checks, that can happen any time. There's no guarantee that this one is getting merged. And, if it does, it won't be anytime soon. There's a lot of nuance here. And even if this does work out, there are 3-4 more downstream PRs needed for all the other ID types.

In

tskit-rust/src/bindings.rs

Line 27 in d9abda5

#![allow(clippy::all)]

you are allowing everything - but maybe it’s not caught since you are using the #! syntax outside the crate root. Maybe you could just apply the attribute to the module declaration in:

tskit-rust/src/lib.rs

Line 65 in d9abda5

pub mod bindings;

#[allow(clippy::all)]
pub mod bindings;

[molpopgen]

I thought you disabled clippy checks for the auto generated code? Right now, chippy seems to fail on actual compilation errors. Anyways, there are much stronger checks that you could enable for this project. If you’d like, I could open a PR for that after we have finished this one.

No, clippy hits everything. This issue with bindgen is new to stable/beta. As for a PR w/more checks, that can happen any time. There's no guarantee that this one is getting merged. And, if it does, it won't be anytime soon. There's a lot of nuance here. And even if this does work out, there are 3-4 more downstream PRs needed for all the other ID types.

In

tskit-rust/src/bindings.rs

Line 27 in d9abda5

#![allow(clippy::all)]

you are allowing everything - but maybe it’s not caught since you are using the #! syntax outside the crate root. Maybe you could just apply the attribute to the module declaration in:

tskit-rust/src/lib.rs

Line 65 in d9abda5

pub mod bindings;

#[allow(clippy::all)]
pub mod bindings;

I'll try this in a separate PR, as it is a separate issue. It came out of nowhere as I've not done any rust work in months, and these warnings only hit due to a recent update.

[molpopgen]

For the record, moving the "allow clippy all" doesn't fix the problem. I just tried this as a standalone change with respect to the main branch.

[juntyr]

For the record, moving the "allow clippy all" doesn't fix the problem. I just tried this as a standalone change with respect to the main branch.

Thanks for trying!

[molpopgen]

For the record, moving the "allow clippy all" doesn't fix the problem. I just tried this as a standalone change with respect to the main branch.

Thanks for trying!

It is quite annoying: cargo cllippy never triggers the warnings, but adding --all-targets does, and that option is VERY useful.

[molpopgen]

Now that all tests are passing, I'm going to let this simmer a bit. There are a few nagging issues, but none that can't be handled in a later PR:

1. No real docs for the new type.
2. Few tests that start with this type, although the From/Into business and the example simulation program gives us good coverage of round-tripping to/from the tsk_id_t.
3. It may be worth implementing Add for this type.

[juntyr]

Now that all tests are passing, I'm going to let this simmer a bit. There are a few nagging issues, but none that can't be handled in a later PR:

1. No real docs for the new type.

That’s a good idea - though 1-2 sentences and a link to the newtype pattern might suffice.

2. Few tests that start with this type, although the From/Into business and the example simulation program gives us good coverage of round-tripping to/from the tsk_id_t.
3. It may be worth implementing Add for this type.

In which situations would you want to perform arithmetic on this type?

[molpopgen]

In which situations would you want to perform arithmetic on this type?

Book-keeping of "the next offspring node" is sometimes handing for forward-time simulations. You may say something like x = x + number of births that have happened. But perhaps using i32 and From solves all of that.

[molpopgen]

That’s a good idea - though 1-2 sentences and a link to the newtype pattern might suffice.

I guess I need to figure out how to do this using rust's doc system. It is a bit limited compared to what we are used to over in Pythonlandia.

[molpopgen]

I'm pretty confident that this is the way to go. I'll sort out the other ID types in future PRs, in case we ever need to git bisect our way out of something nasty. More docs can also happen later.

[molpopgen]

Gonna merge this in a minute--thanks for the input @MomoLangenstein!