diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e293be35..cc13aa4c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -102,31 +102,21 @@ The following code guidelines will help make code review smoother. #### Use of `unwrap` and `expect` -Use `unwrap` only in either of three circumstances: +In general, `unwrap` or `expect` should be seldom used. +Instead, proper handling of these cases should made, whether through better expressing +the logic using types, through error handling, or a combination of these. -1. Based on manual static analysis, you've concluded that it's impossible for -the code to panic; so unwrapping is *safe*. An example would be: +However, there are some instances where they are acceptable: - let list = vec![a, b, c]; - let first = list.first().unwrap(); - -2. The panic caused by `unwrap` would indicate a bug in the software, and it -would be impossible to continue in that case. - -3. The `unwrap` is part of test code, ie. `cfg!(test)` is `true`. - -In the first and second case, document `unwrap` call sites with a comment prefixed -with `SAFETY:` that explains why it's safe to unwrap, eg. - - // SAFETY: Node IDs are valid ref strings. - let r = RefString::try_from(node.to_string()).unwrap(); - -Use `expect` only if the function expects certain invariants that were not met, -either due to bad inputs, or a problem with the environment; and include the -expectation in the message. For example: - - logger::init(log::Level::Debug) - .expect("logger must only be initialized once"); +1. In tests, where `#[allow(clippy::unwrap_used)]` is commonly used. +2. If, after analysing the code for yourself, you have concluded that + a call to `unwrap` does not panic (not under any circumstance, for any given + input). In this case, it is still preferred to use `expect` and provide + a message which gives your argument. + Note that if your analysis does not appear very stable under possible future + refactorings and changes, it might still not be accepted. +3. The presence of `Option::None` or `Result::Err` is truly an unexpected scenario + and you intend the program to panic. #### Module imports