Add a --diff flag to dry-run autofixes (#1431)

Cargo.lock

@@ -1930,6 +1930,7 @@ dependencies = [
  "serde-wasm-bindgen",
  "serde_json",
  "shellexpand",
+ "similar",
  "strum",
  "strum_macros",
  "test-case",

Cargo.toml

@@ -55,6 +55,7 @@ semver = { version = "1.0.16" }
 serde = { version = "1.0.147", features = ["derive"] }
 serde_json = { version = "1.0.87" }
 shellexpand = { version = "3.0.0" }
+similar = { version = "2.2.1" }
 strum = { version = "0.24.1", features = ["strum_macros"] }
 strum_macros = { version = "0.24.3" }
 textwrap = { version = "0.16.0" }

README.md

@@ -323,6 +323,8 @@ Options:
           Attempt to automatically fix lint errors
       --fix-only
           Fix any fixable lint errors, but don't report on leftover violations. Implies `--fix`
+      --diff
+          Avoid writing any fixed files back; instead, output a diff for each changed file to stdout
   -n, --no-cache
           Disable cache reads
       --select <SELECT>
@@ -344,7 +346,7 @@ Options:
       --per-file-ignores <PER_FILE_IGNORES>
           List of mappings from file pattern to code to exclude
       --format <FORMAT>
-          Output serialization format for error messages [possible values: text, json, junit, grouped, github]
+          Output serialization format for error messages [possible values: text, json, junit, grouped, github, gitlab]
       --show-source
           Show violations with source code
       --respect-gitignore

setup.py

@@ -1,5 +1,6 @@
 import sys
 
+import os
 from setuptools import setup
 
 sys.stderr.write(

src/autofix/fixer.rs

@@ -14,6 +14,7 @@ use crate::source_code_locator::SourceCodeLocator;
 pub enum Mode {
     Generate,
     Apply,
+    Diff,
     None,
 }
 

src/cli.rs

@@ -50,6 +50,10 @@ pub struct Cli {
     fix_only: bool,
     #[clap(long, overrides_with("fix_only"), hide = true)]
     no_fix_only: bool,
+    /// Avoid writing any fixed files back; instead, output a diff for each
+    /// changed file to stdout.
+    #[arg(long)]
+    pub diff: bool,
     /// Disable cache reads.
     #[arg(short, long)]
     pub no_cache: bool,
@@ -154,6 +158,7 @@ impl Cli {
                 add_noqa: self.add_noqa,
                 autoformat: self.autoformat,
                 config: self.config,
+                diff: self.diff,
                 exit_zero: self.exit_zero,
                 explain: self.explain,
                 files: self.files,
@@ -213,6 +218,7 @@ pub struct Arguments {
     pub add_noqa: bool,
     pub autoformat: bool,
     pub config: Option<PathBuf>,
+    pub diff: bool,
     pub exit_zero: bool,
     pub explain: Option<CheckCode>,
     pub files: Vec<PathBuf>,

src/linter.rs

@@ -1,3 +1,4 @@
+use std::borrow::Cow;
 use std::fs::write;
 use std::io;
 use std::io::Write;
@@ -7,6 +8,7 @@ use std::path::Path;
 use anyhow::Result;
 use log::debug;
 use rustpython_parser::lexer::LexResult;
+use similar::TextDiff;
 
 use crate::ast::types::Range;
 use crate::autofix::fixer;
@@ -195,14 +197,24 @@ pub fn lint_path(
     let contents = fs::read_file(path)?;
 
     // Lint the file.
-    let (contents, fixed, messages) = lint(contents, path, package, settings, autofix)?;
+    let (transformed, fixed, messages) = lint(&contents, path, package, settings, autofix)?;
 
     // Re-populate the cache.
     cache::set(path, &metadata, settings, autofix, &messages, cache);
 
     // If we applied any fixes, write the contents back to disk.
     if fixed > 0 {
-        write(path, contents)?;
+        if matches!(autofix, fixer::Mode::Apply) {
+            write(path, transformed)?;
+        } else if matches!(autofix, fixer::Mode::Diff) {
+            let mut stdout = io::stdout().lock();
+            TextDiff::from_lines(&contents, &transformed)
+                .unified_diff()
+                .header(&fs::relativize_path(path), &fs::relativize_path(path))
+                .to_writer(&mut stdout)?;
+            stdout.write_all(b"\n")?;
+            stdout.flush()?;
+        }
     }
 
     Ok(Diagnostics { messages, fixed })
@@ -286,18 +298,15 @@ pub fn autoformat_path(path: &Path, settings: &Settings) -> Result<()> {
 pub fn lint_stdin(
     path: Option<&Path>,
     package: Option<&Path>,
-    stdin: &str,
+    contents: &str,
     settings: &Settings,
     autofix: fixer::Mode,
 ) -> Result<Diagnostics> {
     // Validate the `Settings` and return any errors.
     settings.validate()?;
 
-    // Read the file from disk.
-    let contents = stdin.to_string();
-
     // Lint the file.
-    let (contents, fixed, messages) = lint(
+    let (transformed, fixed, messages) = lint(
         contents,
         path.unwrap_or_else(|| Path::new("-")),
         package,
@@ -307,19 +316,30 @@ pub fn lint_stdin(
 
     // Write the fixed contents to stdout.
     if matches!(autofix, fixer::Mode::Apply) {
-        io::stdout().write_all(contents.as_bytes())?;
+        io::stdout().write_all(transformed.as_bytes())?;
+    } else if matches!(autofix, fixer::Mode::Diff) {
+        let header = path.map_or_else(|| Cow::from("stdin"), fs::relativize_path);
+        let mut stdout = io::stdout().lock();
+        TextDiff::from_lines(contents, &transformed)
+            .unified_diff()
+            .header(&header, &header)
+            .to_writer(&mut stdout)?;
+        stdout.write_all(b"\n")?;
+        stdout.flush()?;
     }
 
     Ok(Diagnostics { messages, fixed })
 }
 
 fn lint(
-    mut contents: String,
+    contents: &str,
     path: &Path,
     package: Option<&Path>,
     settings: &Settings,
     autofix: fixer::Mode,
 ) -> Result<(String, usize, Vec<Message>)> {
+    let mut contents = contents.to_string();
+
     // Track the number of fixed errors across iterations.
     let mut fixed = 0;
 
@@ -359,7 +379,8 @@ fn lint(
         )?;
 
         // Apply autofix.
-        if matches!(autofix, fixer::Mode::Apply) && iterations < MAX_ITERATIONS {
+        if matches!(autofix, fixer::Mode::Apply | fixer::Mode::Diff) && iterations < MAX_ITERATIONS
+        {
             if let Some((fixed_contents, applied)) = fix_file(&checks, &locator) {
                 // Count the number of fixed errors.
                 fixed += applied;
@@ -376,7 +397,7 @@ fn lint(
         }
 
         // Convert to messages.
-        let filename = path.to_string_lossy().to_string();
+        let path = path.to_string_lossy();
         break checks
             .into_iter()
             .map(|check| {
@@ -385,7 +406,7 @@ fn lint(
                 } else {
                     None
                 };
-                Message::from_check(check, filename.clone(), source)
+                Message::from_check(check, path.to_string(), source)
             })
             .collect();
     };

src/main_native.rs

@@ -123,27 +123,6 @@ pub(crate) fn inner_main() -> Result<ExitCode> {
             (settings.fix, settings.fix_only, settings.format)
         }
     };
-    // Autofix rules are as follows:
-    // - If `--fix` or `--fix-only` is set, always apply fixes to the filesystem (or
-    //   print them to stdout, if we're reading from stdin).
-    // - Otherwise, if `--format json` is set, generate the fixes (so we print them
-    //   out as part of the JSON payload), but don't write them to disk.
-    // TODO(charlie): Consider adding ESLint's `--fix-dry-run`, which would generate
-    // but not apply fixes. That would allow us to avoid special-casing JSON
-    // here.
-    let autofix = if fix || fix_only {
-        fixer::Mode::Apply
-    } else if matches!(format, SerializationFormat::Json) {
-        fixer::Mode::Generate
-    } else {
-        fixer::Mode::None
-    };
-    let violations = if fix_only {
-        Violations::Hide
-    } else {
-        Violations::Show
-    };
-    let cache = !cli.no_cache;
 
     if let Some(code) = cli.explain {
         commands::explain(&code, &format)?;
@@ -158,9 +137,35 @@ pub(crate) fn inner_main() -> Result<ExitCode> {
         return Ok(ExitCode::SUCCESS);
     }
 
+    // Autofix rules are as follows:
+    // - If `--fix` or `--fix-only` is set, always apply fixes to the filesystem (or
+    //   print them to stdout, if we're reading from stdin).
+    // - Otherwise, if `--format json` is set, generate the fixes (so we print them
+    //   out as part of the JSON payload), but don't write them to disk.
+    // - If `--diff` or `--fix-only` are set, don't print any violations (only
+    //   fixes).
+    // TODO(charlie): Consider adding ESLint's `--fix-dry-run`, which would generate
+    // but not apply fixes. That would allow us to avoid special-casing JSON
+    // here.
+    let autofix = if cli.diff {
+        fixer::Mode::Diff
+    } else if fix || fix_only {
+        fixer::Mode::Apply
+    } else if matches!(format, SerializationFormat::Json) {
+        fixer::Mode::Generate
+    } else {
+        fixer::Mode::None
+    };
+    let violations = if cli.diff || fix_only {
+        Violations::Hide
+    } else {
+        Violations::Show
+    };
+    let cache = !cli.no_cache;
+
     let printer = Printer::new(&format, &log_level, &autofix, &violations);
     if cli.watch {
-        if matches!(autofix, fixer::Mode::Generate | fixer::Mode::Apply) {
+        if !matches!(autofix, fixer::Mode::None) {
             eprintln!("Warning: --fix is not enabled in watch mode.");
         }
         if cli.add_noqa {
@@ -259,7 +264,7 @@ pub(crate) fn inner_main() -> Result<ExitCode> {
         // Always try to print violations (the printer itself may suppress output),
         // unless we're writing fixes via stdin (in which case, the transformed
         // source code goes to stdout).
-        if !(is_stdin && matches!(autofix, fixer::Mode::Apply)) {
+        if !(is_stdin && matches!(autofix, fixer::Mode::Apply | fixer::Mode::Diff)) {
             printer.write_once(&diagnostics)?;
         }
 
@@ -269,8 +274,14 @@ pub(crate) fn inner_main() -> Result<ExitCode> {
             drop(updates::check_for_updates());
         }
 
-        if !diagnostics.messages.is_empty() && !cli.exit_zero && !fix_only {
-            return Ok(ExitCode::FAILURE);
+        if !cli.exit_zero {
+            if cli.diff || fix_only {
+                if diagnostics.fixed > 0 {
+                    return Ok(ExitCode::FAILURE);
+                }
+            } else if !diagnostics.messages.is_empty() {
+                return Ok(ExitCode::FAILURE);
+            }
         }
     }
 

src/printer.rs

@@ -90,7 +90,11 @@ impl<'a> Printer<'a> {
                 Violations::Hide => {
                     let fixed = diagnostics.fixed;
                     if fixed > 0 {
-                        println!("Fixed {fixed} error(s).");
+                        if matches!(self.autofix, fixer::Mode::Apply) {
+                            println!("Fixed {fixed} error(s).");
+                        } else if matches!(self.autofix, fixer::Mode::Diff) {
+                            println!("Would fix {fixed} error(s).");
+                        }
                     }
                 }
             }

src/settings/flags.rs

@@ -20,7 +20,7 @@ impl From<bool> for Autofix {
 impl From<fixer::Mode> for Autofix {
     fn from(value: fixer::Mode) -> Self {
         match value {
-            fixer::Mode::Generate | fixer::Mode::Apply => Autofix::Enabled,
+            fixer::Mode::Generate | fixer::Mode::Diff | fixer::Mode::Apply => Autofix::Enabled,
             fixer::Mode::None => Autofix::Disabled,
         }
     }