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:

hayagriva/src/main.rs

Lines 242 to 252 in 38a56c8

	let selector =
	matches
	.get_one("selector")
	.cloned()
	.map(|src| match Selector::parse(src) {
	Ok(selector) => selector,
	Err(err) => {
	eprintln!("Error while parsing selector: {}", err);
	exit(7);
	}
	});

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.

[PgBiel]

I think this is a step in the right direction, though I'd have appreciated some further discussion before doing all of this work. 😅

It's true that it used to rely on .expect and the error messages produced by that aren't nice, but I believe your changes made the code more complex without much benefit, as well as unnecessarily affecting parts unrelated to the CLI (particularly io).

I believe we could try to do it, initially, as such:

1. Create a separate function which returns a Result, running it from main. This allows using ? to propagate and automatically convert errors.
2. This function would return Result<(), CliError> where CliError can be defined directly on main.rs. This would be helpful as then we can add a method .exit_code() which returns the correct exit code. The main function can then call it to determine which ExitCode to return, as well as what to print. (See remark about exit codes below.)
3. We can implement From<&str> on CliError to allow returning Result<..., &'static str> on retrieve_assets. Then retrieve_assets(...)? would also work without a lot of verbosity.
4. CliError will have a few variants for common CLI errors with specific exit codes, and maybe a generic Other variant with just a String. Conversion would be easy through From, allowing us to reuse format!.
5. Please use map_err with ? instead of matching with Ok(v) => v. Though I don't believe you will have to do that too often with this proposed architecture.

I think this should be enough. Changes to files other than main.rs would then be dropped.

P.S. Regarding exit codes, I'll try to confirm their purpose with the team to avoid any confusion, but we can preserve them for now.

[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.