Rollup merge of #143398 - lolbinarycat:tidy-extra-checks-auto, r=Kobzol

tidy: add support for `--extra-checks=auto:` feature

in preparation for rust-lang/rust#142924

also heavily refactored the parsing of the `--extra-checks` argument to warn about improper usage.

cc ```@GuillaumeGomez```

r? ```@Kobzol```

src/bootstrap/src/core/config/flags.rs

@@ -383,7 +383,10 @@ pub enum Subcommand {
         bless: bool,
         #[arg(long)]
         /// comma-separated list of other files types to check (accepts py, py:lint,
-        /// py:fmt, shell, shell:lint, cpp, cpp:fmt, spellcheck, spellcheck:fix)
+        /// py:fmt, shell, shell:lint, cpp, cpp:fmt, spellcheck)
+        ///
+        /// Any argument can be prefixed with "auto:" to only run if
+        /// relevant files are modified (eg. "auto:py").
         extra_checks: Option<String>,
         #[arg(long)]
         /// rerun tests even if the inputs are unchanged

src/bootstrap/src/utils/change_tracker.rs

@@ -461,4 +461,9 @@ pub const CONFIG_CHANGE_HISTORY: &[ChangeInfo] = &[
         severity: ChangeSeverity::Warning,
         summary: "`download-rustc` has been temporarily disabled for the library profile due to implementation bugs (see #142505).",
     },
+    ChangeInfo {
+        change_id: 143398,
+        severity: ChangeSeverity::Info,
+        summary: "The --extra-checks flag now supports prefixing any check with `auto:` to only run it if relevant files are modified",
+    },
 ];

src/etc/completions/x.fish

@@ -293,7 +293,7 @@ complete -c x -n "__fish_x_using_subcommand doc" -l skip-std-check-if-no-downloa
 complete -c x -n "__fish_x_using_subcommand doc" -s h -l help -d 'Print help (see more with \'--help\')'
 complete -c x -n "__fish_x_using_subcommand test" -l test-args -d 'extra arguments to be passed for the test tool being used (e.g. libtest, compiletest or rustdoc)' -r
 complete -c x -n "__fish_x_using_subcommand test" -l compiletest-rustc-args -d 'extra options to pass the compiler when running compiletest tests' -r
-complete -c x -n "__fish_x_using_subcommand test" -l extra-checks -d 'comma-separated list of other files types to check (accepts py, py:lint, py:fmt, shell, shell:lint, cpp, cpp:fmt, spellcheck, spellcheck:fix)' -r
+complete -c x -n "__fish_x_using_subcommand test" -l extra-checks -d 'comma-separated list of other files types to check (accepts py, py:lint, py:fmt, shell, shell:lint, cpp, cpp:fmt, spellcheck)' -r
 complete -c x -n "__fish_x_using_subcommand test" -l compare-mode -d 'mode describing what file the actual ui output will be compared to' -r
 complete -c x -n "__fish_x_using_subcommand test" -l pass -d 'force {check,build,run}-pass tests to this mode' -r
 complete -c x -n "__fish_x_using_subcommand test" -l run -d 'whether to execute run-* tests' -r

src/etc/completions/x.ps1

