Change usage of anyhow to more specific error enums

[kornelski]

Problem

Most of Cargo's internal helper functions return anyhow::Error. This makes it difficult for callers of such functions to implement custom, specific error handling other than by downcasting. Fallible downcasting of anyhow Error to specific error types seems fragile: the error types are not checked at compile time, so if a helper method changes its implementation, error handling in callers can break at runtime.

Places where a specific error type could be useful:

• handling of SSH auth failures. Customized error message exists, but it depends on a downcast.
• more readable and tailored output of build script error. Currently ProcessError is used for multiple purposes, and its generic verbose output has a high noise to signal ratio for build script failures. Presentation of build script errors is noisy, difficult to understand #10159
• better handling of cargo new in workspaces Feature Request: cargo new --in-workspace [directory] [name] #6378 (comment)

Proposed Solution

I suggest gradually moving away from use of anyhow::Error (edit: for the internal helper functions and library interfaces, not all of Cargo), and use concrete error types (function or module-specific structs or enums). For a start this could be limited to private APIs.

Notes

No response

[alexcrichton]

Personally I don't really know what your goal here is but you've come to a pretty large Rust project and by saying:

I suggest gradually moving away from use of anyhow::Error

You've basically said "please redo your entire error handling strategy". That's a very large change to a project like Cargo and naturally isn't really one we'd just sort of off-hand agree with and start accepting PRs for. Cargo's error handling was designed long ago and hasn't really changed since. That's not to say that it's perfect but I don't think it's necessarily the most productive route to not ask for or try to understand historical context and simply propose complete 180 turnaround changes from Cargo's error handling strategy today.

Cargo's error-handling strategy is designed to not be a maintenance burden on us while also enabling more sophisticated handling of errors where necessary. It's not really clear to me why Cargo's error handling today cannot be used to solve the issues you've mentioned here. I think it would be much better to reuse what Cargo does today instead of trying to design something new out-of-the-blue and completely change Cargo's internal idioms because of a few issues.

[kornelski]

Thanks for the response! It may sound like an ask for a rewrite, but I don't think it is that major. It's more of an ask for permission change a few places where an unnecessary layer of abstraction gets in the way of better error handling.

I hope you agree that in the wider Rust ecosystem there's now an established pattern to use enums or structs (thiserror-like approach) in libraries, and dyn Error/anyhow-like approaches in applications that consume the libraries. It's not that one is better than the other, they just have different trade-offs of exposing vs encapsulating error information. This allows libraries to give specific data in errors to make them actionable, and allows applications to ignore the details when they don't need them.

Cargo is big indeed, so it plays both roles at once, different in different parts of the codebase. In the library-like parts of Cargo, anyhow is not helpful. anyhow is fine and quite easy for the CLI parts of Cargo that mainly just print an error and exit. For more core parts that need to handle specific errors in specific ways, the "anonymizing" nature of anyhow gets in the way, and requires downcasting.

While downcasting technically works, it's not great:

1. It adds boilerplate and dead code. If you look at the PR Avoid downcasting to ProcessError #10163 you'll find that I was able to remove several instances of redundant downcasts and checks that asked "is this a ProcessError or another type of error?" in places that can only ever get a ProcessError. Because of dynamic typing of anyhow errors, it wasn't clear that such checks were unnecessary and a dead code.

2. Downcasting is not checked at compile time. If the underlying implementation details change, then anyhow can start carrying a different type of the underlying error at any time, which can make downcasts fail. While this can be compensated for with unit tests, I'd feel more confident using compile-time type-safe errors.

3. It is a hindrance for contributors:

• I've struggled with it when I was improving handling for ssh authentication errors. It required extra detective work to see what error type was behind the veil of anyhow. If the ssh code used enum-based error type instead, the solution would have been a couple of rust-analyzer-assisted clicks away. I also wasn't confident that my downcast-based solution wasn't a "hack". Also Cargo uses a line-based simple pattern matching to test error output, but lack of control of anyhow's chain of errors makes it difficult to make tests that are effective and not flaky. That one simple change was remarkably difficult to land.

• Contributor who wanted to improve error handling of cargo new in workspaces noticed the same issue with type-erased errors and would resort to "dirty string comparison or duplicating the whole logic" to handle the error. That is not a good error strategy, and anyhow stopped them from contributing a fix.

• I wanted to improve clarity of errors printed when a build.rs fails. This was again problematic due to a general-purpose anyhow error that hid build script output away deeper in the chain of error sources. This is why I've looked at refactoring ProcessError first, so that I could handle this case properly before it's wrapped in anyhow.

[alexcrichton]

Sorry for the delayed response, but there's a lot of changes and PRs in flight in Cargo which are all quite large and at least my own personal time for Cargo is quite limited. I basically don't have time to discuss the nuances of error handling in Cargo. I'm only one member of the Cargo team though and others might be interested in talking about this.

[weihanglo]

Just had a talk with @Muscraft. This is mostly my idea but it would be great if we have this kind of error overhaul. It will help things like JSON format message (#8283) or developing Cargo's own lint rules with -Zlints (#12115). It would also open a door to diagonostics translation (rust-lang/rust#100717). Probably the modularization of Cargo could benefit from it as well.

[weihanglo]

Triage: This is not extremely hard from the angle of technical aspect, though they are tremendously large amount of code need to touch. Hence @rustbot label +E-hard