# Create code review guidelines

<!--
Part of the Carbon Language project, under the Apache License v2.0 with LLVM
Exceptions. See /LICENSE for license information.
SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-->

[Pull request](https://github.com/carbon-language/carbon-lang/pull/42)

## 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:

-   Chapter 9 "Code Review" in
    _[Software Engineering at Google](https://www.amazon.com/Software-Engineering-Google-Lessons-Programming/dp/1492082791)_
-   Chapter 21 "Collaborative Construction" in
    _[Code Complete: A Practical Handbook of Software Construction](https://www.amazon.com/Code-Complete-Practical-Handbook-Construction/dp/0735619670/)_
-   [Respectful Code Reviews (Chromium)](https://chromium.googlesource.com/chromium/src/+/master/docs/cr_respect.md)
-   [Compassionate--Yet Candid--Code Reviews (video)](https://youtu.be/Ea8EiIPZvh0)
-   [The Standard of Code Review](https://google.github.io/eng-practices/review/reviewer/standard.html)
-   [How We Do Code Review](https://devblogs.microsoft.com/appcenter/how-the-visual-studio-mobile-center-team-does-code-review/)
-   [Code Reviewing in the Trenches: Understanding Challenges, Best Practices and Tool Needs](https://www.microsoft.com/en-us/research/wp-content/uploads/2016/05/MS-Code-Review-Tech-Report-MSR-TR-2016-27.pdf)
-   [The Importance of Code Reviews](https://www.sitepoint.com/the-importance-of-code-reviews/)
-   [10 Reasons Why Code Reviews Make Better Code and Better Teams](https://simpleprogrammer.com/why-code-reviews-make-better-code-teams/)
-   [Wikipedia article on code review](https://en.wikipedia.org/wiki/Code_review)
-   [Expectations, Outcomes, and Challenges of Modern Code Review](https://sback.it/publications/icse2013.pdf)

Specific GitHub tooling:

-   [About code owners](https://help.github.com/en/github/creating-cloning-and-archiving-repositories/about-code-owners)
-   [Reviewing changes in pull requests](https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/reviewing-changes-in-pull-requests)

## Proposal

Add a [code review guide](/docs/project/code_review.md) 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.
