Improve integration of errors with std::error::Error

[Hirevo]

Is your feature request related to a problem? Please describe.
Currently, Tantivy's error types implement failure::Fail but not std::error::Error.
This makes them not directly usable with the ? operator inside a function returning Result<_, Box<dyn std::error::Error>> or other catch-all-errors types like anyhow::Error.

Describe the solution you'd like
A solution for solving this is to directly implement std::error::Error for Tantivy's error types.
We could these impls manually but there is a library called thiserror which provides a derive macro for generating an impls for both std::error::Error and std::fmt::Display, in a same manner as with failure (#[error("error msg...")] instead of #[fail(display = "error msg...")]).
PR #756 demonstrate what the migration would look like.

[Optional] describe alternatives you've considered
Staying with failure is still an option, but forces the user to use failure::Error (and take a dependence on it) to collect Tantivy's errors as a std::error::Error.

[mrmonday]

It's worth noting that the failure crate has now been deprecated:

• Deprecate failure rust-lang-deprecated/failure#347
• https://github.com/rust-lang-nursery/failure/tree/135e2a3b9af422d9a9dc37ce7c69354c9b36e94b#failure---a-new-error-management-story /

[asafigan]

I think removing failure should be done before 1.0.0

[saresend]

Hey, is anyone working on this? I recently started working with tantivy for some personal projects and ran into this issue almost immediately. If no one currently is, I'd be happy to put up a PR.

[asafigan]

From everything I have seen, it looks like thiserror is recommended for crates. I think PR #756 should be revisited and hopefully merged. Would this necessitate bumping the version? I feel like it would be a breaking change for some people. It wouldn't break my code base because I immediately map the errors, but it could very well break someone else's.

[fulmicoton]

The way we handle Error definitely needs to be revisited. The main reason why there is no progress is because I lack bandwidth to study the right option / review PR etc.

Would this necessitate bumping the version?

Of course yes.

[saresend]

One potential idea is to just add an additional trait implementation on all error types for the time being? Instead of removing failure immediately it might stand to simply have error types implement std::error::Error and add a deprecation notice for failure. This shouldn't be breaking since downstream code that depends on failure::Error will still have appropriate trait implementations for tantivy errors, while still enabling tantivy to play nice with stdlib error traits.

[asafigan]

Looks like we can't use the deprecated attribute on a trait impl. So we would have to warn users in the documentation and/or readme.

[saresend]

Ah, I didn't realize. How do you feel about having this be a path forward, even if it required deprecation notices included in the documentation?

[fulmicoton]

It will introduce breaking change that are easy to fix on the client, but that 's ok.

Block wand seems to be close to be ready. I think investigating this issue should be the next priority.

[waywardmonkeys]

This can be closed now, right? The migration from failure to thiserror has been done a while ago ...

[Hirevo]

@waywardmonkeys You're right.
As Tantivy no longer has a dependency on failure, and as TantivyError implements std::error::Error, this issue has effectively been addressed (ever since PR #889 which was quite a while ago).
Sorry for the delay on my part to close this issue.