Auto merge of #4232 - mikerite:dev-fmt-4, r=flip1995

Add dev fmt subcommand

changelog: none

ci/base-tests.sh

@@ -24,7 +24,6 @@ export CARGO_TARGET_DIR=`pwd`/target/
 # Perform various checks for lint registration
 ./util/dev update_lints --check
 ./util/dev --limit-stderr-length
-cargo +nightly fmt --all -- --check
 
 # Check running clippy-driver without cargo
 (
@@ -50,32 +49,3 @@ cargo +nightly fmt --all -- --check
 
   # TODO: CLIPPY_CONF_DIR / CARGO_MANIFEST_DIR
 )
-
-# make sure tests are formatted
-
-# some lints are sensitive to formatting, exclude some files
-tests_need_reformatting="false"
-# switch to nightly
-rustup override set nightly
-# avoid loop spam and allow cmds with exit status != 0
-set +ex
-
-# Excluding `ice-3891.rs` because the code triggers a rustc parse error which
-# makes rustfmt fail.
-for file in `find tests -not -path "tests/ui/crashes/ice-3891.rs" | grep "\.rs$"` ; do
-  rustfmt ${file} --check
-  if [ $? -ne 0 ]; then
-    echo "${file} needs reformatting!"
-    tests_need_reformatting="true"
-  fi
-done
-
-set -ex # reset
-
-if [ "${tests_need_reformatting}" == "true" ] ; then
-    echo "Tests need reformatting!"
-    exit 2
-fi
-
-# switch back to master
-rustup override set master

clippy_dev/Cargo.toml

@@ -9,4 +9,5 @@ clap = "2.33"
 itertools = "0.8"
 regex = "1"
 lazy_static = "1.0"
+shell-escape = "0.1"
 walkdir = "2"

clippy_dev/src/fmt.rs

@@ -0,0 +1,167 @@
+use shell_escape::escape;
+use std::ffi::OsStr;
+use std::io;
+use std::path::{Path, PathBuf};
+use std::process::{self, Command};
+use walkdir::WalkDir;
+
+#[derive(Debug)]
+pub enum CliError {
+    CommandFailed(String),
+    IoError(io::Error),
+    ProjectRootNotFound,
+    WalkDirError(walkdir::Error),
+}
+
+impl From<io::Error> for CliError {
+    fn from(error: io::Error) -> Self {
+        CliError::IoError(error)
+    }
+}
+
+impl From<walkdir::Error> for CliError {
+    fn from(error: walkdir::Error) -> Self {
+        CliError::WalkDirError(error)
+    }
+}
+
+struct FmtContext {
+    check: bool,
+    verbose: bool,
+}
+
+pub fn run(check: bool, verbose: bool) {
+    fn try_run(context: &FmtContext) -> Result<bool, CliError> {
+        let mut success = true;
+
+        let project_root = project_root()?;
+
+        success &= cargo_fmt(context, project_root.as_path())?;
+        success &= cargo_fmt(context, &project_root.join("clippy_dev"))?;
+        success &= cargo_fmt(context, &project_root.join("rustc_tools_util"))?;
+
+        for entry in WalkDir::new(project_root.join("tests")) {
+            let entry = entry?;
+            let path = entry.path();
+
+            if path.extension() != Some("rs".as_ref()) || entry.file_name() == "ice-3891.rs" {
+                continue;
+            }
+
+            success &= rustfmt(context, &path)?;
+        }
+
+        Ok(success)
+    }
+
+    fn output_err(err: CliError) {
+        match err {
+            CliError::CommandFailed(command) => {
+                eprintln!("error: A command failed! `{}`", command);
+            },
+            CliError::IoError(err) => {
+                eprintln!("error: {}", err);
+            },
+            CliError::ProjectRootNotFound => {
+                eprintln!("error: Can't determine root of project. Please run inside a Clippy working dir.");
+            },
+            CliError::WalkDirError(err) => {
+                eprintln!("error: {}", err);
+            },
+        }
+    }
+
+    let context = FmtContext { check, verbose };
+    let result = try_run(&context);
+    let code = match result {
+        Ok(true) => 0,
+        Ok(false) => {
+            eprintln!();
+            eprintln!("Formatting check failed.");
+            eprintln!("Run `./util/dev fmt` to update formatting.");
+            1
+        },
+        Err(err) => {
+            output_err(err);
+            1
+        },
+    };
+    process::exit(code);
+}
+
+fn format_command(program: impl AsRef<OsStr>, dir: impl AsRef<Path>, args: &[impl AsRef<OsStr>]) -> String {
+    let arg_display: Vec<_> = args
+        .iter()
+        .map(|a| escape(a.as_ref().to_string_lossy()).to_owned())
+        .collect();
+
+    format!(
+        "cd {} && {} {}",
+        escape(dir.as_ref().to_string_lossy()),
+        escape(program.as_ref().to_string_lossy()),
+        arg_display.join(" ")
+    )
+}
+
+fn exec(
+    context: &FmtContext,
+    program: impl AsRef<OsStr>,
+    dir: impl AsRef<Path>,
+    args: &[impl AsRef<OsStr>],
+) -> Result<bool, CliError> {
+    if context.verbose {
+        println!("{}", format_command(&program, &dir, args));
+    }
+
+    let mut child = Command::new(&program).current_dir(&dir).args(args.iter()).spawn()?;
+    let code = child.wait()?;
+    let success = code.success();
+
+    if !context.check && !success {
+        return Err(CliError::CommandFailed(format_command(&program, &dir, args)));
+    }
+
+    Ok(success)
+}
+
+fn cargo_fmt(context: &FmtContext, path: &Path) -> Result<bool, CliError> {
+    let mut args = vec!["+nightly", "fmt", "--all"];
+    if context.check {
+        args.push("--");
+        args.push("--check");
+    }
+    let success = exec(context, "cargo", path, &args)?;
+
+    Ok(success)
+}
+
+fn rustfmt(context: &FmtContext, path: &Path) -> Result<bool, CliError> {
+    let mut args = vec!["+nightly".as_ref(), path.as_os_str()];
+    if context.check {
+        args.push("--check".as_ref());
+    }
+    let success = exec(context, "rustfmt", std::env::current_dir()?, &args)?;
+    if !success {
+        eprintln!("rustfmt failed on {}", path.display());
+    }
+    Ok(success)
+}
+
+fn project_root() -> Result<PathBuf, CliError> {
+    let current_dir = std::env::current_dir()?;
+    for path in current_dir.ancestors() {
+        let result = std::fs::read_to_string(path.join("Cargo.toml"));
+        if let Err(err) = &result {
+            if err.kind() == io::ErrorKind::NotFound {
+                continue;
+            }
+        }
+
+        let content = result?;
+        if content.contains("[package]\nname = \"clippy\"") {
+            return Ok(path.to_path_buf());
+        }
+    }
+
+    Err(CliError::ProjectRootNotFound)
+}

clippy_dev/src/main.rs

@@ -4,6 +4,8 @@ extern crate regex;
 
 use clap::{App, Arg, SubCommand};
 use clippy_dev::*;
+
+mod fmt;
 mod stderr_length_check;
 
 #[derive(PartialEq)]
