better understand rustfmt stability

[davepacheco]

See #76 for background. I wanted to see if there was a better approach here. I found that rustfmt intends that your code should not be reformatted if you stick to stable options. From my testing, this appears to be true both with stable options and with the unstable options that we're using in Dropshot. (We previously thought that we were seeing a bunch of churn. I now suspect that churn was a result of accidentally switching between nightly and stable versions of otherwise equivalent rustfmt releases. Details below.)

Given that, I see three options here:

1. Keep things the way they are. This means we document in the README that users must use a nightly release for rustfmt and we tell them which specific release to use. Developers are expected to configure this correctly for themselves, whether that's vim, VScode (settings for which are in this repo), or muscle memory (cargo +nightly-2020-07-26 fmt rather than cargo fmt).
2. Keep things mostly the same, but remove require_version from rustfmt.toml. We document that developers must use nightly, and that we expect any nightly is likely to work. (If we run into cases where a specific nightly starts formatting the code differently, we'll see that in the pre-integration check.) In this case, we're using nightly only for its policy that unstable options are supported, not because we need a bleeding-edge version of the software. Like option (1), developers still have to configure things locally. On the plus side, we don't have to keep bumping the specific require_version, CI config, or README instructions. Right now, there's basically no reason to ever bump these unless that build stops becoming available or we adopt a newer rustfmt that actually changes the way code is formatted. But people might be confused about why our instructions reference an ancient 2020-07 nightly build or suggest that we update it. Instead, the CI can use the known-good version until we need to update it, and everybody else can just see and remember "nightly" rather than "nightly-2020-07-26". (This might be a pretty big advantage for people that use "nightly" for other purposes -- like other projects that require "nightly" -- since they can use that toolchain for Dropshot, too.)
3. Drop all the unstable options altogether, including require_version. Keep us on stable rustfmt. This is smoothest for everybody -- you only need one toolchain, you don't need any custom local config, and there's nothing to forget about.

Option 3 sounds great, but some of the defaults really do seem unfortunate. Here's the result of reformatting today's Dropshot with the default options:
7d12647

By my spot check: most of the changes appear due to struct_lit_single_line, and they mostly look a lot less readable to me. Here's a typical example:
7d12647#diff-1feac7e55ea61939c504aaa3cfc07e35ca99d8eb3b38cb154a0866302832a73fR350

Maybe I'm overthinking it, though. That one sucks because Projects are eventually going to have lots of fields, but when they do, they won't fit on one line anyway. @ahl What do you think?

If we don't go with (3), I'm tempted by (2) so that people don't forever need to remember 2020-07-26.

---

Methodology

Feel free to skip this.

I wanted to answer the question: if we just used stable rustfmt, how often would we wind up reformatting our code?

My plan was to identify the rustfmt versions that shipped with stable Rust releases in 2020, reformat Dropshot with the first one, then create a sequence of commits showing the diffs from each stable version's rustfmt to the next. I created two branches to show this:

• rustfmt-test-stable-only tries to test this for versions of rustfmt that ship with stable Rust. This branch starts from the current tip of "master", removes all unstable options (including require_version), and then does what I said: one commit for each subsequent rustfmt version (that shipped with a stable Rust version). Each commit shows the changes between two rustfmt versions.
• rustfmt-test-unstable-options seeks to see how much we'd be broken if we kept our unstable options but used the latest nightly. This branch starts from the current tip of "master" and removes only the require_version option so that we can test with different versions. Then it proceeds as in the other case. This was trickier than above because in order to support the unstable options, we have to be using a nightly version. I didn't go track down which nightly toolchain corresponded with each stable release's rustfmt. Instead, I grabbed 13 nightly builds, spaced about a month apart, and tested those.

As promised, the code never changed in the stable branch. But to our surprise, the code also didn't change in the unstable branch. This contradicted our previous experience! Looking through logs, I now suspect that when we thought things had changed between versions, they actually changed because rustfmt was run under "stable", not "nightly", and it ignored several key options.

See above for a summary of our choices, given this information.

For posterity, here's the (awful, unpolished) script that I used:

#!/bin/bash

#
# Test various versions of `rustfmt` on a code base.
#

set -o errexit
set -o pipefail
set -o xtrace

# "releases" is the set of toolchains (recognized by rustup) that we will test against.
#releases="1.40.0 1.41.0 1.42.0 1.43.0 1.44.0 1.44.1 1.45.0 1.45.1 1.46 1.47 1.48 1.49"
releases=""
releases="$releases nightly-2020-01-02"
releases="$releases nightly-2020-02-01"
releases="$releases nightly-2020-03-01"
releases="$releases nightly-2020-04-06"
releases="$releases nightly-2020-05-01"
releases="$releases nightly-2020-06-01"
releases="$releases nightly-2020-07-01"
releases="$releases nightly-2020-08-13"
releases="$releases nightly-2020-09-07"
releases="$releases nightly-2020-10-01"
releases="$releases nightly-2020-11-09"
releases="$releases nightly-2020-12-07"
releases="$releases nightly-2021-01-01"

# "dropshot_repos" are the local clones of repositories that we will test.
# This has nothing to do with dropshot, actually.
#dropshot_repos="dropshot-stable-options dropshot-unstable-options"
dropshot_repos="dropshot-unstable-options"
last_version=""
last_release=""
for release_version in $releases; do
	cargo +$release_version version
	cargo +$release_version fmt --version

	rustfmt_version="$(cargo +$release_version fmt --version)"

	message="reformat: Rust $release_version ($rustfmt_version)"
	if [[ -n "$last_version" ]]; then
		message="$message (from Rust $last_release ($last_version))"
	fi
	last_version="$rustfmt_version"
	last_release="$release_version"

	echo "$message"

	for dropshot_repo in $dropshot_repos; do
		(cd $dropshot_repo &&
		    echo release_version=$release_version > rustfmt_output &&
		    cargo +$release_version fmt --version >> rustfmt_output 2>&1 &&
		    cargo +$release_version fmt >> rustfmt_output 2>&1 &&
		    git add rustfmt_output &&
		    git commit -a -m "$message")
	done
done

[ahl]

I can get onboard with option 3. As much as there are some formatting choices I don't love, the vast majority of my concern is for stable formatting and simplicity so the actual output matters a little less to me.

If you can't stomach 3, then agreed that 2 looks good. I think that if we go w/ 2 we should change .vscode/settings.json to

{
    "rust-analyzer.rustfmt.overrideCommand": [
        "rustup",
        "run",
        "nightly",
        "rustfmt"
    ]
}

Is that right?

[davepacheco]

Resolved with #242.