add documentation for error cases #471
Conversation
cosmicexplorer
force-pushed
the
document-error-cases
branch
from
ecb5fe2
to
ae0e8d6
Compare
|
(still on my todo list, but prose is harder to review than code!) |
cosmicexplorer
force-pushed
the
document-error-cases
branch
from
ae0e8d6
to
d2effe0
Compare
There was a problem hiding this comment.
Ugh, reviewing this is reminding me how much I disagree with my past self and collaborator about the design of our errors, heh. (We were both much newer to Rust then.) Several of these error cases don't need to be distinguished because clients won't treat them differently (as you yourself point out in some of these). Some should probably be panics per Rust conventions. Some don't have enough information (which key was the bad key??).
But redoing libsignal's API to be ever-more-perfect is not the best use of my time as a Signal employee, and therefore it'd be better to have documentation than not. I know I've delayed on reviewing this for quite a while, and that'll continue a little longer, but I'll pick it up again when I return from vacation.
| @@ -11,82 +15,208 @@ use uuid::Uuid; | |||
|
|
|||
| use std::panic::UnwindSafe; | |||
|
|
|||
| #[cfg(doc)] | |||
| pub use crate::{ | |||
There was a problem hiding this comment.
Nitpick: does it have to be pub for rustdoc to pick it up?
| /// Error states recognized by the Signal Protocol. | ||
| #[derive(Debug, Display, Error)] | ||
| #[ignore_extra_doc_attributes] | ||
| pub enum SignalProtocolError { |
There was a problem hiding this comment.
Heh, this sounds better designed than what's actually going on: the "SignalProtocol" here is really just about the name of the crate, libsignal-protocol. Arguably it could just be Error or something like that; I'd probably document it as "Possible errors produced by the operations in this crate" to match what you did for the Result alias.
| /// Prefer to use lifetimes, static-sized slices, and dedicated wrapper structs in API | ||
| /// signatures to minimize the need to raise this error to FFI boundaries. |
There was a problem hiding this comment.
Documentation should be client-facing, so comments about API design don't really fit.
| /// invalid argument: {0} | ||
| /// | ||
| /// Raised if an invalid argument is provided to any Signal API methods. | ||
| /// | ||
| /// Prefer to use lifetimes, static-sized slices, and dedicated wrapper structs in API | ||
| /// signatures to minimize the need to raise this error to FFI boundaries. | ||
| InvalidArgument(String), |
There was a problem hiding this comment.
(Probably every single one of these can be an assertion, since they should all indicate programmer error… Something to maybe clean up later.)
| /// invalid state for call to {0} to succeed: {1} | ||
| /// | ||
| /// Raised if some optional value was missing before performing some operation which needed it. | ||
| /// | ||
| /// Prefer to avoid returning [std::result::Result] and [Option] from struct methods in cases | ||
| /// where they're not necessary for trait polymorphism, as well as using dedicated wrapper | ||
| /// structs in API signatures, to minimize the need to raise this error. | ||
| InvalidState(&'static str, String), |
There was a problem hiding this comment.
"optional" sounds like it's about Option, but this is really "you were supposed to set up certain state before calling this function, and you didn't". For instance, you can't send a message if you haven't started a session with someone.
(These, likewise, should be audited; many of them could be assertions, but some of them might be expensive to check up front, and should become more specific, more actionable errors.)
| /// protobuf encoding was invalid | ||
| /// | ||
| /// Raised if a field in a protobuf is invalid in some way. | ||
| /// | ||
| /// Prefer to raise [Self::InvalidState] except in methods which directly decode protobufs. | ||
| InvalidProtobufEncoding, |
There was a problem hiding this comment.
This should probably be renamed InvalidEncoding, since the protobuf is an implementation detail. Raising InvalidState would be incorrect since InvalidState is about things the local client can control, and the contents of an incoming message isn't one of those.
| /// ciphertext serialized bytes were too short <{0}> | ||
| /// | ||
| /// Raised if some ciphertext was deserialized from a too-small slice. | ||
| /// | ||
| /// Prefer to make API method signatures and wrapper structs consume and produce static-sized | ||
| /// byte slices to minimize the need to raise this error. | ||
| CiphertextMessageTooShort(usize), |
There was a problem hiding this comment.
This should be unified with InvalidEncoding above; there's nothing a caller would do differently based on which failure happened. If we think the different kinds of failure are important for debugging, we could add a message to InvalidEncoding. Failing that, there shouldn't be any mention of "slice"; this just indicates a parameter was too small.
| /// | ||
| /// Raised if the ciphertext version decoded from a protobuf is older than this client. | ||
| /// | ||
| /// The current client's ciphertext version is at [CIPHERTEXT_MESSAGE_CURRENT_VERSION]. | ||
| LegacyCiphertextVersion(u8), |
There was a problem hiding this comment.
This shouldn't mention "protobuf", and the particular current version depends on which protocol is raising this error. Signal, SenderKey, and SealedSender messages all have different versions.
| /// error while invoking an ffi callback: {0} | ||
| /// | ||
| /// Raised to propagate an error from an FFI callback. | ||
| FfiBindingError(String), |
There was a problem hiding this comment.
This is not about propagating errors; it's when the callback itself can't be invoked. Think JNI-level errors. (It does not actually come up for pure Rust users and maybe doesn't belong here.)
| /// error in method call '{0}': {1} | ||
| /// | ||
| /// Raised to propagate an error through to an FFI exception along with a boxed handle. | ||
| ApplicationCallbackError( | ||
| &'static str, | ||
| #[source] Box<dyn std::error::Error + Send + Sync + UnwindSafe + 'static>, | ||
| ), |
There was a problem hiding this comment.
This is the one where there was an error from a callback and we're just propagating it out, whether through FFI or from a Rust client that returns Err.
| /// invalid sealed sender message: {0} | ||
| /// | ||
| /// Raised if an [crate::sealed_sender::UnidentifiedSenderMessage] could not be | ||
| /// deserialized successfully. | ||
| /// | ||
| /// *TODO: this sounds a lot like [Self::InvalidProtobufEncoding] or [Self::UntrustedIdentity]?* | ||
| InvalidSealedSenderMessage(String), |
There was a problem hiding this comment.
Yep, InvalidEncoding with a message again. :-(
| /// unknown sealed sender message version {0} | ||
| /// | ||
| /// Raised if an version decoded from a [crate::sealed_sender::UnidentifiedSenderMessage] | ||
| /// was unrecognized. | ||
| UnknownSealedSenderVersion(u8), |
There was a problem hiding this comment.
This won't be decoded from UnidentifiedSenderMessage; it's in the outermost envelope.
| /// self send of a sealed sender message | ||
| /// | ||
| /// Raised if [sealed_sender_decrypt] finds that the message came from this exact client. | ||
| SealedSenderSelfSend, |
Broken out of #287.
While we have a terse description of each case of
libsignal_protocol::Errorto implementdisplaydoc::Display, we may also wish to have more in-depth explanation for the purposes of generated documentation. The#[ignore_extra_doc_attributes]flag enables us to provide this additional documentation while retaining the first line for thedisplaydoc::Displayimpl.