Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Typed errors #58

Closed
wants to merge 4 commits into from
Closed

Typed errors #58

wants to merge 4 commits into from

Conversation

MOZGIII
Copy link

@MOZGIII MOZGIII commented Dec 14, 2019

Closes #57

If you want to know what errors to expect - this is the best way.
Properly utilizes Error::source to expose error cause.

The interface I implemented is a bit different: it's more permissive and therefore accepts more error types.
It also exposes the ForceHttpsButUriNotHttps properly so that it can be handled (which I have to do since I have force_https set to true).

Useful in the environment where multiple connectors exist to disambiguate
which connector failed
@seanmonstar
Copy link
Member

I'd consider making a change, but not to an enum. Rather something like the error type for HttpConnector.

@MOZGIII
Copy link
Author

MOZGIII commented Dec 14, 2019

HttpConnector's error type is ConnectError, which is not exposed publicly. This is odd.
What is the problem with using the enum? It is functionally equivalent. There's a concern with an API, that enum is won't be enhanced in the future, but that can be countered with "non-exaustive hack" (https://github.com/rust-lang/rfcs/blob/master/text/2008-non-exhaustive.md). I added it as ba96eaf.

What, in particular, do you like about the HttpConnector's ConnectError? I'll try to describe what differences I see so we can go over them and decide which ones do we want. I'd like to avoid simply copy-pasting the code from HttpConnector because the trade-offs there might be different.

ConnectError is using struct instead of enum. As I understand, it's important for the ABI. Is it important for the API though? For backward compatibility, if we decide to change it into the struct at some point?
It also is boxing errors (just with extra boilerplate) instead of wrapping them. For the purposes of downcasting, there's no difference if one's using a solution like proposed here, but if error handling is done manually it will allow to avoid downcasting altogether.

The main motivation to expose the typed errors as part of the public API is to actually expose what can go wrong with the implementation and where. It is true what it somewhat restricts the ways the implementation can change it's behavior without issuing an API-breaking code change (i.e. a major version). But - and it's a big but - if the errors are not typed, and implementation is allowed to change in such a way that error behavior changes but there's no major version bump - it's kind of worse cause it might break the "unwritten" API contract for users that wouldn't expect that.

@MOZGIII
Copy link
Author

MOZGIII commented Dec 14, 2019

One other thing I've noticed is that the ConnectError struct doesn't expose programmatically accessible "kind" of error at the immediate level (on contrary to enum, which has the keys available). Instead, it relies on Box<str>s (which might also be just &'static strs) to provide a hint on what part of the system had gone wrong, and does not expose and more details except for the error cause, that can be downcasted to a particular type. This is interesting, as it limits the ways the error can be handled to just downcasting, simplifying the possible approaches to the API.

ConnectError also doesn't have a generic for the underlying resolver error type like the enum I'm proposing have. It boxes the value instead. Here we could do that too, however, we'll be adding another pointer to the heap that way, and that just defeats the purpose of complicating the solution that we started from. If anything, I'd rather not introduce typed API if adds an extra pointer. That is compared to the case when user passes Box<dyn Error + Send + Sync> into our Into<Box<dyn Error + Send + Sync>> - it would result in just one level of pointers at runtime since Into<T> for T is universally T.

@seanmonstar seanmonstar closed this Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get rid of BoxError
2 participants