Auto merge of #10398 - Alexendoo:auto-lintcheck, r=xFrednet

bors · bors · commit a2c9782128b9 · 2024-06-16T10:03:12.000Z
Run a diff of lintcheck against the merge base for pull requests changelog: none  This is an MVP of sorts, it consists of #9764 + a GitHub action that feeds the output to the [job summary](https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#adding-a-job-summary). It doesn't yet do anything fancy like `--recursive` or adding comments to the PR, so you'd have to click through to the action to see the results Example output of a change (Alexendoo@0be1ab8): https://github.com/Alexendoo/rust-clippy/actions/runs/4264858870#summary-11583333018 r? `@flip1995`
diff --git a/.github/workflows/lintcheck.yml b/.github/workflows/lintcheck.yml
@@ -0,0 +1,115 @@
+name: Lintcheck
+
+on: pull_request
+
+env:
+  RUST_BACKTRACE: 1
+  CARGO_INCREMENTAL: 0
+
+concurrency:
+  # For a given workflow, if we push to the same PR, cancel all previous builds on that PR.
+  group: "${{ github.workflow }}-${{ github.event.pull_request.number}}"
+  cancel-in-progress: true
+
+jobs:
+  # Generates `lintcheck-logs/base.json` and stores it in a cache
+  base:
+    runs-on: ubuntu-latest
+
+    outputs:
+      key: ${{ steps.key.outputs.key }}
+
+    steps:
+    - name: Checkout
+      uses: actions/checkout@v4
+      with:
+        fetch-depth: 2
+
+    # HEAD is the generated merge commit `refs/pull/N/merge` between the PR and `master`, `HEAD^`
+    # being the commit from `master` that is the base of the merge
+    - name: Checkout base
+      run: git checkout HEAD^
+
+    # Use the lintcheck from the PR to generate the JSON in case the PR modifies lintcheck in some
+    # way
+    - name: Checkout current lintcheck
+      run: |
+        rm -rf lintcheck
+        git checkout ${{ github.sha }} -- lintcheck
+
+    - name: Create cache key
+      id: key
+      run: echo "key=lintcheck-base-${{ hashfiles('lintcheck/**') }}-$(git rev-parse HEAD)" >> "$GITHUB_OUTPUT"
+
+    - name: Cache results
+      id: cache
+      uses: actions/cache@v4
+      with:
+        path: lintcheck-logs/base.json
+        key: ${{ steps.key.outputs.key }}
+
+    - name: Run lintcheck
+      if: steps.cache.outputs.cache-hit != 'true'
+      run: cargo lintcheck --format json
+
+    - name: Rename JSON file
+      if: steps.cache.outputs.cache-hit != 'true'
+      run: mv lintcheck-logs/lintcheck_crates_logs.json lintcheck-logs/base.json
+
+  # Generates `lintcheck-logs/head.json` and stores it in a cache
+  head:
+    runs-on: ubuntu-latest
+
+    outputs:
+      key: ${{ steps.key.outputs.key }}
+
+    steps:
+    - name: Checkout
+      uses: actions/checkout@v4
+
+    - name: Create cache key
+      id: key
+      run: echo "key=lintcheck-head-${{ github.sha }}" >> "$GITHUB_OUTPUT"
+
+    - name: Cache results
+      id: cache
+      uses: actions/cache@v4
+      with:
+        path: lintcheck-logs/head.json
+        key: ${{ steps.key.outputs.key }}
+
+    - name: Run lintcheck
+      if: steps.cache.outputs.cache-hit != 'true'
+      run: cargo lintcheck --format json
+
+    - name: Rename JSON file
+      if: steps.cache.outputs.cache-hit != 'true'
+      run: mv lintcheck-logs/lintcheck_crates_logs.json lintcheck-logs/head.json
+
+  # Retrieves `lintcheck-logs/base.json` and `lintcheck-logs/head.json` from the cache and prints
+  # the diff to the GH actions step summary
+  diff:
+    runs-on: ubuntu-latest
+
+    needs: [base, head]
+
+    steps:
+    - name: Checkout
+      uses: actions/checkout@v4
+
+    - name: Restore base JSON
+      uses: actions/cache/restore@v4
+      with:
+        key: ${{ needs.base.outputs.key }}
+        path: lintcheck-logs/base.json
+        fail-on-cache-miss: true
+
+    - name: Restore head JSON
+      uses: actions/cache/restore@v4
+      with:
+        key: ${{ needs.head.outputs.key }}
+        path: lintcheck-logs/head.json
+        fail-on-cache-miss: true
+
+    - name: Diff results
+      run: cargo lintcheck diff lintcheck-logs/base.json lintcheck-logs/head.json >> $GITHUB_STEP_SUMMARY
diff --git a/lintcheck/Cargo.toml b/lintcheck/Cargo.toml
@@ -16,11 +16,13 @@ cargo_metadata = "0.15.3"
 clap = { version = "4.4", features = ["derive", "env"] }
 crates_io_api = "0.8.1"
 crossbeam-channel = "0.5.6"
+diff = "0.1.13"
 flate2 = "1.0"
 indicatif = "0.17.3"
 rayon = "1.5.1"
 serde = { version = "1.0", features = ["derive"] }
 serde_json = "1.0.85"
+strip-ansi-escapes = "0.1.1"
 tar = "0.4"
 toml = "0.7.3"
 ureq = "2.2"
diff --git a/lintcheck/src/config.rs b/lintcheck/src/config.rs
@@ -1,8 +1,9 @@
-use clap::Parser;
+use clap::{Parser, Subcommand, ValueEnum};
 use std::num::NonZero;
 use std::path::PathBuf;
 
