From eac9df3036861ce809c4f629f7c87191cbf14626 Mon Sep 17 00:00:00 2001
From: Brendan <2bndy5@gmail.com>
Date: Tue, 13 Feb 2024 12:40:22 -0800
Subject: [PATCH 01/39] account for clang-tidy notes without fixes in PR
reviews (#76)
* parse tidy output with fixes applied
* add PR comment for diagnostic with no fix
---
cpp_linter/clang_tools/clang_tidy.py | 46 ++++++++++++++++++----------
cpp_linter/common_fs.py | 15 +++++++++
cpp_linter/rest_api/github_api.py | 30 ++++++++++++++++--
tests/reviews/test_pr_review.py | 6 +++-
4 files changed, 77 insertions(+), 20 deletions(-)
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"\nClick here for the full {tool_name} patch"
body += f"
\n\n\n```diff\n{patch}\n```\n\n\nclang-format reports: "
+ comment += f"{checks_failed} file(s) not formatted
\n\n"
+ closer = "\nclang-tidy reports: "
+ comment += f"{checks_failed} concern(s)
\n\n"
+ closer = "\nclang-format reports: "
- comment += f"{format_checks_failed} file(s) not formatted"
- comment += f"
\n\n{format_comment}\nclang-tidy reports: "
- comment += f"{tidy_checks_failed} concern(s)
\n\n"
- comment += f"{tidy_comment}\nclang-tidy reports: "
comment += f"{checks_failed} concern(s)
\n\n"
closer = "\nClick here for the full {tool_name} patch"
+ + f"
\n\n\n```diff\n{self.full_patch[tool_name]}\n"
+ + "```\n\n\nClick here for the full {tool_name} patch"
- body += f"
\n\n\n```diff\n{patch}\n```\n\n\nclang-format reports: "
+ comment = "\nclang-format{} reports: ".format(
+ "" if version is None else f" (v{version})"
+ )
comment += f"{checks_failed} file(s) not formatted
\n\n"
closer = "\nclang-tidy reports: "
+ comment = "\nclang-tidy{} reports: ".format(
+ "" if version is None else f" (v{version})"
+ )
comment += f"{checks_failed} concern(s)
\n\n"
closer = "\n