Add custom error variants

[tcharding]

When users of the library attempt to implement FromHex they are restricted to returning an error defined by the variants of hex::Error. This gives implementors little choice if they are attempting to return an error that is application specific.

This PR is an attempt to implement the idea suggested by @Kixunil in #124

This PR makes calling the FromHex methods a PITA now because callers must resolve the naming conflict every time they use the trait, is this satisfactory or is there a better way? This seems like a standard Rust problem so there should be a standard Rust solution.

<T as FromHex>::from_byte_iter()

Resolves: #124

Note

This PR makes the usage of FromHex much more cumbersome, see the unit test changes for an example. Is this satisfactory?

[Kixunil]

I was unable to understand the benefit of the more complicated suggestion made by @Kixunil.

A huge one is not matching against arbitrary strings to check for specific errors and being able to pass more information. In practice this is very important for localization. (I did actually implement localization a long time ago and it was awful because of io::Error.) Also being able to signal that Custom is actually impossible helps with performance a bit.

Would appreciate next time asking first when you don't understand something I say. I'd be a better use of time.

[apoelstra]

Would appreciate next time asking first when you don't understand something I say. I'd be a better use of time.

In fairness, he did open a PR and made a point of highlighting you (and nobody would merge this without your appearance and your comments being addressed). I don't think the intention here was to ignore or go around you, only to attempt to make some forward progress.

[Kixunil]

@apoelstra the idea that he could have the intention of bypassing anything didn't even cross my mind. I was merely pointing out that he used his time to implement something that I find not that great before asking for clarification. I would've spent my time on writing the clarification either way so there was no way to save my time. Not a big loss here I guess but could be in other circumstances which I wanted to warn about.

[tcharding]

Its all about learning for me, I try to take the action that makes me learn the most. I have a bad tendency to not internalize things when I'm told them but soon as I've made some action (written some code in this instance) then I remember much better the lessons learned. In this case it took barely 20 minutes to hack up a solution. I appreciate both your inputs @apoelstra and @Kixunil, I do also en devour to take actions that don't impose on your time, though in this I can always improve.

[tcharding]

So good to have clippy running on CI :)

[apoelstra]

Reviewed 90f9ae0. Neat! Infallible really makes our lives easier.

I think we want to reverse the roles of FromHex and FromHexRestricted, so that the "default" one is the one that allows custom error types. Otherwise people writing generic code are unlikely to support custom error types, even when they otherwise could.

[tcharding]

If we do that we loose the backwards compatibility benefits, do we care about that? If we are going break backwards compatibility we might as well just add the custom error associated type to the FromHex trait and not bother with the other one, right? Or am I missing something? Implementors can just use Infallible themselves as the associated type if they do not need a custom error.

[apoelstra]

Ah, I see, I missed that backward compatibility was part of the motivation for this strategy.

Honestly I'm leaning toward just adding an extra variant and breaking compatibility ... this strategy maintains backward-compatibility but also doesn't provide a lot of benefit to users. If I create a type and impl FromHexRestricted on it, it's not going to be usable with any existing code that expects a FromHex, and I don't expect this situation to improve much in the future since I'd guess most FromHex-related code is write-once-never-touch-again code.

[tcharding]

Cool, will re-work the PR, converting to draft till its done. Thanks.

[Kixunil]

Good points, this library isn't stable anyway. I think we could extend Error to have Cusom = Infallible generic and appropriate variant, getting rid of MaybeCustomError. Then the existing implementations will only have to add type CustomError = Infallible;

We could add non_exhaustive while we're at it.

[tcharding]

We could add non_exhaustive while we're at it.

Threw an additional patch on #155

[tcharding]

I had a play with adding a Custom(E) variant, that makes error become pub enum Error<E>. Turns out this is a real PITA because of the std/alloc feature. The E generic type has to be constrained

error[E0207]: the type parameter `E` is not constrained by the impl trait, self type, or predicates

Which leads to this horrendous code, or am I missing something?

impl<E> Foo for Bar {
    fn foo() -> Result<(), Error<
        #[cfg(any(feature = "std", feature = "alloc"))] E: error::Error,
        #[cfg(not(any(feature = "std", feature = "alloc")))] E,
        > {
        ...
    }
}

Is there a better way?

[apoelstra]

Lol. Do we actually ever need to constrain E to be std::Error?

[tcharding]

It won't work with source correctly if not but I guess we could just constrain in with fmt::Display and be done with it?

[apoelstra]

I'm not sure exactly what you mean, but yes, I think you can restrict it only in the places where it's directly needed.

[tcharding]

This is f***ed, I've had three long goes at this and its a never ending cycle of compiler errors and ugly complex code. I'm giving up on this for now.