feat: use diff instead of clang-format XML output

This avoids discrepancies with clang-format's XML output.; see cpp-linter/cpp-linter#168. Instead, we now save the clang-format fixed output to a project-specific cache folder. Then we take the diff between the formatted output and the original file's content. The resulting diff is how we determine the lines changed by clang-format.

This actually lays the ground work for other improvements, specifically how to assemble changes for an auto-fix commit.

Additionally, this also removes the dependency used for XML parsing .

Author: 2bndy5

Cargo.lock

@@ -24,7 +24,6 @@ fast-glob = "1.0.1"
 futures = "0.3.32"
 gix-imara-diff = { version = "0.2.2", features = ["unified_diff"] }
 log = { workspace = true }
-quick-xml = { version = "0.40.1", features = ["serialize"] }
 regex = { workspace = true }
 reqwest = { workspace = true }
 semver =  { workspace = true }

cpp-linter/Cargo.toml

@@ -3,28 +3,25 @@
 
 use std::{
     fs,
+    ops::Range,
+    path::PathBuf,
     process::Command,
     sync::{Arc, Mutex, MutexGuard},
 };
 
+use gix_imara_diff::{Diff, InternedInput};
 use log::Level;
-use serde::Deserialize;
 
 // project-specific crates/modules
 use super::MakeSuggestions;
-use crate::{
-    cli::ClangParams,
-    common_fs::{FileObj, get_line_count_from_offset},
-    error::ClangCaptureError,
-};
+use crate::{cli::ClangParams, common_fs::FileObj, error::ClangCaptureError};
 
-#[derive(Debug, Clone, Deserialize, PartialEq, Eq, Default)]
+#[derive(Debug, Clone, PartialEq, Eq, Default)]
 pub struct FormatAdvice {
     /// A list of [`Replacement`]s that clang-tidy wants to make.
-    #[serde(rename(deserialize = "replacement"), default)]
-    pub replacements: Vec<Replacement>,
+    pub replacements: Vec<Range<u32>>,
 
-    pub patched: Option<Vec<u8>>,
+    pub patched: PathBuf,
 }
 
 impl MakeSuggestions for FormatAdvice {
@@ -37,21 +34,6 @@ impl MakeSuggestions for FormatAdvice {
     }
 }
 
