diff --git a/cpp-linter/src/clang_tools/clang_format.rs b/cpp-linter/src/clang_tools/clang_format.rs index f9c7f620..64051b33 100644 --- a/cpp-linter/src/clang_tools/clang_format.rs +++ b/cpp-linter/src/clang_tools/clang_format.rs @@ -186,20 +186,22 @@ pub fn run_clang_format( } else { // clang-tidy was not run on this file, // so just use the clang-format fixes as the patched content. - let cache_format_fixes = cache_path.join(&file.name); - fs::create_dir_all( - cache_format_fixes - .parent() - .ok_or(ClangCaptureError::UnknownCacheParentPath)?, - ) - .map_err(ClangCaptureError::MkDirFailed)?; - fs::write(&cache_format_fixes, &output.stdout).map_err(|e| { - ClangCaptureError::WriteFileFailed { - file_name: cache_format_fixes.to_string_lossy().to_string(), - source: e, - } - })?; - file.patched_path = Some(cache_format_fixes); + if clang_params.gen_patch { + let cache_format_fixes = cache_path.join(&file.name); + fs::create_dir_all( + cache_format_fixes + .parent() + .ok_or(ClangCaptureError::UnknownCacheParentPath)?, + ) + .map_err(ClangCaptureError::MkDirFailed)?; + fs::write(&cache_format_fixes, &output.stdout).map_err(|e| { + ClangCaptureError::WriteFileFailed { + file_name: cache_format_fixes.to_string_lossy().to_string(), + source: e, + } + })?; + file.patched_path = Some(cache_format_fixes); + } } file.format_advice = Some(format_advice); @@ -447,13 +449,30 @@ mod tests { let tmp_workspace = setup_tmp_workspace(); let tools_dir = tempfile::tempdir().unwrap(); let fake_format = make_fake_tool(tools_dir.path(), "clang-format", ""); - let clang_params = make_clang_params(tmp_workspace.path(), fake_format); + let mut clang_params = make_clang_params(tmp_workspace.path(), fake_format); + clang_params.gen_patch = true; // a directory where the patched file should be cached fs::create_dir_all(clang_params.get_cache_path().join("demo/demo.cpp")).unwrap(); let file = Mutex::new(FileObj::new(PathBuf::from("demo/demo.cpp"))); let err = run_clang_format(&mut file.lock().unwrap(), &clang_params).unwrap_err(); assert!(matches!(err, ClangCaptureError::WriteFileFailed { .. })); } + + #[test] + fn no_gen_patch() { + let tmp_workspace = setup_tmp_workspace(); + let tools_dir = tempfile::tempdir().unwrap(); + let fake_format = + make_fake_tool(tools_dir.path(), "clang-format", "echo 'void main() {}'"); + let mut clang_params = make_clang_params(tmp_workspace.path(), fake_format); + clang_params.gen_patch = false; + let file = Mutex::new(FileObj::new(PathBuf::from("demo/demo.cpp"))); + let mut file = file.lock().unwrap(); + let _ = run_clang_format(&mut file, &clang_params).unwrap(); + assert!(file.patched_path.is_none()); + assert!(file.format_advice.is_some()); + assert!(!file.format_advice.as_ref().unwrap().replacements.is_empty()); + } } fn formalize_style(style: &str, expected: &str) { diff --git a/cpp-linter/src/clang_tools/clang_tidy/mod.rs b/cpp-linter/src/clang_tools/clang_tidy/mod.rs index a245e6e4..cc49ae54 100644 --- a/cpp-linter/src/clang_tools/clang_tidy/mod.rs +++ b/cpp-linter/src/clang_tools/clang_tidy/mod.rs @@ -738,7 +738,9 @@ pub fn run_clang_tidy( let mut paths = FixPaths::new(&target, &working_dir); let (patched, tidy_fixes) = apply_fixes(&fixes, &mut paths, &original_content, &mut notes, &mut logs); - file.patched_path = Some(write_cache(clang_params, &file.name, &patched)?); + if clang_params.gen_patch { + file.patched_path = Some(write_cache(clang_params, &file.name, &patched)?); + } file.tidy_advice = Some(TidyAdvice { notes }); Ok((logs, tidy_fixes)) } diff --git a/cpp-linter/src/clang_tools/clang_tidy/other_files/mod.rs b/cpp-linter/src/clang_tools/clang_tidy/other_files/mod.rs index da194c6a..6cfd920f 100644 --- a/cpp-linter/src/clang_tools/clang_tidy/other_files/mod.rs +++ b/cpp-linter/src/clang_tools/clang_tidy/other_files/mod.rs @@ -292,12 +292,14 @@ pub fn apply_other_file_fixes( }, ) in pending { - let content = analyzed.content(index, clang_params)?; let mut file = files[index] .lock() .map_err(|_| ClangCaptureError::MutexPoisoned)?; - let patched = patch_content(content, &replacements, &file.name, &mut logs[index]); - file.patched_path = Some(write_cache(clang_params, &file.name, &patched)?); + if clang_params.gen_patch { + let content = analyzed.content(index, clang_params)?; + let patched = patch_content(content, &replacements, &file.name, &mut logs[index]); + file.patched_path = Some(write_cache(clang_params, &file.name, &patched)?); + } let name = file.name.to_string_lossy().replace('\\', "/"); let advice = file .tidy_advice diff --git a/cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs b/cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs index d25c86b7..47ad8170 100644 --- a/cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs +++ b/cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs @@ -114,6 +114,7 @@ async fn fix_analyzed_header() { tidy_checks: "-*,readability-non-const-parameter".to_string(), clang_tidy_command: Some(get_tidy_exe()), repo_root: tmp.path().to_path_buf(), + gen_patch: true, ..Default::default() }); let files = as_files(vec![ @@ -306,6 +307,7 @@ fn merge_with_own_fixes() { fs::write(tmp.path().join("other.cpp"), SOURCE).unwrap(); let clang_params = ClangParams { repo_root: tmp.path().to_path_buf(), + gen_patch: true, ..Default::default() }; let files = as_files(vec![ @@ -346,6 +348,27 @@ fn merge_with_own_fixes() { assert_eq!(notes[0].fixed_lines, vec![2]); } +/// An accepted fix records its note, but writes no patch when `gen_patch` is false. +#[test] +fn accepted_fix_without_patch() { + let tmp = workspace(); + let clang_params = ClangParams { + repo_root: tmp.path().to_path_buf(), + gen_patch: false, + ..Default::default() + }; + let files = as_files(vec![ + FileObj::new(PathBuf::from("fix.cpp")), + FileObj::new(PathBuf::from("fix.h")), + ]); + let tidy_fixes = [(0, header_fixes(tmp.path()))]; + let mut logs = vec![vec![], vec![]]; + apply_other_file_fixes(&files, &tidy_fixes, &clang_params, &mut logs).unwrap(); + assert!(patched(&files[1]).is_none()); + assert!(messages(&logs[0])[0].starts_with("Applying the fix")); + assert_eq!(header_notes(&files[1]).len(), 1); +} + /// A fix that removes a whole changed line (including its line break) /// does not reach into the next line. #[test] @@ -355,6 +378,7 @@ fn remove_changed_line() { let clang_params = ClangParams { repo_root: tmp.path().to_path_buf(), lines_changed_only: LinesChangedOnly::On, + gen_patch: true, ..Default::default() }; let files = as_files(vec![ @@ -425,6 +449,7 @@ fn analyzed_file_missing() { let tmp = workspace(); let clang_params = ClangParams { repo_root: tmp.path().to_path_buf(), + gen_patch: true, ..Default::default() }; let files = as_files(vec![ @@ -485,6 +510,7 @@ fn overlapping_fixes() { fs::write(tmp.path().join("other.cpp"), SOURCE).unwrap(); let clang_params = ClangParams { repo_root: tmp.path().to_path_buf(), + gen_patch: true, ..Default::default() }; let files = as_files(vec![ @@ -523,6 +549,7 @@ fn overlaps_own_fix() { let tmp = workspace(); let clang_params = ClangParams { repo_root: tmp.path().to_path_buf(), + gen_patch: true, ..Default::default() }; let files = as_files(vec![ diff --git a/cpp-linter/src/clang_tools/clang_tidy/test.rs b/cpp-linter/src/clang_tools/clang_tidy/test.rs index ebcaf0ac..7c51d1f6 100644 --- a/cpp-linter/src/clang_tools/clang_tidy/test.rs +++ b/cpp-linter/src/clang_tools/clang_tidy/test.rs @@ -144,6 +144,7 @@ fn use_extra_args() { clang_tidy_command: Some(exe_path), clang_format_command: None, repo_root: tmp_workspace.path().to_path_buf(), + gen_patch: false, }; let mut file_lock = arc_file.lock().unwrap(); fs::create_dir_all(clang_params.get_cache_path()).unwrap(); @@ -378,7 +379,8 @@ fn tidy_cache_not_writable() { let tmp_workspace = crate::test_common::setup_tmp_workspace(); let tools_dir = tempfile::tempdir().unwrap(); let fake_tidy = crate::test_common::make_fake_tool(tools_dir.path(), "clang-tidy", ""); - let clang_params = make_clang_params(tmp_workspace.path(), Some(fake_tidy)); + let mut clang_params = make_clang_params(tmp_workspace.path(), Some(fake_tidy)); + clang_params.gen_patch = true; // a directory where the patched file should be cached fs::create_dir_all(clang_params.get_cache_path().join("demo/demo.cpp")).unwrap(); let file = Mutex::new(FileObj::new(PathBuf::from("demo/demo.cpp"))); @@ -429,6 +431,7 @@ fn fixes_do_not_modify_worktree() { tidy_checks: "-*,readability-non-const-parameter".to_string(), clang_tidy_command: Some(get_tidy_exe()), repo_root: tmp_workspace.path().to_path_buf(), + gen_patch: true, ..Default::default() }; let file = Mutex::new(FileObj::new(PathBuf::from("fix.cpp"))); @@ -473,6 +476,36 @@ fn fixes_do_not_modify_worktree() { assert!(header_fix.path.ends_with("fix.h")); } +#[test] +fn fixes_not_cached_when_gen_patch_false() { + let tmp_workspace = tempfile::TempDir::with_prefix("cpp-linter-unit-tests_").unwrap(); + fs::write(tmp_workspace.path().join("fix.h"), FIX_HEADER).unwrap(); + fs::write(tmp_workspace.path().join("fix.cpp"), FIX_SOURCE).unwrap(); + let clang_params = ClangParams { + tidy_checks: "-*,readability-non-const-parameter".to_string(), + clang_tidy_command: Some(get_tidy_exe()), + repo_root: tmp_workspace.path().to_path_buf(), + gen_patch: false, + ..Default::default() + }; + let file = Mutex::new(FileObj::new(PathBuf::from("fix.cpp"))); + let mut file = file.lock().unwrap(); + let (_logs, fixes) = run_clang_tidy(&mut file, &clang_params).unwrap(); + + // patched_path is None because gen_patch is false + assert!(file.patched_path.is_none()); + + // but notes and advice are still populated + let notes = &file.tidy_advice.as_ref().unwrap().notes; + let note = notes + .iter() + .find(|n| n.diagnostic == "readability-non-const-parameter") + .unwrap(); + assert_eq!(note.fixed_lines, vec![2]); + assert_eq!(fixes.own.len(), 1); + assert_eq!(fixes.others.len(), 1); +} + fn dummy_fixes(target: &Path) -> String { let target = target.to_string_lossy(); format!( diff --git a/cpp-linter/src/cli/mod.rs b/cpp-linter/src/cli/mod.rs index b00c87a2..ef7e52e6 100644 --- a/cpp-linter/src/cli/mod.rs +++ b/cpp-linter/src/cli/mod.rs @@ -576,6 +576,23 @@ pub struct FeedbackOptions { help_heading = "Feedback options", ))] pub passive_reviews: bool, + + /// Set to `true` to generate a patch file of applied clang fixes. + /// + /// The path to the generated patch file is printed in the logs and + /// set as the output variable `fix-patch-path` in CI workflows. + #[cfg_attr(feature = "bin", arg( + short = 'F', + long, + aliases = ["fix"], + default_value_t = false, + default_missing_value = "true", + num_args = 0..=1, + action = ArgAction::Set, + value_parser = FalseyValueParser::new(), + help_heading = "Feedback options", + ))] + pub gen_patch: bool, } /// Parses a single value of the `--extensions` option. diff --git a/cpp-linter/src/cli/structs.rs b/cpp-linter/src/cli/structs.rs index 732261bb..9c2a91fb 100644 --- a/cpp-linter/src/cli/structs.rs +++ b/cpp-linter/src/cli/structs.rs @@ -222,6 +222,12 @@ pub struct ClangParams { /// /// A project-specific cache folder is created in this path. pub repo_root: PathBuf, + + /// Whether to generate a patch file of applied clang fixes. + /// + /// This should be asserted if posting PR reviews or generating a patch file + /// without posing a PR review. + pub gen_patch: bool, } impl ClangParams { @@ -283,6 +289,7 @@ impl From<&Cli> for ClangParams { tidy_filter, format_filter, repo_root, + gen_patch: args.feedback_options.pr_review || args.feedback_options.gen_patch, } } } @@ -321,6 +328,11 @@ pub struct FeedbackInput { /// The root of the repository, used to locate relative file paths in processing. pub repo_root: PathBuf, + + /// Whether to generate a patch file of applied clang fixes. + /// + /// Only used when `pr_review` is false. + pub gen_patch: bool, } #[cfg(feature = "bin")] @@ -338,6 +350,7 @@ impl From<&Cli> for FeedbackInput { repo_root: args.source_options.repo_root.clone(), summary_output_file: args.feedback_options.summary_output_file.clone(), sarif_file: args.feedback_options.sarif_file.clone(), + gen_patch: args.feedback_options.gen_patch, } } } @@ -356,6 +369,7 @@ impl Default for FeedbackInput { repo_root: PathBuf::from("."), summary_output_file: None, sarif_file: None, + gen_patch: false, } } } diff --git a/cpp-linter/src/rest_client.rs b/cpp-linter/src/rest_client.rs index c3399ba0..fff5d545 100644 --- a/cpp-linter/src/rest_client.rs +++ b/cpp-linter/src/rest_client.rs @@ -147,6 +147,7 @@ impl RestClient { tidy_checks_failed, &clang_versions, None, + feedback_inputs.gen_patch || feedback_inputs.pr_review, )?; if feedback_inputs.step_summary { self.client.append_step_summary(&summary)?; @@ -261,7 +262,7 @@ impl RestClient { } else { self.client.post_pr_review(&options).await?; } - } else { + } else if feedback_inputs.gen_patch { for file in files { let file = file .lock() @@ -280,6 +281,7 @@ impl RestClient { tidy_checks_failed, &clang_versions, Some(u16::MAX as u64), + feedback_inputs.gen_patch || feedback_inputs.pr_review, )?); } let options = ThreadCommentOptions { @@ -400,6 +402,7 @@ impl RestClient { tidy_checks_failed: u64, clang_versions: &ClangVersions, max_len: Option, + patch_exists: bool, ) -> Result { let mut comment = format!("{COMMENT_MARKER}# Cpp-Linter Report "); let mut remaining_length = @@ -429,6 +432,7 @@ impl RestClient { tidy_checks_failed, &tidy_version.to_string(), &mut remaining_length, + patch_exists, )?; } } else { @@ -480,6 +484,10 @@ impl RestClient { } line_buf.clear(); } + log::info!( + "Patch file written to {}", + auto_fix_patch_path.to_string_lossy().replace("\\", "/") + ); } Ok(()) } @@ -537,6 +545,7 @@ fn make_tidy_comment( tidy_checks_failed: u64, version_used: &String, remaining_length: &mut u64, + patch_exists: bool, ) -> Result<(), ClientError> { let opener = format!( "\n
clang-tidy (v{version_used}) reports: {tidy_checks_failed} concern(s)\n\n" @@ -552,7 +561,7 @@ fn make_tidy_comment( for tidy_note in &tidy_advice.notes { let file_path = PathBuf::from(&tidy_note.filename); if file_path == file.name { - let uses_auto_fix = !tidy_note.fixed_lines.is_empty(); + let uses_auto_fix = !tidy_note.fixed_lines.is_empty() && patch_exists; let mut tmp_note = format!("- {}\n\n", tidy_note.filename); tmp_note.push_str(&format!( " {filename}:{line}:{cols}: {severity}: [{diagnostic}]{auto_fixable}{skipped_fixes}\n > {rationale}\n{concerned_code}", @@ -791,6 +800,7 @@ mod test { 1, &"1.2.3".to_string(), &mut remaining_length, + true, ) .unwrap(); assert!(comment.contains( @@ -872,7 +882,8 @@ mod test { #[test] fn comment_without_tidy_concerns() { let files = make_dummy_files("warning"); - let comment = RestClient::make_comment(&files, 1, 0, &dummy_versions(), None).unwrap(); + let comment = + RestClient::make_comment(&files, 1, 0, &dummy_versions(), None, false).unwrap(); assert!(comment.contains("clang-format (v1.2.3) reports")); assert!(!comment.contains("clang-tidy (v1.2.3) reports")); } @@ -950,15 +961,16 @@ mod test { let files = make_dummy_files("warning"); crate::test_common::poison_mutex(&files[0]); // poisoned while making the clang-format comment - assert!(RestClient::make_comment(&files, 1, 1, &dummy_versions(), None).is_err()); + assert!(RestClient::make_comment(&files, 1, 1, &dummy_versions(), None, false).is_err()); // poisoned while making the clang-tidy comment - assert!(RestClient::make_comment(&files, 0, 1, &dummy_versions(), None).is_err()); + assert!(RestClient::make_comment(&files, 0, 1, &dummy_versions(), None, false).is_err()); } #[test] fn comment_too_long_for_tidy_note() { let files = make_dummy_files("warning"); - let unlimited = RestClient::make_comment(&files, 0, 1, &dummy_versions(), None).unwrap(); + let unlimited = + RestClient::make_comment(&files, 0, 1, &dummy_versions(), None, false).unwrap(); assert!(unlimited.contains("A test dummy rationale")); // one byte short of the room needed for the clang-tidy note let limited = RestClient::make_comment( @@ -967,6 +979,7 @@ mod test { 1, &dummy_versions(), Some(unlimited.len() as u64 - 1), + false, ) .unwrap(); assert!(!limited.contains("A test dummy rationale")); diff --git a/cpp-linter/tests/comments.rs b/cpp-linter/tests/comments.rs index 7e58de57..7f19db70 100644 --- a/cpp-linter/tests/comments.rs +++ b/cpp-linter/tests/comments.rs @@ -50,6 +50,7 @@ struct TestParams { pub fail_posting: bool, pub bad_existing_comments: bool, pub relative_out_files: bool, + pub gen_patch: bool, } impl Default for TestParams { @@ -65,6 +66,7 @@ impl Default for TestParams { fail_posting: false, bad_existing_comments: false, relative_out_files: false, + gen_patch: false, } } } @@ -278,6 +280,7 @@ async fn setup(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) { "--summary-output-file={}", summary_out_file_path.to_str().unwrap() ), + format!("--gen-patch={}", test_params.gen_patch), format!("--sarif-file={}", sarif_file_path.to_str().unwrap()), ]; if test_params.force_lgtm { @@ -310,7 +313,9 @@ async fn setup(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) { .trim() .to_string() }); - if let Some(patch_path_from_output) = patch_path_from_output { + if test_params.gen_patch + && let patch_path_from_output = patch_path_from_output.unwrap() + { // verify patch path exists and print its content println!("Patch path from GITHUB_OUTPUT: {patch_path_from_output}"); let patch_path = Path::new(&patch_path_from_output); @@ -390,6 +395,7 @@ async fn new_push_all_lines() { test_comment(&TestParams { lines_changed_only: LinesChangedOnly::Off, no_lgtm: false, + gen_patch: true, ..Default::default() }) .await; @@ -399,6 +405,7 @@ async fn new_push_all_lines() { async fn new_push_changed_lines() { test_comment(&TestParams { no_lgtm: false, + gen_patch: true, ..Default::default() }) .await;