@@ -339,7 +339,7 @@ Register-ArgumentCompleter -Native -CommandName 'x' -ScriptBlock {
         'x;test' {
             [CompletionResult]::new('--test-args', '--test-args', [CompletionResultType]::ParameterName, 'extra arguments to be passed for the test tool being used (e.g. libtest, compiletest or rustdoc)')
             [CompletionResult]::new('--compiletest-rustc-args', '--compiletest-rustc-args', [CompletionResultType]::ParameterName, 'extra options to pass the compiler when running compiletest tests')
-            [CompletionResult]::new('--extra-checks', '--extra-checks', [CompletionResultType]::ParameterName, 'comma-separated list of other files types to check (accepts py, py:lint, py:fmt, shell, shell:lint, cpp, cpp:fmt, spellcheck, spellcheck:fix)')
+            [CompletionResult]::new('--extra-checks', '--extra-checks', [CompletionResultType]::ParameterName, 'comma-separated list of other files types to check (accepts py, py:lint, py:fmt, shell, shell:lint, cpp, cpp:fmt, spellcheck)')
             [CompletionResult]::new('--compare-mode', '--compare-mode', [CompletionResultType]::ParameterName, 'mode describing what file the actual ui output will be compared to')
             [CompletionResult]::new('--pass', '--pass', [CompletionResultType]::ParameterName, 'force {check,build,run}-pass tests to this mode')
             [CompletionResult]::new('--run', '--run', [CompletionResultType]::ParameterName, 'whether to execute run-* tests')

src/etc/completions/x.py.fish

@@ -293,7 +293,7 @@ complete -c x.py -n "__fish_x.py_using_subcommand doc" -l skip-std-check-if-no-d
 complete -c x.py -n "__fish_x.py_using_subcommand doc" -s h -l help -d 'Print help (see more with \'--help\')'
 complete -c x.py -n "__fish_x.py_using_subcommand test" -l test-args -d 'extra arguments to be passed for the test tool being used (e.g. libtest, compiletest or rustdoc)' -r
 complete -c x.py -n "__fish_x.py_using_subcommand test" -l compiletest-rustc-args -d 'extra options to pass the compiler when running compiletest tests' -r
-complete -c x.py -n "__fish_x.py_using_subcommand test" -l extra-checks -d 'comma-separated list of other files types to check (accepts py, py:lint, py:fmt, shell, shell:lint, cpp, cpp:fmt, spellcheck, spellcheck:fix)' -r
+complete -c x.py -n "__fish_x.py_using_subcommand test" -l extra-checks -d 'comma-separated list of other files types to check (accepts py, py:lint, py:fmt, shell, shell:lint, cpp, cpp:fmt, spellcheck)' -r
 complete -c x.py -n "__fish_x.py_using_subcommand test" -l compare-mode -d 'mode describing what file the actual ui output will be compared to' -r
 complete -c x.py -n "__fish_x.py_using_subcommand test" -l pass -d 'force {check,build,run}-pass tests to this mode' -r
 complete -c x.py -n "__fish_x.py_using_subcommand test" -l run -d 'whether to execute run-* tests' -r

src/etc/completions/x.py.ps1

@@ -339,7 +339,7 @@ Register-ArgumentCompleter -Native -CommandName 'x.py' -ScriptBlock {
         'x.py;test' {
             [CompletionResult]::new('--test-args', '--test-args', [CompletionResultType]::ParameterName, 'extra arguments to be passed for the test tool being used (e.g. libtest, compiletest or rustdoc)')
             [CompletionResult]::new('--compiletest-rustc-args', '--compiletest-rustc-args', [CompletionResultType]::ParameterName, 'extra options to pass the compiler when running compiletest tests')
-            [CompletionResult]::new('--extra-checks', '--extra-checks', [CompletionResultType]::ParameterName, 'comma-separated list of other files types to check (accepts py, py:lint, py:fmt, shell, shell:lint, cpp, cpp:fmt, spellcheck, spellcheck:fix)')
+            [CompletionResult]::new('--extra-checks', '--extra-checks', [CompletionResultType]::ParameterName, 'comma-separated list of other files types to check (accepts py, py:lint, py:fmt, shell, shell:lint, cpp, cpp:fmt, spellcheck)')
             [CompletionResult]::new('--compare-mode', '--compare-mode', [CompletionResultType]::ParameterName, 'mode describing what file the actual ui output will be compared to')
             [CompletionResult]::new('--pass', '--pass', [CompletionResultType]::ParameterName, 'force {check,build,run}-pass tests to this mode')
             [CompletionResult]::new('--run', '--run', [CompletionResultType]::ParameterName, 'whether to execute run-* tests')

src/etc/completions/x.py.zsh

@@ -338,7 +338,7 @@ _arguments "${_arguments_options[@]}" : \
 _arguments "${_arguments_options[@]}" : \
 '*--test-args=[extra arguments to be passed for the test tool being used (e.g. libtest, compiletest or rustdoc)]:ARGS:_default' \
 '*--compiletest-rustc-args=[extra options to pass the compiler when running compiletest tests]:ARGS:_default' \
-'--extra-checks=[comma-separated list of other files types to check (accepts py, py\:lint, py\:fmt, shell, shell\:lint, cpp, cpp\:fmt, spellcheck, spellcheck\:fix)]:EXTRA_CHECKS:_default' \
+'--extra-checks=[comma-separated list of other files types to check (accepts py, py\:lint, py\:fmt, shell, shell\:lint, cpp, cpp\:fmt, spellcheck)]:EXTRA_CHECKS:_default' \
 '--compare-mode=[mode describing what file the actual ui output will be compared to]:COMPARE MODE:_default' \
 '--pass=[force {check,build,run}-pass tests to this mode]:check | build | run:_default' \
 '--run=[whether to execute run-* tests]:auto | always | never:_default' \

src/etc/completions/x.zsh

@@ -338,7 +338,7 @@ _arguments "${_arguments_options[@]}" : \
 _arguments "${_arguments_options[@]}" : \
 '*--test-args=[extra arguments to be passed for the test tool being used (e.g. libtest, compiletest or rustdoc)]:ARGS:_default' \
 '*--compiletest-rustc-args=[extra options to pass the compiler when running compiletest tests]:ARGS:_default' \
-'--extra-checks=[comma-separated list of other files types to check (accepts py, py\:lint, py\:fmt, shell, shell\:lint, cpp, cpp\:fmt, spellcheck, spellcheck\:fix)]:EXTRA_CHECKS:_default' \
+'--extra-checks=[comma-separated list of other files types to check (accepts py, py\:lint, py\:fmt, shell, shell\:lint, cpp, cpp\:fmt, spellcheck)]:EXTRA_CHECKS:_default' \
 '--compare-mode=[mode describing what file the actual ui output will be compared to]:COMPARE MODE:_default' \
 '--pass=[force {check,build,run}-pass tests to this mode]:check | build | run:_default' \
 '--run=[whether to execute run-* tests]:auto | always | never:_default' \

src/tools/tidy/src/ext_tool_checks.rs

@@ -20,8 +20,11 @@
 use std::ffi::OsStr;
 use std::path::{Path, PathBuf};
 use std::process::Command;
+use std::str::FromStr;
 use std::{fmt, fs, io};
 
+use crate::CiInfo;
+
 const MIN_PY_REV: (u32, u32) = (3, 9);
 const MIN_PY_REV_STR: &str = "≥3.9";
 
@@ -36,15 +39,19 @@ const RUFF_CONFIG_PATH: &[&str] = &["src", "tools", "tidy", "config", "ruff.toml
 const RUFF_CACHE_PATH: &[&str] = &["cache", "ruff_cache"];
 const PIP_REQ_PATH: &[&str] = &["src", "tools", "tidy", "config", "requirements.txt"];
 
+// this must be kept in sync with with .github/workflows/spellcheck.yml
+const SPELLCHECK_DIRS: &[&str] = &["compiler", "library", "src/bootstrap", "src/librustdoc"];
+
 pub fn check(
     root_path: &Path,
     outdir: &Path,
+    ci_info: &CiInfo,
     bless: bool,
     extra_checks: Option<&str>,
     pos_args: &[String],
     bad: &mut bool,
 ) {
-    if let Err(e) = check_impl(root_path, outdir, bless, extra_checks, pos_args) {
+    if let Err(e) = check_impl(root_path, outdir, ci_info, bless, extra_checks, pos_args) {
         tidy_error!(bad, "{e}");
     }
 }
@@ -52,6 +59,7 @@ pub fn check(
 fn check_impl(
     root_path: &Path,
     outdir: &Path,
+    ci_info: &CiInfo,
     bless: bool,
     extra_checks: Option<&str>,
     pos_args: &[String],
@@ -61,25 +69,45 @@ fn check_impl(
 
     // Split comma-separated args up
     let lint_args = match extra_checks {
-        Some(s) => s.strip_prefix("--extra-checks=").unwrap().split(',').collect(),
+        Some(s) => s
+            .strip_prefix("--extra-checks=")
+            .unwrap()
+            .split(',')
+            .map(|s| {
+                if s == "spellcheck:fix" {
+                    eprintln!("warning: `spellcheck:fix` is no longer valid, use `--extra-checks=spellcheck --bless`");
+                }
+                (ExtraCheckArg::from_str(s), s)
+            })
+            .filter_map(|(res, src)| match res {
+                Ok(arg) => {
+                    if arg.is_inactive_auto(ci_info) {
+                        None
+                    } else {
+                        Some(arg)
+                    }
+                }
+                Err(err) => {
+                    // only warn because before bad extra checks would be silently ignored.
+                    eprintln!("warning: bad extra check argument {src:?}: {err:?}");
+                    None
+                }
+            })
+            .collect(),
         None => vec![],
     };
 
-    if lint_args.contains(&"spellcheck:fix") {
-        return Err(Error::Generic(
-            "`spellcheck:fix` is no longer valid, use `--extra=check=spellcheck --bless`"
-                .to_string(),
-        ));
+    macro_rules! extra_check {
+        ($lang:ident, $kind:ident) => {
+            lint_args.iter().any(|arg| arg.matches(ExtraCheckLang::$lang, ExtraCheckKind::$kind))
+        };
     }
 
-    let python_all = lint_args.contains(&"py");
-    let python_lint = lint_args.contains(&"py:lint") || python_all;
-    let python_fmt = lint_args.contains(&"py:fmt") || python_all;
-    let shell_all = lint_args.contains(&"shell");
-    let shell_lint = lint_args.contains(&"shell:lint") || shell_all;
-    let cpp_all = lint_args.contains(&"cpp");
-    let cpp_fmt = lint_args.contains(&"cpp:fmt") || cpp_all;
-    let spellcheck = lint_args.contains(&"spellcheck");
+    let python_lint = extra_check!(Py, Lint);
+    let python_fmt = extra_check!(Py, Fmt);
+    let shell_lint = extra_check!(Shell, Lint);
+    let cpp_fmt = extra_check!(Cpp, Fmt);
+    let spellcheck = extra_check!(Spellcheck, None);
 
     let mut py_path = None;
 
@@ -234,15 +262,9 @@ fn check_impl(
 
     if spellcheck {
         let config_path = root_path.join("typos.toml");
-        // sync target files with .github/workflows/spellcheck.yml
-        let mut args = vec![
-            "-c",
-            config_path.as_os_str().to_str().unwrap(),
-            "./compiler",
-            "./library",
-            "./src/bootstrap",
-            "./src/librustdoc",
-        ];
+        let mut args = vec!["-c", config_path.as_os_str().to_str().unwrap()];
+
+        args.extend_from_slice(SPELLCHECK_DIRS);
 
         if bless {
             eprintln!("spellcheck files and fix");
@@ -638,3 +660,136 @@ impl From<io::Error> for Error {
         Self::Io(value)
     }
 }
+
+#[derive(Debug)]
+enum ExtraCheckParseError {
+    #[allow(dead_code, reason = "shown through Debug")]
+    UnknownKind(String),
+    #[allow(dead_code)]
+    UnknownLang(String),
+    UnsupportedKindForLang,
+    /// Too many `:`
+    TooManyParts,
+    /// Tried to parse the empty string
+    Empty,
+    /// `auto` specified without lang part.
+    AutoRequiresLang,
+}
+
+struct ExtraCheckArg {
+    auto: bool,
+    lang: ExtraCheckLang,
+    /// None = run all extra checks for the given lang
+    kind: Option<ExtraCheckKind>,
+}
+
+impl ExtraCheckArg {
+    fn matches(&self, lang: ExtraCheckLang, kind: ExtraCheckKind) -> bool {
+        self.lang == lang && self.kind.map(|k| k == kind).unwrap_or(true)
+    }
+
+    /// Returns `true` if this is an auto arg and the relevant files are not modified.
+    fn is_inactive_auto(&self, ci_info: &CiInfo) -> bool {
+        if !self.auto {
+            return false;
+        }
+        let ext = match self.lang {
+            ExtraCheckLang::Py => ".py",
+            ExtraCheckLang::Cpp => ".cpp",
+            ExtraCheckLang::Shell => ".sh",
+            ExtraCheckLang::Spellcheck => {
+                return !crate::files_modified(ci_info, |s| {
+                    SPELLCHECK_DIRS.iter().any(|dir| Path::new(s).starts_with(dir))
+                });
+            }
+        };
+        !crate::files_modified(ci_info, |s| s.ends_with(ext))
+    }
+
+    fn has_supported_kind(&self) -> bool {
+        let Some(kind) = self.kind else {
+            // "run all extra checks" mode is supported for all languages.
+            return true;
+        };
+        use ExtraCheckKind::*;
+        let supported_kinds: &[_] = match self.lang {
+            ExtraCheckLang::Py => &[Fmt, Lint],
+            ExtraCheckLang::Cpp => &[Fmt],
+            ExtraCheckLang::Shell => &[Lint],
+            ExtraCheckLang::Spellcheck => &[],
+        };
+        supported_kinds.contains(&kind)
+    }
+}
+
+impl FromStr for ExtraCheckArg {
+    type Err = ExtraCheckParseError;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        let mut auto = false;
+        let mut parts = s.split(':');
+        let Some(mut first) = parts.next() else {
+            return Err(ExtraCheckParseError::Empty);
+        };
+        if first == "auto" {
+            let Some(part) = parts.next() else {
+                return Err(ExtraCheckParseError::AutoRequiresLang);
+            };
+            auto = true;
+            first = part;
+        }
+        let second = parts.next();
+        if parts.next().is_some() {
+            return Err(ExtraCheckParseError::TooManyParts);
+        }
+        let arg = Self { auto, lang: first.parse()?, kind: second.map(|s| s.parse()).transpose()? };
+        if !arg.has_supported_kind() {
+            return Err(ExtraCheckParseError::UnsupportedKindForLang);
+        }
+
+        Ok(arg)
+    }
+}
+
+#[derive(PartialEq, Copy, Clone)]
+enum ExtraCheckLang {
+    Py,
+    Shell,
+    Cpp,
+    Spellcheck,
+}
+
+impl FromStr for ExtraCheckLang {
+    type Err = ExtraCheckParseError;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        Ok(match s {
+            "py" => Self::Py,
+            "shell" => Self::Shell,
+            "cpp" => Self::Cpp,
+            "spellcheck" => Self::Spellcheck,
+            _ => return Err(ExtraCheckParseError::UnknownLang(s.to_string())),
+        })
+    }
+}
+
+#[derive(PartialEq, Copy, Clone)]
+enum ExtraCheckKind {
+    Lint,
+    Fmt,
+    /// Never parsed, but used as a placeholder for
+    /// langs that never have a specific kind.
+    None,
+}
+
+impl FromStr for ExtraCheckKind {
+    type Err = ExtraCheckParseError;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        Ok(match s {
+            "lint" => Self::Lint,
+            "fmt" => Self::Fmt,
+            _ => return Err(ExtraCheckParseError::UnknownKind(s.to_string())),
+        })
+    }
+}

src/tools/tidy/src/lib.rs

@@ -124,6 +124,40 @@ pub fn git_diff<S: AsRef<OsStr>>(base_commit: &str, extra_arg: S) -> Option<Stri
     Some(String::from_utf8_lossy(&output.stdout).into())
 }
 
+/// Returns true if any modified file matches the predicate, if we are in CI, or if unable to list modified files.
+pub fn files_modified(ci_info: &CiInfo, pred: impl Fn(&str) -> bool) -> bool {
+    if CiEnv::is_ci() {
+        // assume everything is modified on CI because we really don't want false positives there.
+        return true;
+    }
+    let Some(base_commit) = &ci_info.base_commit else {
+        eprintln!("No base commit, assuming all files are modified");
+        return true;
+    };
+    match crate::git_diff(&base_commit, "--name-status") {
+        Some(output) => {
+            let modified_files = output.lines().filter_map(|ln| {
+                let (status, name) = ln
+                    .trim_end()
+                    .split_once('\t')
+                    .expect("bad format from `git diff --name-status`");
+                if status == "M" { Some(name) } else { None }
+            });
+            for modified_file in modified_files {
+                if pred(modified_file) {
+                    return true;
+                }
+            }
+            false
+        }
+        None => {
+            eprintln!("warning: failed to run `git diff` to check for changes");
+            eprintln!("warning: assuming all files are modified");
+            true
+        }
+    }
+}
+
 pub mod alphabetical;
 pub mod bins;
 pub mod debug_artifacts;

src/tools/tidy/src/main.rs

@@ -173,7 +173,15 @@ fn main() {
         };
         check!(unstable_book, &src_path, collected);
 
-        check!(ext_tool_checks, &root_path, &output_directory, bless, extra_checks, pos_args);
+        check!(
+            ext_tool_checks,
+            &root_path,
+            &output_directory,
+            &ci_info,
+            bless,
+            extra_checks,
+            pos_args
+        );
     });
 
     if bad.load(Ordering::Relaxed) {

src/tools/tidy/src/rustdoc_json.rs

@@ -14,22 +14,10 @@ pub fn check(src_path: &Path, ci_info: &crate::CiInfo, bad: &mut bool) {
     };
 
     // First we check that `src/rustdoc-json-types` was modified.
-    match crate::git_diff(&base_commit, "--name-status") {
-        Some(output) => {
-            if !output
-                .lines()
-                .any(|line| line.starts_with("M") && line.contains(RUSTDOC_JSON_TYPES))
-            {
-                // `rustdoc-json-types` was not modified so nothing more to check here.
-                println!("`rustdoc-json-types` was not modified.");
-                return;
-            }
-        }
-        None => {
-            *bad = true;
-            eprintln!("error: failed to run `git diff` in rustdoc_json check");
-            return;
-        }
+    if !crate::files_modified(ci_info, |p| p == RUSTDOC_JSON_TYPES) {
+        // `rustdoc-json-types` was not modified so nothing more to check here.
+        println!("`rustdoc-json-types` was not modified.");
+        return;
     }
     // Then we check that if `FORMAT_VERSION` was updated, the `Latest feature:` was also updated.
     match crate::git_diff(&base_commit, src_path.join("rustdoc-json-types")) {