Repository: cpp-linter/cpp-linter (language: python)

Issue:
PR review may not account for problems with no suggested fix
Looking at the [list of supported clang-tidy diagnostics](https://clang.llvm.org/extra/clang-tidy/checks/list.html), there are many diagnostics that cannot be fixed automatically, only those with "Yes" in the "Offers fixes" column of the table.

This could be compensated by adding to the parsing of clang-tidy output.

<details><summary>example output</summary>

```
clang-tidy -p build -checks='cppcoreguidelines-*,clang-analyzer-*,modernize-*' --fix-errors src/demo.cpp src/demo.hpp
4025 warnings generated.
4030 warnings generated.
C:\dev\test-cpp-linter-action\src\demo.cpp:3:10: warning: inclusion of deprecated C++ header 'stdio.h'; consider using 'cstdio' instead [modernize-deprecated-headers]
#include <stdio.h>
         ^~~~~~~~~
         <cstdio>
C:\dev\test-cpp-linter-action\src\demo.cpp:3:10: note: FIX-IT applied suggested code changes
C:\dev\test-cpp-linter-action\src\demo.cpp:8:5: warning: use a trailing return type for this function [modernize-use-trailing-return-type]
int main(){
~~~ ^
auto       -> int
C:\dev\test-cpp-linter-action\src\demo.cpp:8:1: note: FIX-IT applied suggested code changes
int main(){
^
C:\dev\test-cpp-linter-action\src\demo.cpp:8:11: note: FIX-IT applied suggested code changes
int main(){
          ^
C:\dev\test-cpp-linter-action\src\demo.cpp:10:13: warning: statement should be inside braces [readability-braces-around-statements]
    for (;;) break;
            ^
             {
C:\dev\test-cpp-linter-action\src\demo.cpp:10:13: note: FIX-IT applied suggested code changes
C:\dev\test-cpp-linter-action\src\demo.cpp:10:20: note: FIX-IT applied suggested code changes
    for (;;) break;
                   ^
C:\dev\test-cpp-linter-action\src\demo.cpp:13:5: warning: do not call c-style vararg functions [cppcoreguidelines-pro-type-vararg]
    printf("Hello world!\n");
    ^
C:\dev\test-cpp-linter-action\src\demo.hpp:6:11: warning: use default member initializer for 'useless' [modernize-use-default-member-init]
    char* useless;
          ^
                 {"\0"}
C:\dev\test-cpp-linter-action\src\demo.hpp:6:18: note: FIX-IT applied suggested code changes
    char* useless;
                 ^
C:\dev\test-cpp-linter-action\src\demo.hpp:8:23: note: FIX-IT applied suggested code changes
    Dummy() :numb(0), useless("\0"){}
                      ^
C:\dev\test-cpp-linter-action\src\demo.hpp:7:9: warning: use default member initializer for 'numb' [modernize-use-default-member-init]
    int numb;
        ^
            {0}
C:\dev\test-cpp-linter-action\src\demo.hpp:7:13: note: FIX-IT applied suggested code changes
    int numb;
            ^
C:\dev\test-cpp-linter-action\src\demo.hpp:8:14: note: FIX-IT applied suggested code changes
    Dummy() :numb(0), useless("\0"){}
             ^
C:\dev\test-cpp-linter-action\src\demo.hpp:11:11: warning: use a trailing return type for this function [modernize-use-trailing-return-type]
    void *not_useful(char *str){useless = str;}
    ~~~~~~^
    auto                        -> void *
C:\dev\test-cpp-linter-action\src\demo.hpp:11:5: note: FIX-IT applied suggested code changes
    void *not_useful(char *str){useless = str;}
    ^
C:\dev\test-cpp-linter-action\src\demo.hpp:11:32: note: FIX-IT applied suggested code changes
    void *not_useful(char *str){useless = str;}
                               ^
clang-tidy applied 11 of 11 suggested fixes.
Suppressed 4023 warnings (4019 in non-user code, 4 with check filters).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
``` 

</details>

Notice that there is a line that signifies when a fix is applied:

> ```
> C:\dev\test-cpp-linter-action\src\demo.hpp:11:32: note: FIX-IT applied suggested code changes
>     void *not_useful(char *str){useless = str;}
>                                ^
> ```

But if I run clang-tidy again on the modified files, I still get a warning that cannot be automatically fixed:

```
clang-tidy -p build -checks='cppcoreguidelines-*,clang-analyzer-*,modernize-*' --fix-errors src/demo.cpp src/demo.hpp
4302 warnings generated.
4304 warnings generated.
C:\dev\test-cpp-linter-action\src\demo.cpp:12:5: warning: do not call c-style vararg functions [cppcoreguidelines-pro-type-vararg]
    printf("Hello world!\n");
    ^
Suppressed 4303 warnings (4299 in non-user code, 4 with check filters).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
``` 

This means that the warning from [cppcoreguidelines-pro-type-vararg](https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/pro-type-vararg.html) will not show in the PR review.


Discussion / hints:

Fix patch:
diff --git a/cpp_linter/clang_tools/clang_tidy.py b/cpp_linter/clang_tools/clang_tidy.py
index 5887b514..6cc2e5d1 100644
--- a/cpp_linter/clang_tools/clang_tidy.py
+++ b/cpp_linter/clang_tools/clang_tidy.py
@@ -4,12 +4,12 @@
 from pathlib import Path, PurePath
 import re
 import subprocess
-from typing import Tuple, Union, List, cast, Optional, Dict
+from typing import Tuple, Union, List, cast, Optional, Dict, Set
 from ..loggers import logger
 from ..common_fs import FileObj
 
 NOTE_HEADER = re.compile(r"^(.+):(\d+):(\d+):\s(\w+):(.*)\[([a-zA-Z\d\-\.]+)\]$")
-
+FIXED_NOTE = re.compile(r"^.+:(\d+):\d+:\snote: FIX-IT applied suggested code changes$")
 
 class TidyNotification:
     """Create a object that decodes info from the clang-tidy output's initial line that
@@ -74,6 +74,8 @@ def __init__(
         self.filename = rel_path
         #: A `list` of lines for the code-block in the notification.
         self.fixit_lines: List[str] = []
+        #: A list of line numbers where a suggested fix was applied.
+        self.applied_fixes: Set[int] = set()
 
     @property
     def diagnostic_link(self) -> str:
@@ -95,12 +97,14 @@ def __init__(self, notes: List[TidyNotification]) -> None:
         self.notes = notes
 
     def diagnostics_in_range(self, start: int, end: int) -> str:
-        """Get a markdown formatted list of diagnostics found between a ``start``
+        """Get a markdown formatted list of fixed diagnostics found between a ``start``
         and ``end`` range of lines."""
         diagnostics = ""
         for note in self.notes:
-            if note.line in range(start, end + 1):  # range is inclusive
-                diagnostics += f"- {note.rationale} [{note.diagnostic_link}]\n"
+            for fix_line in note.applied_fixes:
+                if fix_line in range(start, end + 1):  # range is inclusive
+                    diagnostics += f"- {note.rationale} [{note.diagnostic_link}]\n"
+                    break
         return diagnostics
 
 
@@ -165,6 +169,11 @@ def run_clang_tidy(
     for extra_arg in extra_args:
         arg = extra_arg.strip('"')
         cmds.append(f'--extra-arg={arg}')
+    if tidy_review:
+        # clang-tidy overwrites the file contents when applying fixes.
+        # create a cache of original contents
+        original_buf = Path(file_obj.name).read_bytes()
+        cmds.append("--fix-errors")  # include compiler-suggested fixes
     cmds.append(filename)
     logger.info('Running "%s"', " ".join(cmds))
     results = subprocess.run(cmds, capture_output=True)
@@ -177,17 +186,11 @@ def run_clang_tidy(
     advice = parse_tidy_output(results.stdout.decode(), database=db_json)
 
     if tidy_review:
-        # clang-tidy overwrites the file contents when applying fixes.
-        # create a cache of original contents
-        original_buf = Path(file_obj.name).read_bytes()
-        cmds.insert(1, "--fix-errors")  # include compiler-suggested fixes
-        # run clang-tidy again to apply any fixes
-        logger.info('Getting fixes with "%s"', " ".join(cmds))
-        subprocess.run(cmds, check=True)
         # store the modified output from clang-tidy
         advice.patched = Path(file_obj.name).read_bytes()
         # re-write original file contents
         Path(file_obj.name).write_bytes(original_buf)
+
     return advice
 
 
@@ -202,20 +205,31 @@ def parse_tidy_output(
         ``compile_commands.json file``.
     """
     notification = None
+    found_fix = False
     tidy_notes = []
     for line in tidy_out.splitlines():
-        match = re.match(NOTE_HEADER, line)
-        if match is not None:
+        note_match = re.match(NOTE_HEADER, line)
+        fixed_match = re.match(FIXED_NOTE, line)
+        if note_match is not None:
             notification = TidyNotification(
                 cast(
                     Tuple[str, Union[int, str], Union[int, str], str, str, str],
-                    match.groups(),
+                    note_match.groups(),
                 ),
                 database,
             )
             tidy_notes.append(notification)
-        elif notification is not None:
+            # begin capturing subsequent lines as part of notification details
+            found_fix = False
+        elif fixed_match is not None and notification is not None:
+            notification.applied_fixes.add(int(fixed_match.group(1)))
+            # suspend capturing subsequent lines as they are not needed
+            found_fix = True
+        elif notification is not None and not found_fix:
             # append lines of code that are part of
             # the previous line's notification
             notification.fixit_lines.append(line)
+        # else: line is part of the applied fix. We don't need to capture
+        # this line because the fix has been applied to the file already.
+
     return TidyAdvice(notes=tidy_notes)
diff --git a/cpp_linter/common_fs.py b/cpp_linter/common_fs.py
index 6120dd2c..2b90eb2b 100644
--- a/cpp_linter/common_fs.py
+++ b/cpp_linter/common_fs.py
@@ -120,6 +120,21 @@ def is_hunk_contained(self, hunk: DiffHunk) -> Optional[Tuple[int, int]]:
             start = hunk.new_start
             # make it span 1 line
             end = start
+        return self.is_range_contained(start, end)
+
+    def is_range_contained(self, start: int, end: int) -> Optional[Tuple[int, int]]:
+        """Does the given ``start`` and ``end`` line numbers fit within a single diff
+        hunk?
+
+        This is a helper function to `is_hunk_contained()`.
+
+        .. tip:: This is mostly useful to create comments that can be posted within a
+            git changes' diff. Ideally, designed for PR reviews based on patches
+            generated by clang tools' output.
+
+        :returns: The appropriate starting and ending line numbers of the given hunk.
+            If hunk cannot fit in a single hunk, this returns `None`.
+        """
         for hunk in self.diff_chunks:
             chunk_range = range(hunk[0], hunk[1])
             if start in chunk_range and end in chunk_range:
diff --git a/cpp_linter/rest_api/github_api.py b/cpp_linter/rest_api/github_api.py
index 715be1d9..75d53b1e 100644
--- a/cpp_linter/rest_api/github_api.py
+++ b/cpp_linter/rest_api/github_api.py
@@ -393,12 +393,12 @@ def post_review(
                 payload_comments.extend(comments)
                 if total and total != len(comments):
                     body += f"Only {len(comments)} out of {total} {tool_name} "
-                    body += "suggestions fit within this pull request's diff.\n"
+                    body += "concerns fit within this pull request's diff.\n"
             if patch:
                 body += f"\n<details><summary>Click here for the full {tool_name} patch"
                 body += f"</summary>\n\n\n```diff\n{patch}\n```\n\n\n</details>\n\n"
-            else:
-                body += f"No objections from {tool_name}.\n"
+            elif not total:
+                body += f"No concerns from {tool_name}.\n"
         if total_changes:
             event = "REQUEST_CHANGES"
         else:
@@ -471,6 +471,30 @@ def create_review_comments(
                     body += f"\n```suggestion\n{suggestion}```"
                 comment["body"] = body
                 comments.append(comment)
+
+        if tool_advice and isinstance(tool_advice[0], TidyAdvice):
+            # now check for clang-tidy warnings with no fixes applied
+            for file, tidy_advice in zip(files, tool_advice):
+                assert isinstance(tidy_advice, TidyAdvice)
+                for note in tidy_advice.notes:
+                    if not note.applied_fixes:  # if no fix was applied
+                        total += 1
+                        line_numb = int(note.line)
+                        if file.is_range_contained(start=line_numb, end=line_numb + 1):
+                            diag: Dict[str, Any] = {
+                                "path": file.name,
+                                "line": note.line,
+                            }
+                            body = f"### clang-tidy diagnostic\n**{file.name}:"
+                            body += f"{note.line}:{note.cols}:** {note.severity}: "
+                            body += f"[{note.diagnostic_link}]\n> {note.rationale}\n"
+                            if note.fixit_lines:
+                                body += f'```{Path(file.name).suffix.lstrip(".")}\n'
+                                for line in note.fixit_lines:
+                                    body += f"{line}\n"
+                                body += "```\n"
+                            diag["body"] = body
+                            comments.append(diag)
         return (comments, total, full_patch)
 
     def _dismiss_stale_reviews(self, url: str):


Test patch:
diff --git a/tests/reviews/test_pr_review.py b/tests/reviews/test_pr_review.py
index 71c22111..e72bb46d 100644
--- a/tests/reviews/test_pr_review.py
+++ b/tests/reviews/test_pr_review.py
@@ -10,6 +10,10 @@
 
 TEST_REPO = "cpp-linter/test-cpp-linter-action"
 TEST_PR = 27
+DEFAULT_TIDY_CHECKS = (
+    "boost-*,bugprone-*,performance-*,readability-*,portability-*,modernize-*,"
+    "clang-analyzer-*,cppcoreguidelines-*"
+)
 
 
 @pytest.mark.parametrize(
@@ -120,7 +124,7 @@ def test_post_review(
         format_advice, tidy_advice = capture_clang_tools_output(
             files,
             version=environ.get("CLANG_VERSION", "16"),
-            checks="",
+            checks=DEFAULT_TIDY_CHECKS,
             style="file",
             lines_changed_only=changes,
             database="",