Serialization support for ncollide

[syberant]

This issue is related to dimforge/nphysics#203. It is about implementing (de)serialization capabilities to ncollide so they can be used when adding those capabilities to nphysics.

As said in the linked issue I'm willing to make a PR for this myself and I will use this issue to discuss the questions specific to ncollide.

The first thing I did in my temporary fork was adding #![deny(bare_trait_objects)] to src/lib.rs and using cargo fix to put dyn in front of every trait object, thereby making them easily recognisable. This was ~250 changes and originally meant only for development purposes but I think it is sufficiently handy and non-intrusive that it may be a good idea to keep it. If this is done it should most likely be in a different PR to keep changes manageable.

Then I started derive-ing the crap out of everything and doing a few manual implementations which only contained unimplemented! when I ran into problems. I'm still looking at the problem of trait objects but typetag, as suggested by @Ralith, seems to fix it without security concerns by basically making an enum out of things. I also looked at serde_traitobject but that serialized the vtable pointer and could allow arbitrary code execution (I think, about 99% sure and the crate also warns of its insecurity) and is specific to a binary. Question: Is a dependency like typetag acceptable? Otherwise I would end up probably implementing about the same thing but less mature.

I have also looked at the serialization support of some dependencies:

• petgraph supports serialization when using the feature "serde-1"
• nalgebra supports serialization when using the feature "serde-serialize"
• slab does not support serialization but has as of yet unreleased functionality which should make it easier and only leave a simple manual implementation left to do.
• smallvec supports serialization but I'm not even sure if it's in use anymore (couldn't find it at least).

I'm currently in the middle of my test week and will resume work on this next weekend, sorry for being a bit slow right now.

[Ralith]

put dyn in front of every trait object, thereby making them easily recognisable. This was ~250 changes and originally meant only for development purposes but I think it is sufficiently handy and non-intrusive that it may be a good idea to keep it.

Explicit dyn is the preferred style for those very reasons, so I strongly encourage you to submit that PR.

[sebcrozet]

Thank you for taking this @syberant!

The first thing I did in my temporary fork was adding #![deny(bare_trait_objects)] to src/lib.rs and using cargo fix to put dyn in front of every trait object, thereby making them easily recognisable. [...] If this is done it should most likely be in a different PR to keep changes manageable.

I agree adding dyn is desirable and should be in a different PR. However, I'm doing some restructuring in ncollide and merging a PR like this will lead to a lot of merge conflict I'm not willing to face right now. So I suggest we postpone the execution of cargo fix for a month or so unless it is absolutely necessary for your current work on serialization.

Question: Is a dependency like typetag acceptable? Otherwise I would end up probably implementing about the same thing but less mature.

It is acceptable as long as it is compatible with WASM.

• smallvec supports serialization but I'm not even sure if it's in use anymore (couldn't find it at least).

We used it at some point in the past. If it isn't used any more, feel free to remove the dependency.

I'm currently in the middle of my test week and will resume work on this next weekend, sorry for being a bit slow right now.

Don't worry about this, the is no rush!

[sebcrozet]

It looks like typetag relies on the ctor crate which I doubt is compatible with WASM. This means the typetag dependency (and thus everything relying on trait-object serialization) must be put behind a feature.

[Ralith]

The entirety of serialization support should be an optional feature, since serde is a large dep that is not universally required.

[sebcrozet]

@Ralith Yes, this is true. What I meant to say is that we should have two features. One feature, say, serde-serialize that enables serialization for everything but trait-objects (and that will work on WASM too). And to get serialization of trait-objects too, the user has to enable another feature, say, trait-object-serialize that will enable the typetag dependency (and thus breaks WASM compatibility).

[syberant]

Thanks for catching the issue so quickly, I hadn't thought about WASM-compatibility of serialization and was using the already existing feature serde-serialize but will now make another feature trait-object-serialize for the trait-object specific parts.

[sebcrozet]

Relevant issues regarding compatibility with WASM:

• Support #[ctor] on wasm target mmastrac/rust-ctor#14
• Support static constructors (ctors) rustwasm/wasm-bindgen#1216
• How hard would module initalizations / ctors be? koute/stdweb#321