Lack of consistent error type

[jgarvin]

Summary 💡

I'm trying to use various methods in git_repository. I usually use thiserror to build an Error type for my crate using #[from] to be able to losslessly wrap every error my crate encounters, this is a pretty common idiom so I'm confident you've encountered it. When another crate I use uses the same pattern, this is pretty easy, I just extend my Error enum with the 1 new case for that crate:

#[derive(thiserror::Error, Debug)]
enum Error {
    // ... existing variants
    NewCrateError(#[from] NewCrate::Error)
}

And then I can just stick ? on any fallible operation and the conversion into my crate's Error type magically happens. I'm running into 2 problems trying to use git-repository this way:

1. There is not 1 error type for the crate with N variants (1 variant per error), but instead N totally separate error types. This means I have to add a ton of variants to my enum.
2. Some of the error types in git-repository are generic, e.g. git_repository::git_odb::find::existing::Error<git_repository::git_odb::store::find::Error> so there's not even a finite number of them, further exploding the number of variants I need to add.

I wouldn't say git-repository is doing anything "wrong", which is why this is a feature request. I don't know if there's a good motivation for doing errors this way, it just makes it more difficult to apply the idiom that I've learned for lossless error propagation.

If I could write one impl that would let me box any error coming from git-repository that would also work nicely, but as is now I think the only trait they have in common is the std::error::Error trait, so AFAIK there is no good way to only do this for git-repository errors automatically.

Motivation 🔦

Lossless error handling, generally avoiding boxing where I can. I could switch to something like anyhow, but I'm writing a library crate so I prefer to keep user's options open.

[Byron]

Thanks a lot for letting me know! The problem described here isn't necessarily specific to gitoxide crates, but it's something I'd be happy to improve on if there was a way.

About 1: various Error types per methods instead of per crate.

This is intentional and I consider an aggregate type (using thiserror anyway) a mistake. As a matter of fact, git-repository started out like that and I changed it when realizing that it's terrible to not know which of the many variants actually apply.

As a library crate, there is no other way but to treat each method call separately or to manually map these errors into a Box. The latter isn't preventing folks from matching on error variants via downcast_ref() either, it's just more cumbersome and more error prone (as it's easy to miss a type to match on).

Due to this limitation in thiserror, even if git-repository Error's were tagged with a common trait, one wouldn't be able to Box them.

About 2: generics in errors propagate which doesn't scale and is cumbersome

Yes, I hear you and it's something I also don't like. What git-repository could do is to not propagate the generic type ,which is an actual type after all. I will take a look and put this issue onto a project board that will be reviewed before trying to have a 1.0 release (if this issue doesn't get resolved earlier to the extend possible).

I just reviewed this and believe there wouldn't be any need to use generics on the side of the library user. They can use this public error type. The underpinning generics can be hidden more, but I'd see no need to do that as long as users can refer to this type using exactly one path, git_repository::object::find::existing::Error, and it doesn't need generics either.

Even though I am closing this issue as I think no action is possible and necessary respectively, please don't hesitate to keep posting here in case you have an idea on how to improve the situation. Thank you.

Edit: I see that matching on these generic errors would be less pleasant, and I wonder if there is a viable way to pull these types apart without duplicating all possible cases. I don't see a way to do this. If there was example code that matches on this error type, maybe it's more obvious what could be done?

[jgarvin]

This is intentional and I consider an aggregate type (using thiserror anyway) a mistake. As a matter of fact, git-repository started out like that and I changed it when realizing that it's terrible to not know which of the many variants actually apply.

Hmm, I can see how one big sum type erases information about what errors can actually happen for a specific method. However my desire to preserve the error info is so that users can detect and respond in the usually very rare case that they care to do something different based on how the failure happened. I think the more common case is just caring whether the operation succeeded at all. So while yes information is erased by using a sum type, the common case is for users to not care. To summarize:

1. N distinct error types makes the rare case of reacting differently to specific errors easier, but makes the common case of only caring about failure and using ? to propagate harder.

2. 1 error sum type makes the rare case when you care about handling specific errors differently more difficult (because you don't know which are relevant for a given method) but still possible (presumably if you want to behave differently in a specific case you have become aware that it can happen, probably by having run into it), and the common case of just propagating failure with ? becomes easier.

My assumption is that even in the rare situations where you want to vary behavior based on the type of error you probably only can do something useful in a subset of the possible errors, even when the error type already narrows the possibilities down to the failures actually specifically possible for that method.

Also the sum type gives you some encapsulation / implementation freedom -- you can change which errors a method produces without breaking API, as long as it's not a completely new variant. I think if a user really wants to handle Error::Crate1Error(Crate1::Error::Crate2Error(... that they are likely assuming implementation details already so 🤷‍♂️

Re: a common trait, even if thiserror won't let me sprinkle an annotation to automatically generate the From impl for me, it's still easier for me to write 1 From impl myself that boxes and know I've covered the whole git-repository crate than try to keep up with a manual list. If there were a common trait I assume I could do still do this?:

#[derive(thiserror::Error, Debug)]
enum Error {
    // ... existing variants using thiserror annotations 
    GitRepo(Box<dyn CommonTrait>) // no annotation
}

Impl<T: CommonTrait> From<T> for Error {
    // ... Implementation that boxes and creates Error::GitRepo
}

[jgarvin]

For the time being I decided it was good enough for my purposes to just always do fallible_op().map_err(|x| Error::GitError(x.to_string()))? with a variant GitError(String) which seems to work.

[Byron]

Even though the word 'terrible' isn't well suited to make an argument, it definitely describes the feelings I had when trying to use the early git_repository::Error to change behaviour based on one of its variant. It's too error prone to not feel that the code that works can break later because some other variant has been added that should also be checked for. With per-method Errors one has a better chance to get it right and it becomes feasible to even match on all variants without default to get a compile error if something is added.

As gitoxide consists of a bunch of crates, it has to go the extra mile and deal with a lot of custom Errors and error cases in the form of variants. Applications use anyhow and are done, but libraries on top of git-repository typically don't have that luxury.

Regarding the example with CommonTrait, I actually don't know if it's possible to manually implement conversions like proposed. If so, that would definitely solve one of my current problems.

If there were a common trait I assume I could do still do this?

Can you validate that this works? Because if so, I'd love to provide such a trait to allow library crates to do such conversion. That would be so much better than converting to String and loose the source-chain in the process.

By the way, I totally agree that many crates would't match on specific variants of an error, it's a rather rare event. But somehow it's not rare enough for me to not encounter it regularly, and when I do I am happy the set of possible variants is limited to only what's actually possible, with each variant yielding the possibility to describe the exact context clearly. What git2 does isn't anything I can do, but maybe there are solutions waiting to be discovered that get the best of both worlds into one codebase :).

[jgarvin]

I haven't tried the common trait technique yet but wanted to document something related here while I remember.

https://docs.rs/git-repository/latest/git_repository/object/tree/diff/struct.Platform.html#method.for_each_to_obtain_tree
https://docs.rs/git-repository/latest/git_repository/object/tree/diff/for_each/enum.Error.html

for_each_to_obtain_tree is generic on the Error that is returned from the user's callback, but then the git_repository::object::tree::diff::for_each::Error erases the user's error type behind a box. If the method is already generic on my error type anyway, and you're okay with generic error types, why not make your error generic on my error? In other words why not?:

/// The error return by methods on the [diff platform][Platform].
#[derive(Debug, thiserror::Error)]
#[allow(missing_docs)]
pub enum Error<UserError: std::error::Error> {
    #[error(transparent)]
    Diff(#[from] git_diff::tree::changes::Error),
    #[error("The user-provided callback failed")]
    CallbackError(#[source] UserError),
}

[Byron]

I think so far no Error implementation is explicitly using generics in git-repository, even though the type-definition used for find::existing::Error is most definitely leaky when matching on it.

It's great to call out this inconsistency, as I also would prefer to either fully embrace generics in error types (in git-repository) or remove them entirely. Generally it's my intention to not expose generics and keep the types simple, and easy to use and to type, so fixing find::existing::Error seems to be the way to go.

What do you think?

[iceghost]

I have no issue with error type erasure, but for places like for_each_to_obtain_tree where we have a user error generic type returned from a callback, can we change the bound from

E: std::error::Error + Sync + Send + 'static

to

E: Into<Box<dyn std::error::Error + Sync + Send + 'static>>

?

This way, more error types are possible, including anyhow::Error and even Box<dyn Error> (I was surprised to see this not work, due to the Sized requirement).

[Byron]

That's a great idea, I didn't think of that!

The change is happening in this PR.