Create code review guidelines

Pull request

Problem

Carbon should ensure that all checked-in changes to the repository are properly code reviewed, and that the process for code review is effective across a number of dimensions:

  • Ensure high quality of code, documentation, and other artifacts. We consider these all “code reviews” regardless of whether the final artefact is “code” in a technical sense.
  • Encourage broad participation and contribution to the community through code reviews.
  • Ensure code reviews are inclusive, respectful, and welcoming.
  • Have clear, discoverable, and mechanically enforced (where possible) rules for who can, who should, and who must review any particular change.

Background

General code review:

Specific GitHub tooling:

Proposal

Add a code review guide to the project, and reference it from our contributing guide and pull request workflow documentation.

Also create initial CODEOWNERS files in both this repository and the toolchain repository based on current review activity and team roles. These are really only suggested as an initial guess and should likely be iterated frequently as more people join and begin contributing. The carbon-lang repository file is included directly, and carbon-language/carbon-toolchain#1 updates the carbon-toolchain repository.

Alternatives considered

Post-commit review

Some projects, such as LLVM, use post-commit review for some changes. This has both advantages and disadvantages.

Advantages:

  • Enables sustaining velocity in the face of high latency in code review.
    • Little need to rebase or resolve merge conflicts due to immediately landing patches even while under review.
    • Cross-developer dependencies don’t create challenges.
  • Optimizes commit velocity amongst a small group of developers with extensive experience working together and minimal review comments on others’ code.

Disadvantages:

  • Does not meaningfully scale beyond small group of developers with preexisting shared understanding of desired form of code.
    • Relies on relative infrequency of needing code review comments.
    • Developers largely need to be well aligned and reliably writing code others would already approve.
  • Disincentivizes code review relative to writing code.
    • This effect is strong enough to create a significant fraction of commits that simply see no review in the LLVM community.
  • Creates significant barriers for new contributors.
    • In practice, existing contributors are much more likely to be able to accurately create post-commit review passing changes than new contributors.
    • Because of this, new contributors will have a very difficult time joining the community.
    • This has specifically been cited by people joining the LLVM community.

Skipping review when no functionality is changed (NFC commits)

Another practice popular in the LLVM community is to skip pre-commit review for changes for which “no functionality changes” or NFC commits. Common examples are reformatting or basic code cleanup. The idea is that these are exceedingly lower risk compared changes to functionality.

Advantages:

  • Can avoid waiting for a review on trivial cleanups and refactorings.
    • May be especially useful as they tend to be merge conflict prone and likely to be lead-ups to changes sent out for review.
    • The advantage is lessened when using stacked reviews to parallelize them.
  • Avoid spending reviewer time on more trivial changes.
    • Unclear how much time this is as NFC changes are typically relatively fast to review due to their nature. Deciding “did it change behavior?” is easier than deciding “given that it changes behavior, is the change good?”.

Disadvantages:

  • No concrete and objective rubric for whether or not a change is NFC, and whether it perhaps is significant enough to still warrant review.
    • Debating this adds a new cost to the entire process.
    • In some cases, deciding whether a change is NFC roughly requires a code review, making it pointless to skip the code review.
    • A specific utility of code review is to discover when something unexpected happens. The fact that the author believes a change is NFC doesn’t address this utility as the unexpected thing may be the functionality changed.
  • Loses the knowledge sharing benefit of code review for cleanups and refactorings.
    • Especially unfortunate as these are exactly the kinds of changes often recommend for people starting to get familiar with a project.
  • Fails to ensure consistency or ease of understanding of the code.
    • Despite not changing functionality, a change may decrease how easily understood the code is or may move it to be inconsistent with the wider codebase.
    • Avoiding these two things are some of the primary goals of code review.

Rationale

This proposal contains the right goals for the code review process in light of our project goals, and the proposal is well-tailored to achieve them. Specifically:

  • Ensure high quality of code, documentation, and other artifacts. We consider these all “code reviews” regardless of whether the final artifact is “code” in a technical sense.
  • Encourage broad participation and contribution to the community through code reviews.
  • Ensure code reviews are inclusive, respectful, and welcoming.
  • Have clear, discoverable, and mechanically enforced (where possible) rules for who can, who should, and who must review any particular change.

We want pre-commit rather than post-commit review, and we want all changes to go through review. These guidelines are consistent with standard code review best practice, including what’s described in the cited sources.