Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add dev fmt subcommand #4232

Merged
merged 10 commits into from Jul 13, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions appveyor.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
30 changes: 0 additions & 30 deletions ci/base-tests.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
(
Expand All @@ -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
1 change: 1 addition & 0 deletions clippy_dev/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,5 @@ clap = "2.33"
itertools = "0.8"
regex = "1"
lazy_static = "1.0"
shell-escape = "0.1"
walkdir = "2"
171 changes: 171 additions & 0 deletions clippy_dev/src/fmt.rs
Original file line number Diff line number Diff line change
@@ -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<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"
// 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<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)
}
40 changes: 32 additions & 8 deletions clippy_dev/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ extern crate regex;

use clap::{App, Arg, SubCommand};
use clippy_dev::*;

mod fmt;
mod stderr_length_check;

#[derive(PartialEq)]
Expand All @@ -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")
Expand Down Expand Up @@ -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);
}
},
_ => {},
}
}

Expand Down
19 changes: 9 additions & 10 deletions clippy_dev/src/stderr_length_check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Expand Down
12 changes: 7 additions & 5 deletions doc/adding_lints.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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

Expand Down
23 changes: 23 additions & 0 deletions tests/fmt.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
#[test]
fn fmt() {
This conversation was marked as resolved.
Show resolved Hide resolved
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."
);
}