-/// A single replacement that clang-format wants to make.
-#[derive(Debug, PartialEq, Eq, Default, Clone, Copy, Deserialize)]
-pub struct Replacement {
-    /// The byte offset where the replacement will start.
-    #[serde(rename = "@offset")]
-    pub offset: u32,
-
-    /// The line number described by the [`Replacement::offset`].
-    ///
-    /// This value is not provided by the XML output, but we calculate it after
-    /// deserialization.
-    #[serde(default)]
-    pub line: u32,
-}
-
 /// Get a string that summarizes the given `--style`
 pub fn summarize_style(style: &str) -> String {
     let mut char_iter = style.chars();
@@ -97,49 +79,40 @@ pub fn run_clang_format(
     for range in &ranges {
         cmd.arg(format!("--lines={}:{}", range.start(), range.end()));
     }
+    let cache_path = clang_params.project_cache_dir.join("patches");
+    let cache_format_fixes = cache_path.join(file.name.with_added_extension("format"));
+    fs::create_dir_all(
+        cache_format_fixes
+            .parent()
+            .ok_or(ClangCaptureError::UnknownCacheParentPath)?,
+    )
+    .map_err(ClangCaptureError::MkDirFailed)?;
     let file_name = file.name.to_string_lossy().to_string();
     cmd.arg(file.name.to_path_buf().as_os_str());
-    let patched = if !clang_params.format_review {
-        None
-    } else {
-        logs.push((
-            Level::Info,
-            format!(
-                "Getting format fixes with \"{} {}\"",
-                cmd.get_program().to_string_lossy(),
-                cmd.get_args()
-                    .map(|a| a.to_string_lossy())
-                    .collect::<Vec<_>>()
-                    .join(" ")
-            ),
-        ));
-        Some(
-            cmd.output()
-                .map_err(|e| ClangCaptureError::FailedToRunCommand {
-                    task: format!("get fixes from clang-format {file_name}"),
-                    source: e,
-                })?
-                .stdout,
-        )
-    };
-    cmd.arg("--output-replacements-xml");
     logs.push((
-        log::Level::Info,
+        Level::Info,
         format!(
-            "Running \"{} {}\"",
+            "Getting format fixes with \"{} {}\"",
             cmd.get_program().to_string_lossy(),
             cmd.get_args()
-                .map(|x| x.to_string_lossy())
+                .map(|a| a.to_string_lossy())
                 .collect::<Vec<_>>()
                 .join(" ")
         ),
     ));
     let output = cmd
         .output()
         .map_err(|e| ClangCaptureError::FailedToRunCommand {
-            task: format!("Failed to get replacements from clang-format: {file_name}"),
+            task: format!("get fixes from clang-format {file_name}"),
             source: e,
         })?;
+    fs::write(&cache_format_fixes, &output.stdout).map_err(|e| {
+        ClangCaptureError::WriteFileFailed {
+            file_name: cache_format_fixes.to_string_lossy().to_string(),
+            source: e,
+        }
+    })?;
+
     if !output.stderr.is_empty() || !output.status.success() {
         logs.push((
             log::Level::Debug,
@@ -149,46 +122,43 @@ pub fn run_clang_format(
             ),
         ));
     }
-    let mut format_advice = if !output.stdout.is_empty() {
-        let xml =
-            String::from_utf8(output.stdout).map_err(|e| ClangCaptureError::NonUtf8Output {
-                task: format!("XML output from clang-format (for {file_name})"),
-                source: e,
-            })?;
-        quick_xml::de::from_str::<FormatAdvice>(&xml).map_err(|e| {
-            ClangCaptureError::XmlParsingFailed {
-                file_name: file_name.clone(),
-                source: e,
-            }
-        })?
-    } else {
-        FormatAdvice::default()
-    };
-    format_advice.patched = patched;
-    if !format_advice.replacements.is_empty() {
-        let original_contents =
-            fs::read(&file.name).map_err(|e| ClangCaptureError::ReadFileFailed {
-                file_name: file_name.clone(),
-                source: e,
-            })?;
-        // get line and column numbers from format_advice.offset
-        let mut filtered_replacements = Vec::new();
-        for replacement in &mut format_advice.replacements {
-            let line_number = get_line_count_from_offset(&original_contents, replacement.offset);
-            replacement.line = line_number;
-            for range in &ranges {
-                if range.contains(&line_number) {
-                    filtered_replacements.push(*replacement);
-                    break;
-                }
-            }
-            if ranges.is_empty() {
-                // lines_changed_only is disabled
-                filtered_replacements.push(*replacement);
-            }
+
+    // use a diff between patched and original contents to get format results
+    let original_contents =
+        fs::read_to_string(&file.name).map_err(|e| ClangCaptureError::ReadFileFailed {
+            file_name: file_name.clone(),
+            source: e,
+        })?;
+    let patched_contents = String::from_utf8(output.stdout.to_vec()).map_err(|e| {
+        ClangCaptureError::NonUtf8Output {
+            task: "clang-format".to_string(),
+            source: e,
         }
-        format_advice.replacements = filtered_replacements;
-    }
+    })?;
+    let input = InternedInput::new(original_contents.as_str(), patched_contents.as_str());
+    let mut diff = Diff::compute(gix_imara_diff::Algorithm::Histogram, &input);
+    diff.postprocess_lines(&input);
+    let format_advice = FormatAdvice {
+        replacements: diff
+            .hunks()
+            .filter_map(|hunk| {
+                if ranges.is_empty() {
+                    Some(hunk.before)
+                } else {
+                    // only include replacements that fall within the specified line ranges
+                    if ranges.iter().any(|range| {
+                        range.contains(&hunk.before.start)
+                            && range.contains(&hunk.before.end.saturating_sub(1))
+                    }) {
+                        Some(hunk.before)
+                    } else {
+                        None
+                    }
+                }
+            })
+            .collect(),
+        patched: cache_format_fixes,
+    };
     file.format_advice = Some(format_advice);
     Ok(logs)
 }
@@ -197,56 +167,7 @@ pub fn run_clang_format(
 mod tests {
     #![allow(clippy::unwrap_used)]
 
-    use super::{FormatAdvice, Replacement, summarize_style};
-
-    #[test]
-    fn parse_blank_xml() {
-        let xml = String::new();
-        let result = quick_xml::de::from_str::<FormatAdvice>(&xml);
-        assert!(result.is_err());
-    }
-
-    #[test]
-    fn parse_xml_no_replacements() {
-        let xml_raw = r#"<?xml version='1.0'?>
-<replacements xml:space='preserve' incomplete_format='false'>
-</replacements>"#
-            .as_bytes()
-            .to_vec();
-        let expected = FormatAdvice::default();
-        let xml = String::from_utf8(xml_raw).unwrap();
-        let document = quick_xml::de::from_str::<FormatAdvice>(&xml).unwrap();
-        assert_eq!(expected, document);
-    }
-
-    #[test]
-    fn parse_xml() {
-        let xml_raw = r#"<?xml version='1.0'?>
-<replacements xml:space='preserve' incomplete_format='false'>
-<replacement offset='113' length='5'>&#10;      </replacement>
-<replacement offset='147' length='0'> </replacement>
-<replacement offset='161' length='0'></replacement>
-<replacement offset='165' length='19'>&#10;&#10;</replacement>
-</replacements>"#
-            .as_bytes()
-            .to_vec();
-
-        let expected = FormatAdvice {
-            replacements: [113, 147, 161, 165]
-                .iter()
-                .map(|offset| Replacement {
-                    offset: *offset,
-                    ..Default::default()
-                })
-                .collect(),
-            patched: None,
-        };
-
-        let xml = String::from_utf8(xml_raw).unwrap();
-
-        let document = quick_xml::de::from_str::<FormatAdvice>(&xml).unwrap();
-        assert_eq!(expected, document);
-    }
+    use super::summarize_style;
 
     fn formalize_style(style: &str, expected: &str) {
         assert_eq!(summarize_style(style), expected);

cpp-linter/src/clang_tools/clang_format.rs

@@ -496,6 +496,7 @@ mod test {
             format_review: false,
             clang_tidy_command: Some(exe_path),
             clang_format_command: None,
+            project_cache_dir: PathBuf::from(".").join(".cpp-linter-cache"),
         };
         let mut file_lock = arc_file.lock().unwrap();
         let logs = run_clang_tidy(&mut file_lock, &clang_params)

cpp-linter/src/clang_tools/clang_tidy.rs

@@ -178,6 +178,7 @@ pub struct ClangParams {
     pub format_filter: Option<FileFilter>,
     pub tidy_review: bool,
     pub format_review: bool,
+    pub project_cache_dir: PathBuf,
 }
 
 #[cfg(feature = "bin")]
@@ -215,6 +216,8 @@ impl From<&Cli> for ClangParams {
             format_filter,
             tidy_review: args.feedback_options.tidy_review,
             format_review: args.feedback_options.format_review,
+            project_cache_dir: PathBuf::from(&args.source_options.repo_root)
+                .join(".cpp-linter-cache"),
         }
     }
 }

cpp-linter/src/cli/structs.rs

@@ -143,11 +143,8 @@ impl FileObj {
     ) -> Result<(), FileObjError> {
         let original_content = fs::read_to_string(&self.name).map_err(FileObjError::ReadFile)?;
         let file_name = self.name.to_str().unwrap_or_default().replace("\\", "/");
-        if let Some(advice) = &self.format_advice
-            && let Some(patched) = &advice.patched
-        {
-            let patched = String::from_utf8(patched.to_vec())
-                .map_err(|e| FileObjError::FromUtf8Error(file_name.clone(), e))?;
+        if let Some(advice) = &self.format_advice {
+            let patched = fs::read_to_string(&advice.patched).map_err(FileObjError::ReadFile)?;
             let (diff, input) = make_patch(patched.as_str(), &original_content);
             advice.get_suggestions(review_comments, self, &diff, &input, summary_only);
         }
@@ -219,48 +216,13 @@ impl FileObj {
     }
 }
 
-/// Gets the line number for a given `offset` (of bytes) from the given
-/// buffer `contents`.
-///
-/// The `offset` given to this function is expected to originate from
-/// diagnostic information provided by clang-format. Any `offset` out of
-/// bounds is clamped to the given `contents` buffer's length.
-pub fn get_line_count_from_offset(contents: &[u8], offset: u32) -> u32 {
-    let offset = (offset as usize).min(contents.len());
-    let lines = contents[0..offset].split(|byte| byte == &b'\n');
-    lines.count() as u32
-}
-
 #[cfg(test)]
 mod test {
-    use std::{fs, path::PathBuf};
+    use std::path::PathBuf;
 
-    use super::{FileObj, get_line_count_from_offset};
+    use super::FileObj;
     use crate::cli::LinesChangedOnly;
 
-    // *********************** tests for translating byte offset into line/column
-
-    #[test]
-    fn translate_byte_offset() {
-        let contents = fs::read(PathBuf::from("tests/demo/demo.cpp")).unwrap();
-        let lines = get_line_count_from_offset(&contents, 144);
-        assert_eq!(lines, 13);
-    }
-
-    #[test]
-    fn get_line_count_edge_cases() {
-        // Empty content
-        assert_eq!(get_line_count_from_offset(&[], 0), 1);
-
-        // No newlines
-        assert_eq!(get_line_count_from_offset(b"abc", 3), 1);
-
-        // Consecutive newlines
-        assert_eq!(get_line_count_from_offset(b"a\n\nb", 3), 3);
-
-        // Offset beyond content length
-        assert_eq!(get_line_count_from_offset(b"a\nb\n", 10), 3);
-    }
     // *********************** tests for FileObj::get_ranges()
 
     #[test]