feat: format command

Add a `format` command:
- linters can be identified as formatters with the `is_formatter` config option
- `lintrunner format` will run formatting lints and accept all changes
- add infer_subcommands so you can do `lintrunner f`.

Author: suo

src/lib.rs

@@ -121,7 +121,7 @@ fn get_paths_from_file(file: AbsPath) -> Result<Vec<AbsPath>> {
 }
 
 /// Represents the set of paths the user wants to lint.
-pub enum PathsToLint {
+pub enum PathsOpt {
     /// The user didn't specify any paths, so we'll automatically determine
     /// which paths to check.
     Auto,
@@ -150,7 +150,7 @@ pub enum RenderOpt {
 
 pub fn do_lint(
     linters: Vec<Linter>,
-    paths_to_lint: PathsToLint,
+    paths_opt: PathsOpt,
     should_apply_patches: bool,
     render_opt: RenderOpt,
     enable_spinners: bool,
@@ -161,8 +161,8 @@ pub fn do_lint(
         linters.iter().map(|l| &l.code).collect::<Vec<_>>()
     );
 
-    let mut files = match paths_to_lint {
-        PathsToLint::Auto => {
+    let mut files = match paths_opt {
+        PathsOpt::Auto => {
             let git_root = get_git_root()?;
             let relative_to = match revision_opt {
                 RevisionOpt::Head => None,
@@ -173,9 +173,9 @@ pub fn do_lint(
             };
             get_changed_files(&git_root, relative_to.as_deref())?
         }
-        PathsToLint::PathsCmd(paths_cmd) => get_paths_from_cmd(paths_cmd)?,
-        PathsToLint::Paths(paths) => get_paths_from_input(paths)?,
-        PathsToLint::PathsFile(file) => get_paths_from_file(file)?,
+        PathsOpt::PathsCmd(paths_cmd) => get_paths_from_cmd(paths_cmd)?,
+        PathsOpt::Paths(paths) => get_paths_from_input(paths)?,
+        PathsOpt::PathsFile(file) => get_paths_from_file(file)?,
     };
 
     // Sort and unique the files so we pass a consistent ordering to linters

src/lint_config.rs

@@ -12,6 +12,10 @@ pub struct LintRunnerConfig {
     pub linters: Vec<LintConfig>,
 }
 
+fn is_false(b: &bool) -> bool {
+    return *b == false;
+}
+
 /// Represents a single linter, along with all the information necessary to invoke it.
 ///
 /// This goes in the linter configuration TOML file.
@@ -30,7 +34,7 @@ pub struct LintRunnerConfig {
 ///     '@{{PATHSFILE}}'
 /// ]
 /// ```
-#[derive(Serialize, Deserialize)]
+#[derive(Serialize, Deserialize, Clone)]
 pub struct LintConfig {
     /// The name of the linter, conventionally capitals and numbers, no spaces,
     /// dashes, or underscores
@@ -103,6 +107,13 @@ pub struct LintConfig {
     /// ```toml
     /// command = ['python3', 'my_linter_init.py', '--dry-run={{DRYRUN}}']
     pub init_command: Option<Vec<String>>,
+
+    /// If true, this linter will be considered a formatter, and will invoked by
+    /// `lintrunner format`. Formatters should be *safe*: people should be able
+    /// to blindly accept the output without worrying that it will change the
+    /// meaning of their code.
+    #[serde(skip_serializing_if = "is_false", default = "bool::default")]
+    pub is_formatter: bool,
 }
 
 /// Given options specified by the user, return a list of linters to run.

src/main.rs

@@ -10,80 +10,80 @@ use lintrunner::{
     path::AbsPath,
     persistent_data::PersistentDataStore,
     render::print_error,
-    PathsToLint, RenderOpt, RevisionOpt,
+    PathsOpt, RenderOpt, RevisionOpt,
 };
 
 #[derive(Debug, Parser)]
-#[structopt(name = "lintrunner", about = "A lint runner")]
+#[clap(name = "lintrunner", about = "A lint runner", infer_subcommands(true))]
 struct Args {
     /// Verbose mode (-v, or -vv to show full list of paths being linted)
-    #[clap(short, long, parse(from_occurrences))]
+    #[clap(short, long, parse(from_occurrences), global = true)]
     verbose: u8,
 
     /// Path to a toml file defining which linters to run
-    #[clap(long, default_value = ".lintrunner.toml")]
+    #[clap(long, default_value = ".lintrunner.toml", global = true)]
     config: String,
 
     /// If set, any suggested patches will be applied
-    #[clap(short, long)]
+    #[clap(short, long, global = true)]
     apply_patches: bool,
 
     /// Shell command that returns new-line separated paths to lint
     ///
     /// Example: To run on all files in the repo, use `--paths-cmd='git grep -Il .'`.
-    #[clap(long, conflicts_with = "paths-from")]
+    #[clap(long, conflicts_with = "paths-from", global = true)]
     paths_cmd: Option<String>,
 
     /// File with new-line separated paths to lint
-    #[clap(long)]
+    #[clap(long, global = true)]
     paths_from: Option<String>,
 
     /// Lint all files that differ between the working directory and the
     /// specified revision. This argument can be any <tree-ish> that is accepted
     /// by `git diff-tree`
-    #[clap(long, short, conflicts_with_all=&["paths", "paths-cmd", "paths-from"])]
+    #[clap(long, short, conflicts_with_all=&["paths", "paths-cmd", "paths-from"], global = true)]
     revision: Option<String>,
 
     /// Lint all files that differ between the merge base of HEAD with the
     /// specified revision and HEAD. This argument can be any <tree-sh> that is
     /// accepted by `git diff-tree`
     ///
     /// Example: lintrunner -m master
-    #[clap(long, short, conflicts_with_all=&["paths", "paths-cmd", "paths-from", "revision"])]
+    #[clap(long, short, conflicts_with_all=&["paths", "paths-cmd", "paths-from", "revision"], global = true)]
     merge_base_with: Option<String>,
 
     /// Comma-separated list of linters to skip (e.g. --skip CLANGFORMAT,NOQA)
-    #[clap(long)]
+    #[clap(long, global = true)]
     skip: Option<String>,
 
     /// Comma-separated list of linters to run (opposite of --skip)
-    #[clap(long)]
+    #[clap(long, global = true)]
     take: Option<String>,
 
     /// With 'default' show lint issues in human-readable format, for interactive use.
     /// With 'json', show lint issues as machine-readable JSON (one per line)
     /// With 'oneline', show lint issues in compact format (one per line)
-    #[clap(long, arg_enum, default_value_t = RenderOpt::Default)]
+    #[clap(long, arg_enum, default_value_t = RenderOpt::Default, global=true)]
     output: RenderOpt,
 
+    /// Paths to lint. lintrunner will still respect the inclusions and
     #[clap(subcommand)]
     cmd: Option<SubCommand>,
 
-    /// Paths to lint. lintrunner will still respect the inclusions and
     /// exclusions defined in .lintrunner.toml; manually specifying a path will
     /// not override them.
-    #[clap(conflicts_with_all = &["paths-cmd", "paths-from"])]
+    #[clap(conflicts_with_all = &["paths-cmd", "paths-from"], global = true)]
     paths: Vec<String>,
 
     /// If set, always output with ANSI colors, even if we detect the output is
     /// not a user-attended terminal.
-    #[clap(long)]
+    #[clap(long, global = true)]
     force_color: bool,
 
     /// If set, use ths provided path to store any metadata generated by
     /// lintrunner. By default, this is a platform-specific location for
     /// application data (e.g. $XDG_DATA_HOME for UNIX systems.)
-    #[clap(long)]
+    #[clap(long, global = true)]
     data_path: Option<String>,
 }
 
@@ -95,6 +95,12 @@ enum SubCommand {
         #[clap(long, short)]
         dry_run: bool,
     },
+    /// Run and accept changes for formatting linters only. Equivalent to
+    /// `lintrunner --apply-patches --take <formatters>`.
+    Format,
+
+    /// Run linters. This is the default if no subcommand is provided.
+    Lint,
 }
 
 fn do_main() -> Result<i32> {
@@ -120,6 +126,10 @@ fn do_main() -> Result<i32> {
 
     let config_path = AbsPath::try_from(&args.config)
         .with_context(|| format!("Could not read lintrunner config at: '{}'", args.config))?;
+
+    let cmd = args.cmd.unwrap_or(SubCommand::Lint);
+    let lint_runner_config = LintRunnerConfig::new(&config_path)?;
+
     let skipped_linters = args.skip.map(|linters| {
         linters
             .split(',')
@@ -133,28 +143,32 @@ fn do_main() -> Result<i32> {
             .collect::<HashSet<_>>()
     });
 
-    let lint_runner_config = LintRunnerConfig::new(&config_path)?;
+    // If we are formatting, the universe of linters to select from should be
+    // restricted to only formatters.
+    // (NOTE: we pay an allocation for `placeholder` even in cases where we are
+    // just passing through a reference in the else-branch. This doesn't matter,
+    // but if we want to fix it we should impl Cow for LintConfig and use that
+    // instead.).
+    let mut placeholder = Vec::new();
+    let all_linters = if let SubCommand::Format = &cmd {
+        let iter = lint_runner_config
+            .linters
+            .iter()
+            .filter(|l| l.is_formatter)
+            .map(|l| l.clone());
+        placeholder.extend(iter);
+        &placeholder
+    } else {
+        // If we're not formatting, all linters defined in the config are
+        // eligible to run.
+        &lint_runner_config.linters
+    };
 
-    let linters = get_linters_from_config(
-        &lint_runner_config.linters,
-        skipped_linters,
-        taken_linters,
-        &config_path,
-    )?;
+    let linters =
+        get_linters_from_config(all_linters, skipped_linters, taken_linters, &config_path)?;
 
     let enable_spinners = args.verbose == 0 && args.output == RenderOpt::Default;
-
-    let paths_to_lint = if let Some(paths_file) = args.paths_from {
-        let path_file = AbsPath::try_from(&paths_file)
-            .with_context(|| format!("Failed to find `--paths-from` file '{}'", paths_file))?;
-        PathsToLint::PathsFile(path_file)
-    } else if let Some(paths_cmd) = args.paths_cmd {
-        PathsToLint::PathsCmd(paths_cmd)
-    } else if !args.paths.is_empty() {
-        PathsToLint::Paths(args.paths)
-    } else {
-        PathsToLint::Auto
-    };
+    let persistent_data_store = PersistentDataStore::new(&config_path)?;
 
     let revision_opt = if let Some(revision) = args.revision {
         RevisionOpt::Revision(revision)
@@ -164,19 +178,40 @@ fn do_main() -> Result<i32> {
         RevisionOpt::Head
     };
 
-    let persistent_data_store = PersistentDataStore::new(&config_path)?;
+    let paths_opt = if let Some(paths_file) = args.paths_from {
+        let path_file = AbsPath::try_from(&paths_file)
+            .with_context(|| format!("Failed to find `--paths-from` file '{}'", paths_file))?;
+        PathsOpt::PathsFile(path_file)
+    } else if let Some(paths_cmd) = args.paths_cmd {
+        PathsOpt::PathsCmd(paths_cmd)
+    } else if !args.paths.is_empty() {
+        PathsOpt::Paths(args.paths)
+    } else {
+        PathsOpt::Auto
+    };
 
-    match args.cmd {
-        Some(SubCommand::Init { dry_run }) => {
+    match cmd {
+        SubCommand::Init { dry_run } => {
             // Just run initialization commands, don't actually lint.
             do_init(linters, dry_run, &persistent_data_store, &config_path)
         }
-        None => {
+        SubCommand::Format => {
+            check_init_changed(&persistent_data_store, &lint_runner_config)?;
+            do_lint(
+                linters,
+                paths_opt,
+                true, // always apply patches when we use the format command
+                args.output,
+                enable_spinners,
+                revision_opt,
+            )
+        }
+        SubCommand::Lint => {
             // Default command is to just lint.
             check_init_changed(&persistent_data_store, &lint_runner_config)?;
             do_lint(
                 linters,
-                paths_to_lint,
+                paths_opt,
                 args.apply_patches,
                 args.output,
                 enable_spinners,

tests/integration_test.rs

@@ -562,3 +562,44 @@ fn excluding_dryrun_fails() -> Result<()> {
 
     Ok(())
 }
+
+#[test]
+fn format_command_doesnt_use_nonformat_linter() -> Result<()> {
+    let data_path = tempfile::tempdir()?;
+    let lint_message = LintMessage {
+        path: Some("tests/fixtures/fake_source_file.rs".to_string()),
+        line: Some(9),
+        char: Some(1),
+        code: "DUMMY".to_string(),
+        name: "dummy failure".to_string(),
+        severity: LintSeverity::Advice,
+        original: None,
+        replacement: None,
+        description: Some("A dummy linter failure".to_string()),
+    };
+    let config = temp_config(&format!(
+        "\
+            [[linter]]
+            code = 'TESTLINTER'
+            include_patterns = ['**']
+            command = ['echo', '{}']
+        ",
+        serde_json::to_string(&lint_message)?
+    ))?;
+
+    let mut cmd = Command::cargo_bin("lintrunner")?;
+    cmd.arg(format!("--config={}", config.path().to_str().unwrap()));
+    cmd.arg(format!(
+        "--data-path={}",
+        data_path.path().to_str().unwrap()
+    ));
+
+    // Run on a file to ensure that the linter is run.
+    cmd.arg("format");
+    cmd.arg("README.md");
+    // Should succeed because TESTLINTER was not run
+    cmd.assert().success();
+    assert_output_snapshot("format_command_doesnt_use_nonformat_linter", &mut cmd)?;
+
+    Ok(())
+}

tests/snapshots/integration_test__format_command_doesnt_use_nonformat_linter.snap

@@ -0,0 +1,13 @@
+---
+source: tests/integration_test.rs
+assertion_line: 20
+expression: output_lines
+---
+- "STDOUT:"
+- ok No lint issues.
+- Successfully applied all patches.
+- ""
+- ""
+- "STDERR:"
+- "WARNING: No previous init data found. If this is the first time you're running lintrunner, you should run `lintrunner init`."
+