Auto merge of #13104 - xFrednet:00000-lintcheck-better-md-links, r=Alexendoo

Lintcheck: Add URL to lint emission place in diff

This PR adds links to the emission code in our lintcheck CI. When reviewing changes, I would like to be able to see the bigger context, which isn't always included in the lint message. This PR adds a nice link to the lintcheck diff that allows for simple investigation of the code in question.

At first, I wanted to simply use the doc.rs links and call it a day, but then I figured out that some crates might have their source files remapped. Cargo was the crate that made me notice this. As a response, I made the link configurable. (See https://github.com/rust-lang/docs.rs/issues/2551 for a detailed explanation and possible solution to remove this workaround in the future.)

It's probably easiest to review the individual commits. The last one is just a dummy to showcase the change.

[🖼️ rendered 🖼️](https://github.com/rust-lang/rust-clippy/actions/runs/9960834924?pr=13104)

---

r? `@Alexendoo`

changelog: none

---

That's it, I hope that everyone who's reading this has a beautiful day :D

lintcheck/lintcheck_crates.toml

@@ -1,6 +1,6 @@
 [crates]
 # some of these are from cargotest
-cargo = {name = "cargo", version = '0.64.0'}
+cargo = {name = "cargo", version = '0.64.0', online_link = 'https://docs.rs/cargo/{version}/src/{file}.html#{line}'}
 iron = {name = "iron", version = '0.6.1'}
 ripgrep = {name = "ripgrep", version = '12.1.1'}
 xsv = {name = "xsv", version = '0.13.0'}

lintcheck/src/driver.rs

@@ -11,6 +11,7 @@
 fn run_clippy(addr: &str) -> Option<i32> {
     let driver_info = DriverInfo {
         package_name: env::var("CARGO_PKG_NAME").ok()?,
+        version: env::var("CARGO_PKG_VERSION").ok()?,
     };
 
     let mut stream = BufReader::new(TcpStream::connect(addr).unwrap());

lintcheck/src/input.rs

@@ -10,6 +10,10 @@
 
 use crate::{Crate, LINTCHECK_DOWNLOADS, LINTCHECK_SOURCES};
 
+const DEFAULT_DOCS_LINK: &str = "https://docs.rs/{krate}/{version}/src/{krate}/{file}.html#{line}";
+const DEFAULT_GITHUB_LINK: &str = "{url}/blob/{hash}/src/{file}#L{line}";
+const DEFAULT_PATH_LINK: &str = "{path}/src/{file}:{line}";
+
 /// List of sources to check, loaded from a .toml file
 #[derive(Debug, Deserialize)]
 pub struct SourceList {
@@ -33,32 +37,60 @@ struct TomlCrate {
     git_hash: Option<String>,
     path: Option<String>,
     options: Option<Vec<String>>,
+    /// Magic values:
+    /// * `{krate}` will be replaced by `self.name`
+    /// * `{version}` will be replaced by `self.version`
+    /// * `{url}` will be replaced with `self.git_url`
+    /// * `{hash}` will be replaced with `self.git_hash`
+    /// * `{path}` will be replaced with `self.path`
+    /// * `{file}` will be replaced by the path after `src/`
+    /// * `{line}` will be replaced by the line
+    ///
+    /// If unset, this will be filled by [`read_crates`] since it depends on
+    /// the source.
+    online_link: Option<String>,
+}
+
+impl TomlCrate {
+    fn file_link(&self, default: &str) -> String {
+        let mut link = self.online_link.clone().unwrap_or_else(|| default.to_string());
+        link = link.replace("{krate}", &self.name);
+
+        if let Some(version) = &self.version {
+            link = link.replace("{version}", version);
+        }
+        if let Some(url) = &self.git_url {
+            link = link.replace("{url}", url);
+        }
+        if let Some(hash) = &self.git_hash {
+            link = link.replace("{hash}", hash);
+        }
+        if let Some(path) = &self.path {
+            link = link.replace("{path}", path);
+        }
+        link
+    }
 }
 
 /// Represents an archive we download from crates.io, or a git repo, or a local repo/folder
 /// Once processed (downloaded/extracted/cloned/copied...), this will be translated into a `Crate`
+#[derive(Debug, Deserialize, Eq, Hash, PartialEq, Ord, PartialOrd)]
+pub struct CrateWithSource {
+    pub name: String,
+    pub source: CrateSource,
+    pub file_link: String,
+    pub options: Option<Vec<String>>,
+}
+
 #[derive(Debug, Deserialize, Eq, Hash, PartialEq, Ord, PartialOrd)]
 pub enum CrateSource {
-    CratesIo {
+    CratesIo { version: String },
-        name: String,
+    Git { url: String, commit: String },
-        version: String,
+    Path { path: PathBuf },
-        options: Option<Vec<String>>,
-    },
-    Git {
-        name: String,
-        url: String,
-        commit: String,
-        options: Option<Vec<String>>,
-    },
-    Path {
-        name: String,
-        path: PathBuf,
-        options: Option<Vec<String>>,
-    },
 }
 
 /// Read a `lintcheck_crates.toml` file
-pub fn read_crates(toml_path: &Path) -> (Vec<CrateSource>, RecursiveOptions) {
+pub fn read_crates(toml_path: &Path) -> (Vec<CrateWithSource>, RecursiveOptions) {
     let toml_content: String =
         fs::read_to_string(toml_path).unwrap_or_else(|_| panic!("Failed to read {}", toml_path.display()));
     let crate_list: SourceList =
@@ -71,23 +103,32 @@ pub fn read_crates(toml_path: &Path) -> (Vec<CrateSource>, RecursiveOptions) {
     let mut crate_sources = Vec::new();
     for tk in tomlcrates {
         if let Some(ref path) = tk.path {
-            crate_sources.push(CrateSource::Path {
+            crate_sources.push(CrateWithSource {
                 name: tk.name.clone(),
-                path: PathBuf::from(path),
+                source: CrateSource::Path {
+                    path: PathBuf::from(path),
+                },
+                file_link: tk.file_link(DEFAULT_PATH_LINK),
                 options: tk.options.clone(),
             });
         } else if let Some(ref version) = tk.version {
-            crate_sources.push(CrateSource::CratesIo {
+            crate_sources.push(CrateWithSource {
                 name: tk.name.clone(),
-                version: version.to_string(),
+                source: CrateSource::CratesIo {
+                    version: version.to_string(),
+                },
+                file_link: tk.file_link(DEFAULT_DOCS_LINK),
                 options: tk.options.clone(),
             });
         } else if tk.git_url.is_some() && tk.git_hash.is_some() {
             // otherwise, we should have a git source
-            crate_sources.push(CrateSource::Git {
+            crate_sources.push(CrateWithSource {
                 name: tk.name.clone(),
-                url: tk.git_url.clone().unwrap(),
+                source: CrateSource::Git {
-                commit: tk.git_hash.clone().unwrap(),
+                    url: tk.git_url.clone().unwrap(),
+                    commit: tk.git_hash.clone().unwrap(),
+                },
+                file_link: tk.file_link(DEFAULT_GITHUB_LINK),
                 options: tk.options.clone(),
             });
         } else {
@@ -117,7 +158,7 @@ pub fn read_crates(toml_path: &Path) -> (Vec<CrateSource>, RecursiveOptions) {
     (crate_sources, crate_list.recursive)
 }
 
-impl CrateSource {
+impl CrateWithSource {
     /// Makes the sources available on the disk for clippy to check.
     /// Clones a git repo and checks out the specified commit or downloads a crate from crates.io or
     /// copies a local folder
@@ -139,8 +180,11 @@ fn get(path: &str) -> Result<ureq::Response, ureq::Error> {
                 retries += 1;
             }
         }
-        match self {
+        let name = &self.name;
-            CrateSource::CratesIo { name, version, options } => {
+        let options = &self.options;
+        let file_link = &self.file_link;
+        match &self.source {
+            CrateSource::CratesIo { version } => {
                 let extract_dir = PathBuf::from(LINTCHECK_SOURCES);
                 let krate_download_dir = PathBuf::from(LINTCHECK_DOWNLOADS);
 
@@ -171,14 +215,10 @@ fn get(path: &str) -> Result<ureq::Response, ureq::Error> {
                     name: name.clone(),
                     path: extract_dir.join(format!("{name}-{version}/")),
                     options: options.clone(),
+                    base_url: file_link.clone(),
                 }
             },
-            CrateSource::Git {
+            CrateSource::Git { url, commit } => {
-                name,
-                url,
-                commit,
-                options,
-            } => {
                 let repo_path = {
                     let mut repo_path = PathBuf::from(LINTCHECK_SOURCES);
                     // add a -git suffix in case we have the same crate from crates.io and a git repo
@@ -217,9 +257,10 @@ fn get(path: &str) -> Result<ureq::Response, ureq::Error> {
                     name: name.clone(),
                     path: repo_path,
                     options: options.clone(),
+                    base_url: file_link.clone(),
                 }
             },
-            CrateSource::Path { name, path, options } => {
+            CrateSource::Path { path } => {
                 fn is_cache_dir(entry: &DirEntry) -> bool {
                     fs::read(entry.path().join("CACHEDIR.TAG"))
                         .map(|x| x.starts_with(b"Signature: 8a477f597d28d172789f06886806bc55"))
@@ -256,6 +297,7 @@ fn is_cache_dir(entry: &DirEntry) -> bool {
                     name: name.clone(),
                     path: dest_crate_root,
                     options: options.clone(),
+                    base_url: file_link.clone(),
                 }
             },
         }

lintcheck/src/json.rs

@@ -11,6 +11,7 @@ struct LintJson {
     lint: String,
     file_name: String,
     byte_pos: (u32, u32),
+    file_link: String,
     rendered: String,
 }
 
@@ -29,6 +30,7 @@ pub(crate) fn output(clippy_warnings: Vec<ClippyWarning>) -> String {
             LintJson {
                 file_name: span.file_name.clone(),
                 byte_pos: (span.byte_start, span.byte_end),
+                file_link: warning.url,
                 lint: warning.lint,
                 rendered: warning.diag.rendered.unwrap(),
             }
@@ -50,11 +52,12 @@ fn print_warnings(title: &str, warnings: &[LintJson]) {
     }
 
     println!("### {title}");
-    println!("```");
     for warning in warnings {
+        println!("{title} `{}` at {}", warning.lint, warning.file_link);
+        println!("```");
         print!("{}", warning.rendered);
+        println!("```");
     }
-    println!("```");
 }
 
 fn print_changed_diff(changed: &[(LintJson, LintJson)]) {
@@ -63,8 +66,9 @@ fn print_changed_diff(changed: &[(LintJson, LintJson)]) {
     }
 
     println!("### Changed");
-    println!("```diff");
     for (old, new) in changed {
+        println!("Changed `{}` at {}", new.lint, new.file_link);
+        println!("```diff");
         for change in diff::lines(&old.rendered, &new.rendered) {
             use diff::Result::{Both, Left, Right};
 
@@ -80,8 +84,8 @@ fn print_changed_diff(changed: &[(LintJson, LintJson)]) {
                 },
             }
         }
+        println!("```");
     }
-    println!("```");
 }
 
 pub(crate) fn diff(old_path: &Path, new_path: &Path) {

lintcheck/src/main.rs

@@ -38,7 +38,7 @@
 use std::{env, fs};
 
 use cargo_metadata::Message;
-use input::{read_crates, CrateSource};
+use input::read_crates;
 use output::{ClippyCheckOutput, ClippyWarning, RustcIce};
 use rayon::prelude::*;
 
@@ -53,6 +53,7 @@ struct Crate {
     // path to the extracted sources that clippy can check
     path: PathBuf,
     options: Option<Vec<String>>,
+    base_url: String,
 }
 
 impl Crate {
@@ -185,7 +186,7 @@ fn run_clippy_lints(
         // get all clippy warnings and ICEs
         let mut entries: Vec<ClippyCheckOutput> = Message::parse_stream(stdout.as_bytes())
             .filter_map(|msg| match msg {
-                Ok(Message::CompilerMessage(message)) => ClippyWarning::new(message.message),
+                Ok(Message::CompilerMessage(message)) => ClippyWarning::new(message.message, &self.base_url),
                 _ => None,
             })
             .map(ClippyCheckOutput::ClippyWarning)
@@ -292,13 +293,7 @@ fn lintcheck(config: LintcheckConfig) {
         .into_iter()
         .filter(|krate| {
             if let Some(only_one_crate) = &config.only {
-                let name = match krate {
+                krate.name == *only_one_crate
-                    CrateSource::CratesIo { name, .. }
-                    | CrateSource::Git { name, .. }
-                    | CrateSource::Path { name, .. } => name,
-                };
-
-                name == only_one_crate
             } else {
                 true
             }

lintcheck/src/output.rs

@@ -53,11 +53,12 @@ pub fn from_stderr_and_status(crate_name: &str, status: ExitStatus, stderr: &str
 pub struct ClippyWarning {
     pub lint: String,
     pub diag: Diagnostic,
+    /// The URL that points to the file and line of the lint emission
+    pub url: String,
 }
 
-#[allow(unused)]
 impl ClippyWarning {
-    pub fn new(mut diag: Diagnostic) -> Option<Self> {
+    pub fn new(mut diag: Diagnostic, base_url: &str) -> Option<Self> {
         let lint = diag.code.clone()?.code;
         if !(lint.contains("clippy") || diag.message.contains("clippy"))
             || diag.message.contains("could not read cargo metadata")
@@ -69,7 +70,20 @@ pub fn new(mut diag: Diagnostic) -> Option<Self> {
         let rendered = diag.rendered.as_mut().unwrap();
         *rendered = strip_ansi_escapes::strip_str(&rendered);
 
-        Some(Self { lint, diag })
+        let span = diag.spans.iter().find(|span| span.is_primary).unwrap();
+        let file = &span.file_name;
+        let url = if let Some(src_split) = file.find("/src/") {
+            // This removes the inital `target/lintcheck/sources/<crate>-<version>/`
+            let src_split = src_split + "/src/".len();
+            let (_, file) = file.split_at(src_split);
+
+            let line_no = span.line_start;
+            base_url.replace("{file}", file).replace("{line}", &line_no.to_string())
+        } else {
+            file.clone()
+        };
+
+        Some(Self { lint, diag, url })
     }
 
     pub fn span(&self) -> &DiagnosticSpan {

lintcheck/src/recursive.rs

@@ -20,6 +20,7 @@
 #[derive(Debug, Eq, Hash, PartialEq, Clone, Serialize, Deserialize)]
 pub(crate) struct DriverInfo {
     pub package_name: String,
+    pub version: String,
 }
 
 pub(crate) fn serialize_line<T, W>(value: &T, writer: &mut W)
@@ -61,10 +62,17 @@ fn process_stream(
     let mut stderr = String::new();
     stream.read_to_string(&mut stderr).unwrap();
 
+    // It's 99% likely that dependencies compiled with recursive mode are on crates.io
+    // and therefore on docs.rs. This links to the sources directly, do avoid invalid
+    // links due to remaped paths. See rust-lang/docs.rs#2551 for more details.
+    let base_url = format!(
+        "https://docs.rs/crate/{}/{}/source/src/{{file}}#{{line}}",
+        driver_info.package_name, driver_info.version
+    );
     let messages = stderr
         .lines()
         .filter_map(|json_msg| serde_json::from_str::<Diagnostic>(json_msg).ok())
-        .filter_map(ClippyWarning::new);
+        .filter_map(|diag| ClippyWarning::new(diag, &base_url));
 
     for message in messages {
         sender.send(message).unwrap();