@@ -14,6 +16,21 @@ enum UpdateMode {
 
 fn main() {
     let matches = App::new("Clippy developer tooling")
+        .subcommand(
+            SubCommand::with_name("fmt")
+                .about("Run rustfmt on all projects and tests")
+                .arg(
+                    Arg::with_name("check")
+                        .long("check")
+                        .help("Use the rustfmt --check option"),
+                )
+                .arg(
+                    Arg::with_name("verbose")
+                        .short("v")
+                        .long("verbose")
+                        .help("Echo commands run"),
+                ),
+        )
         .subcommand(
             SubCommand::with_name("update_lints")
                 .about("Updates lint registration and information from the source code")
@@ -46,14 +63,21 @@ fn main() {
     if matches.is_present("limit-stderr-length") {
         stderr_length_check::check();
     }
-    if let Some(matches) = matches.subcommand_matches("update_lints") {
-        if matches.is_present("print-only") {
-            print_lints();
-        } else if matches.is_present("check") {
-            update_lints(&UpdateMode::Check);
-        } else {
-            update_lints(&UpdateMode::Change);
-        }
+
+    match matches.subcommand() {
+        ("fmt", Some(matches)) => {
+            fmt::run(matches.is_present("check"), matches.is_present("verbose"));
+        },
+        ("update_lints", Some(matches)) => {
+            if matches.is_present("print-only") {
+                print_lints();
+            } else if matches.is_present("check") {
+                update_lints(&UpdateMode::Check);
+            } else {
+                update_lints(&UpdateMode::Change);
+            }
+        },
+        _ => {},
     }
 }
 

clippy_dev/src/stderr_length_check.rs

@@ -23,16 +23,15 @@ pub fn check() {
 }
 
 fn exceeding_stderr_files(files: impl Iterator<Item = walkdir::DirEntry>) -> impl Iterator<Item = String> {
-    files
-        .filter_map(|file| {
-            let path = file.path().to_str().expect("Could not convert path to str").to_string();
-            let linecount = count_linenumbers(&path);
-            if linecount > LIMIT {
-                Some(path)
-            } else {
-                None
-            }
-        })
+    files.filter_map(|file| {
+        let path = file.path().to_str().expect("Could not convert path to str").to_string();
+        let linecount = count_linenumbers(&path);
+        if linecount > LIMIT {
+            Some(path)
+        } else {
+            None
+        }
+    })
 }
 
 fn stderr_files() -> impl Iterator<Item = walkdir::DirEntry> {

doc/adding_lints.md

@@ -345,16 +345,18 @@ list][lint_list].
 
 ### Running rustfmt
 
-[Rustfmt](https://github.com/rust-lang/rustfmt) is a tool for formatting Rust code according
-to style guidelines. Your code has to be formatted by `rustfmt` before a PR can be merged.
+[Rustfmt](https://github.com/rust-lang/rustfmt) is a tool for formatting Rust
+code according to style guidelines. Your code has to be formatted by `rustfmt`
+before a PR can be merged. Clippy uses nightly `rustfmt` in the CI.
 
 It can be installed via `rustup`:
 
 ```bash
-rustup component add rustfmt
+rustup component add rustfmt --toolchain=nightly
 ```
 
-Use `cargo fmt --all` to format the whole codebase.
+Use `./util/dev fmt` to format the whole codebase. Make sure that `rustfmt` is
+installed for the nightly toolchain.
 
 ### Debugging
 
@@ -371,7 +373,7 @@ Before submitting your PR make sure you followed all of the basic requirements:
 - [ ] `cargo test` passes locally
 - [ ] Executed `util/dev update_lints`
 - [ ] Added lint documentation
-- [ ] Run `cargo fmt`
+- [ ] Run `./util/dev fmt`
 
 ### Cheatsheet
 

tests/fmt.rs

@@ -0,0 +1,23 @@
+#[test]
+fn fmt() {
+    if option_env!("RUSTC_TEST_SUITE").is_some() {
+        return;
+    }
+
+    let root_dir = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR"));
+    let dev_dir = root_dir.join("clippy_dev");
+    let output = std::process::Command::new("cargo")
+        .current_dir(dev_dir)
+        .args(&["+nightly", "run", "--", "fmt", "--check"])
+        .output()
+        .unwrap();
+
+    println!("status: {}", output.status);
+    println!("stdout: {}", String::from_utf8_lossy(&output.stdout));
+    println!("stderr: {}", String::from_utf8_lossy(&output.stderr));
+
+    assert!(
+        output.status.success(),
+        "Formatting check failed. Run `./util/dev fmt` to update formatting."
+    );
+}