search

signalapp / libsignal

Home to the Signal Protocol as well as other cryptographic primitives which make Signal possible.
GNU Affero General Public License v3.0
3.64k stars 421 forks source link

tracking issue for some Rust API learnability improvements #285

Closed cosmicexplorer closed 2 years ago

cosmicexplorer commented 3 years ago

Goal: Add docstrings and determine appropriate visibility specifiers for the Rust API

152 describes a lack of API documentation. I found this to be a blocker when looking to depend on this crate from my own (also AGPLv3) rust project. There has clearly been a lot of extremely thoughtful work modularizing the jni, node, and ffi backends (that part was awesome!), and this is demonstrated in the much higher prevalence of docstrings in the libsignal_bridge crate than in the libsignal_protocol crate (compare the compiled docsites I've listed near the bottom of this post). I would like to help fix this.

I plan to do this mostly by adding docstrings to modules, structs, etc. Docstrings can be converted into a fantastic searchable webpage for API docs with cargo doc. To get an idea of what I'm thinking of, this is a github pages static site of the most recent docs build I have performed: https://cosmicexplorer.github.io/libsignal-client/target/doc/libsignal_protocol/.

Additional orthogonal changes

However, in my perusal, I have also found a couple other types of changes I think will improve the API's learnability:

  1. Add (documented!) traits for standard behavior across structs. There are currently several methods spelled slightly differently and implemented as pub fn methods directly on structs, e.g. https://github.com/signalapp/libsignal-client/blob/87f205e80cc21ce0b1aa1a0e81f1a291d62fba49/rust/protocol/src/protocol.rs#L121-L123
  2. Prefer to return slices with a static size over arbitrary-sized &[u8] and Box<[u8]> -- this helps keep e.g. secret key data on the stack (vs Box<[u8]>) and avoids having to handle any lifetimes (vs &[u8]). See https://github.com/signalapp/libsignal-client/blob/87f205e80cc21ce0b1aa1a0e81f1a291d62fba49/rust/protocol/src/identity_key.rs#L30-L32

This issue previously described a desire to modify module paths as well in service of generated documentation, but that was discussed and decided against here: https://github.com/signalapp/libsignal-client/pull/286#discussion_r625977154.

Progress

I'm currently working on this branch: https://github.com/cosmicexplorer/libsignal-client/tree/docs. At the time of writing, the diff contains 15 sequential commits, spanning a net +2492/-1671: https://github.com/signalapp/libsignal-client/compare/master...cosmicexplorer:docs?expand=1.

Nightly compiled docs.rs sites

These are github pages static sites of the most recent docs build I have performed (I push to gh-pages on my fork):

Individual PRs

I'm tracking the changes in order below as they are made into pull requests:

cosmicexplorer commented 3 years ago

As per @jrose-signal in https://github.com/signalapp/libsignal-client/pull/286#discussion_r625977154 I have removed the goal to change module visibility at all from this tracking issue. I do not think it is necessary at all for the goal of documentation, and I think this documentation work should ideally be in service of the existing module organization, not seeking to revise it unless there's some unforeseen issue.

cosmicexplorer commented 3 years ago

Also as I noted in https://github.com/signalapp/libsignal-client/pull/286#discussion_r625979795:

I think that cargo doc actually has flags to build things regardless of pub(crate) status, which I'm realizing is the only motivation to mess with the module structure at all.

So I'm hoping we can use pub(crate) or #[cfg(doc)] markers alone to accomplish this without disturbing much of the underlying code.

cosmicexplorer commented 2 years ago

Closing this in favor of #467!