CONTRIBUTING: reword section on unwrap and expect
Change the expectations for when these methods are used. Importantly, remove the note on SAFETY. This will be documented in another section.
This commit is contained in:
parent
21608788cb
commit
70c2f89bdf
|
|
@ -102,31 +102,21 @@ The following code guidelines will help make code review smoother.
|
||||||
|
|
||||||
#### Use of `unwrap` and `expect`
|
#### 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
|
However, there are some instances where they are acceptable:
|
||||||
the code to panic; so unwrapping is *safe*. An example would be:
|
|
||||||
|
|
||||||
let list = vec![a, b, c];
|
1. In tests, where `#[allow(clippy::unwrap_used)]` is commonly used.
|
||||||
let first = list.first().unwrap();
|
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
|
||||||
2. The panic caused by `unwrap` would indicate a bug in the software, and it
|
input). In this case, it is still preferred to use `expect` and provide
|
||||||
would be impossible to continue in that case.
|
a message which gives your argument.
|
||||||
|
Note that if your analysis does not appear very stable under possible future
|
||||||
3. The `unwrap` is part of test code, ie. `cfg!(test)` is `true`.
|
refactorings and changes, it might still not be accepted.
|
||||||
|
3. The presence of `Option::None` or `Result::Err` is truly an unexpected scenario
|
||||||
In the first and second case, document `unwrap` call sites with a comment prefixed
|
and you intend the program to panic.
|
||||||
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");
|
|
||||||
|
|
||||||
#### Module imports
|
#### Module imports
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue