diff --git a/tools/check_conventions.py b/tools/check_conventions.py index 5be3c30d5..61b187c98 100644 --- a/tools/check_conventions.py +++ b/tools/check_conventions.py @@ -6,9 +6,12 @@ import re import subprocess import sys from collections import defaultdict -from pathlib import PurePosixPath +from pathlib import Path, PurePosixPath from urllib.parse import unquote +sys.path.insert(0, str(Path(__file__).resolve().parent)) +from nid_names import CPP_TOKEN + RULES = { "comment": ("error", "docs/dev/CONVENTIONS.md", "Comments are only for technical debt, so they need a change to docs/dev/TechnicalDebt.md or docs/dev/debt/ in the same pull request; #endif and namespace ends are always allowed"), "silent-stub": ("error", "CONTRIBUTING.md#code", "Unimplemented exports call NotImplemented_nid_no_patch(__func__); silent stubs are listed under Silent stubs in docs/dev/debt/.md, by their file or by name with a path in their library"), @@ -36,7 +39,6 @@ DEBT_TARGET = re.compile(r"\]\((?:\.\./)+(core/[^)#\s]+)") CONVENTIONAL = re.compile(r"^(feat|fix|docs|style|refactor|perf|test|build|ci|chore|revert)(\([^)]+\))?!?: \S") STRINGS = re.compile(r'"(?:\\.|[^"\\])*"|\'(?:\\.|[^\'\\])*\'') -TOKENS = re.compile(r'"(?:\\.|[^"\\\n])*"|\'(?:\\.|[^\'\\\n])*\'|//[^\n]*|/\*.*?\*/', re.S) ALLOWED_COMMENT = re.compile(r"^\s*(#\s*endif\b|}\s*;?\s*//\s*(end\s+)?namespace\b)") EXPORT = re.compile(r"\bAPS5_VABI\s+(\w+)\s*\(") STUB_BODY = re.compile(r"^(?:\(void\)\s*\w+\s*;|static_cast\(\s*\w+\s*\)\s*;)*" @@ -76,7 +78,7 @@ class Check: self.numstat[path] = (added, deleted) paths = iter(self.status) path, number = None, None - for line in git("diff", "-U0", "--no-color", "--no-renames", self.base, head).splitlines(): + for line in git("diff", "-U0", "--no-color", "--no-renames", self.base, head).split("\n"): if line.startswith("diff --git "): path, number = next(paths), None elif line.startswith("@@"): @@ -99,9 +101,10 @@ class Check: exports = {} for path, lines in self.files(lambda p: p.suffix in CPP): core = path.startswith("core/") and "tests" not in PurePosixPath(path).parts - source = None + source = self.show(path).split("\n") + masked = cpp_text("\n".join(source), preserve_comments=True).split("\n") for number, text in lines: - bare = STRINGS.sub('""', text) + bare = masked[number - 1] if ("//" in bare or "/*" in bare) and not ALLOWED_COMMENT.match(bare): self.report("comment", path, number) if EXTENSION.search(bare): @@ -130,7 +133,7 @@ class Check: return pattern = r"(^|[^[:alnum:]_])APS5_VABI[[:space:]]+(" + "|".join(exports) + r")[[:space:]]*\(" result = subprocess.run(["git", "grep", "-nE", "--full-name", pattern, self.head, "--", ":(top)core/libs/prx"], capture_output=True, encoding="utf-8", errors="replace") - for line in result.stdout.splitlines(): + for line in filter(None, result.stdout.split("\n")): _, path, number, text = line.split(":", 3) name = EXPORT.search(text).group(1) own, own_number = exports[name] @@ -331,9 +334,18 @@ def allowed_notes(path): return path in ("README.md", "CONTRIBUTING.md") or p.parts[0] == ".github" or p.parts[:2] in (("docs", "dev"), ("docs", "user")) +def cpp_text(text, preserve_comments=False): + def mask(match): + if match.group("code") is not None or preserve_comments and match.group("comment") is not None: + return match.group() + blank = re.sub(r"[^\n]", " ", match.group()) + return blank if match.group("comment") is not None else '""' + blank[2:] + + return CPP_TOKEN.sub(mask, text) + + def function_body(lines, start): - text = "\n".join(lines[start:start + 400]) - text = TOKENS.sub(lambda match: "" if match.group(0).startswith("/") else '""', text) + text = cpp_text("\n".join(lines[start:start + 400])) brace, semicolon = text.find("{"), text.find(";") if brace < 0 or 0 <= semicolon < brace: return None diff --git a/tools/tests/test_check_conventions_comments.py b/tools/tests/test_check_conventions_comments.py index aca0df9b0..25e0b1f87 100644 --- a/tools/tests/test_check_conventions_comments.py +++ b/tools/tests/test_check_conventions_comments.py @@ -6,6 +6,7 @@ import sys import tempfile import unittest from pathlib import Path +from unittest.mock import patch sys.path.insert(0, str(Path(__file__).resolve().parents[1])) from check_conventions import STUB_BODY, SYSTEM_DEPENDENCY, Check, function_body, links, output @@ -39,6 +40,18 @@ class FunctionBodyTests(unittest.TestCase): source = 'int APS5_VABI sceExample() {\nuse("// }", "/* { */");\nreturn 0;\n}' self.assertEqual(function_body(source.splitlines(), 0), 'use("","");return0;') + def test_raw_literal_braces_and_comments_preserve_following_statements(self): + source = 'int APS5_VABI sceExample() {\nuse(R"json({"brace":"}","comment":"/* hidden */"})json");\nvalidate();\nreturn 0;\n}' + body = function_body(source.splitlines(), 0) + self.assertEqual(body, 'use("");validate();return0;') + self.assertIsNone(STUB_BODY.fullmatch(body)) + + def test_raw_literal_return_keeps_a_literal_sentinel(self): + source = 'const char* APS5_VABI sceExample() { return R"json({"url":"https://example.test"})json"; }' + body = function_body(source.splitlines(), 0) + self.assertEqual(body, 'return"";') + self.assertIsNone(STUB_BODY.fullmatch(body)) + def test_apostrophes_in_comments_do_not_open_character_literals(self): source = "int APS5_VABI sceExample() {\n// don't fail here\nvalidate();\n// it's fine\nreturn 0;\n}" body = function_body(source.splitlines(), 0) @@ -58,6 +71,59 @@ class FunctionBodyTests(unittest.TestCase): self.assertIsNone(function_body(source.splitlines(), 0)) +class RawLiteralCommentTests(unittest.TestCase): + path = "core/example.cpp" + + def findings(self, source, added): + check = Check.__new__(Check) + check.tree = set() + check.status = {} + check.findings = [] + lines = source.split("\n") + check.added = {self.path: [(number, lines[number - 1]) for number in added]} + with patch.object(check, "show", return_value=source), patch.object(check, "duplicates"): + check.code() + return check.findings + + def test_isolated_checker_loads_its_trusted_sibling_from_another_directory(self): + checker = Path(__file__).resolve().parents[1] / "check_conventions.py" + with tempfile.TemporaryDirectory() as directory: + Path(directory, "nid_names.py").write_text('raise RuntimeError("untrusted current directory")\n', encoding="utf-8") + result = subprocess.run([sys.executable, "-I", str(checker), "--help"], cwd=directory, capture_output=True, text=True) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + self.assertIn("check a branch", result.stdout) + + def test_raw_url_does_not_create_a_comment(self): + source = 'static const char* Metadata = R"json({"url":"https://example.test"})json";' + self.assertEqual(self.findings(source, [1]), []) + + def test_blank_added_cpp_lines_preserve_positions(self): + for source, added in (("\n", [1]), ("int value;\n\n", [2])): + with self.subTest(source=source): + self.assertEqual(self.findings(source, added), []) + + def test_unicode_separators_in_literals_keep_physical_lines(self): + for separator in ("\x85", "\u2028", "\u2029"): + with self.subTest(separator=separator): + source = f'static const char* Metadata = R"text(https://example.test/{separator}more)text";\nint value; // pending\n' + self.assertEqual(self.findings(source, [1, 2]), [("comment", self.path, 2, "")]) + + def test_added_line_inside_an_existing_multiline_raw_literal_is_not_a_comment(self): + source = 'static const char* Metadata = R"text(\nhttps://example.test/metadata\n)text";\n' + self.assertEqual(self.findings(source, [2]), []) + + def test_actual_comment_after_a_raw_literal_retains_its_line(self): + for comment in ("// pending implementation", "/* pending implementation */"): + with self.subTest(comment=comment): + source = f'static const char* Metadata = R"text(\nhttps://example.test/metadata\n)text"; {comment}\n' + self.assertEqual(self.findings(source, [2, 3]), [("comment", self.path, 3, "")]) + + def test_quotes_and_raw_looking_text_do_not_hide_actual_comments(self): + for comment in ('// " R"text(https://example.test)text"', '/* " R"text(https://example.test)text" */'): + with self.subTest(comment=comment): + self.assertEqual(self.findings(comment, [1]), [("comment", self.path, 1, "")]) + + class SilentStubTests(unittest.TestCase): def test_commented_stub_is_reported_when_debt_change_allows_comments(self): checker = Path(__file__).resolve().parents[1] / "check_conventions.py" @@ -432,6 +498,23 @@ class RepositoryChecksTests(unittest.TestCase): check.code() self.assertIn(("comment", "core/example.cpp", 3, ""), check.findings) + def test_unicode_raw_literals_keep_git_patch_and_grep_records_intact(self): + literals = 'R"text(foo\u2028+tail\u2029+tail)text"' + fixtures = ( + ("core/example.cpp", f"static const char* Metadata = {literals};\n"), + ("core/libs/prx/libSceExample/Export.cpp", f"const char* APS5_VABI sceGetLabel() {{ return {literals}; }}\n"), + ) + for name, definition in fixtures: + with self.subTest(path=name): + path = self.root / name + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(definition + "int value; // pending\n", encoding="utf-8") + self.commit() + check = Check("HEAD~1", "HEAD") + self.assertEqual([number for number, _ in check.added[name]], [1, 2]) + check.code() + self.assertEqual(check.findings, [("comment", name, 2, "")]) + def test_checks_from_a_subdirectory_see_the_whole_repository(self): definition = "int APS5_VABI sceFooBar(int a, int b) { return a + b; }\n" first = self.root / "core/libs/prx/libSceA/Export.cpp"