Review process for external Rust crates

Here because you need to do a review? Skip straight to the PROCESS and REVIEW GUIDELINES.

SUMMARY

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.

OBJECTIVE

Propose and establish a set of guidelines for anyone reviewing external Rust code.

BACKGROUND

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.

PRINCIPLES

Allocate review effort according to risk. 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.

Consider future costs and benefits. 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.

Be wary of surprises. Remember that we will pay the cost of clearing up a confusing piece of code or its API many times over.

No default outcome. All of these principles apply both to the decision to use external code and to the decision to implement ourselves.

Give back. 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.

PROCESS

Crate status Architecture Quality Code OSRB*
Added to Cargo.toml
Added dependency
Version updated see below

* Complete before uploading.

OSRB review for crate updates is only required when licenses (including per-file licenses) change.

Adding a crate to Cargo.toml means Fuchsia code can use it directly. "Added dependency" refers to newly vendored crates which are not added to Cargo.toml, but are a dependency of some other crate.

When adding or updating an external crate:

  1. Follow the external Rust crates documentation to add the crate to your tree, but do not upload a change to Gerrit yet.
  2. If there are any new crates, including dependencies, or changes to licenses in new versions of existing crates, request OSRB approval.
  3. After receiving OSRB approval for any new crates, upload a change to Gerrit.
    • Cite OSRB approval bugs in the commit message.
    • Separate first-party code changes from external code changes where possible.
    • If the crate requires updating a number of transitive dependencies, consider using cargo update to batch the transitive updates into one or more batches to reduce the CL size. In cases where a dependency isn't used on our supported platforms, we can minimize review overhead by replacing the crate with a stub crate using cargo's patch feature.
  4. Add reviewers to the CL. Anyone (including you) can review, as long as they understand this section and the guidelines below.
    • You can find reviewers by asking in #rust on the Fuchsia discord (or other chat rooms where Fuchsia rustaceans can be found), or on this page.1
    • If the review requires domain-specific expertise (including unsafe code), look for reviewers with that expertise.
    • A reviewer should know which crates they are being asked to review, if not all of them. You may assign crates to individual reviewers, which helps with large CLs.
  5. Once all code has been reviewed, add one of the 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 reviewing an external crate:

  1. Review the code according to the guidelines below, applying your best judgment and seeking outside assistance when necessary.
  2. Comment on the code saying which crates you reviewed. Note any concerns you have or caveats about the review. Note any surprising behavior or bugs inline. In general, note any risks, even if you do not think they should block merging.
    1. On the top line of your CL-level comment, say which crate(s) and versions you reviewed, along with and use an asterisk if there are any caveats or risks noted in your review. You can abbreviate this by saying "all crates" or "all crates except…" This is so the OWNERS can quickly skim the comments before giving final approval.
  3. If the code looks acceptable for merging, add CR+1. If you found significant bugs or other red flags, add CR-1 and optionally suggest a resolution:
    1. Patching the crate upstream before merging.
    2. Closing the CL and looking for alternatives.
    3. In uncommon cases when the crate is a dependency that isn't used on our supported platforms, we can replace the offending crate with a customized version or a stub crate using the cargo patch feature.

When approving an external addition or update (OWNERS):

  1. Look for evidence the guidelines below have been followed on the CL.
    1. Also see the above checklist as a reminder.
    2. Prefer comments on the CL to informal communication, which leaves no audit trail, whenever possible.
    3. If evidence is missing, point the CL owner to this document and ask them to complete the process.
  2. Review any risks noted by the reviewers.
    1. If you need clarification, ask for it.
    2. If you think any risks should block merging or warrant further discussion, say so and add CR-2.
  3. If you can see that the guidelines were appropriately followed, that any risks are acceptable, and you are comfortable with merging, add CR+2.

REVIEW GUIDELINES

There are three categories of code, each of which includes the category before it: new crates being added for first-party use, new crates being added for any use, and all new code. For any given category, the guidelines of the later categories also apply to it.

Architecture review for crates used by Fuchsia code

When adding a new crate to be used by code in our tree (in other words, it is listed in Cargo.toml), new users and reviewers should ask the questions below.

The architectural review is meant to catch "obvious" problems, and is intended to be fairly 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 approvers can make a final judgment.

Note that these questions do not apply to transitive dependencies that we won't use directly. (When adding existing transitive dependencies to Cargo.toml, we should review them for these questions.)

Do we have a similar crate in the tree that could be used instead?

How many dependencies will this crate pull in, and how big are they?

Is the cost of reviewing and updating them disproportionate to the benefit we gain by using the crate?

Does this crate make sense in the context of our architecture?

Examples for code running on Fuchsia targets:

  • Usability in an async context (does the API require blocking semantics?)
  • Heavy reliance on POSIX emulation

Does this crate have a sensible API with sufficient documentation?

  • If the API is simple and self-evident, minimal documentation is OK.
  • If the API contains complex abstractions, lack of documentation has a cost.
  • If the API has undocumented invariants, especially unsafe, it's highly risky.

Quality review for newly vendored crates

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.

Make sure the crate is OSRB approved.

Think about the future.

Is the review of this crate and its dependencies worth the benefits of using its API? Include future reviews of updates.

What other contexts might this crate be used in? Consider that the implementation will not be reviewed again for each new use.

In particular, if you think a crate should only ever be used on a particular platform, or review it with that assumption in mind, this should be stated in a comment in Cargo.toml. If a crate is not safe to use in certain contexts, those must be restricted from the build or the crate cannot be imported.

What would happen if the maintainer abandons the crate? Would we be willing to fork and maintain it ourselves?

Pay attention to signals of quality.

First-order signals
  • Testing: Lack of testing in a crate is a red flag. Tests don't all need to be reviewed, but it is worth spot checking some tests for meaningful semantic checks and good coverage. Also check if a crate's tests are passing in its CI, or if they pass in a local checkout with cargo test. Solid testing is a very good sign.
Second-order signals
  • Multiple maintainers
  • Well-known authors
  • Well-used reverse dependencies ("Dependents" on crates.io)
  • Activity in the repository and issue tracker

All of these can be gleaned from crates.io, or the source repository which is usually linked to from there.

Missing second-order criteria should not disqualify the crate, but they can be a useful data point. Code review, while always required, is almost never exhaustive. These signals can help fill in gaps and tip the balance in moments of ambiguity.

Code review for all external code

When reviewing external code, whether it is a new crate or updates to an existing one:

Look for risks.

unsafe code. Unsafe code should only be used when necessary. It should be 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

It is common practice to ask someone more experienced in Rust to review unsafe code. If you aren't completely comfortable reviewing unsafe, please ask another reviewer.

Code that requires specialized domain expertise to understand. If possible, find a domain expert to review this code. Examples include unsafe, low-level atomics and concurrency, cryptography, and network protocol implementations.

Code meant to be used in critical paths. This includes security-critical paths, like cryptography, as well as performance-critical paths. Pay special attention to this code to make sure it doesn't compromise the critical path.

Overly complicated code. Idiomatic Rust leverages appropriate levels of abstraction, using the type system to manage invariants when possible. If the code is hard to follow and leaves you without confidence that those invariants are well managed, it may be of low quality and contain avoidable bugs.

Remember the alternative. 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?

Verify for correctness, but don't go overboard.

Verify that the implementation makes sense given the function signature, trait, and so on.

Look for surprises, and note any that you find in CL comments (ideally inline with the code).

Focus on the code in front of you. It's okay to assume that other functions do what they say (you'll review them eventually). Tracing the entire function call graph shouldn't be necessary; if it is, that's usually a bad sign. Use your best judgment to focus your efforts.

build.rs changes should be reflected in our build. build.rs scripts do not run in our build, but need to get translated when vendoring the crate. 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.

Skip what's irrelevant.

You don't need to review the following:

  • Code style
  • Unchanged implementation code that was already reviewed
  • Individual test cases and benchmarks
  • Platform-specific code on platforms we are sure we would never use it on4
  • Documentation that isn't directly helpful in understanding the API surface and implementation well enough to review it

Some of these are still relevant when assessing the quality of new crates, discussed above.

Note: When a second major version of a crate is added, the old version will be moved to a new path, which makes it show up as new code in Gerrit. If that code was already in our tree, you can skip it as "already reviewed". Check Cargo.lock to see which versions are pre-existing. The checksum hash for this version in Cargo.lock should not have changed.

DOCUMENT HISTORY

Prior to landing on fuchsia.dev, this document existed as an internal document for a couple of years.

Created: 2021-03-04

Author: tmandry

Contributors: adamperry, raggi, dnordstrom, dhobsd, etryzelaar

Reviewers: computerdruid, adamperry, etryzelaar, dhobsd, raggi, sadmac, bruodalbo, robtsuk, bryanhenry

Timeline:

  • 2021-03-10: Initial draft
  • 2021-04-07: Second draft
  • 2021-04-13: Final / adopted draft

APPENDIX

More reading

Notes


  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.