From 747cea1ae7e499376ff2e403b9b955b58485efb4 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Sat, 17 Feb 2018 09:59:50 -0500 Subject: [PATCH] add some more conventions --- src/conventions.md | 106 ++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 95 insertions(+), 11 deletions(-) diff --git a/src/conventions.md b/src/conventions.md index 14ef7119..93a07ad4 100644 --- a/src/conventions.md +++ b/src/conventions.md @@ -1,17 +1,24 @@ +This file offers some tips on the coding conventions for rustc. This +chapter covers [formatting](#f), [coding for correctness](#cc), +[using crates from crates.io](#cio), and some tips on +[structuring your PR for easy review](#er). + + + +# Formatting and the tidy script + rustc is slowly moving towards the [Rust standard coding style][fmt]; -at the moment, however, it follows a rather more *chaotic* -style. There are a few things that are always true. +at the moment, however, it follows a rather more *chaotic* style. We +do have some mandatory formatting conventions, which are automatically +enforced by a script we affectionately[^not_true] call the "tidy" +script. The tidy script runs automatically when you do `./x.py test`. +[^not_true]: Secretly, I hate tidy. -nmatsakis [fmt]: https://github.com/rust-lang-nursery/fmt-rfcs -# The tidy script - -Running `./x.py test` begins with a "tidy" step. This tidy step checks -that your source files meet some basic conventions. - -## Copyright notice +### Copyright notice All files must begin with the following copyright notice: @@ -50,14 +57,91 @@ the copyright notice) like so: Prefer 4-space indent. -# Warnings and lints + -In general, Rust crates +# Coding for correctness -# Policy on using crates from crates.io +Beyond formatting, there are a few other tips that are worth +following. + +## Prefer exhaustive matches + +Using `_` in a match is convenient, but it means that when new +variants are added to the enum, they may not get handled correctly. +Ask yourself: if a new variant were added to this enum, what's the +chance that it would want to use the `_` code, versus having some +other treatment? Unless the answer is "low", then prefer an +exhaustive match. (The same advice applies to `if let` and `while +let`, which are effectively tests for a single variant.) + +## Use "TODO" comments for things you don't want to forget + +As a useful tool to yourself, you can insert a `// TODO` comment +for something that you want to get back to before you land your PR: + +```rust,ignore +fn do_something() { + if something_else { + unimplemented!(): // TODO write this + } +} +``` + +The tidy script will report an error for a `// TODO` comment, so this +code would not be able to land until the TODO is fixed (or removed). + +This can also be useful in a PR as a way to signal from one commit that you are +leaving a bug that a later commit will fix: + +```rust,ignore +if foo { + return true; // TODO wrong, but will be fixed in a later commit +} +``` + + + +# Using crates from crates.io It is allowed to use crates from crates.io, though external dependencies should not be added gratuitously. All such crates must have a suitably permissive license. There is an automatic check which inspects the Cargo metadata to ensure this. + + +# How to structure your PR + +How you prepare the commits in your PR can make a big difference for the reviewer. +Here are some tips. + +**Isolate "pure refactorings" into their own commit.** For example, if +you rename a method, then put that rename into its own commit, along +with the renames of all the uses. + +**More commits is usually better.** If you are doing a large change, +it's almost always better to break it up into smaller steps that can +be independently understood. The one thing to be aware of is that if +you introduce some code following one strategy, then change it +dramatically (versus adding to it) in a later commit, that +'back-and-forth' can be confusing. + +**If you run rustfmt and the file was not already formatted, isolate +that into its own commit.** This is really the same as the previous +rule, but it's worth highlighting. It's ok to rustfmt files, but since +we do not currently run rustfmt all the time, that can introduce a lot +of noise into your commit. Please isolate that into its own +commit. This also makes rebases a lot less painful, since rustfmt +tends to cause a lot of merge conflicts, and having those isolated +into their own commit makes htem easier to resolve. + +**No merges.** We do not allow merge commits into our history, other +than those by bors. If you get a merge conflict, rebase instead via a +command like `git rebase -i rust-lang/master` (presuming you use the +name `rust-lang` for your remote). + +**Individual commits do not have to build (but it's nice).** We do not +require that every intermediate commit successfully builds -- we only +expect to be able to bisect at a PR level. However, if you *can* make +individual commits build, that is always helpful. +