Discussion: return Result instead of unwrap(), expect(), panic() #247
Comments
|
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
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.
We already have
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. |
Sounds good. Yeah, I will aim to follow this style of making small, granular error enums. |
Addresses issue kindelia#247. (more to come later) kindelia_core: * removes all unwrap() from net.rs * defines error enums in net.rs: ProtoCommError and ParseAddressError * return Result from affected fn in net.rs * change IPV6 panic!() to unimplemented!() * accept addr arg for Node::new() so it can avoid returning Result * update test and bench as necessary kindelia: * add dep on anyhow crate * return anyhow::Result from config methods * return anyhow::Result from main methods, including main() * return anyhow::Result from FileInput::read_to_string()
Addresses kindelia#247. use anyhow::Context trait to Replace calls to map_err(|e| anyhow!("...{}", e)) with .context("..."). This does two things: 1. Makes the code and error messages more succinct and readable. 2. Preserves the original error
Addresses issue kindelia#247. (more to come later) kindelia_core: * removes all unwrap() from net.rs * defines error enums in net.rs: ProtoCommError and ParseAddressError * return Result from affected fn in net.rs * change IPV6 panic!() to unimplemented!() * accept addr arg for Node::new() so it can avoid returning Result * update test and bench as necessary kindelia: * add dep on anyhow crate * return anyhow::Result from config methods * return anyhow::Result from main methods, including main() * return anyhow::Result from FileInput::read_to_string()
Addresses kindelia#247. use anyhow::Context trait to Replace calls to map_err(|e| anyhow!("...{}", e)) with .context("..."). This does two things: 1. Makes the code and error messages more succinct and readable. 2. Preserves the original error
Addresses kindelia#247. use anyhow::Context trait to Replace calls to map_err(|e| anyhow!("...{}", e)) with .context("..."). This does two things: 1. Makes the code and error messages more succinct and readable. 2. Preserves the original error
Addresses issue kindelia#247. (more to come later) kindelia_core: * removes all unwrap() from net.rs * defines error enums in net.rs: ProtoCommError and ParseAddressError * return Result from affected fn in net.rs * change IPV6 panic!() to unimplemented!() * accept addr arg for Node::new() so it can avoid returning Result * update test and bench as necessary kindelia: * add dep on anyhow crate * return anyhow::Result from config methods * return anyhow::Result from main methods, including main() * return anyhow::Result from FileInput::read_to_string()
|
Just dropping a link to this blog post about difficulty of writing "high assurance" rust due to unwraps, panics, esp in 3rd party crates. |
Addressing issue kindelia#247. This causes clippy to emit an error if unwrap, expect, or panic is used in any code invoked by kindelia/src/main.rs test cases are not affected. (panics are allowed)
Addressing issue kindelia#247. This causes clippy to emit an error if unwrap, expect, or panic is used in any code invoked by kindelia/src/main.rs test cases are not affected. (panics are allowed)
Addressing issue kindelia#247. This causes clippy to emit an error if unwrap, expect, or panic is used in any code invoked by kindelia/src/main.rs test cases are not affected. (panics are allowed)
In another project I've worked on, it was team policy to always return a
Resultand neverunwrap(),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 aResultinstead 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:
Hopefully I'm preaching to the converted here and not saying anything controversial, but please let me know your thoughts.
The text was updated successfully, but these errors were encountered: