From 4c771c3478d79d0d617752324f215d9bf5fe7512 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Tue, 25 Jun 2019 06:43:38 +0200 Subject: [PATCH 1/9] Add dev fmt subcommand --- clippy_dev/Cargo.toml | 1 + clippy_dev/src/fmt.rs | 167 ++++++++++++++++++++++++++ clippy_dev/src/main.rs | 40 ++++-- clippy_dev/src/stderr_length_check.rs | 19 ++- tests/fmt.rs | 20 +++ 5 files changed, 229 insertions(+), 18 deletions(-) create mode 100644 clippy_dev/src/fmt.rs create mode 100644 tests/fmt.rs diff --git a/clippy_dev/Cargo.toml b/clippy_dev/Cargo.toml index 602cd92da2b3..e2e946d06f27 100644 --- a/clippy_dev/Cargo.toml +++ b/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" diff --git a/clippy_dev/src/fmt.rs b/clippy_dev/src/fmt.rs new file mode 100644 index 000000000000..72f8d836ee7f --- /dev/null +++ b/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 for CliError { + fn from(error: io::Error) -> Self { + CliError::IoError(error) + } +} + +impl From 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 { + 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, dir: impl AsRef, args: &[impl AsRef]) -> 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, + dir: impl AsRef, + args: &[impl AsRef], +) -> Result { + 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 { + 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 { + 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 { + 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) +} diff --git a/clippy_dev/src/main.rs b/clippy_dev/src/main.rs index 302db24c74e3..8fdc4254d0c7 100644 --- a/clippy_dev/src/main.rs +++ b/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); + } + }, + _ => unreachable!(), } } diff --git a/clippy_dev/src/stderr_length_check.rs b/clippy_dev/src/stderr_length_check.rs index 6c5107aebfd3..ba8e5f83ef4e 100644 --- a/clippy_dev/src/stderr_length_check.rs +++ b/clippy_dev/src/stderr_length_check.rs @@ -23,16 +23,15 @@ pub fn check() { } fn exceeding_stderr_files(files: impl Iterator) -> impl Iterator { - 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 { diff --git a/tests/fmt.rs b/tests/fmt.rs new file mode 100644 index 000000000000..d7544bc50b21 --- /dev/null +++ b/tests/fmt.rs @@ -0,0 +1,20 @@ +#[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(&["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()); +} From 11707f3443c44c2cdfe99a0873dacf26e8681352 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Tue, 25 Jun 2019 07:30:29 +0200 Subject: [PATCH 2/9] Fix crash on `dev --limit-stderr-length` --- clippy_dev/src/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_dev/src/main.rs b/clippy_dev/src/main.rs index 8fdc4254d0c7..5fa7a87a5dea 100644 --- a/clippy_dev/src/main.rs +++ b/clippy_dev/src/main.rs @@ -77,7 +77,7 @@ fn main() { update_lints(&UpdateMode::Change); } }, - _ => unreachable!(), + _ => {}, } } From aeac3da2c1e6774d8d2f3569d064fb756b2554ae Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Wed, 26 Jun 2019 06:08:33 +0200 Subject: [PATCH 3/9] Improve fmt test failure message --- tests/fmt.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/fmt.rs b/tests/fmt.rs index d7544bc50b21..47be4ec125b7 100644 --- a/tests/fmt.rs +++ b/tests/fmt.rs @@ -16,5 +16,8 @@ fn fmt() { println!("stdout: {}", String::from_utf8_lossy(&output.stdout)); println!("stderr: {}", String::from_utf8_lossy(&output.stderr)); - assert!(output.status.success()); + assert!( + output.status.success(), + "Formatting check failed. Run `./util/dev fmt` to update formatting." + ); } From 503474a647c4e0c0b025d7aabbf007317410b4f2 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Wed, 3 Jul 2019 07:35:55 +0200 Subject: [PATCH 4/9] Remove format checks from CI script --- ci/base-tests.sh | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/ci/base-tests.sh b/ci/base-tests.sh index d67541f7df05..125cc898a219 100755 --- a/ci/base-tests.sh +++ b/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 ( @@ -60,16 +59,6 @@ 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 From 0c00391ed060c9184f520639bf5f51e650057d3d Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Thu, 4 Jul 2019 06:35:33 +0200 Subject: [PATCH 5/9] Remove format checks from CI scripts again. --- ci/base-tests.sh | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/ci/base-tests.sh b/ci/base-tests.sh index 125cc898a219..c5d3eb3c902d 100755 --- a/ci/base-tests.sh +++ b/ci/base-tests.sh @@ -49,22 +49,3 @@ export CARGO_TARGET_DIR=`pwd`/target/ # 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 - -set -ex # reset - -if [ "${tests_need_reformatting}" == "true" ] ; then - echo "Tests need reformatting!" - exit 2 -fi - -# switch back to master -rustup override set master From 3977843ab5592f8fb9c20f7f8436bae362234c52 Mon Sep 17 00:00:00 2001 From: flip1995 Date: Thu, 4 Jul 2019 14:35:27 +0200 Subject: [PATCH 6/9] Update documentation to the dev fmt command --- doc/adding_lints.md | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/doc/adding_lints.md b/doc/adding_lints.md index 2a8ef01ba1d0..0e73bdba1086 100644 --- a/doc/adding_lints.md +++ b/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 From c0c2a8d9c1b2d4f41ad7d61d11a0980116dc795c Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Fri, 5 Jul 2019 07:49:19 +0200 Subject: [PATCH 7/9] Work around rustup fallback error on Windows --- tests/fmt.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/fmt.rs b/tests/fmt.rs index 47be4ec125b7..2500132d7ad3 100644 --- a/tests/fmt.rs +++ b/tests/fmt.rs @@ -8,7 +8,7 @@ fn fmt() { let dev_dir = root_dir.join("clippy_dev"); let output = std::process::Command::new("cargo") .current_dir(dev_dir) - .args(&["run", "--", "fmt", "--check"]) + .args(&["+nightly", "run", "--", "fmt", "--check"]) .output() .unwrap(); From 186b5b2ee2870c4e92b718c53e2814364845e984 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Mon, 8 Jul 2019 07:20:11 +0200 Subject: [PATCH 8/9] Add rustfmt nightly to appveyor install --- appveyor.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/appveyor.yml b/appveyor.yml index 2d8704a703bc..66a1c83b205d 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -23,6 +23,7 @@ install: - del rust-toolchain - cargo install rustup-toolchain-install-master --debug || echo "rustup-toolchain-install-master already installed" - rustup-toolchain-install-master %RUSTC_HASH% -f -n master + - rustup component add rustfmt --toolchain nightly - rustup default master - set PATH=%PATH%;C:\Users\appveyor\.rustup\toolchains\master\bin - rustc -V From 2c90083f6294f901e2e8b5640cc084feb02431ee Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Thu, 11 Jul 2019 05:21:44 +0000 Subject: [PATCH 9/9] Avoid rustfmt bug on Windows --- clippy_dev/src/fmt.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/clippy_dev/src/fmt.rs b/clippy_dev/src/fmt.rs index 72f8d836ee7f..5ccdbec14288 100644 --- a/clippy_dev/src/fmt.rs +++ b/clippy_dev/src/fmt.rs @@ -44,7 +44,11 @@ pub fn run(check: bool, verbose: bool) { let entry = entry?; let path = entry.path(); - if path.extension() != Some("rs".as_ref()) || entry.file_name() == "ice-3891.rs" { + if path.extension() != Some("rs".as_ref()) + || entry.file_name() == "ice-3891.rs" + // Avoid rustfmt bug rust-lang/rustfmt#1873 + || cfg!(windows) && entry.file_name() == "implicit_hasher.rs" + { continue; }