Hide error internals

[tcharding]

In an effort to make give us flexibility in the 1.0 release hide all the error internals.

• Patch 1: Adds getters to InvalidLengthError
• Patch 2 and 3: Preparatory cleanup
• Patch 4: Hide the errors
• Patch 5: Remove unnecessary non_exhaustive
• Patch 6: Adds getters to support rust-bitcoin (tested in: DEMO: Use hex with hidden errors rust-bitcoin#3830)

[apoelstra]

In c35eba5:

I think, rather than having a pair of accessors that return options, we should just expose HexToArrayErrorInner (maybe by another name, maybe not) and maybe make that #[non_exhaustive].

As written, we have a ton of API freedom -- as you say, we can do whatever we want to the error type and maybe eventually make both accessors just return None. However, I think we get the same freedom by returning a #[non_exhaustive] HexToArrayErrorInner, and this will be a bit nicer for users.

However, if in rust-bitcoin we're already checking with unreachable! that exactly one of these two variants is populated, that's a sign to me that we should just make the enum exhaustive. Since #[non_exhaustive] would be saying "we reserve the right to introduce panics in rust-bitcoin by updating a dependency".

[apoelstra]

You might say -- why even add the Inner type?

The reason is to let us add extra context or other fields to the main error type.

We could also make the Inner variants (not the enum itself) #[non_exhaustive] to let us extend the variants.

[tcharding]

I had a bit of a play, wrote a long response, learned something, and deleted it - WIN.

Take 2 ...

You mean this, right?

/// Hex decoding error.
#[derive(Debug, Clone, PartialEq, Eq)]
// We are free to change the inner type.
pub struct HexToBytesError(pub(crate) CharLengthError); 

#[derive(Debug, Clone, PartialEq, Eq)]
#[non_exhaustive]
pub enum CharLengthError {   // Name describing exactly these two variants.
    /// Non-hexadecimal character.
    InvalidChar(InvalidCharError),
    /// Purported hex string had odd length.
    OddLengthString(OddLengthStringError),
}

And have a getter that returns Option<CharLengthError>.

If I got you correctly then I think a few things:

• In general we should not make any enum error public, even the CharLengthError.
• Better to have multiple getters (as is done in this PR) returning options of OddLengthStringError and InvalidcharError).

Reasons:

• enums are less flexible (that's the whole reason we are hiding them, right)
• Exposing CharLengthError does not add much value but forces us to commit to more stuff. We already commit to the public OddLengthStringError and InvalidcharError (and hide their internals)

[apoelstra]

And have a getter that returns Option<CharLengthError>.

I wouldn't even have it return an Option. It forces users to just unwrap and then our "non-breaking changes" will turn non-panicking code into panicking code.

#[non_exhaustive]

Lol, and then this just does it again for no reason. If we're already returning an Option<T> why bother making T non-exhaustive? I had meant to make the variants of CharLengthError nonexhaustive.

enums are less flexible (that's the whole reason we are hiding them, right)

Yes, but if users want to match on errors then this is what we need to expose

Exposing CharLengthError does not add much value but forces us to commit to more stuff. We already commit to the public OddLengthStringError and InvalidcharError (and hide their internals)

It does provide value -- as evidenced by it being used in rust-bitcoin. If our API forces us to add unwraps in rust-bitcoin then it's not a reasonable API.

[tcharding]

if users want to match on errors

I thought that the point of hiding the errors was explicitly to prevent this? In no other code, where we have hidden errors, are we left with a public enum (IIRC).

[apoelstra]

I thought that the point of hiding the errors was explicitly to prevent this?

No, definitely not. The point is that allowing this ties our hands and (in most cases) nobody is doing it so there's no reason to do so. But in this case people clearly are using it. So then we're removing actual, necessary functionality for the sake of giving ourselves more API freedom.

[apoelstra]

When I see stuff like rust-bitcoin/rust-bitcoin#3835 it's tempting to just 1.0 the existing hex errors without closing them at all.

But I think we can do a bit of closing -- in particular it would be good to replace the "top-level errors" which are directly returned with opaque structs. Even if we have accessors that basically force the internal representation to stay the same, this gives us freedom to add additional context to them if we want to.

[tcharding]

Cool, thanks. I'll have another play with it.

[tcharding]

Check it out, this is pretty nice if I don't say so myself.

[apoelstra]

ACK 2b88813; successfully ran local tests; looks great!