refactor(errors)!: added proper error handling

[Andrew15-5]

Today I discovered a new issue with Typst + Hayagriva + CSL. I thought of installing the Hayagriva CLI to test what's wrong, but then I was confused by the order and requiredness of the CLI arguments. But then eventually I hit a panic with:

hayagriva bib.yaml reference --select . --csl bib-style.csl

And with backtrace I only was able to find that this is the problem:

https://github.com/typst/hayagriva/blob/38a56c8dea69531befea140aa51ca913fe9ca586/src/main.rs#L242-L252

To cut the story short, the problem was with coercing the CLI argument to &str because of Selector::parse() which also panicked with try_get_one. So the solution is to get String instead. Solved. Thought I would make a quick PR. But then I also tried this:

hayagriva bib.yaml reference --csl ieee
hayagriva bib.yaml reference --style ieee --format biblatex

And they both produced different panic errors. And indeed, throughout the main.rs (and not only there) there are a ton of .expect(). This was shocking to me, especially compared to Typst. So one thing lead to another and here we are.

I asked the rustaceans a few pointers on various things and this is the more or less final result. My main concern is the return type of public functions in io module. For super easy error handling in main() I had to convert to the final error right away (also one of the advices from the community). Which can go both ways: either we don't touch the public API and revert some changes, or we embrace the breakage and perhaps look into other places where this unified error can be used. And maybe more panics can be removed in the codebase.

The good thing is that I didn't use any new dependencies. There are 2 best ways to handle errors in the main (in the CLI):

• moving everything to run() and printing all the errors once in the main() where the run() is called;
• using main() -> ExitCode and returning either SUCCESS/FAILURE or using from(u8) which I didn't know of before.

main() -> Return<(), ExitCode> isn't great, because not only you have to map Debug impl to Display impl for thiserror's error messages to work, but also on ? in main() you always will get Error: at the end of any error message.

I think the code can significantly simplified even further if the secondary main() function is used (run()). For now I created a few helpful macros to easily return the necessary exit code from the main. BTW, I haven't found any table with "error code → error type" mappings in the docs. So different exit codes now look random, but I kept them.

I think that other than run() and deciding on the "public API return type change" there isn't anything else I think should be done. Well, OK, there are 2 other things that can be done separately: my initial issue and the renaming of the e.g./i.e (which is like about 7 lines of change).

[Andrew15-5]

https://github.com/typst/hayagriva/actions/runs/12382653188/job/34563808908?pr=262

Executing

HAYAGRIVA_ARCHIVER_UPDATE=1 cargo test --features csl-json

fixes the error.

I have no idea if it's something outdated, or it's like a snapshot or something.

[Andrew15-5]

Another question that I have is whether it is good to move all error types into the error module, or should they be imported from where they are? Wouldn't this create a cyclic dependency?

I like how I printed nested errors in Dioxus with anyhow and .with_context(). Although if we only have about 2 levels of nesting, then it's probably not worth it.

[PgBiel]

https://github.com/typst/hayagriva/actions/runs/12382653188/job/34563808908?pr=262

Executing

HAYAGRIVA_ARCHIVER_UPDATE=1 cargo test --features csl-json

fixes the error.

I have no idea if it's something outdated, or it's like a snapshot or something.

Please undo the commit and rebase on main, I've updated it now. (We've been meaning to switch this to a weekly workflow instead of something that runs alongside usual CI, but haven't found time for that yet...)

[Andrew15-5]

Please undo the commit and rebase on main

Is this really necessary? The updated file has the same SHA256 and SHA512 sum. So merging this to main won't make a difference.

[PgBiel]

It helps me while reviewing so I ensure your PR isn't doing more than it should.

[Andrew15-5]

Fair enough.

[Andrew15-5]

There is a good snippet in https://docs.rs/anyhow/latest/anyhow/struct.Error.html#display-representations:

use anyhow::{Context, Result};

fn main() {
    if let Err(err) = try_main() {
        eprintln!("ERROR: {}", err);
        err.chain().skip(1).for_each(|cause| eprintln!("because: {}", cause));
        std::process::exit(1);
    }
}

fn try_main() -> Result<()> {
    ...
}

The main point is nested function call. The Result will have the custom Error error. Unless we switch to anyhow.

[Andrew15-5]

I'd have appreciated some further discussion before doing all of this work.

It was very spontaneous and was done in one (very long) coding session (basically one day). At some point it was too late to back out, so as a result it is much easier to just make a PR rather than stalling and spending some time on prior discussions. But now it has become the reason to discuss all this.

There is a lot to cover, so I'll do that later.

[Andrew15-5]

Create a separate function which returns a Result

Do I add Anyhow?