From 4798383c33e28179d1ad5c226d9801ceac05601c Mon Sep 17 00:00:00 2001 From: xFrednet Date: Sun, 21 Jul 2024 12:52:55 +0200 Subject: [PATCH 1/6] Lintcheck: Limit summary size to 1024k and copy to logs --- .github/workflows/lintcheck.yml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/.github/workflows/lintcheck.yml b/.github/workflows/lintcheck.yml index 46fd3089f44..bc7792deee4 100644 --- a/.github/workflows/lintcheck.yml +++ b/.github/workflows/lintcheck.yml @@ -115,4 +115,10 @@ jobs: uses: actions/download-artifact@v4 - name: Diff results - run: ./target/debug/lintcheck diff {base,head}/ci_crates_logs.json >> $GITHUB_STEP_SUMMARY + # GH's summery has a maximum size of 1024k: + # https://docs.github.com/actions/using-workflows/workflow-commands-for-github-actions#adding-a-markdown-summary + # That's why we first log to file and then to the summary and logs + run: | + ./target/debug/lintcheck diff {base,head}/ci_crates_logs.json >> ci_crates.diff + head -c 1024000 ci_crates.diff >> $GITHUB_STEP_SUMMARY + cat ci_crates.diff From 4fea5199e4936692b2682a61a9205077604b9bbb Mon Sep 17 00:00:00 2001 From: xFrednet Date: Sun, 21 Jul 2024 14:55:46 +0200 Subject: [PATCH 2/6] Lintcheck: Order summary by lint and truncate messages --- .github/workflows/lintcheck.yml | 13 ++- lintcheck/src/config.rs | 8 +- lintcheck/src/json.rs | 179 ++++++++++++++++++++++++++++---- lintcheck/src/main.rs | 2 +- 4 files changed, 177 insertions(+), 25 deletions(-) diff --git a/.github/workflows/lintcheck.yml b/.github/workflows/lintcheck.yml index bc7792deee4..bfa40eed576 100644 --- a/.github/workflows/lintcheck.yml +++ b/.github/workflows/lintcheck.yml @@ -119,6 +119,13 @@ jobs: # https://docs.github.com/actions/using-workflows/workflow-commands-for-github-actions#adding-a-markdown-summary # That's why we first log to file and then to the summary and logs run: | - ./target/debug/lintcheck diff {base,head}/ci_crates_logs.json >> ci_crates.diff - head -c 1024000 ci_crates.diff >> $GITHUB_STEP_SUMMARY - cat ci_crates.diff + ./target/debug/lintcheck diff {base,head}/ci_crates_logs.json >> full_diff.md + ./target/debug/lintcheck diff {base,head}/ci_crates_logs.json --truncate >> truncated_diff.md + head -c 1024000 truncated_diff.md >> $GITHUB_STEP_SUMMARY + cat truncated_diff.md + + - name: Upload full diff + uses: actions/upload-artifact@v4 + with: + name: diff + path: full_diff.md diff --git a/lintcheck/src/config.rs b/lintcheck/src/config.rs index b35a62eed44..6bec1753fc7 100644 --- a/lintcheck/src/config.rs +++ b/lintcheck/src/config.rs @@ -53,7 +53,13 @@ pub(crate) struct LintcheckConfig { #[derive(Subcommand, Clone, Debug)] pub(crate) enum Commands { /// Display a markdown diff between two lintcheck log files in JSON format - Diff { old: PathBuf, new: PathBuf }, + Diff { + old: PathBuf, + new: PathBuf, + /// This will limit the number of warnings that will be printed for each lint + #[clap(long)] + truncate: bool, + }, /// Create a lintcheck crates TOML file containing the top N popular crates Popular { /// Output TOML file name diff --git a/lintcheck/src/json.rs b/lintcheck/src/json.rs index 74f36f000bd..8cee26b4ccc 100644 --- a/lintcheck/src/json.rs +++ b/lintcheck/src/json.rs @@ -1,3 +1,7 @@ +#![expect(unused)] + +use std::collections::BTreeSet; +use std::fmt::Write; use std::fs; use std::path::Path; @@ -6,7 +10,10 @@ use crate::ClippyWarning; -#[derive(Deserialize, Serialize)] +const DEFAULT_LIMIT_PER_LINT: usize = 300; +const TRUNCATION_TOTAL_TARGET: usize = 1000; + +#[derive(Debug, Deserialize, Serialize)] struct LintJson { lint: String, krate: String, @@ -52,14 +59,16 @@ fn load_warnings(path: &Path) -> Vec { serde_json::from_slice(&file).unwrap_or_else(|e| panic!("failed to deserialize {}: {e}", path.display())) } -fn print_warnings(title: &str, warnings: &[LintJson]) { +fn print_warnings(title: &str, warnings: &[LintJson], truncate_after: usize) { if warnings.is_empty() { return; } - //We have to use HTML here to be able to manually add an id. - println!(r#"

