Change `error-chain` to `thiserror`

[RoDmitry]

Changed error-chain to thiserror, and updated dependencies. Maybe you would also want to remove error-chain from examples and tests.

Closes #66

---

This change is 

[RoDmitry]

Fixed

[RoDmitry]

Fixed. Is it better?

[RoDmitry]

Absolutely everything regarding the internal details of these errors are exposed

Still don't understand why it's bad

[faern]

Absolutely everything regarding the internal details of these errors are exposed

Still don't understand why it's bad

Again, see https://github.com/mullvad/udp-over-tcp/pull/57. If every aspect of the error is part of the public API then every small "internal" change becomes a breaking change. Let's say we wanted to change InvalidRuleCombination(String), into InvalidRuleCombination(&'static str), for some reason. Because it's part of the public API we now need to release this as a breaking change (0.5.0 instead of 0.4.7). Because otherwise it would break compilation for users who might hypothetically destructure the error and rely on the embedded enum variant type to be a String.

If we don't expose details that a user usually won't ever need to rely on, the API surface both becomes cleaner, and we are freer to change the internals without breaking stuff.

[RoDmitry]

I see the reasoning, but I don't understand why would you lose a part of the error. For InvalidRuleCombination(String), string is the part of the error's display implementation. If you drop it, how would you display it? And also I don't understand why would anybody match and rely on the internal string, if they don't need it. Maybe they actually would need that string? Because that string makes this variant different from the same variant with the different string. It's not as "internal" as you claim.

[RoDmitry]

If that is "internal", but internally we don't use it, maybe we actually don't need it then? But it's used in the display of the error message. That's why I would like to leave it as is. So a user would get more information about the error.

[pronebird]

Absolutely everything regarding the internal details of these errors are exposed

Still don't understand why it's bad

Again, see mullvad/udp-over-tcp#57. If every aspect of the error is part of the public API then every small "internal" change becomes a breaking change. Let's say we wanted to change InvalidRuleCombination(String), into InvalidRuleCombination(&'static str), for some reason. Because it's part of the public API we now need to release this as a breaking change (0.5.0 instead of 0.4.7). Because otherwise it would break compilation for users who might hypothetically destructure the error and rely on the embedded enum variant type to be a String.

If we don't expose details that a user usually won't ever need to rely on, the API surface both becomes cleaner, and we are freer to change the internals without breaking stuff.

Unless I misunderstood you. ApplyTcpOptionsErrorInternal in that PR can be replaced with much shorter implementation using thiserror (with error descriptions omitted):

#[derive(Debug, Error)]
enum ApplyTcpOptionsError {
    RecvBuffer(#[source] io::Error),
    SendBuffer(#[source] Io::Error),
    #[cfg(target_os = "linux")]
    Mark(#[source] nix::Error),
    TcpNoDelay(#[source] io::Error),
}

What you did there basically you told consumer that whatever you do internally is not their business because source() returns dyn std::error::Error. It would be unfortunate if consumer had to guess the error type to match against specific ioctl error if they want their code to behave in some fashion.

It doesn't help you either if you decide to remove some ApplyTcpOptionsErrorKind, it would be a breaking change if anyone actually matched against it.

String to &'static str does not change on a whim either, just like TcpNoDelay(io::Error) will not suddenly transform into TcpNoDelay(nix::Error).

In your case if anyone matched against io::Error, you'd silently break their assumption because source() returns dyn std::error::Error. It's good for you, but it's bad for anyone actually matching those errors.

The PR above also introduces ApplyTcpOptionsErrorKind which mirrors ApplyTcpOptionsError but strips out associated values. More manual labor/code -- less code is better in my opinion.

I'd personally be in favor of not going that route. I don't know who your users are in udp-over-tcp and what level of detail they need when it comes to error matching, but in pfctl-rs we do not handle 99% of ioctl errors beyond a few that we map to StateAlreadyActive and it would be better to be transparent and let consumers decide how they want to handle those errors. My opinion that it would be enough to have one flat error or two, i.e: crate::Error and crate::TryCopyToError.

What do you really propose to do here?

[faern]

Thanks for wanting to contribute! However, we merged a different solution to this issue over in #107. The main issue in this PR was how the entire API of the error was still public. Part of wanting to get rid of error-chain was that we wanted the API of the errors ta change. A much less public API surface of the errors allows changing the error more without breaking the API.