diff --git a/appveyor.yml b/appveyor.yml index 675c16bcc4b9..9f6cc45af81e 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -22,6 +22,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 diff --git a/ci/base-tests.sh b/ci/base-tests.sh index d67541f7df05..c5d3eb3c902d 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 ( @@ -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 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..5ccdbec14288 --- /dev/null +++ b/clippy_dev/src/fmt.rs @@ -0,0 +1,171 @@ +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" + // Avoid rustfmt bug rust-lang/rustfmt#1873 + || cfg!(windows) && entry.file_name() == "implicit_hasher.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..5fa7a87a5dea 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); + } + }, + _ => {}, } } 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/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 diff --git a/tests/fmt.rs b/tests/fmt.rs new file mode 100644 index 000000000000..2500132d7ad3 --- /dev/null +++ b/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." + ); +}