Auto merge of #65939 - anp:incremental-rustfmt-rollout, r=Mark-Simulacrum

Enable incremental rustfmt adoption

Enables an incremental rollout of rustfmt usage within the compiler via a granular ignore configuration and automated enforcement. The decision to format the repository was approved in https://github.com/rust-lang/compiler-team/issues/80#issuecomment-491324079.

This PR includes:

* an `[ignore]` section to `rustfmt.toml` including most of the repository
* `./x.py` downloads rustfmt from a specific nightly (we do not pin to beta or stable as we need unstable features)
* an `./x.py fmt [--check]` command which runs cargo-fmt
* `./x.py fmt --check` runs during the same test step as `src/tools/tidy`, on master, but not on beta or stable as we don't want to depend on nightly rustfmt there.
* a commit to format `src/librustc_fs_util` as an initial target and to ensure enforcement is working from the start

Cargo.lock

@@ -192,6 +192,7 @@ dependencies = [
  "cmake",
  "filetime",
  "getopts",
+ "ignore",
  "lazy_static 1.3.0",
  "libc",
  "num_cpus",
@@ -1525,9 +1526,9 @@ checksum = "c3360c7b59e5ffa2653671fb74b4741a5d343c03f331c0a4aeda42b5c2b0ec7d"
 
 [[package]]
 name = "ignore"
-version = "0.4.7"
+version = "0.4.10"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "8dc57fa12805f367736a38541ac1a9fc6a52812a0ca959b1d4d4b640a89eb002"
+checksum = "0ec16832258409d571aaef8273f3c3cc5b060d784e159d1a0f3b0017308f84a7"
 dependencies = [
  "crossbeam-channel",
  "globset",

rustfmt.toml

@@ -4,3 +4,86 @@
 # be picked up automatically).
 version = "Two"
 use_small_heuristics = "Max"
+
+# by default we ignore everything in the repository
+# tidy only checks files which are not ignored, each entry follows gitignore style
+ignore = [
+    # remove directories below, or opt out their subdirectories, as they are formatted
+    "src/bootstrap/",
+    "src/build_helper/",
+    "src/liballoc/",
+    "src/libarena/",
+    "src/libcore/",
+    "src/libfmt_macros/",
+    "src/libgraphviz/",
+    "src/libpanic_abort/",
+    "src/libpanic_unwind/",
+    "src/libproc_macro/",
+    "src/libprofiler_builtins/",
+    "src/librustc/",
+    "src/librustc_apfloat/",
+    "src/librustc_asan/",
+    "src/librustc_codegen_llvm/",
+    "src/librustc_codegen_ssa/",
+    "src/librustc_codegen_utils/",
+    "src/librustc_data_structures/",
+    "src/librustc_driver/",
+    "src/librustc_errors/",
+    "src/librustc_feature/",
+    "src/librustc_incremental/",
+    "src/librustc_index/",
+    "src/librustc_interface/",
+    "src/librustc_lexer/",
+    "src/librustc_lint/",
+    "src/librustc_llvm/",
+    "src/librustc_lsan/",
+    "src/librustc_macros/",
+    "src/librustc_metadata/",
+    "src/librustc_mir/",
+    "src/librustc_msan/",
+    "src/librustc_parse/",
+    "src/librustc_passes/",
+    "src/librustc_plugin/",
+    "src/librustc_plugin_impl/",
+    "src/librustc_privacy/",
+    "src/librustc_resolve/",
+    "src/librustc_save_analysis/",
+    "src/librustc_session/",
+    "src/librustc_target/",
+    "src/librustc_traits/",
+    "src/librustc_tsan/",
+    "src/librustc_typeck/",
+    "src/librustdoc/",
+    "src/libserialize/",
+    "src/libstd/",
+    "src/libsyntax/",
+    "src/libsyntax_expand/",
+    "src/libsyntax_ext/",
+    "src/libsyntax_pos/",
+    "src/libterm/",
+    "src/libtest/",
+    "src/libunwind/",
+    "src/rtstartup/",
+    "src/rustc/",
+    "src/rustllvm/",
+    "src/test/",
+    "src/tools/",
+    "src/etc",
+
+    # do not format submodules
+    "src/doc/book",
+    "src/doc/edition-guide",
+    "src/doc/embedded-book",
+    "src/doc/nomicon",
+    "src/doc/reference",
+    "src/doc/rust-by-example",
+    "src/doc/rustc-guide",
+    "src/llvm-project",
+    "src/stdarch",
+    "src/tools/cargo",
+    "src/tools/clippy",
+    "src/tools/miri",
+    "src/tools/rls",
+    "src/tools/rust-installer",
+    "src/tools/rustfmt",
+]

src/bootstrap/Cargo.toml

@@ -47,6 +47,7 @@ serde_json = "1.0.2"
 toml = "0.5"
 lazy_static = "1.3.0"
 time = "0.1"
+ignore = "0.4.10"
 
 [dev-dependencies]
 pretty_assertions = "0.5"

src/bootstrap/bootstrap.py

@@ -322,6 +322,7 @@ class RustBuild(object):
         self.date = ''
         self._download_url = ''
         self.rustc_channel = ''
+        self.rustfmt_channel = ''
         self.build = ''
         self.build_dir = os.path.join(os.getcwd(), "build")
         self.clean = False
@@ -344,6 +345,7 @@ class RustBuild(object):
         """
         rustc_channel = self.rustc_channel
         cargo_channel = self.cargo_channel
+        rustfmt_channel = self.rustfmt_channel
 
         def support_xz():
             try:
@@ -393,13 +395,29 @@ class RustBuild(object):
             with output(self.cargo_stamp()) as cargo_stamp:
                 cargo_stamp.write(self.date)
 
-    def _download_stage0_helper(self, filename, pattern, tarball_suffix):
+        if self.rustfmt() and self.rustfmt().startswith(self.bin_root()) and (
+            not os.path.exists(self.rustfmt())
+            or self.program_out_of_date(self.rustfmt_stamp())
+        ):
+            if rustfmt_channel:
+                tarball_suffix = '.tar.xz' if support_xz() else '.tar.gz'
+                [channel, date] = rustfmt_channel.split('-', 1)
+                filename = "rustfmt-{}-{}{}".format(channel, self.build, tarball_suffix)
+                self._download_stage0_helper(filename, "rustfmt-preview", tarball_suffix, date)
+                self.fix_executable("{}/bin/rustfmt".format(self.bin_root()))
+                self.fix_executable("{}/bin/cargo-fmt".format(self.bin_root()))
+                with output(self.rustfmt_stamp()) as rustfmt_stamp:
+                    rustfmt_stamp.write(self.date)
+
+    def _download_stage0_helper(self, filename, pattern, tarball_suffix, date=None):
+        if date is None:
+            date = self.date
         cache_dst = os.path.join(self.build_dir, "cache")
-        rustc_cache = os.path.join(cache_dst, self.date)
+        rustc_cache = os.path.join(cache_dst, date)
         if not os.path.exists(rustc_cache):
             os.makedirs(rustc_cache)
 
-        url = "{}/dist/{}".format(self._download_url, self.date)
+        url = "{}/dist/{}".format(self._download_url, date)
         tarball = os.path.join(rustc_cache, filename)
         if not os.path.exists(tarball):
             get("{}/{}".format(url, filename), tarball, verbose=self.verbose)
@@ -493,6 +511,16 @@ class RustBuild(object):
         """
         return os.path.join(self.bin_root(), '.cargo-stamp')
 
+    def rustfmt_stamp(self):
+        """Return the path for .rustfmt-stamp
+
+        >>> rb = RustBuild()
+        >>> rb.build_dir = "build"
+        >>> rb.rustfmt_stamp() == os.path.join("build", "stage0", ".rustfmt-stamp")
+        True
+        """
+        return os.path.join(self.bin_root(), '.rustfmt-stamp')
+
     def program_out_of_date(self, stamp_path):
         """Check if the given program stamp is out of date"""
         if not os.path.exists(stamp_path) or self.clean:
@@ -565,6 +593,12 @@ class RustBuild(object):
         """Return config path for rustc"""
         return self.program_config('rustc')
 
+    def rustfmt(self):
+        """Return config path for rustfmt"""
+        if not self.rustfmt_channel:
+            return None
+        return self.program_config('rustfmt')
+
     def program_config(self, program):
         """Return config path for the given program
 
@@ -868,6 +902,9 @@ def bootstrap(help_triggered):
     build.rustc_channel = data['rustc']
     build.cargo_channel = data['cargo']
 
+    if "rustfmt" in data:
+        build.rustfmt_channel = data['rustfmt']
+
     if 'dev' in data:
         build.set_dev_environment()
     else:
@@ -895,6 +932,8 @@ def bootstrap(help_triggered):
     env["RUSTC_BOOTSTRAP"] = '1'
     env["CARGO"] = build.cargo()
     env["RUSTC"] = build.rustc()
+    if build.rustfmt():
+        env["RUSTFMT"] = build.rustfmt()
     run(args, env=env, verbose=build.verbose)
 
 

src/bootstrap/bootstrap_test.py

@@ -20,14 +20,14 @@ class Stage0DataTestCase(unittest.TestCase):
         os.mkdir(os.path.join(self.rust_root, "src"))
         with open(os.path.join(self.rust_root, "src",
                                "stage0.txt"), "w") as stage0:
-            stage0.write("#ignore\n\ndate: 2017-06-15\nrustc: beta\ncargo: beta")
+            stage0.write("#ignore\n\ndate: 2017-06-15\nrustc: beta\ncargo: beta\nrustfmt: beta")
 
     def tearDown(self):
         rmtree(self.rust_root)
 
     def test_stage0_data(self):
         """Extract data from stage0.txt"""
-        expected = {"date": "2017-06-15", "rustc": "beta", "cargo": "beta"}
+        expected = {"date": "2017-06-15", "rustc": "beta", "cargo": "beta", "rustfmt": "beta"}
         data = bootstrap.stage0_data(self.rust_root)
         self.assertDictEqual(data, expected)
 

src/bootstrap/builder.rs

@@ -314,6 +314,7 @@ pub enum Kind {
     Check,
     Clippy,
     Fix,
+    Format,
     Test,
     Bench,
     Dist,
@@ -353,7 +354,7 @@ impl<'a> Builder<'a> {
                 tool::Miri,
                 native::Lld
             ),
-            Kind::Check | Kind::Clippy | Kind::Fix => describe!(
+            Kind::Check | Kind::Clippy | Kind::Fix | Kind::Format => describe!(
                 check::Std,
                 check::Rustc,
                 check::Rustdoc
@@ -514,7 +515,7 @@ impl<'a> Builder<'a> {
             Subcommand::Bench { ref paths, .. } => (Kind::Bench, &paths[..]),
             Subcommand::Dist { ref paths } => (Kind::Dist, &paths[..]),
             Subcommand::Install { ref paths } => (Kind::Install, &paths[..]),
-            Subcommand::Clean { .. } => panic!(),
+            Subcommand::Format { .. } | Subcommand::Clean { .. } => panic!(),
         };
 
         let builder = Builder {

src/bootstrap/config.rs

@@ -5,6 +5,7 @@
 
 use std::collections::{HashMap, HashSet};
 use std::env;
+use std::ffi::OsString;
 use std::fs;
 use std::path::{Path, PathBuf};
 use std::process;
@@ -149,6 +150,7 @@ pub struct Config {
     // These are either the stage0 downloaded binaries or the locally installed ones.
     pub initial_cargo: PathBuf,
     pub initial_rustc: PathBuf,
+    pub initial_rustfmt: Option<PathBuf>,
     pub out: PathBuf,
 }
 
@@ -348,12 +350,16 @@ struct TomlTarget {
 impl Config {
     fn path_from_python(var_key: &str) -> PathBuf {
         match env::var_os(var_key) {
-            // Do not trust paths from Python and normalize them slightly (#49785).
+            Some(var_val) => Self::normalize_python_path(var_val),
-            Some(var_val) => Path::new(&var_val).components().collect(),
             _ => panic!("expected '{}' to be set", var_key),
         }
     }
 
+    /// Normalizes paths from Python slightly. We don't trust paths from Python (#49785).
+    fn normalize_python_path(path: OsString) -> PathBuf {
+        Path::new(&path).components().collect()
+    }
+
     pub fn default_opts() -> Config {
         let mut config = Config::default();
         config.llvm_optimize = true;
@@ -380,6 +386,7 @@ impl Config {
 
         config.initial_rustc = Config::path_from_python("RUSTC");
         config.initial_cargo = Config::path_from_python("CARGO");
+        config.initial_rustfmt = env::var_os("RUSTFMT").map(Config::normalize_python_path);
 
         config
     }

src/bootstrap/flags.rs

@@ -53,6 +53,9 @@ pub enum Subcommand {
     Fix {
         paths: Vec<PathBuf>,
     },
+    Format {
+        check: bool,
+    },
     Doc {
         paths: Vec<PathBuf>,
     },
@@ -102,6 +105,7 @@ Subcommands:
     check       Compile either the compiler or libraries, using cargo check
     clippy      Run clippy
     fix         Run cargo fix
+    fmt         Run rustfmt
     test        Build and run some test suites
     bench       Build and run some benchmarks
     doc         Build documentation
@@ -160,6 +164,7 @@ To learn more about a subcommand, run `./x.py <subcommand> -h`"
                 || (s == "check")
                 || (s == "clippy")
                 || (s == "fix")
+                || (s == "fmt")
                 || (s == "test")
                 || (s == "bench")
                 || (s == "doc")
@@ -222,6 +227,9 @@ To learn more about a subcommand, run `./x.py <subcommand> -h`"
             "clean" => {
                 opts.optflag("", "all", "clean all build artifacts");
             }
+            "fmt" => {
+                opts.optflag("", "check", "check formatting instead of applying.");
+            }
             _ => {}
         };
 
@@ -323,6 +331,17 @@ Arguments:
         ./x.py fix src/libcore src/libproc_macro",
                 );
             }
+            "fmt" => {
+                subcommand_help.push_str(
+                    "\n
+Arguments:
+    This subcommand optionally accepts a `--check` flag which succeeds if formatting is correct and
+    fails if it is not. For example:
+
+        ./x.py fmt
+        ./x.py fmt --check",
+                );
+            }
             "test" => {
                 subcommand_help.push_str(
                     "\n
@@ -388,7 +407,7 @@ Arguments:
 
             let maybe_rules_help = Builder::get_help(&build, subcommand.as_str());
             extra_help.push_str(maybe_rules_help.unwrap_or_default().as_str());
-        } else if subcommand.as_str() != "clean" {
+        } else if !(subcommand.as_str() == "clean" || subcommand.as_str() == "fmt") {
             extra_help.push_str(
                 format!(
                     "Run `./x.py {} -h -v` to see a list of available paths.",
@@ -439,6 +458,11 @@ Arguments:
                     all: matches.opt_present("all"),
                 }
             }
+            "fmt" => {
+                Subcommand::Format {
+                    check: matches.opt_present("check"),
+                }
+            }
             "dist" => Subcommand::Dist { paths },
             "install" => Subcommand::Install { paths },
             _ => {

src/bootstrap/format.rs

@@ -0,0 +1,64 @@
+//! Runs rustfmt on the repository.
+
+use crate::Build;
+use std::process::Command;
+use ignore::WalkBuilder;
+use std::path::Path;
+use build_helper::t;
+
+fn rustfmt(build: &Build, path: &Path, check: bool) {
+    let rustfmt_path = build.config.initial_rustfmt.as_ref().unwrap_or_else(|| {
+        eprintln!("./x.py fmt is not supported on this channel");
+        std::process::exit(1);
+    });
+
+    let mut cmd = Command::new(&rustfmt_path);
+    // avoid the submodule config paths from coming into play,
+    // we only allow a single global config for the workspace for now
+    cmd.arg("--config-path").arg(&build.src.canonicalize().unwrap());
+    cmd.arg("--unstable-features");
+    cmd.arg("--skip-children");
+    if check {
+        cmd.arg("--check");
+    }
+    cmd.arg(&path);
+    let cmd_debug = format!("{:?}", cmd);
+    let status = cmd.status().expect("executing rustfmt");
+    assert!(status.success(), "running {} successful", cmd_debug);
+}
+
+#[derive(serde::Deserialize)]
+struct RustfmtConfig {
+    ignore: Vec<String>,
+}
+
+pub fn format(build: &Build, check: bool) {
+    let mut builder = ignore::types::TypesBuilder::new();
+    builder.add_defaults();
+    builder.select("rust");
+    let matcher = builder.build().unwrap();
+    let rustfmt_config = build.src.join("rustfmt.toml");
+    if !rustfmt_config.exists() {
+        eprintln!("Not running formatting checks; rustfmt.toml does not exist.");
+        eprintln!("This may happen in distributed tarballs.");
+        return;
+    }
+    let rustfmt_config = t!(std::fs::read_to_string(&rustfmt_config));
+    let rustfmt_config: RustfmtConfig = t!(toml::from_str(&rustfmt_config));
+    let mut ignore_fmt = ignore::overrides::OverrideBuilder::new(&build.src);
+    for ignore in rustfmt_config.ignore {
+        ignore_fmt.add(&format!("!{}", ignore)).expect(&ignore);
+    }
+    let ignore_fmt = ignore_fmt.build().unwrap();
+
+    let walker = WalkBuilder::new(&build.src)
+        .types(matcher)
+        .overrides(ignore_fmt)
+        .build();
+    for entry in walker {
+        let entry = t!(entry);
+        if entry.file_type().map_or(false, |t| t.is_file()) {
+            rustfmt(build, &entry.path(), check);
+        }
+    }
+}

src/bootstrap/lib.rs

@@ -147,6 +147,7 @@ mod builder;
 mod cache;
 mod tool;
 mod toolstate;
+mod format;
 
 #[cfg(windows)]
 mod job;
@@ -421,6 +422,10 @@ impl Build {
             job::setup(self);
         }
 
+        if let Subcommand::Format { check } = self.config.cmd {
+            return format::format(self, check);
+        }
+
         if let Subcommand::Clean { all } = self.config.cmd {
             return clean::clean(self, all);
         }

src/bootstrap/test.rs

@@ -779,6 +779,9 @@ impl Step for Tidy {
     /// This tool in `src/tools` checks up on various bits and pieces of style and
     /// otherwise just implements a few lint-like checks that are specific to the
     /// compiler itself.
+    ///
+    /// Once tidy passes, this step also runs `fmt --check` if tests are being run
+    /// for the `dev` or `nightly` channels.
     fn run(self, builder: &Builder<'_>) {
         let mut cmd = builder.tool_cmd(Tool::Tidy);
         cmd.arg(builder.src.join("src"));
@@ -792,6 +795,11 @@ impl Step for Tidy {
 
         builder.info("tidy check");
         try_run(builder, &mut cmd);
+
+        if builder.config.channel == "dev" || builder.config.channel == "nightly" {
+            builder.info("fmt check");
+            crate::format::format(&builder.build, true);
+        }
     }
 
     fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {

src/librustc_fs_util/lib.rs

@@ -1,7 +1,7 @@
-use std::path::{Path, PathBuf};
 use std::ffi::CString;
 use std::fs;
 use std::io;
+use std::path::{Path, PathBuf};
 
 // Unfortunately, on windows, it looks like msvcrt.dll is silently translating
 // verbatim paths under the hood to non-verbatim paths! This manifests itself as
@@ -21,8 +21,8 @@ use std::io;
 //   https://github.com/rust-lang/rust/issues/25505#issuecomment-102876737
 #[cfg(windows)]
 pub fn fix_windows_verbatim_for_gcc(p: &Path) -> PathBuf {
-    use std::path;
     use std::ffi::OsString;
+    use std::path;
     let mut components = p.components();
     let prefix = match components.next() {
         Some(path::Component::Prefix(p)) => p,
@@ -68,12 +68,10 @@ pub fn link_or_copy<P: AsRef<Path>, Q: AsRef<Path>>(p: P, q: Q) -> io::Result<Li
 
     match fs::hard_link(p, q) {
         Ok(()) => Ok(LinkOrCopy::Link),
-        Err(_) => {
+        Err(_) => match fs::copy(p, q) {
-            match fs::copy(p, q) {
+            Ok(_) => Ok(LinkOrCopy::Copy),
-                Ok(_) => Ok(LinkOrCopy::Copy),
+            Err(e) => Err(e),
-                Err(e) => Err(e),
+        },
-            }
-        }
     }
 }
 
@@ -86,29 +84,28 @@ pub enum RenameOrCopyRemove {
 /// Rename `p` into `q`, preferring to use `rename` if possible.
 /// If `rename` fails (rename may fail for reasons such as crossing
 /// filesystem), fallback to copy & remove
-pub fn rename_or_copy_remove<P: AsRef<Path>, Q: AsRef<Path>>(p: P,
+pub fn rename_or_copy_remove<P: AsRef<Path>, Q: AsRef<Path>>(
-                                                             q: Q)
+    p: P,
-                                                             -> io::Result<RenameOrCopyRemove> {
+    q: Q,
+) -> io::Result<RenameOrCopyRemove> {
     let p = p.as_ref();
     let q = q.as_ref();
     match fs::rename(p, q) {
         Ok(()) => Ok(RenameOrCopyRemove::Rename),
-        Err(_) => {
+        Err(_) => match fs::copy(p, q) {
-            match fs::copy(p, q) {
+            Ok(_) => {
-                Ok(_) => {
+                fs::remove_file(p)?;
-                    fs::remove_file(p)?;
+                Ok(RenameOrCopyRemove::CopyRemove)
-                    Ok(RenameOrCopyRemove::CopyRemove)
-                }
-                Err(e) => Err(e),
             }
-        }
+            Err(e) => Err(e),
+        },
     }
 }
 
 #[cfg(unix)]
 pub fn path_to_c_string(p: &Path) -> CString {
-    use std::os::unix::ffi::OsStrExt;
     use std::ffi::OsStr;
+    use std::os::unix::ffi::OsStrExt;
     let p: &OsStr = p.as_ref();
     CString::new(p.as_bytes()).unwrap()
 }

src/stage0.txt

@@ -16,6 +16,11 @@ date: 2019-12-18
 rustc: beta
 cargo: beta
 
+# We use a nightly rustfmt to format the source because it solves some bootstrapping
+# issues with use of new syntax in this repo. If you're looking at the beta/stable branch, this key should be omitted,
+# as we don't want to depend on rustfmt from nightly there.
+rustfmt: nightly-2019-12-18
+
 # When making a stable release the process currently looks like:
 #
 #   1. Produce stable build, upload it to dev-static