{title}

"#); + print_h3(&warnings[0].lint, title); println!(); + + let warnings = truncate(warnings, truncate_after); + for warning in warnings { println!("{}", warning.info_text(title)); println!(); @@ -70,14 +79,16 @@ fn print_warnings(title: &str, warnings: &[LintJson]) { } } -fn print_changed_diff(changed: &[(LintJson, LintJson)]) { +fn print_changed_diff(changed: &[(LintJson, LintJson)], truncate_after:usize) { if changed.is_empty() { return; } - //We have to use HTML here to be able to manually add an id. - println!(r#"

Changed

"#); + print_h3(&changed[0].0.lint, "Changed"); println!(); + + let changes = truncate(changed, truncate_after); + for (old, new) in changed { println!("{}", new.info_text("Changed")); println!(); @@ -101,7 +112,25 @@ fn print_changed_diff(changed: &[(LintJson, LintJson)]) { } } -pub(crate) fn diff(old_path: &Path, new_path: &Path) { +fn print_h3(lint: &str, title: &str) { + let html_id = to_html_id(lint); + // We have to use HTML here to be able to manually add an id. + println!(r#"

{title}

"#); +} + +fn truncate(list: &[T], truncate_after: usize) -> &[T] { + if list.len() > truncate_after { + println!("{} warnings have been truncated for this summary.", list.len() - truncate_after); + println!(); + + list.split_at(truncate_after).0 + } else { + list + } + +} + +pub(crate) fn diff(old_path: &Path, new_path: &Path, truncate: bool) { let old_warnings = load_warnings(old_path); let new_warnings = load_warnings(new_path); @@ -121,31 +150,141 @@ pub(crate) fn diff(old_path: &Path, new_path: &Path) { } } - print!( - r##"{}, {}, {}"##, - count_string("added", added.len()), - count_string("removed", removed.len()), - count_string("changed", changed.len()), - ); - println!(); + let lint_warnings = group_by_lint(added, removed, changed); + + print_summary_table(&lint_warnings); println!(); - print_warnings("Added", &added); - print_warnings("Removed", &removed); - print_changed_diff(&changed); + let truncate_after = if truncate { + // Max 9 ensures that we at least have three messages per lint + DEFAULT_LIMIT_PER_LINT.min(TRUNCATION_TOTAL_TARGET / lint_warnings.len()).max(9) + } else { + // No lint should ever each this number of lint emissions, so this is equivialent to + // No truncation + usize::MAX + }; + + for lint in lint_warnings { + print_lint_warnings(&lint, truncate_after) + } +} + +fn print_lint_warnings(lint: &LintWarnings, truncate_after: usize) { + let name = &lint.name; + let html_id = to_html_id(name); + + // The additional anchor is added for non GH viewers that don't prefix ID's + println!(r#"

{name}

