vigna / webgraph-rs

A Rust port of the WebGraph framework
Apache License 2.0
32 stars 6 forks source link

Replace anyhow::Error with enums in bvgraph::load #113

Closed progval closed 2 months ago

progval commented 2 months ago

This allows crates using epserde to unwrap errors and match them.

In my case, I would like to display a custom message on WrongTypeHash/VersionMismatch with instructions to regenerate files with the current epserde version. Currently this is not enough for this use case (because epserde still returns anyhow::Error), but https://github.com/vigna/epserde-rs/pull/32 will take care of it once merged.

vigna commented 2 months ago

Two questions:

progval commented 2 months ago

the anyhow docs show an example of match using downcasting. Why does that not work for you?

Sorry, I missed that. It should be good enough for me. (It lacks match exhaustiveness checking, but this may actually be a blessing as it allows you to add new errors without it being a breaking change.)

I remember we introduced anyhow elsewhere because it provides backtraces and context messages. Why don't we need that anymore?

I added the context messages, but wrapping enums in other enums achieves the same result. I don't know about backtraces.

vigna commented 2 months ago

Note that I'm not against the idea. We are using anyhow anywhere because that's what Tommaso was using at that time. I have read in the past year various comments suggesting that thiserror is better for libraries and anyhow is better for applications, but frankly with little support logic. There should certainly be a chapter about that in the guidelines and these PRs might be a good starting point for the discussion.

progval commented 2 months ago

I'd say it's because anyhow loses the hierarchy of where an error bubbles through. For example, if you do both File::open("graph.graph").with_context("could not open graph.graph")? and File::open("graph.ef").with_context("could not open graph.ef")? then the caller can downcast to an io::Error, but can't tell which file failed to open

anyhow also has a higher overhead due to the dynamic dispatch, but we care in webgraph/swh-graph because it's only while loading, so not in any hot loop.