Discussion: return Result instead of unwrap(), expect(), panic()

[dan-da]

In another project I've worked on, it was team policy to always return a Result and never unwrap(), expect(), panic!(), etc that abort the process.

This is a little harder to do at first, but forces one to consider and handle all cases when writing code. Rather than panicking, errors can be propagated up the call stack with ?, usually to the app layer, where it makes sense to handle them -- log, notify user, etc. The resulting code tends to be more correct and rigorous.

Further if every function is initially written to return a Result, then we don't end up with a situation where a bunch of higher level code has to be re-written when a low-level method is fixed to return a Result where it previously was calling unwrap or friends. It's sort of a "longer we wait, the harder it gets" situation.

I decided to create this issue because I am presently looking at one such case for another issue. Here Node::new() should be modified to return a Result instead of calling expect. Or alternatively it could obtain the already-parsed genesis block statements as a parameter.

Given the serious and ambitious task that Kindelia is undertaking to handle the world's money and contracts, I would suggest that this would be a good policy to adopt for future PR reviews. I believe there are some clippy lints that could eventually be turned on as well.

Here is an article that talks a bit about it.

I understand this project is moving fast and probably no one wants to take the time/effort to review and update existing code even if you agree in principle.

As such, I am willing to take on that task and begin retrofitting existing code to use more Error types and return Results. I think its a chore someone like myself without deep knowledge of the system can do, and maybe learn in the process.

I believe/hope that this work can be done in a series of small PRs rather than one huge thing. Time will tell. Another (final?) piece of the work would be adding lint checks into the CI process.

Some quick/dirty numbers for unwrap/expect/panic occurrences:

$ cd kindelia
$ find . -name \*.rs | grep -v test | grep -v bench | xargs grep unwrap | wc -l
111
$ find . -name \*.rs | grep -v test | grep -v bench | xargs grep expect | wc -l
50
$ find . -name \*.rs | grep -v test | grep -v bench | xargs grep panic | wc -l
35

Hopefully I'm preaching to the converted here and not saying anything controversial, but please let me know your thoughts.

[steinerkelvin]

Yeah, we were already aware the codebase has a large number of unnecessary panics, and that's bad. And now that I know of the lib combo of thiserror with anyhow I'm more optimistic about doing these refactors without wasting a lot of time.

In another project I've worked on, it was team policy to always return a Result and never unwrap(), expect(), panic!(), etc that abort the process.

This policy is probably good and should be adopted. However, I must note that some (infrequent) cases of irrecoverable errors coming from potential implementation bugs and should indeed panic.

adding lint checks into the CI process.

We already have clippy running on CI. But some lints are disabled, especially on hvm.rs; once when organize and split that file we can enable the lints back (and maybe add more?).

I believe/hope that this work can be done in a series of small PRs rather than one huge thing.

I would really appreciate it.

There are some refactors being done by @racs4 (at #249), tho, so we should probably wait for them until we go further with this.

[dan-da]

There are some refactors being done by @racs4 (at #249), tho, so we should probably wait for them until we go further with this.

Sounds good.

Yeah, I will aim to follow this style of making small, granular error enums.

[dan-da]

Just dropping a link to this blog post about difficulty of writing "high assurance" rust due to unwraps, panics, esp in 3rd party crates.

https://blog.yossarian.net/2022/03/10/Things-I-hate-about-Rust-redux#its-difficult-to-write-high-assurance-rust