| |
|
| |
#### Use of `unwrap` and `expect`
|
| |
|
| - |
Use `unwrap` only in either of three circumstances:
|
| - |
|
| - |
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:
|
| + |
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.
|
| |
|
| - |
let list = vec![a, b, c];
|
| - |
let first = list.first().unwrap();
|
| + |
However, there are some instances where they are acceptable:
|
| |
|
| - |
2. The panic caused by `unwrap` would indicate a bug in the software, and it
|
| - |
would be impossible to continue in that case.
|
| + |
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.
|
| |
|
| - |
3. The `unwrap` is part of test code, ie. `cfg!(test)` is `true`.
|
| + |
#### Use of the terms "safe" and "safety"
|
| |
|
| - |
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.
|
| + |
As we are programming in Rust, the terms safe and unsafe are narrowly defined to
|
| + |
refer to the safety in the sense that is common to the Rust programming community,
|
| + |
i.e. "Safe Rust" and "Unsafe Rust", as mentioned in [The Rustonomicon].
|
| |
|
| - |
// SAFETY: Node IDs are valid ref strings.
|
| - |
let r = RefString::try_from(node.to_string()).unwrap();
|
| + |
With this in mind, we only accept the use of "safe" about code if it is related
|
| + |
to implementing something within the realm of Unsafe Rust, as well as upholding
|
| + |
guarantees demanded by Safe Rust. The comment must describe why the use of
|
| + |
Unsafe Rust is in fact safe (i.e. which invariants are considered), and under
|
| + |
which conditions.
|
| |
|
| - |
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:
|
| + |
We will not accept the use of "safe" to describe conditions under which code
|
| + |
will not panic, i.e. why it would be panic-free to use `unreachable!`, `unwrap`,
|
| + |
or `expect`. As we differentiate between "panic-free Rust" and Safe Rust.
|
| + |
We will, however, still recommend that those uses are documented – generally
|
| + |
using a `# Panics` header followed by an explanatory paragraph in a documenting
|
| + |
comment.
|
| |
|
| - |
logger::init(log::Level::Debug)
|
| - |
.expect("logger must only be initialized once");
|
| + |
[The Rustonomicon]: https://doc.rust-lang.org/nomicon/meet-safe-and-unsafe.html
|
| |
|
| |
#### Module imports
|
| |
|
| |
|
| |
**Preparing commits**
|
| |
|
| - |
1. Each commit in your patch must pass all the tests, lints and checks. This is
|
| - |
so that they can be built into binaries and to make git bisecting possible.
|
| - |
2. Do not include any commits that are fixes or refactorings of previous patch
|
| + |
1. It is preferred that each commit in your patch passes all the tests, lints
|
| + |
and checks. This is so that they can be built into binaries and to make git
|
| + |
bisecting possible. There are times when it is necessary to have commits that
|
| + |
do not pass checks, e.g. the commit would become too large.
|
| + |
2. Do not include any commits that are fixes or refactoring of previous patch
|
| |
commits. These should be squashed to the minimal diff required to make the
|
| |
change, unless it's helpful to make a large change over multiple commits,
|
| |
while still respecting (1). Do not include `fixup!` commits either.
|
| |
mainly concerns a certain area of the codebase. For example. These prefixes
|
| |
should usually be the name of the crate, minus any common prefix. Eg.
|
| |
`cli:`, and *not* `radicle-cli:`. For documentation, you can use `docs:`,
|
| - |
and for CI-related files, you can use `ci:`.
|
| + |
and for CI-related files, you can use `ci:`. Additionally, you can add
|
| + |
further scope within the crate, e.g. `node/service`.
|
| |
|
| |
To help with the above, use `git commit --amend` and `git rebase -i`. You can
|
| |
also interactively construct a commit from a working tree using `git add -p`.
|