Here because you need to do a review? Skip straight to the process and review guidelines.
Rust's external ecosystem of libraries, or crates, is an enormous benefit of using the language. This body of existing code can be a force multiplier for a team of any size, accelerating and de-risking development.
At the same time, external code, like all code, comes with risks. We should use external crates, but use them wisely! This doc is about how to maximize the benefits while understanding and minimizing risks.
With roughly 1 million lines of external Rust code in our tree at the time of writing, code review of new crates and updates to existing crates takes considerable work. In order to cope with the scale of Rust in our codebase today, we must empower any team using Rust to review crates that the team depends on.
At the same time, there is uncertainty about how to review external code. Reviews of external code are different from first-party code reviews, and norms in the industry are not well established. In fact, many smaller organizations do not do external code review at all, relying entirely on other quality signals.
On a project like Fuchsia, we cannot tolerate such risks and must review code that we use to ship our products. At the same time, we should focus our efforts to maximize benefits while minimizing process overhead.
Time and effort are limited. Identify the biggest risks that external code may introduce, and focus efforts on minimizing these. Compare these risks to those of writing a new implementation.
Look beyond the here and now – a convenient shortcut today could become a big problem later. Solving a more general class of problems now can have repeated benefits over the life of the project.
Remember that we will pay the cost of clearing up a confusing piece of code or its API many times over.
All of these principles apply both to the decision to use external code and to the decision to implement ourselves.
Open source libraries can be a major benefit to the project. When we review them, make the details of those reviews accessible and legible to people outside the project.
Reviewing third-party code can be a lot of work for everyone involved. It's important to follow the process for requesting and performing third-party reviews to keep the amount of overhead and repeated work to a minimum.
When you're adding or updating an external crate and need your changes reviewed by someone else:
cargo update
to batch the transitive updates into one or more batches to reduce the CL size. To reduce review overhead, dependencies that aren‘t used on our supported platforms can be replaced with an empty crate using cargo’s manifest patching feature.rust_crates
OWNERS
for final approval.[^2] Their job is to make sure the process has been correctly followed, which should be clearly supported in CL comments. Being proactive will help ensure a quick approval.When someone else has requested a review from you, make sure to:
An external crate review involves up to four primary components:
Which components are required depend on the nature of the change:
Crate action | Architecture review required | Quality review required | Code review required | OSRB review required |
---|---|---|---|---|
Added as a direct dependency in Cargo.toml | ✅ | ✅ | ✅ | ✅ |
Vendored as an indirect dependency of another crate | ❌ | ✅ | ✅ | ✅ |
Updated version | ❌ | ❌ | ✅ | if licenses changed* |
* When updating an external crate, OSRB approval is only required if licenses change (including per-file license changes).
The architectural review is meant to catch “obvious” problems, and is intended to be lightweight. If there is uncertainty as to whether a crate makes sense from an architectural perspective, it should be noted on the CL so the author and reviewers can make a final judgment.
When adding a direct dependency for use in-tree (i.e. it is listed directly in our Cargo.toml
manifest), new users and reviewers should ask these questions:
Note that these questions do not apply to transitive dependencies that we won't use directly. We should ask and answer these questions for existing transitive dependencies when they are promoted to direct dependencies.
In addition to the review guidelines below, reviewers should give extra consideration to new crates, whether they are being used directly by us or as a dependency of another crate.
Warning: You must receive approval from the OSRB before pushing a commit to Gerrit that adds external code.
Consider how our relationship with this crate may change over time.
Consider the cost of reviewing future updates as well.
The implementation will not be reviewed again for each new use. If you think a crate should only ever be used on a particular platform, or you review it with that assumption in mind, state it in a comment in the Cargo.toml
.
Some crates are not safe to use in every possible context, such as on particular platforms. If there are any contexts where the crate is not safe to use, then we must modify the build to prevent the crate from being used in in them. Otherwise, the crate cannot be imported.
Would we be willing to fork and maintain it ourselves?
All of these quality signals can be found using crates.io or the source repository which is usually linked to from there.
First-order signals provide us direct evidence of a crate's quality:
cargo test
. Solid testing is a very good sign.Second-order signals provide indirect evidence of a crate's quality, and should be used as supporting evidence. Missing second-order signals should not disqualify using a crate. Instead, these signals should help fill in gaps in confidence and tip the balance in moments of ambiguity:
When reviewing external code, whether a new crate or updates to an existing one:
Common risks present in external code include:
unsafe
codeUnsafe code should only be used when necessary. It should be easy to easy to follow and/or document its invariants and how they are preserved. Unsafe APIs should be very rare and must always document the invariants the caller needs to uphold.[^3]
Unsafe code in external crates must be reviewed by an unsafe Rust code reviewer. See “Request an unsafe code review” for more details.
As always, it's important to remember our alternatives. Assuming we need this functionality, would we be navigating the same risks if we wrote the code ourselves? Would doing so actually produce better results, and would it be worth the effort of writing and maintaining that code?
In an ideal world, we‘d be able to formally verify all of the external code that we use. That’s usually not realistic though, so we need to make the best use of our time and effort to raise our confidence while still keeping the process moving along. Do your best to:
build.rs
changes are reflected in our buildbuild.rs
scripts do not run in our build, but need to get translated when vendoring crates. cargo-gnaw
has limited support for this, but it doesn‘t catch everything. Look over changes to these and verify anything relevant to our build is reflected in our build rules. If you aren’t comfortable reviewing these, ask the CL owner to find another reviewer for them.You don't need to review the following:
Some of these are still relevant when assessing the quality of new crates, discussed above.
To make sure Fuchsia is built on external code that is sound, we do a thorough review of all unsafe code in external crates. Unsafe code usually requires special expertise to review, and so when a crate adds or updates unsafe code it must be approved by an unsafe reviewer.
To request an unsafe code review:
If your review is time-sensitive, increase the priority on your bug and leave a comment explaining your situation.
[^1]: In the near future we expect to create a Rust reviewer list that automatically finds and assigns reviewers from a pool of volunteers. [^2]: In the near future we expect Rust third party crates to be assigned more granular ownership for reviews which will allow crate upgrades to be managed by a broader set of people. [^3]: Also see our guidelines for unsafe in first-party code. [^4]: We support Fuchsia, Linux, and Mac, and should expect to support Windows at some point. Some of our Rust code is also compiled for wasm targets.