"#); + println!(); + + print!( + r##"{}, {}, {}"##, + count_string(name, "added", lint.added.len()), + count_string(name, "removed", lint.removed.len()), + count_string(name, "changed", lint.changed.len()), + ); + println!(); + + print_warnings("Added", &lint.added, truncate_after / 3); + print_warnings("Removed", &lint.removed, truncate_after / 3); + print_changed_diff(&lint.changed, truncate_after / 3); +} + +#[derive(Debug)] +struct LintWarnings { + name: String, + added: Vec, + removed: Vec, + changed: Vec<(LintJson, LintJson)>, +} + +fn group_by_lint( + mut added: Vec, + mut removed: Vec, + mut changed: Vec<(LintJson, LintJson)>, +) -> Vec { + /// Collects items from an iterator while the condition is met + fn collect_while(iter: &mut std::iter::Peekable>, mut condition: F) -> Vec + where + F: FnMut(&T) -> bool, + { + let mut items = vec![]; + while iter.peek().map_or(false, |item| condition(item)) { + items.push(iter.next().unwrap()); + } + items + } + + // Sort + added.sort_unstable_by(|a, b| a.lint.cmp(&b.lint)); + removed.sort_unstable_by(|a, b| a.lint.cmp(&b.lint)); + changed.sort_unstable_by(|(a, _), (b, _)| a.lint.cmp(&b.lint)); + + // Collect lint names + let lint_names: BTreeSet<_> = added + .iter() + .chain(removed.iter()) + .chain(changed.iter().map(|(a, _)| a)) + .map(|warning| &warning.lint) + .cloned() + .collect(); + + let mut added_iter = added.into_iter().peekable(); + let mut removed_iter = removed.into_iter().peekable(); + let mut changed_iter = changed.into_iter().peekable(); + + let mut lints = vec![]; + for name in lint_names.into_iter() { + lints.push(LintWarnings { + added: collect_while(&mut added_iter, |warning| warning.lint == name), + removed: collect_while(&mut removed_iter, |warning| warning.lint == name), + changed: collect_while(&mut changed_iter, |(warning, _)| warning.lint == name), + name, + }); + } + + lints +} + +// This function limits the number of lints in the collection if needed. +// +// It's honestly amazing that a function like this is needed. But some restriction +// lints (Looking at you `implicit_return` and `if_then_some_else_none`) trigger a lot +// like 60'000+ times in our CI. +// +// Each lint in the list will be limited to 200 messages + +fn print_summary_table(lints: &[LintWarnings]) { + println!("| Lint | Added | Removed | Changed |"); + println!("| ------------------------------------------ | ------: | ------: | ------: |"); + + for lint in lints { + println!( + "| {:<62} | {:>7} | {:>7} | {:>7} |", + format!("[`{}`](#user-content-{})", lint.name, to_html_id(&lint.name)), + lint.added.len(), + lint.removed.len(), + lint.changed.len() + ); + } +} + +fn to_html_id(lint_name: &str) -> String { + lint_name.replace("clippy::", "").replace("_", "-") } /// This generates the `x added` string for the start of the job summery. /// It linkifies them if possible to jump to the respective heading. -fn count_string(label: &str, count: usize) -> String { +fn count_string(lint: &str, label: &str, count: usize) -> String { // Headlines are only added, if anything will be displayed under the headline. // We therefore only want to add links to them if they exist if count == 0 { format!("0 {label}") } else { + let lint_id = to_html_id(lint); // GitHub's job summaries don't add HTML ids to headings. That's why we // manually have to add them. GitHub prefixes these manual ids with // `user-content-` and that's how we end up with these awesome links :D - format!("[{count} {label}](#user-content-{label})") + format!("[{count} {label}](#user-content-{lint_id}-{label})") } } diff --git a/lintcheck/src/main.rs b/lintcheck/src/main.rs index db1ecb9e663..0dd62ded293 100644 --- a/lintcheck/src/main.rs +++ b/lintcheck/src/main.rs @@ -260,7 +260,7 @@ fn main() { let config = LintcheckConfig::new(); match config.subcommand { - Some(Commands::Diff { old, new }) => json::diff(&old, &new), + Some(Commands::Diff { old, new, truncate }) => json::diff(&old, &new, truncate), Some(Commands::Popular { output, number }) => popular_crates::fetch(output, number).unwrap(), None => lintcheck(config), } From 1f879fc0cf423c1ee1b8f30dcdbf0a708061c8be Mon Sep 17 00:00:00 2001 From: xFrednet Date: Mon, 22 Jul 2024 17:38:03 +0200 Subject: [PATCH 3/6] Lintcheck: Minor cleanup and function moves --- lintcheck/src/json.rs | 216 +++++++++++++++++++++--------------------- 1 file changed, 106 insertions(+), 110 deletions(-) diff --git a/lintcheck/src/json.rs b/lintcheck/src/json.rs index 8cee26b4ccc..2fa527c5f87 100644 --- a/lintcheck/src/json.rs +++ b/lintcheck/src/json.rs @@ -1,7 +1,4 @@ -#![expect(unused)] - use std::collections::BTreeSet; -use std::fmt::Write; use std::fs; use std::path::Path; @@ -10,6 +7,7 @@ use crate::ClippyWarning; +/// This is the total number. 300 warnings results in 100 messages per section. const DEFAULT_LIMIT_PER_LINT: usize = 300; const TRUNCATION_TOTAL_TARGET: usize = 1000; @@ -59,77 +57,6 @@ fn load_warnings(path: &Path) -> Vec { serde_json::from_slice(&file).unwrap_or_else(|e| panic!("failed to deserialize {}: {e}", path.display())) } -fn print_warnings(title: &str, warnings: &[LintJson], truncate_after: usize) { - if warnings.is_empty() { - return; - } - - print_h3(&warnings[0].lint, title); - println!(); - - let warnings = truncate(warnings, truncate_after); - - for warning in warnings { - println!("{}", warning.info_text(title)); - println!(); - println!("```"); - println!("{}", warning.rendered.trim_end()); - println!("```"); - println!(); - } -} - -fn print_changed_diff(changed: &[(LintJson, LintJson)], truncate_after:usize) { - if changed.is_empty() { - return; - } - - print_h3(&changed[0].0.lint, "Changed"); - println!(); - - let changes = truncate(changed, truncate_after); - - for (old, new) in changed { - println!("{}", new.info_text("Changed")); - println!(); - println!("```diff"); - for change in diff::lines(old.rendered.trim_end(), new.rendered.trim_end()) { - use diff::Result::{Both, Left, Right}; - - match change { - Both(unchanged, _) => { - println!(" {unchanged}"); - }, - Left(removed) => { - println!("-{removed}"); - }, - Right(added) => { - println!("+{added}"); - }, - } - } - println!("```"); - } -} - -fn print_h3(lint: &str, title: &str) { - let html_id = to_html_id(lint); - // We have to use HTML here to be able to manually add an id. - println!(r#"

{title}

"#); -} - -fn truncate(list: &[T], truncate_after: usize) -> &[T] { - if list.len() > truncate_after { - println!("{} warnings have been truncated for this summary.", list.len() - truncate_after); - println!(); - - list.split_at(truncate_after).0 - } else { - list - } - -} - pub(crate) fn diff(old_path: &Path, new_path: &Path, truncate: bool) { let old_warnings = load_warnings(old_path); let new_warnings = load_warnings(new_path); @@ -156,8 +83,10 @@ pub(crate) fn diff(old_path: &Path, new_path: &Path, truncate: bool) { println!(); let truncate_after = if truncate { - // Max 9 ensures that we at least have three messages per lint - DEFAULT_LIMIT_PER_LINT.min(TRUNCATION_TOTAL_TARGET / lint_warnings.len()).max(9) + // Max 15 ensures that we at least have five messages per lint + DEFAULT_LIMIT_PER_LINT + .min(TRUNCATION_TOTAL_TARGET / lint_warnings.len()) + .max(15) } else { // No lint should ever each this number of lint emissions, so this is equivialent to // No truncation @@ -165,31 +94,10 @@ pub(crate) fn diff(old_path: &Path, new_path: &Path, truncate: bool) { }; for lint in lint_warnings { - print_lint_warnings(&lint, truncate_after) + print_lint_warnings(&lint, truncate_after); } } -fn print_lint_warnings(lint: &LintWarnings, truncate_after: usize) { - let name = &lint.name; - let html_id = to_html_id(name); - - // The additional anchor is added for non GH viewers that don't prefix ID's - println!(r#"

{name}

"#); - println!(); - - print!( - r##"{}, {}, {}"##, - count_string(name, "added", lint.added.len()), - count_string(name, "removed", lint.removed.len()), - count_string(name, "changed", lint.changed.len()), - ); - println!(); - - print_warnings("Added", &lint.added, truncate_after / 3); - print_warnings("Removed", &lint.removed, truncate_after / 3); - print_changed_diff(&lint.changed, truncate_after / 3); -} - #[derive(Debug)] struct LintWarnings { name: String, @@ -209,7 +117,7 @@ fn collect_while(iter: &mut std::iter::Peekable>, F: FnMut(&T) -> bool, { let mut items = vec![]; - while iter.peek().map_or(false, |item| condition(item)) { + while iter.peek().map_or(false, &mut condition) { items.push(iter.next().unwrap()); } items @@ -234,7 +142,7 @@ fn collect_while(iter: &mut std::iter::Peekable>, let mut changed_iter = changed.into_iter().peekable(); let mut lints = vec![]; - for name in lint_names.into_iter() { + for name in lint_names { lints.push(LintWarnings { added: collect_while(&mut added_iter, |warning| warning.lint == name), removed: collect_while(&mut removed_iter, |warning| warning.lint == name), @@ -246,13 +154,26 @@ fn collect_while(iter: &mut std::iter::Peekable>, lints } -// This function limits the number of lints in the collection if needed. -// -// It's honestly amazing that a function like this is needed. But some restriction -// lints (Looking at you `implicit_return` and `if_then_some_else_none`) trigger a lot -// like 60'000+ times in our CI. -// -// Each lint in the list will be limited to 200 messages +fn print_lint_warnings(lint: &LintWarnings, truncate_after: usize) { + let name = &lint.name; + let html_id = to_html_id(name); + + // The additional anchor is added for non GH viewers that don't prefix ID's + println!(r#"## `{name}` "#); + println!(); + + print!( + r##"{}, {}, {}"##, + count_string(name, "added", lint.added.len()), + count_string(name, "removed", lint.removed.len()), + count_string(name, "changed", lint.changed.len()), + ); + println!(); + + print_warnings("Added", &lint.added, truncate_after / 3); + print_warnings("Removed", &lint.removed, truncate_after / 3); + print_changed_diff(&lint.changed, truncate_after / 3); +} fn print_summary_table(lints: &[LintWarnings]) { println!("| Lint | Added | Removed | Changed |"); @@ -269,8 +190,83 @@ fn print_summary_table(lints: &[LintWarnings]) { } } +fn print_warnings(title: &str, warnings: &[LintJson], truncate_after: usize) { + if warnings.is_empty() { + return; + } + + print_h3(&warnings[0].lint, title); + println!(); + + let warnings = truncate(warnings, truncate_after); + + for warning in warnings { + println!("{}", warning.info_text(title)); + println!(); + println!("```"); + println!("{}", warning.rendered.trim_end()); + println!("```"); + println!(); + } +} + +fn print_changed_diff(changed: &[(LintJson, LintJson)], truncate_after: usize) { + if changed.is_empty() { + return; + } + + print_h3(&changed[0].0.lint, "Changed"); + println!(); + + let changed = truncate(changed, truncate_after); + + for (old, new) in changed { + println!("{}", new.info_text("Changed")); + println!(); + println!("```diff"); + for change in diff::lines(old.rendered.trim_end(), new.rendered.trim_end()) { + use diff::Result::{Both, Left, Right}; + + match change { + Both(unchanged, _) => { + println!(" {unchanged}"); + }, + Left(removed) => { + println!("-{removed}"); + }, + Right(added) => { + println!("+{added}"); + }, + } + } + println!("```"); + } +} + +fn truncate(list: &[T], truncate_after: usize) -> &[T] { + if list.len() > truncate_after { + println!( + "{} warnings have been truncated for this summary.", + list.len() - truncate_after + ); + println!(); + + list.split_at(truncate_after).0 + } else { + list + } +} + +fn print_h3(lint: &str, title: &str) { + let html_id = to_html_id(lint); + // We have to use HTML here to be able to manually add an id. + println!(r#"### {title} "#); +} + +/// GitHub's markdown parsers doesn't like IDs with `::` and `_`. This simplifies +/// the lint name for the HTML ID. fn to_html_id(lint_name: &str) -> String { - lint_name.replace("clippy::", "").replace("_", "-") + lint_name.replace("clippy::", "").replace('_', "-") } /// This generates the `x added` string for the start of the job summery. @@ -281,10 +277,10 @@ fn count_string(lint: &str, label: &str, count: usize) -> String { if count == 0 { format!("0 {label}") } else { - let lint_id = to_html_id(lint); + let html_id = to_html_id(lint); // GitHub's job summaries don't add HTML ids to headings. That's why we // manually have to add them. GitHub prefixes these manual ids with // `user-content-` and that's how we end up with these awesome links :D - format!("[{count} {label}](#user-content-{lint_id}-{label})") + format!("[{count} {label}](#user-content-{html_id}-{label})") } } From 10bf729e3f6d03f8d96206ae796ff8590ccf50b0 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Mon, 22 Jul 2024 18:10:03 +0200 Subject: [PATCH 4/6] Lintcheck: Include truncated diff in CI artifacts --- .github/workflows/lintcheck.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/lintcheck.yml b/.github/workflows/lintcheck.yml index bfa40eed576..1778f4421ad 100644 --- a/.github/workflows/lintcheck.yml +++ b/.github/workflows/lintcheck.yml @@ -128,4 +128,7 @@ jobs: uses: actions/upload-artifact@v4 with: name: diff - path: full_diff.md + if-no-files-found: ignore + path: | + full_diff.md + truncated_diff.md From 23b231a5d6f74831c239e954f60e44abe46d052b Mon Sep 17 00:00:00 2001 From: xFrednet Date: Mon, 22 Jul 2024 18:14:48 +0200 Subject: [PATCH 5/6] Lintcheck: Fix Errors, because of course --- .github/workflows/lintcheck.yml | 2 +- lintcheck/src/json.rs | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/.github/workflows/lintcheck.yml b/.github/workflows/lintcheck.yml index 1778f4421ad..6a5139b6dc0 100644 --- a/.github/workflows/lintcheck.yml +++ b/.github/workflows/lintcheck.yml @@ -119,10 +119,10 @@ jobs: # https://docs.github.com/actions/using-workflows/workflow-commands-for-github-actions#adding-a-markdown-summary # That's why we first log to file and then to the summary and logs run: | - ./target/debug/lintcheck diff {base,head}/ci_crates_logs.json >> full_diff.md ./target/debug/lintcheck diff {base,head}/ci_crates_logs.json --truncate >> truncated_diff.md head -c 1024000 truncated_diff.md >> $GITHUB_STEP_SUMMARY cat truncated_diff.md + ./target/debug/lintcheck diff {base,head}/ci_crates_logs.json >> full_diff.md - name: Upload full diff uses: actions/upload-artifact@v4 diff --git a/lintcheck/src/json.rs b/lintcheck/src/json.rs index 2fa527c5f87..bedcad79065 100644 --- a/lintcheck/src/json.rs +++ b/lintcheck/src/json.rs @@ -82,6 +82,10 @@ pub(crate) fn diff(old_path: &Path, new_path: &Path, truncate: bool) { print_summary_table(&lint_warnings); println!(); + if lint_warnings.is_empty() { + return; + } + let truncate_after = if truncate { // Max 15 ensures that we at least have five messages per lint DEFAULT_LIMIT_PER_LINT From bdf3e585a3c04eb2f40ae3dbc8815b683cb55eae Mon Sep 17 00:00:00 2001 From: xFrednet Date: Wed, 24 Jul 2024 22:23:17 +0200 Subject: [PATCH 6/6] Lintcheck: Review comments <3 --- lintcheck/Cargo.toml | 2 +- lintcheck/src/json.rs | 97 +++++++++++++------------------------------ 2 files changed, 31 insertions(+), 68 deletions(-) diff --git a/lintcheck/Cargo.toml b/lintcheck/Cargo.toml index 3c86dfe324f..350418eeeb8 100644 --- a/lintcheck/Cargo.toml +++ b/lintcheck/Cargo.toml @@ -16,7 +16,7 @@ clap = { version = "4.4", features = ["derive", "env"] } crossbeam-channel = "0.5.6" diff = "0.1.13" flate2 = "1.0" -itertools = "0.12" +itertools = "0.13" rayon = "1.5.1" serde = { version = "1.0", features = ["derive"] } serde_json = "1.0.85" diff --git a/lintcheck/src/json.rs b/lintcheck/src/json.rs index bedcad79065..4211bce90b2 100644 --- a/lintcheck/src/json.rs +++ b/lintcheck/src/json.rs @@ -1,8 +1,7 @@ -use std::collections::BTreeSet; use std::fs; use std::path::Path; -use itertools::EitherOrBoth; +use itertools::{EitherOrBoth, Itertools}; use serde::{Deserialize, Serialize}; use crate::ClippyWarning; @@ -23,7 +22,7 @@ struct LintJson { impl LintJson { fn key(&self) -> impl Ord + '_ { - (self.file_name.as_str(), self.byte_pos, self.lint.as_str()) + (self.lint.as_str(), self.file_name.as_str(), self.byte_pos) } fn info_text(&self, action: &str) -> String { @@ -61,24 +60,36 @@ pub(crate) fn diff(old_path: &Path, new_path: &Path, truncate: bool) { let old_warnings = load_warnings(old_path); let new_warnings = load_warnings(new_path); - let mut added = Vec::new(); - let mut removed = Vec::new(); - let mut changed = Vec::new(); + let mut lint_warnings = vec![]; - for change in itertools::merge_join_by(old_warnings, new_warnings, |old, new| old.key().cmp(&new.key())) { - match change { - EitherOrBoth::Both(old, new) => { - if old.rendered != new.rendered { - changed.push((old, new)); - } - }, - EitherOrBoth::Left(old) => removed.push(old), - EitherOrBoth::Right(new) => added.push(new), + for (name, changes) in &itertools::merge_join_by(old_warnings, new_warnings, |old, new| old.key().cmp(&new.key())) + .chunk_by(|change| change.as_ref().into_left().lint.to_string()) + { + let mut added = Vec::new(); + let mut removed = Vec::new(); + let mut changed = Vec::new(); + for change in changes { + match change { + EitherOrBoth::Both(old, new) => { + if old.rendered != new.rendered { + changed.push((old, new)); + } + }, + EitherOrBoth::Left(old) => removed.push(old), + EitherOrBoth::Right(new) => added.push(new), + } + } + + if !added.is_empty() || !removed.is_empty() || !changed.is_empty() { + lint_warnings.push(LintWarnings { + name, + added, + removed, + changed, + }); } } - let lint_warnings = group_by_lint(added, removed, changed); - print_summary_table(&lint_warnings); println!(); @@ -110,60 +121,12 @@ struct LintWarnings { changed: Vec<(LintJson, LintJson)>, } -fn group_by_lint( - mut added: Vec, - mut removed: Vec, - mut changed: Vec<(LintJson, LintJson)>, -) -> Vec { - /// Collects items from an iterator while the condition is met - fn collect_while(iter: &mut std::iter::Peekable>, mut condition: F) -> Vec - where - F: FnMut(&T) -> bool, - { - let mut items = vec![]; - while iter.peek().map_or(false, &mut condition) { - items.push(iter.next().unwrap()); - } - items - } - - // Sort - added.sort_unstable_by(|a, b| a.lint.cmp(&b.lint)); - removed.sort_unstable_by(|a, b| a.lint.cmp(&b.lint)); - changed.sort_unstable_by(|(a, _), (b, _)| a.lint.cmp(&b.lint)); - - // Collect lint names - let lint_names: BTreeSet<_> = added - .iter() - .chain(removed.iter()) - .chain(changed.iter().map(|(a, _)| a)) - .map(|warning| &warning.lint) - .cloned() - .collect(); - - let mut added_iter = added.into_iter().peekable(); - let mut removed_iter = removed.into_iter().peekable(); - let mut changed_iter = changed.into_iter().peekable(); - - let mut lints = vec![]; - for name in lint_names { - lints.push(LintWarnings { - added: collect_while(&mut added_iter, |warning| warning.lint == name), - removed: collect_while(&mut removed_iter, |warning| warning.lint == name), - changed: collect_while(&mut changed_iter, |(warning, _)| warning.lint == name), - name, - }); - } - - lints -} - fn print_lint_warnings(lint: &LintWarnings, truncate_after: usize) { let name = &lint.name; let html_id = to_html_id(name); // The additional anchor is added for non GH viewers that don't prefix ID's - println!(r#"## `{name}` "#); + println!(r#"## `{name}` "#); println!(); print!( @@ -264,7 +227,7 @@ fn truncate(list: &[T], truncate_after: usize) -> &[T] { fn print_h3(lint: &str, title: &str) { let html_id = to_html_id(lint); // We have to use HTML here to be able to manually add an id. - println!(r#"### {title} "#); + println!(r#"### {title} "#); } /// GitHub's markdown parsers doesn't like IDs with `::` and `_`. This simplifies