clippy_dev: refactor rustfmt calls

This commit is contained in:
Jason Newcomb 2025-04-16 22:56:00 -04:00
parent 9d47e0c8ce
commit 232be55859
9 changed files with 180 additions and 204 deletions

View file

@ -10,7 +10,6 @@ clap = { version = "4.4", features = ["derive"] }
indoc = "1.0"
itertools = "0.12"
opener = "0.7"
shell-escape = "0.1"
walkdir = "2.3"
[package.metadata.rust-analyzer]

View file

@ -1,19 +1,15 @@
use crate::utils::{ClippyInfo, ErrAction, UpdateMode, panic_action, run_with_args_split, run_with_output};
use itertools::Itertools;
use rustc_lexer::{TokenKind, tokenize};
use shell_escape::escape;
use std::ffi::{OsStr, OsString};
use std::fs;
use std::io::{self, Read};
use std::ops::ControlFlow;
use std::path::{Path, PathBuf};
use std::path::PathBuf;
use std::process::{self, Command, Stdio};
use std::{fs, io};
use walkdir::WalkDir;
pub enum Error {
CommandFailed(String, String),
Io(io::Error),
RustfmtNotInstalled,
WalkDir(walkdir::Error),
IntellijSetupActive,
Parse(PathBuf, usize, String),
CheckFailed,
}
@ -24,37 +20,15 @@ impl From<io::Error> for Error {
}
}
impl From<walkdir::Error> for Error {
fn from(error: walkdir::Error) -> Self {
Self::WalkDir(error)
}
}
impl Error {
fn display(&self) {
match self {
Self::CheckFailed => {
eprintln!("Formatting check failed!\nRun `cargo dev fmt` to update.");
},
Self::CommandFailed(command, stderr) => {
eprintln!("error: command `{command}` failed!\nstderr: {stderr}");
},
Self::Io(err) => {
eprintln!("error: {err}");
},
Self::RustfmtNotInstalled => {
eprintln!("error: rustfmt nightly is not installed.");
},
Self::WalkDir(err) => {
eprintln!("error: {err}");
},
Self::IntellijSetupActive => {
eprintln!(
"error: a local rustc repo is enabled as path dependency via `cargo dev setup intellij`.\n\
Not formatting because that would format the local repo as well!\n\
Please revert the changes to `Cargo.toml`s with `cargo dev remove intellij`."
);
},
Self::Parse(path, line, msg) => {
eprintln!("error parsing `{}:{line}`: {msg}", path.display());
},
@ -62,12 +36,6 @@ impl Error {
}
}
struct FmtContext {
check: bool,
verbose: bool,
rustfmt_path: String,
}
struct ClippyConf<'a> {
name: &'a str,
attrs: &'a str,
@ -257,155 +225,120 @@ fn fmt_conf(check: bool) -> Result<(), Error> {
Ok(())
}
fn run_rustfmt(context: &FmtContext) -> Result<(), Error> {
// if we added a local rustc repo as path dependency to clippy for rust analyzer, we do NOT want to
// format because rustfmt would also format the entire rustc repo as it is a local
// dependency
if fs::read_to_string("Cargo.toml")
.expect("Failed to read clippy Cargo.toml")
.contains("[target.'cfg(NOT_A_PLATFORM)'.dependencies]")
{
return Err(Error::IntellijSetupActive);
fn run_rustfmt(clippy: &ClippyInfo, update_mode: UpdateMode) {
let mut rustfmt_path = String::from_utf8(run_with_output(
"rustup which rustfmt",
Command::new("rustup").args(["which", "rustfmt"]),
))
.expect("invalid rustfmt path");
rustfmt_path.truncate(rustfmt_path.trim_end().len());
let mut cargo_path = String::from_utf8(run_with_output(
"rustup which cargo",
Command::new("rustup").args(["which", "cargo"]),
))
.expect("invalid cargo path");
cargo_path.truncate(cargo_path.trim_end().len());
// Start all format jobs first before waiting on the results.
let mut children = Vec::with_capacity(16);
for &path in &[
".",
"clippy_config",
"clippy_dev",
"clippy_lints",
"clippy_utils",
"rustc_tools_util",
"lintcheck",
] {
let mut cmd = Command::new(&cargo_path);
cmd.current_dir(clippy.path.join(path))
.args(["fmt"])
.env("RUSTFMT", &rustfmt_path)
.stdout(Stdio::null())
.stdin(Stdio::null())
.stderr(Stdio::piped());
if update_mode.is_check() {
cmd.arg("--check");
}
match cmd.spawn() {
Ok(x) => children.push(("cargo fmt", x)),
Err(ref e) => panic_action(&e, ErrAction::Run, "cargo fmt".as_ref()),
}
}
check_for_rustfmt(context)?;
cargo_fmt(context, ".".as_ref())?;
cargo_fmt(context, "clippy_dev".as_ref())?;
cargo_fmt(context, "rustc_tools_util".as_ref())?;
cargo_fmt(context, "lintcheck".as_ref())?;
let chunks = WalkDir::new("tests")
.into_iter()
.filter_map(|entry| {
let entry = entry.expect("failed to find tests");
let path = entry.path();
if path.extension() != Some("rs".as_ref())
|| path
.components()
.nth_back(1)
.is_some_and(|c| c.as_os_str() == "syntax-error-recovery")
|| entry.file_name() == "ice-3891.rs"
{
None
} else {
Some(entry.into_path().into_os_string())
run_with_args_split(
|| {
let mut cmd = Command::new(&rustfmt_path);
if update_mode.is_check() {
cmd.arg("--check");
}
})
.chunks(250);
cmd.stdout(Stdio::null())
.stdin(Stdio::null())
.stderr(Stdio::piped())
.args(["--config", "show_parse_errors=false"]);
cmd
},
|cmd| match cmd.spawn() {
Ok(x) => children.push(("rustfmt", x)),
Err(ref e) => panic_action(&e, ErrAction::Run, "rustfmt".as_ref()),
},
WalkDir::new("tests")
.into_iter()
.filter_entry(|p| p.path().file_name().is_none_or(|x| x != "skip_rustfmt"))
.filter_map(|e| {
let e = e.expect("error reading `tests`");
e.path()
.as_os_str()
.as_encoded_bytes()
.ends_with(b".rs")
.then(|| e.into_path().into_os_string())
}),
);
for chunk in &chunks {
rustfmt(context, chunk)?;
for (name, child) in &mut children {
match child.wait() {
Ok(status) => match (update_mode, status.exit_ok()) {
(UpdateMode::Check | UpdateMode::Change, Ok(())) => {},
(UpdateMode::Check, Err(_)) => {
let mut s = String::new();
if let Some(mut stderr) = child.stderr.take()
&& stderr.read_to_string(&mut s).is_ok()
{
eprintln!("{s}");
}
eprintln!("Formatting check failed!\nRun `cargo dev fmt` to update.");
process::exit(1);
},
(UpdateMode::Change, Err(e)) => {
let mut s = String::new();
if let Some(mut stderr) = child.stderr.take()
&& stderr.read_to_string(&mut s).is_ok()
{
eprintln!("{s}");
}
panic_action(&e, ErrAction::Run, name.as_ref());
},
},
Err(ref e) => panic_action(e, ErrAction::Run, name.as_ref()),
}
}
Ok(())
}
// the "main" function of cargo dev fmt
pub fn run(check: bool, verbose: bool) {
let output = Command::new("rustup")
.args(["which", "rustfmt"])
.stderr(Stdio::inherit())
.output()
.expect("error running `rustup which rustfmt`");
if !output.status.success() {
eprintln!("`rustup which rustfmt` did not execute successfully");
process::exit(1);
pub fn run(clippy: &ClippyInfo, update_mode: UpdateMode) {
if clippy.has_intellij_hook {
eprintln!(
"error: a local rustc repo is enabled as path dependency via `cargo dev setup intellij`.\n\
Not formatting because that would format the local repo as well!\n\
Please revert the changes to `Cargo.toml`s with `cargo dev remove intellij`."
);
return;
}
let mut rustfmt_path = String::from_utf8(output.stdout).expect("invalid rustfmt path");
rustfmt_path.truncate(rustfmt_path.trim_end().len());
run_rustfmt(clippy, update_mode);
let context = FmtContext {
check,
verbose,
rustfmt_path,
};
if let Err(e) = run_rustfmt(&context).and_then(|()| fmt_conf(check)) {
if let Err(e) = fmt_conf(update_mode.is_check()) {
e.display();
process::exit(1);
}
}
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())).collect();
format!(
"cd {} && {} {}",
escape(dir.as_ref().to_string_lossy()),
escape(program.as_ref().to_string_lossy()),
arg_display.join(" ")
)
}
fn exec_fmt_command(
context: &FmtContext,
program: impl AsRef<OsStr>,
dir: impl AsRef<Path>,
args: &[impl AsRef<OsStr>],
) -> Result<(), Error> {
if context.verbose {
println!("{}", format_command(&program, &dir, args));
}
let output = Command::new(&program)
.env("RUSTFMT", &context.rustfmt_path)
.current_dir(&dir)
.args(args.iter())
.output()
.unwrap();
let success = output.status.success();
match (context.check, success) {
(_, true) => Ok(()),
(true, false) => Err(Error::CheckFailed),
(false, false) => {
let stderr = std::str::from_utf8(&output.stderr).unwrap_or("");
Err(Error::CommandFailed(
format_command(&program, &dir, args),
String::from(stderr),
))
},
}
}
fn cargo_fmt(context: &FmtContext, path: &Path) -> Result<(), Error> {
let mut args = vec!["fmt", "--all"];
if context.check {
args.push("--check");
}
exec_fmt_command(context, "cargo", path, &args)
}
fn check_for_rustfmt(context: &FmtContext) -> Result<(), Error> {
let program = "rustfmt";
let dir = std::env::current_dir()?;
let args = &["--version"];
if context.verbose {
println!("{}", format_command(program, &dir, args));
}
let output = Command::new(program).current_dir(&dir).args(args.iter()).output()?;
if output.status.success() {
Ok(())
} else if std::str::from_utf8(&output.stderr)
.unwrap_or("")
.starts_with("error: 'rustfmt' is not installed")
{
Err(Error::RustfmtNotInstalled)
} else {
Err(Error::CommandFailed(
format_command(program, &dir, args),
std::str::from_utf8(&output.stderr).unwrap_or("").to_string(),
))
}
}
fn rustfmt(context: &FmtContext, paths: impl Iterator<Item = OsString>) -> Result<(), Error> {
let mut args = Vec::new();
if context.check {
args.push(OsString::from("--check"));
}
args.extend(paths);
exec_fmt_command(context, &context.rustfmt_path, std::env::current_dir()?, &args)
}

View file

@ -1,5 +1,6 @@
#![feature(
rustc_private,
exit_status_error,
if_let_guard,
let_chains,
os_str_slice,

View file

@ -26,7 +26,7 @@ fn main() {
allow_staged,
allow_no_vcs,
} => dogfood::dogfood(fix, allow_dirty, allow_staged, allow_no_vcs),
DevCommand::Fmt { check, verbose } => fmt::run(check, verbose),
DevCommand::Fmt { check } => fmt::run(&clippy, utils::UpdateMode::from_check(check)),
DevCommand::UpdateLints { check } => update_lints::update(utils::UpdateMode::from_check(check)),
DevCommand::NewLint {
pass,
@ -125,9 +125,6 @@ enum DevCommand {
#[arg(long)]
/// Use the rustfmt --check option
check: bool,
#[arg(short, long)]
/// Echo commands run
verbose: bool,
},
#[command(name = "update_lints")]
/// Updates lint registration and information from the source code

View file

@ -1,5 +1,5 @@
use crate::utils::{
File, FileAction, FileUpdater, RustSearcher, Token, UpdateMode, UpdateStatus, panic_file, update_text_region_fn,
ErrAction, File, FileUpdater, RustSearcher, Token, UpdateMode, UpdateStatus, panic_action, update_text_region_fn,
};
use itertools::Itertools;
use std::collections::HashSet;
@ -203,7 +203,7 @@ fn read_src_with_module(src_root: &Path) -> impl use<'_> + Iterator<Item = (DirE
WalkDir::new(src_root).into_iter().filter_map(move |e| {
let e = match e {
Ok(e) => e,
Err(ref e) => panic_file(e, FileAction::Read, src_root),
Err(ref e) => panic_action(e, ErrAction::Read, src_root),
};
let path = e.path().as_os_str().as_encoded_bytes();
if let Some(path) = path.strip_suffix(b".rs")

View file

@ -4,10 +4,11 @@ use core::slice;
use core::str::FromStr;
use rustc_lexer::{self as lexer, FrontmatterAllowed};
use std::env;
use std::ffi::OsStr;
use std::fs::{self, OpenOptions};
use std::io::{self, Read as _, Seek as _, SeekFrom, Write};
use std::path::{Path, PathBuf};
use std::process::{self, ExitStatus};
use std::process::{self, Command, ExitStatus, Stdio};
#[cfg(not(windows))]
static CARGO_CLIPPY_EXE: &str = "cargo-clippy";
@ -15,15 +16,16 @@ static CARGO_CLIPPY_EXE: &str = "cargo-clippy";
static CARGO_CLIPPY_EXE: &str = "cargo-clippy.exe";
#[derive(Clone, Copy)]
pub enum FileAction {
pub enum ErrAction {
Open,
Read,
Write,
Create,
Rename,
Delete,
Run,
}
impl FileAction {
impl ErrAction {
fn as_str(self) -> &'static str {
match self {
Self::Open => "opening",
@ -32,13 +34,14 @@ impl FileAction {
Self::Create => "creating",
Self::Rename => "renaming",
Self::Delete => "deleting",
Self::Run => "running",
}
}
}
#[cold]
#[track_caller]
pub fn panic_file(err: &impl Display, action: FileAction, path: &Path) -> ! {
pub fn panic_action(err: &impl Display, action: ErrAction, path: &Path) -> ! {
panic!("error {} `{}`: {}", action.as_str(), path.display(), *err)
}
@ -54,7 +57,7 @@ impl<'a> File<'a> {
let path = path.as_ref();
match options.open(path) {
Ok(inner) => Self { inner, path },
Err(e) => panic_file(&e, FileAction::Open, path),
Err(e) => panic_action(&e, ErrAction::Open, path),
}
}
@ -65,7 +68,7 @@ impl<'a> File<'a> {
match options.open(path) {
Ok(inner) => Some(Self { inner, path }),
Err(e) if e.kind() == io::ErrorKind::NotFound => None,
Err(e) => panic_file(&e, FileAction::Open, path),
Err(e) => panic_action(&e, ErrAction::Open, path),
}
}
@ -83,7 +86,7 @@ impl<'a> File<'a> {
pub fn read_append_to_string<'dst>(&mut self, dst: &'dst mut String) -> &'dst mut String {
match self.inner.read_to_string(dst) {
Ok(_) => {},
Err(e) => panic_file(&e, FileAction::Read, self.path),
Err(e) => panic_action(&e, ErrAction::Read, self.path),
}
dst
}
@ -105,7 +108,7 @@ impl<'a> File<'a> {
Err(e) => Err(e),
};
if let Err(e) = res {
panic_file(&e, FileAction::Write, self.path);
panic_action(&e, ErrAction::Write, self.path);
}
}
}
@ -183,10 +186,7 @@ fn toml_iter(s: &str) -> impl Iterator<Item = (usize, TomlPart<'_>)> {
.filter_map(|(pos, s)| {
if let Some(s) = s.strip_prefix('[') {
s.split_once(']').map(|(name, _)| (pos, TomlPart::Table(name)))
} else if matches!(
s.as_bytes().get(0),
Some(b'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9' | b'_')
) {
} else if matches!(s.bytes().next(), Some(b'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9' | b'_')) {
s.split_once('=').map(|(key, value)| (pos, TomlPart::Value(key, value)))
} else {
None
@ -200,6 +200,7 @@ pub struct CargoPackage<'a> {
pub not_a_platform_range: Range<usize>,
}
#[must_use]
pub fn parse_cargo_package(s: &str) -> CargoPackage<'_> {
let mut in_package = false;
let mut in_platform_deps = false;
@ -232,7 +233,7 @@ pub fn parse_cargo_package(s: &str) -> CargoPackage<'_> {
},
_ => {},
},
_ => {},
TomlPart::Value(..) => {},
}
}
CargoPackage {
@ -268,9 +269,8 @@ impl ClippyInfo {
version,
has_intellij_hook: !package.not_a_platform_range.is_empty(),
};
} else {
panic!("error reading clippy version from {}", file.path.display());
}
panic!("error reading clippy version from `{}`", file.path.display());
}
}
@ -323,6 +323,11 @@ impl UpdateMode {
pub fn from_check(check: bool) -> Self {
if check { Self::Check } else { Self::Change }
}
#[must_use]
pub fn is_check(self) -> bool {
matches!(self, Self::Check)
}
}
#[derive(Default)]
@ -611,7 +616,7 @@ pub fn try_rename_file(old_name: &Path, new_name: &Path) -> bool {
match OpenOptions::new().create_new(true).write(true).open(new_name) {
Ok(file) => drop(file),
Err(e) if matches!(e.kind(), io::ErrorKind::AlreadyExists | io::ErrorKind::NotFound) => return false,
Err(ref e) => panic_file(e, FileAction::Create, new_name),
Err(ref e) => panic_action(e, ErrAction::Create, new_name),
}
match fs::rename(old_name, new_name) {
Ok(()) => true,
@ -622,7 +627,7 @@ pub fn try_rename_file(old_name: &Path, new_name: &Path) -> bool {
if matches!(e.kind(), io::ErrorKind::NotFound | io::ErrorKind::NotADirectory) {
false
} else {
panic_file(e, FileAction::Rename, old_name);
panic_action(e, ErrAction::Rename, old_name);
}
},
}
@ -633,7 +638,7 @@ pub fn try_rename_dir(old_name: &Path, new_name: &Path) -> bool {
match fs::create_dir(new_name) {
Ok(()) => {},
Err(e) if matches!(e.kind(), io::ErrorKind::AlreadyExists | io::ErrorKind::NotFound) => return false,
Err(ref e) => panic_file(e, FileAction::Create, new_name),
Err(ref e) => panic_action(e, ErrAction::Create, new_name),
}
// Windows can't reliably rename to an empty directory.
#[cfg(windows)]
@ -648,14 +653,55 @@ pub fn try_rename_dir(old_name: &Path, new_name: &Path) -> bool {
if matches!(e.kind(), io::ErrorKind::NotFound | io::ErrorKind::NotADirectory) {
false
} else {
panic_file(e, FileAction::Rename, old_name);
panic_action(e, ErrAction::Rename, old_name);
}
},
}
}
pub fn write_file(path: &Path, contents: &str) {
fs::write(path, contents).unwrap_or_else(|e| panic_file(&e, FileAction::Write, path));
fs::write(path, contents).unwrap_or_else(|e| panic_action(&e, ErrAction::Write, path));
}
#[must_use]
pub fn run_with_output(path: &(impl AsRef<Path> + ?Sized), cmd: &mut Command) -> Vec<u8> {
fn f(path: &Path, cmd: &mut Command) -> Vec<u8> {
match cmd
.stdin(Stdio::null())
.stdout(Stdio::piped())
.stderr(Stdio::inherit())
.output()
{
Ok(x) => match x.status.exit_ok() {
Ok(()) => x.stdout,
Err(ref e) => panic_action(e, ErrAction::Run, path),
},
Err(ref e) => panic_action(e, ErrAction::Run, path),
}
}
f(path.as_ref(), cmd)
}
pub fn run_with_args_split(
mut make_cmd: impl FnMut() -> Command,
mut run_cmd: impl FnMut(&mut Command),
args: impl Iterator<Item: AsRef<OsStr>>,
) {
let mut cmd = make_cmd();
let mut len = 0;
for arg in args {
len += arg.as_ref().len();
cmd.arg(arg);
// Very conservative limit
if len > 10000 {
run_cmd(&mut cmd);
cmd = make_cmd();
len = 0;
}
}
if len != 0 {
run_cmd(&mut cmd);
}
}
#[expect(clippy::must_use_candidate)]
@ -663,7 +709,7 @@ pub fn delete_file_if_exists(path: &Path) -> bool {
match fs::remove_file(path) {
Ok(()) => true,
Err(e) if matches!(e.kind(), io::ErrorKind::NotFound | io::ErrorKind::IsADirectory) => false,
Err(ref e) => panic_file(e, FileAction::Delete, path),
Err(ref e) => panic_action(e, ErrAction::Delete, path),
}
}
@ -671,6 +717,6 @@ pub fn delete_dir_if_exists(path: &Path) {
match fs::remove_dir_all(path) {
Ok(()) => {},
Err(e) if matches!(e.kind(), io::ErrorKind::NotFound | io::ErrorKind::NotADirectory) => {},
Err(ref e) => panic_file(e, FileAction::Delete, path),
Err(ref e) => panic_action(e, ErrAction::Delete, path),
}
}

View file

@ -1,5 +1,5 @@
error: expected one of `!`, `(`, `+`, `,`, `::`, `<`, or `>`, found `)`
--> tests/ui/syntax-error-recovery/non_expressive_names_error_recovery.rs:6:19
--> tests/ui/skip_rustfmt/non_expressive_names_error_recovery.rs:6:19
|
LL | fn aa(a: Aa<String) {
| ^ expected one of 7 possible tokens