Skip to content

Commit aee7581

Browse files
committed
fix: close behavior gaps with cpp-linter v1
Compared with the Python cpp-linter (v1), v2: - showed clang-tidy annotations on the whole file instead of the line - posted an approving PR review despite `--no-lgtm` - posted a new review when every suggestion was already posted - listed files in a random order - let one failed check pass under `PRE_COMMIT=1` - rejected or mishandled some inputs v1 accepts (`--extensions` with spaces or dots, `--extra-arg` with extra spaces or quotes, numeric `--verbosity`, `CPP_LINTER_PR_REVIEW_SUMMARY_ONLY=True`, paths reported as written in a compilation database) - posted unfixed diagnostics about other files on the analyzed file's lines in PR reviews - silently ignored `--pr-review` outside CI Also bold the clang-tidy count in the thread comment, and fix the `--style` help that still said the style is passed to clang-tidy.
1 parent aca88b5 commit aee7581

7 files changed

Lines changed: 272 additions & 27 deletions

File tree

‎cpp-linter/src/clang_tools/clang_tidy/mod.rs‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -191,7 +191,9 @@ fn parse_tidy_output(
191191
for unit in db_json {
192192
let unit_path =
193193
PathBuf::from_iter([unit.directory.as_str(), unit.file.as_str()]);
194-
if unit_path == filename {
194+
// clang-tidy may report the path as it is written in the database
195+
// (relative to the unit's directory) or joined with the directory.
196+
if unit_path == filename || Path::new(&unit.file) == filename {
195197
filename =
196198
normalize_path(&PathBuf::from_iter([&unit.directory, &unit.file]));
197199
found_unit = true;

‎cpp-linter/src/clang_tools/clang_tidy/test.rs‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,16 @@ fn parse_with_database_units() {
299299
assert_eq!(notes[0].filename, "demo.cpp");
300300
// path of a file not in the database is also made relative to the repo root
301301
assert_eq!(notes[1].filename, "unknown.cpp");
302+
303+
// clang-tidy can also report the path exactly as written in the database (eg. meson)
304+
let notes = parse_tidy_output(
305+
b"../demo.cpp:1:1: warning: some rationale [modernize-use-auto]\n",
306+
&database_json,
307+
&env::current_dir().unwrap(),
308+
)
309+
.unwrap();
310+
assert_eq!(notes.len(), 1);
311+
assert_eq!(notes[0].filename, "demo.cpp");
302312
}
303313

304314
fn make_clang_params(

‎cpp-linter/src/cli/mod.rs‎

Lines changed: 62 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,7 @@ use std::str::FromStr;
99
use clang_tools_manager::{RequestedVersion, logger::CLI_HELP_STYLE};
1010
#[cfg(feature = "bin")]
1111
use clap::{
12-
ArgAction, Args, Parser, Subcommand, ValueEnum,
13-
builder::{FalseyValueParser, NonEmptyStringValueParser},
14-
value_parser,
12+
ArgAction, Args, Parser, Subcommand, ValueEnum, builder::FalseyValueParser, value_parser,
1513
};
1614

1715
mod structs;
@@ -22,8 +20,10 @@ pub use structs::{ClangParams, FeedbackInput, LinesChangedOnly, ThreadComments};
2220
#[derive(Debug, Clone, PartialEq, Eq, ValueEnum)]
2321
pub enum Verbosity {
2422
/// Enables the [`log::Level::Info`] log level and above.
23+
#[value(alias = "20")]
2524
Info,
2625
/// Enables the [`log::Level::Debug`] log level and above.
26+
#[value(alias = "10")]
2727
Debug,
2828
}
2929

@@ -179,14 +179,17 @@ pub struct GeneralOptions {
179179
)]
180180
pub struct SourceOptions {
181181
/// A comma-separated list of file extensions to analyze.
182+
///
183+
/// Surrounding whitespace and a leading `.` are ignored,
184+
/// so `.cpp, .h` is the same as `cpp,h`.
182185
#[cfg_attr(
183186
feature = "bin",
184187
arg(
185188
short,
186189
long,
187190
value_delimiter = ',',
188191
default_value = "c,h,C,H,cpp,hpp,cc,hh,c++,h++,cxx,hxx",
189-
value_parser = NonEmptyStringValueParser::new(),
192+
value_parser = parse_extension,
190193
help_heading = "Source options"
191194
)
192195
)]
@@ -320,8 +323,9 @@ pub struct FormatOptions {
320323
/// entirely.
321324
///
322325
/// > [!NOTE]
323-
/// > If this is not a blank string, then it is also passed to clang-tidy
324-
/// > (if [`--tidy_checks`](#-c-tidy-checks) is not `-*`).
326+
/// > If this is not a blank string, then fixes suggested by clang-tidy
327+
/// > (if [`--tidy-checks`](#-c-tidy-checks) is not `-*`) are also
328+
/// > formatted with this style.
325329
/// > This is done to ensure suggestions from both clang-tidy and
326330
/// > clang-format are consistent.
327331
#[cfg_attr(
@@ -574,6 +578,20 @@ pub struct FeedbackOptions {
574578
pub passive_reviews: bool,
575579
}
576580

581+
/// Parses a single value of the `--extensions` option.
582+
///
583+
/// Surrounding whitespace and a leading `.` are removed.
584+
/// An empty value is an error.
585+
#[cfg(feature = "bin")]
586+
fn parse_extension(val: &str) -> Result<String, String> {
587+
let ext = val.trim().trim_start_matches('.');
588+
if ext.is_empty() {
589+
Err(format!("invalid file extension: {val:?}"))
590+
} else {
591+
Ok(ext.to_string())
592+
}
593+
}
594+
577595
/// Converts the parsed value of the `--extra-arg` option into an optional vector of strings.
578596
///
579597
/// This is for adapting to 2 scenarios where `--extra-arg` is either
@@ -603,12 +621,15 @@ pub fn convert_extra_arg_val(args: &[String]) -> Vec<String> {
603621
// specified once; split and return result
604622
v.trim_matches('\'')
605623
.trim_matches('"')
606-
.split(' ')
624+
.split_whitespace()
607625
.map(|i| i.to_string())
608626
.collect()
609627
} else {
610-
// specified multiple times; just return a clone of the values
611-
val.map(|i| i.to_string()).collect()
628+
// specified multiple times; just strip any quotes and skip blank values
629+
val.map(|i| i.trim_matches('\'').trim_matches('"').trim())
630+
.filter(|i| !i.is_empty())
631+
.map(|i| i.to_string())
632+
.collect()
612633
}
613634
}
614635

@@ -658,6 +679,38 @@ mod test {
658679
assert_eq!(extra_args, ["-std=c++17", "-Wall"])
659680
}
660681

682+
#[test]
683+
fn extensions_are_normalized() {
684+
let args = Cli::parse_from(vec!["cpp-linter", "-e", " .cpp, .h,hpp"]);
685+
assert_eq!(args.source_options.extensions, ["cpp", "h", "hpp"]);
686+
assert!(Cli::try_parse_from(vec!["cpp-linter", "-e", "c,.,h"]).is_err());
687+
}
688+
689+
#[test]
690+
fn numeric_verbosity() {
691+
let args = Cli::parse_from(vec!["cpp-linter", "-v", "10"]);
692+
assert!(args.general_options.verbosity.is_debug());
693+
let args = Cli::parse_from(vec!["cpp-linter", "-v", "20"]);
694+
assert!(!args.general_options.verbosity.is_debug());
695+
}
696+
697+
#[test]
698+
fn extra_arg_extra_spaces() {
699+
let args = Cli::parse_from(vec!["cpp-linter", "--extra-arg=' -std=c++17 -Wall '"]);
700+
let extra_args = convert_extra_arg_val(&args.tidy_options.extra_arg);
701+
assert_eq!(extra_args, ["-std=c++17", "-Wall"]);
702+
703+
let args = Cli::parse_from(vec![
704+
"cpp-linter",
705+
"--extra-arg=\"-std=c++17\"",
706+
"--extra-arg=",
707+
"--extra-arg= ",
708+
"--extra-arg='-Wall'",
709+
]);
710+
let extra_args = convert_extra_arg_val(&args.tidy_options.extra_arg);
711+
assert_eq!(extra_args, ["-std=c++17", "-Wall"]);
712+
}
713+
661714
#[test]
662715
fn extra_arg_2() {
663716
let args = Cli::parse_from(vec![

‎cpp-linter/src/common_fs.rs‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -250,7 +250,12 @@ impl FileObj {
250250
// Count of clang-tidy diagnostics that had no fixes applied
251251
let mut total = 0;
252252
for note in &advice.notes {
253-
if note.fixed_lines.is_empty() && self.is_line_in_diff(&note.line) {
253+
// Skip diagnostics about other files (eg. included headers);
254+
// their line numbers do not refer to this file.
255+
if note.filename == file_name
256+
&& note.fixed_lines.is_empty()
257+
&& self.is_line_in_diff(&note.line)
258+
{
254259
// notification had no suggestion applied in `patched`
255260
total += 1;
256261
if summary_only {
@@ -531,6 +536,11 @@ mod test {
531536
note(1, vec![]),
532537
// not in any existing suggestion; creates a new suggestion
533538
note(3, vec!["line3".to_string()]),
539+
// a diagnostic about an included header is not about this file's line 4
540+
TidyNotification {
541+
filename: "test_file.h".to_string(),
542+
..note(4, vec![])
543+
},
534544
],
535545
});
536546

‎cpp-linter/src/rest_client.rs‎

Lines changed: 41 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,7 @@ impl RestClient {
206206
let summary_only = ["true", "on", "1"].contains(
207207
&env::var("CPP_LINTER_PR_REVIEW_SUMMARY_ONLY")
208208
.unwrap_or("false".to_string())
209+
.to_lowercase()
209210
.as_str(),
210211
);
211212
let mut review_comments = ReviewComments::default();
@@ -236,9 +237,10 @@ impl RestClient {
236237
let total_review_comments = options.comments.len() as u32;
237238
self.client.cull_pr_reviews(&mut options).await?;
238239
let has_changes = review_comments.tool_total > 0;
240+
let is_lgtm = options.comments.is_empty() && !has_changes;
239241
options.action = if feedback_inputs.passive_reviews {
240242
ReviewAction::Comment
241-
} else if options.comments.is_empty() && !has_changes {
243+
} else if is_lgtm {
242244
ReviewAction::Approve
243245
} else {
244246
ReviewAction::RequestChanges
@@ -249,7 +251,15 @@ impl RestClient {
249251
total_review_comments,
250252
summary_only,
251253
);
252-
self.client.post_pr_review(&options).await?;
254+
if is_lgtm && feedback_inputs.no_lgtm {
255+
log::debug!("Not posting an approved review because `no-lgtm` is true");
256+
} else if options.comments.is_empty() && total_review_comments > 0 {
257+
log::info!(
258+
"All review comments were already posted in previous reviews; not posting a new review."
259+
);
260+
} else {
261+
self.client.post_pr_review(&options).await?;
262+
}
253263
} else {
254264
for file in files {
255265
let file = file
@@ -348,18 +358,22 @@ impl RestClient {
348358
for note in &tidy_advice.notes {
349359
let path = file.name.to_string_lossy().replace('\\', "/");
350360
if note.filename == path {
351-
let title = format!("{}:{}:{}", note.filename, note.line, note.cols);
361+
let title = format!(
362+
"{}:{}:{} [{}]",
363+
note.filename, note.line, note.cols, note.diagnostic
364+
);
352365
let annotation = FileAnnotation {
353366
severity: match note.severity.as_str() {
354367
"warning" => AnnotationLevel::Warning,
355368
"error" => AnnotationLevel::Error,
356369
_ => AnnotationLevel::Notice, // default to notice for all else
357370
},
358371
path,
359-
start_line: None,
372+
// `line` is required to anchor the annotation to a line in the file
373+
start_line: Some(note.line as usize),
360374
end_line: Some(note.line as usize),
361-
start_column: None,
362-
end_column: Some(note.cols as usize),
375+
start_column: Some(note.cols as usize),
376+
end_column: None,
363377
title: Some(title),
364378
message: note.rationale.clone(),
365379
};
@@ -524,7 +538,7 @@ fn make_tidy_comment(
524538
remaining_length: &mut u64,
525539
) -> Result<(), ClientError> {
526540
let opener = format!(
527-
"\n<details><summary>clang-tidy (v{version_used}) reports: {tidy_checks_failed}<strong> concern(s)</strong></summary>\n\n"
541+
"\n<details><summary>clang-tidy (v{version_used}) reports: <strong>{tidy_checks_failed} concern(s)</strong></summary>\n\n"
528542
);
529543
let mut show_fix_explanation = false;
530544
let mut tidy_comment = String::new();
@@ -800,6 +814,26 @@ mod test {
800814
);
801815
}
802816

817+
#[test]
818+
fn tidy_annotation_points_to_line() {
819+
let annotations =
820+
RestClient::make_annotations(&make_dummy_files("warning"), "llvm").unwrap();
821+
let tidy = annotations
822+
.iter()
823+
.find(|a| matches!(a.severity, AnnotationLevel::Warning))
824+
.unwrap();
825+
assert_eq!(tidy.start_line, Some(1));
826+
assert_eq!(tidy.start_column, Some(1));
827+
assert_eq!(
828+
tidy.title.as_deref(),
829+
Some("tests/demo/demo.cpp:1:1 [clang-diagnostic-error]")
830+
);
831+
assert!(
832+
tidy.fmt_github()
833+
.starts_with("::warning file=tests/demo/demo.cpp,line=1,col=1,")
834+
);
835+
}
836+
803837
#[test]
804838
fn comment_without_tidy_concerns() {
805839
let files = make_dummy_files("warning");

‎cpp-linter/src/run.rs‎

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,9 @@ pub async fn run_main(args: Vec<String>) -> Result<()> {
6767
},
6868
);
6969
let is_pr = rest_api_client.is_pr();
70+
if !is_pr && cli.feedback_options.pr_review && rest_api_client.client_kind() == "local" {
71+
log::warn!("Pull request reviews are only posted when running in a supported CI platform.");
72+
}
7073

7174
let mut file_filter = FileFilter::new(
7275
&cli.source_options
@@ -108,7 +111,7 @@ pub async fn run_main(args: Vec<String>) -> Result<()> {
108111
}
109112

110113
rest_api_client.start_log_group("Get list of specified source files");
111-
let files = if !matches!(cli.source_options.lines_changed_only, LinesChangedOnly::Off)
114+
let mut files = if !matches!(cli.source_options.lines_changed_only, LinesChangedOnly::Off)
112115
|| cli.source_options.files_changed_only
113116
{
114117
// parse_diff(github_rest_api_payload)
@@ -151,6 +154,9 @@ pub async fn run_main(args: Vec<String>) -> Result<()> {
151154
}
152155
all_files
153156
};
157+
// The file lists from git and the directory walk are unordered.
158+
// Sort them, so the order of feedback (comments, annotations, etc) is deterministic.
159+
files.sort_by(|a, b| a.name.cmp(&b.name));
154160
let mut arc_files = vec![];
155161
log::info!("Giving attention to the following files:");
156162
for file in files {
@@ -195,7 +201,7 @@ pub async fn run_main(args: Vec<String>) -> Result<()> {
195201
.post_feedback(&arc_files, user_inputs, clang_versions)
196202
.await?;
197203
rest_api_client.end_log_group("Posting feedback");
198-
if env::var("PRE_COMMIT").is_ok_and(|v| v == "1") && checks_failed > 1 {
204+
if env::var("PRE_COMMIT").is_ok_and(|v| v == "1") && checks_failed > 0 {
199205
return Err(anyhow!("Some checks did not pass"));
200206
}
201207
Ok(())
@@ -238,6 +244,28 @@ pub(crate) mod test {
238244
drop(tmp_gh_out);
239245
}
240246

247+
#[tokio::test]
248+
async fn pr_review_outside_ci() {
249+
let tmp_gh_out = setup_tmp_gh_out_path();
250+
let tmp_workspace = setup_tmp_workspace();
251+
unsafe {
252+
env::set_var("GITHUB_ACTIONS", "false");
253+
}
254+
// a PR review is requested, but only a warning is logged outside CI
255+
run_main(vec![
256+
"cpp-linter".to_string(),
257+
"-l".to_string(),
258+
"false".to_string(),
259+
"--pr-review".to_string(),
260+
"--repo-root".to_string(),
261+
tmp_workspace.path().to_str().unwrap().to_string(),
262+
"demo/demo.cpp".to_string(),
263+
])
264+
.await
265+
.unwrap();
266+
drop(tmp_gh_out);
267+
}
268+
241269
#[tokio::test]
242270
async fn no_mod_sys() {
243271
let tmp_gh_out = setup_tmp_gh_out_path();

0 commit comments

Comments
 (0)