-#[derive(Clone, Debug, Parser)]
+#[derive(Parser, Clone, Debug)]
+#[command(args_conflicts_with_subcommands = true)]
 pub(crate) struct LintcheckConfig {
     /// Number of threads to use (default: all unless --fix or --recursive)
     #[clap(
@@ -35,12 +36,36 @@ pub(crate) struct LintcheckConfig {
     /// Apply a filter to only collect specified lints, this also overrides `allow` attributes
     #[clap(long = "filter", value_name = "clippy_lint_name", use_value_delimiter = true)]
     pub lint_filter: Vec<String>,
-    /// Change the reports table to use markdown links
-    #[clap(long)]
-    pub markdown: bool,
+    /// Set the output format of the log file
+    #[clap(long, short, default_value = "text")]
+    pub format: OutputFormat,
     /// Run clippy on the dependencies of crates specified in crates-toml
     #[clap(long, conflicts_with("max_jobs"))]
     pub recursive: bool,
+    #[command(subcommand)]
+    pub subcommand: Option<Commands>,
+}
+
+#[derive(Subcommand, Clone, Debug)]
+pub(crate) enum Commands {
+    Diff { old: PathBuf, new: PathBuf },
+}
+
+#[derive(ValueEnum, Debug, Clone, Copy, PartialEq, Eq)]
+pub(crate) enum OutputFormat {
+    Text,
+    Markdown,
+    Json,
+}
+
+impl OutputFormat {
+    fn file_extension(self) -> &'static str {
+        match self {
+            OutputFormat::Text => "txt",
+            OutputFormat::Markdown => "md",
+            OutputFormat::Json => "json",
+        }
+    }
 }
 
 impl LintcheckConfig {
@@ -53,7 +78,7 @@ impl LintcheckConfig {
         config.lintcheck_results_path = PathBuf::from(format!(
             "lintcheck-logs/{}_logs.{}",
             filename.display(),
-            if config.markdown { "md" } else { "txt" }
+            config.format.file_extension(),
         ));
 
         // look at the --threads arg, if 0 is passed, use the threads count
diff --git a/lintcheck/src/json.rs b/lintcheck/src/json.rs
@@ -0,0 +1,122 @@
+use std::collections::HashMap;
+use std::fmt::Write;
+use std::fs;
+use std::hash::Hash;
+use std::path::Path;
+
+use crate::ClippyWarning;
+
+/// Creates the log file output for [`crate::config::OutputFormat::Json`]
+pub(crate) fn output(clippy_warnings: &[ClippyWarning]) -> String {
+    serde_json::to_string(&clippy_warnings).unwrap()
+}
+
+fn load_warnings(path: &Path) -> Vec<ClippyWarning> {
+    let file = fs::read(path).unwrap_or_else(|e| panic!("failed to read {}: {e}", path.display()));
+
+    serde_json::from_slice(&file).unwrap_or_else(|e| panic!("failed to deserialize {}: {e}", path.display()))
+}
+
+/// Group warnings by their primary span location + lint name
+fn create_map(warnings: &[ClippyWarning]) -> HashMap<impl Eq + Hash + '_, Vec<&ClippyWarning>> {
+    let mut map = HashMap::<_, Vec<_>>::with_capacity(warnings.len());
+
+    for warning in warnings {
+        let span = warning.span();
+        let key = (&warning.lint_type, &span.file_name, span.byte_start, span.byte_end);
+
+        map.entry(key).or_default().push(warning);
+    }
+
+    map
+}
+
+fn print_warnings(title: &str, warnings: &[&ClippyWarning]) {
+    if warnings.is_empty() {
+        return;
+    }
+
+    println!("### {title}");
+    println!("```");
+    for warning in warnings {
+        print!("{}", warning.diag);
+    }
+    println!("```");
+}
+
+fn print_changed_diff(changed: &[(&[&ClippyWarning], &[&ClippyWarning])]) {
+    fn render(warnings: &[&ClippyWarning]) -> String {
+        let mut rendered = String::new();
+        for warning in warnings {
+            write!(&mut rendered, "{}", warning.diag).unwrap();
+        }
+        rendered
+    }
+
+    if changed.is_empty() {
+        return;
+    }
+
+    println!("### Changed");
+    println!("```diff");
+    for &(old, new) in changed {
+        let old_rendered = render(old);
+        let new_rendered = render(new);
+
+        for change in diff::lines(&old_rendered, &new_rendered) {
+            use diff::Result::{Both, Left, Right};
+
+            match change {
+                Both(unchanged, _) => {
+                    println!(" {unchanged}");
+                },
+                Left(removed) => {
+                    println!("-{removed}");
+                },
+                Right(added) => {
+                    println!("+{added}");
+                },
+            }
+        }
+    }
+    println!("```");
+}
+
+pub(crate) fn diff(old_path: &Path, new_path: &Path) {
+    let old_warnings = load_warnings(old_path);
+    let new_warnings = load_warnings(new_path);
+
+    let old_map = create_map(&old_warnings);
+    let new_map = create_map(&new_warnings);
+
+    let mut added = Vec::new();
+    let mut removed = Vec::new();
+    let mut changed = Vec::new();
+
+    for (key, new) in &new_map {
+        if let Some(old) = old_map.get(key) {
+            if old != new {
+                changed.push((old.as_slice(), new.as_slice()));
+            }
+        } else {
+            added.extend(new);
+        }
+    }
+
+    for (key, old) in &old_map {
+        if !new_map.contains_key(key) {
+            removed.extend(old);
+        }
+    }
+
+    print!(
+        "{} added, {} removed, {} changed\n\n",
+        added.len(),
+        removed.len(),
+        changed.len()
+    );
+
+    print_warnings("Added", &added);
+    print_warnings("Removed", &removed);
+    print_changed_diff(&changed);
+}
diff --git a/lintcheck/src/main.rs b/lintcheck/src/main.rs
