Add marker traits for table-specific metadata.

[molpopgen]

Currently, metadata is a single trait. When one thinks of external tools exporting to tskit, one starts to worry that loads of Vec<T: tskit::metadata::MetadataRoundTrip> accumulate, leading to the possibility that the wrong metadata will be sent to the wrong table.

This PR is an attempt to add safety to the traits, meaning that a mutation table row can only get "mutation meta data", which client code must specify via a marker trait.

It seems that we can also provide little macros like register_mutation_metadata!(...) to streamline this a bit.

[molpopgen]

cc @MomoLangenstein -- it'd be nice to see your thoughts on this. It is a minimal-ish API change, and it may affect how you are exporting your data. But it seems that the next release is about "type safety", which led me to this.

[juntyr]

cc @MomoLangenstein -- it'd be nice to see your thoughts on this. It is a minimal-ish API change, and it may affect how you are exporting your data. But it seems that the next release is about "type safety", which led me to this.

This looks like a good change to me as well. Do you plan on adding marker traits for the other metadata types as well?

I also noticed that you swapped out dynamic dispatch for generics in one location - what do you think about doing that for the entire API as well? Use generics is … well more generic and more expressive … and you can then still express the dynamic dispatch case in that.

[molpopgen]

Yes, I'll do all the table types similarly. I just wanted some feedback prior to diving in. I'll also look at generics, too.

…

On Fri, Aug 27, 2021, 10:32 PM Momo Langenstein ***@***.***> wrote: cc @MomoLangenstein <https://github.com/MomoLangenstein> -- it'd be nice to see your thoughts on this. It is a minimal-ish API change, and it may affect how you are exporting your data. But it seems that the next release is about "type safety", which led me to this. This looks like a good change to me as well. Do you plan on adding marker traits for the other metadata types as well? I also noticed that you swapped out dynamic dispatch for generics in one location - what do you think about doing that for the entire API as well? Use generics is … well more generic and more expressive … and you can then still express the dynamic dispatch case in that. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#158 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABQ6OHYDB3JPLR7YLKN37NDT7BYBTANCNFSM5C6K7ZHA> .

[molpopgen]

The change to a generic for the current diff involves a private class, and was to enable absorbing all the new trait markers. But it won't break API to make all the other functions generics as well, so may as well do it in this pull request.

[molpopgen]

@MomoLangenstein -- I now realized the issue with generics vs dyn. Having a function accept Option<& dyn Something> works, but Option<&M> does not, failing to compile when None is passed in. We really don't want metadata to be passed in as non-&, as it may be arbitrarily expensive to clone. It also feels hackish to supply a dummy type to satisfy the trait bounds to accommodate None.

[juntyr]

@MomoLangenstein -- I now realized the issue with generics vs dyn. Having a function accept Option<& dyn Something> works, but Option<&M> does not, failing to compile when None is passed in. We really don't want metadata to be passed in as non-&, as it may be arbitrarily expensive to clone. It also feels hackish to supply a dummy type to satisfy the trait bounds to accommodate None.

Ah, I see where that compilation error comes from. Have you already merged the API changes to provide methods which accept no metadata or metadata unconditionally? In that case, there are methods that always work for the user and if they want to supply an option, then it’s their responsibility to type it (since they go down the explicit route, they should pass Some in some case, meaning the Option will be typed. In this case, you could then implement the optional API using the two explicit cases (I.e. switch it around) to avoid an internal compilation failure.

Another option would be to give M the default value () and implement the metadata trait for it (which would generate empty metadata).

The reason why I’d personally still prefer generics is that they are more Rusty and provide the compiler with better optimisation potential.

[molpopgen]

Yes, the _with_some_metadata variant is merged. To make the generic version work, I need more code duplication. Right now, all variants of an "add row" function work by calling one of them. To have the different versions work as you suggest, 2 of the 3 at least would need to make unsafe calls to the C API.

I'll think more on this.

[molpopgen]

In this case, you could then implement the optional API using the two explicit cases (I.e. switch it around) to avoid an internal compilation failure.

Aha--this is the key....

[juntyr]

Is there a C++ version of the tskit API? In that case, you could use cxx to generate safe Rust calls to the API to avoid having to use unsafe code (though I’m not one of the people who scream in fear every time unsafe pops up - instead I just put a comment on top of every unsafe block to explain why it’s actually safe)

[molpopgen]

Is there a C++ version of the tskit API? In that case, you could use cxx to generate safe Rust calls to the API to avoid having to use unsafe code (though I’m not one of the people who scream in fear every time unsafe pops up - instead I just put a comment on top of every unsafe block to explain why it’s actually safe)

No, the canonical API is C, and always will be. The unsafe bit here is that None for metadata means NULL/nullptr in C/C++. So, it is easy to deal with, and safe enough. More generally, the C API does a lot with bare pointers. It's all quite normal, and well-implemented on the C side. It just invokes fear in rustc, of course.

[molpopgen]

The most recent commit changes to generics (for one of the tables). It is now a bit funky, maybe, as the back-end private struct still expects an Option, but that can be fiddled with if necessary.

[juntyr]

That change for one of the tables looks good to me

[molpopgen]

That change for one of the tables looks good to me

Thanks. I'll sort the rest out on Monday, I hope.

[daniel-goldstein]

Thanks for the ping on this, very interesting! I share the same sentiments about generics where possible, and don't see it as much of a bad thing to have two unsafe calls out to the C implementation. I like that you no longer have create an Option when you don't need one. Maybe it's since I haven't used this API, but it's unclear to me when you wouldn't statically know which of the explicit cases (no Option) you would use.

It is now a bit funky, maybe, as the back-end private struct still expects an Option, but that can be fiddled with if necessary.

Are you saying it would be feasible for EncodedMetadata to not hold an optional? That does seem simpler in this world.

[molpopgen]

Thanks for the ping on this, very interesting! I share the same sentiments about generics where possible, and don't see it as much of a bad thing to have two unsafe calls out to the C implementation. I like that you no longer have create an Option when you don't need one. Maybe it's since I haven't used this API, but it's unclear to me when you wouldn't statically know which of the explicit cases (no Option) you would use.

It is now a bit funky, maybe, as the back-end private struct still expects an Option, but that can be fiddled with if necessary.

Are you saying it would be feasible for EncodedMetadata to not hold an optional? That does seem simpler in this world.

The option exists because metadata is optional. But you are right that you should know statically which function to call. The encoded metadata class is not visible to client code, so the option is harmless for the most part, but can (and probably should) be removed.

[molpopgen]

I've eliminated the option entirely. I'm liking this mich more.

[juntyr]

It looks great - apart from:

if self.encoded.is_empty() {
   0
} else {
   self.encoded.len() as tsk_size_t
}

[molpopgen]

It looks great - apart from:

if self.encoded.is_empty() {
   0
} else {
   self.encoded.len() as tsk_size_t
}

Yeah, oops! I'll fix that.