fix(tools): detect exports already present on main

tools/pr_overlap.py

@@ -1,4 +1,5 @@
 import argparse
+import io
 import json
 import os
 import re
@@ -7,9 +8,10 @@
 from concurrent.futures import ThreadPoolExecutor
 from itertools import combinations
 from pathlib import Path
+import tarfile
 from urllib.parse import quote, urlencode
 
-from nid_names import compute_nid, source_aliases
+import nid_names
 
 PARENT = 'if .parent then "\\(.parent.owner.login)/\\(.parent.name)" else .nameWithOwner end'
 EXPORT = re.compile(r"^\+.*\bAPS5_VABI\s+(\w+)\s*\(")
@@ -19,10 +21,33 @@
 HUNK = re.compile(r"^@@ -(\d+)(?:,(\d+))?", re.M)
 MARKER = "<!-- pr-overlap -->"
 FETCH_REFS = 100
+_REPO_ROOTS = {}
 
 
 def git(*args):
-    result = subprocess.run(["git", "-c", "core.quotePath=false", *args], capture_output=True)
+    cwd = Path.cwd()
+    root = _REPO_ROOTS.get(cwd)
+    if root is None:
+        root = subprocess.run(
+            ["git", "rev-parse", "--show-toplevel"], cwd=cwd, capture_output=True, check=True
+        ).stdout.decode("utf-8", errors="replace").strip()
+        _REPO_ROOTS[cwd] = root
+    args = list(args)
+    if "--" in args:
+        separator = args.index("--")
+        for index in range(separator + 1, len(args)):
+            pathspec = args[index]
+            if pathspec.startswith(("../", "..\\")):
+                absolute = (cwd / pathspec).resolve()
+                try:
+                    args[index] = absolute.relative_to(Path(root)).as_posix()
+                except ValueError:
+                    pass
+    result = subprocess.run(
+        ["git", "-c", "core.quotePath=false", *args],
+        cwd=root,
+        capture_output=True,
+    )
     result.stdout = result.stdout.decode("utf-8", errors="replace")
     result.stderr = result.stderr.decode("utf-8", errors="replace")
     result.check_returncode()
@@ -91,8 +116,15 @@ def fetch(base, prs):
 
 
 def merge(*args):
+    cwd = Path.cwd()
+    root = _REPO_ROOTS.get(cwd)
+    if root is None:
+        root = subprocess.run(
+            ["git", "rev-parse", "--show-toplevel"], cwd=cwd, capture_output=True, check=True
+        ).stdout.decode("utf-8", errors="replace").strip()
+        _REPO_ROOTS[cwd] = root
     result = subprocess.run(["git", "-c", "core.quotePath=false", "merge-tree", "--write-tree", "--name-only", "--no-messages", "-z", *args],
-                            capture_output=True)
+                            cwd=root, capture_output=True)
     lines = result.stdout.decode("utf-8", errors="replace").rstrip("\0").split("\0")
     if result.returncode not in (0, 1) or not lines[0]:
         error = result.stderr.decode("utf-8", errors="replace").strip()
@@ -105,16 +137,18 @@ def commit(tree):
                "-p", "refs/pr/base", "-m", "pr_overlap").strip()
 
 
-def exports(diff):
+def exports(diff, prefix="+"):
     found = set()
     lines = diff.splitlines()
     for i, line in enumerate(lines):
-        m = EXPORT.match(line)
+        if not line.startswith(prefix) or line.startswith(prefix * 3):
+            continue
+        m = EXPORT.match(prefix + line[1:])
         if not m or m.group(1).endswith(("_nid_no_patch", "_nid_no_patch_cut")):
             continue
         text = line[1:]
         for nxt in lines[i + 1:]:
-            if ";" in text or "{" in text or not nxt.startswith("+") or nxt.startswith("+++"):
+            if ";" in text or "{" in text or not nxt.startswith(prefix) or nxt.startswith(prefix * 3):
                 break
             text += nxt[1:]
         semi, brace = text.find(";"), text.find("{")
@@ -124,19 +158,71 @@ def exports(diff):
     return found
 
 
-def aliases(diff):
-    added = "\n".join(line[1:] for line in diff.splitlines() if line.startswith("+") and not line.startswith("+++"))
-    return {nid for nid, _ in source_aliases(added)}
+def changed_source(diff, prefix):
+    files, current, path = [], [], None
+    for line in diff.splitlines():
+        if line.startswith("diff --git "):
+            if current and path:
+                files.append((path, "\n".join(current)))
+            path = line.split(" b/", 1)[1] if " b/" in line else None
+            current = []
+        elif line.startswith(prefix) and not line.startswith(prefix * 3):
+            current.append(line[1:])
+    if current and path:
+        files.append((path, "\n".join(current)))
+    return files
+
+
+def library_for_path(path):
+    parts = Path(path).parts
+    return "/".join(parts[:4]) if len(parts) > 3 and parts[:3] == ("core", "libs", "prx") else ""
+
+
+def source_exports(text, library=""):
+    found = {}
+    for match in nid_names.source_definitions(text):
+        name = nid_names.strip_nid_postfix(match.group(1))
+        if not name.endswith(("_nid_no_patch", "_nid_no_patch_cut")):
+            found.setdefault((library, nid_names.compute_nid(name)), set()).add(name)
+    for name in nid_names.source_assembly_definitions(text):
+        name = nid_names.strip_nid_postfix(name)
+        if not name.endswith(("_nid_no_patch", "_nid_no_patch_cut")):
+            found.setdefault((library, nid_names.compute_nid(name)), set()).add(name)
+    for nid, name in nid_names.source_aliases(text):
+        found.setdefault((library, nid), set()).add(name)
+    return found
 
 
-def export_ids(pr):
-    return {compute_nid(name) for name in pr["exports"]} | pr.get("aliases", set())
+def source_changed_exports(diff, prefix):
+    found = {}
+    for path, text in changed_source(diff, prefix):
+        library = library_for_path(path)
+        for key, names in source_exports(text, library).items():
+            found.setdefault(key, set()).update(names)
+        file_diff = "\n".join(prefix + line for line in text.splitlines())
+        for name in exports(file_diff, prefix):
+            canonical = nid_names.strip_nid_postfix(name)
+            found.setdefault((library, nid_names.compute_nid(canonical)), set()).add(canonical)
+    return found
 
 
-def same_exports(pa, pb, shared):
-    common = (export_ids(pa) & export_ids(pb)).difference(*(export_ids(pr) for pr in shared))
-    named = {name for name in pa["exports"] | pb["exports"] if compute_nid(name) in common}
-    return named | common.difference(compute_nid(name) for name in named)
+def base_exports(ref):
+    root = git("rev-parse", "--show-toplevel").strip()
+    archive = subprocess.run(
+        ["git", "-C", root, "archive", "--format=tar", ref, "core/libs"], capture_output=True, check=True
+    ).stdout
+    found = {}
+    with tarfile.open(fileobj=io.BytesIO(archive), mode="r:") as source:
+        for entry in source:
+            path = Path(entry.name)
+            if not entry.isfile() or path.suffix != ".cpp" or any(
+                    part.lower() in {"test", "tests"} for part in path.parts[2:-1]):
+                continue
+            text = source.extractfile(entry).read().decode("utf-8", errors="replace")
+            library = library_for_path(entry.name)
+            for key, names in source_exports(text, library).items():
+                found.setdefault(key, set()).update(names)
+    return found
 
 
 def scan(pr):
@@ -151,7 +237,8 @@ def scan(pr):
             pr["added"].add(path)
     diff = git("diff", "-U0", "--no-color", old, new, "--", "core/libs/prx/*.cpp")
     pr["exports"] = exports(diff)
-    pr["aliases"] = aliases(diff)
+    pr["added_exports"] = source_changed_exports(diff, "+")
+    pr["removed_exports"] = source_changed_exports(diff, "-")
 
 
 def scan_all(prs, jobs):
@@ -206,7 +293,7 @@ def overlaps(prs, a, b):
     found = {"added": (pa["added"] & pb["added"]).difference(*(pr["added"] for pr in shared))}
     if pa["author"] != pb["author"]:
         found["conflicts"] = conflicts(pa, pb)
-        found["exports"] = same_exports(pa, pb, shared)
+        found["exports"] = (pa["exports"] & pb["exports"]).difference(*(pr["exports"] for pr in shared))
         if not (pa["tree"] and pb["tree"]):
             uncompared = pa.get("files", set()) & pb.get("files", set())
             if uncompared:
@@ -227,9 +314,10 @@ def pull(blob, number):
     return f'[PR {number}]({blob.split("/blob/", 1)[0].replace("https://github.com/", "https://www.github.com/", 1)}/pull/{number})'
 
 
-def report(rows, blob):
+def report(rows, blob, main_exports=None):
     if not rows:
-        return ""
+        if not main_exports:
+            return ""
     confirmed = {number: found for number, found in rows.items()
                  if found.get("conflicts") or found.get("exports") or found["added"]}
     uncompared = {number: found["uncompared"] for number, found in rows.items() if found.get("uncompared")}
@@ -244,6 +332,12 @@ def report(rows, blob):
         ])
         for number, found in sorted(confirmed.items()):
             lines.append(f'| {pull(blob, number)} | {spans(found.get("conflicts"), blob)} | {cell(found.get("exports"))} | {cell(found["added"])} |')
+    if main_exports:
+        if confirmed:
+            lines.append("")
+        lines.append("Exports introduced here that are already present in `main`:")
+        lines.append("")
+        lines.append(f"- {cell(main_exports)}")
     if uncompared:
         if confirmed:
             lines.append("")
@@ -259,10 +353,24 @@ def report(rows, blob):
     return "\n".join(lines) + "\n"
 
 
-def write_reports(out, rows, blob):
+def write_reports(out, rows, blob, main_exports=None):
+    main_exports = main_exports or {}
     out.mkdir(parents=True, exist_ok=True)
     for number, found in rows.items():
-        (out / f"{number}.md").write_text(report(found, blob), encoding="utf-8")
+        (out / f"{number}.md").write_text(report(found, blob, main_exports.get(number)), encoding="utf-8")
+
+
+def main_export_findings(prs, current_exports):
+    findings = {}
+    for number, pr in prs.items():
+        candidates = set(pr.get("added_exports", {})) - set(pr.get("removed_exports", {}))
+        names = set()
+        for key in candidates & set(current_exports):
+            names.update(pr["added_exports"][key])
+            names.update(current_exports[key])
+        if names:
+            findings[number] = names
+    return findings
 
 
 def describe_failure(error):
@@ -288,10 +396,7 @@ def main(argv=None):
     if args.jobs < 1:
         parser.error("--jobs must be at least 1")
     global REPO
-    if args.out:
-        args.out = args.out.resolve()
     try:
-        os.chdir(git("rev-parse", "--show-toplevel").strip())
         REPO = args.repo or upstream()
         prs = open_prs(args.base)
         fetch(args.base, prs)
@@ -300,7 +405,9 @@ def main(argv=None):
             prs = own(prs, sha)
             prs[0] = {"ref": sha, "author": None, "depends": set()}
         scan_all(prs, args.jobs)
+        current_exports = base_exports("refs/pr/base")
         rows = {n: {} for n in prs}
+        main_exports = main_export_findings(prs, current_exports)
         pairs = [(0, n) for n in prs if n] if args.branch else combinations(prs, 2)
         for a, b in pairs:
             if found := overlaps(prs, a, b):
@@ -309,9 +416,9 @@ def main(argv=None):
     except subprocess.CalledProcessError as error:
         raise SystemExit(describe_failure(error))
     if args.branch:
-        print(report(rows[0], blob) or "No overlap with open pull requests.", end="")
+        print(report(rows[0], blob, main_exports.get(0)) or "No overlap with open pull requests.", end="")
         return 0
-    write_reports(args.out, rows, blob)
+    write_reports(args.out, rows, blob, main_exports)
     return 0
 
 

tools/tests/test_pr_overlap_encoding.py

@@ -42,6 +42,8 @@ def command_output(stdout, stderr="", returncode=0):
             f"sys.exit({returncode})\n")
 
     def run(command, **kwargs):
+        if command[:3] == ["git", "rev-parse", "--show-toplevel"]:
+            return RUN(command, **kwargs)
         return RUN([sys.executable, "-c", code], **kwargs)
 
     with simulated_cp1252(), \

tools/tests/test_pr_overlap_exports.py

@@ -4,6 +4,7 @@
 
 sys.path.insert(0, str(Path(__file__).resolve().parents[1]))
 import pr_overlap
+import nid_names
 
 TRAILING = "+extern \"C\" std::int64_t APS5_VABI recv_nid_postfix(int, void*, std::uint64_t, int) noexcept;   \n"
 DIFF = """diff --git a/core/libs/prx/libkernel/File/src/Open.cpp b/core/libs/prx/libkernel/File/src/Open.cpp
@@ -52,23 +53,80 @@ def test_removed_context_and_helper_lines_are_not_exports(self):
         self.assertNotIn("context_nid_postfix", found)
         self.assertNotIn("Helper_nid_no_patch", found)
 
+    def test_added_definitions_match_base_names_with_nid_postfix(self):
+        nid = nid_names.compute_nid("sceKernelRead")
+        base = {("core/libs/prx/libkernel", nid): {"sceKernelRead_nid_postfix"}}
+        added = pr_overlap.source_changed_exports(
+            "diff --git a/core/libs/prx/libkernel/a.cpp b/core/libs/prx/libkernel/a.cpp\n+int APS5_VABI sceKernelRead(int fd) { return fd; }\n", "+")
+        self.assertEqual(set(added) & set(base), {("core/libs/prx/libkernel", nid)})
 
-    def test_explicit_export_aliases_are_collected(self):
-        diff = ('+int APS5_VABI placeholder_nid_no_patch() {\n'
-                '+    NotImplemented_nid_no_patch(__func__);\n'
-                '+}\n'
-                '+APS5_EXPORT("Pz262plj20w", placeholder_nid_no_patch);\n'
-                '-APS5_EXPORT("sKneJjUXWkQ", removed_nid_no_patch);\n')
-        self.assertEqual(pr_overlap.exports(diff), set())
-        self.assertEqual(pr_overlap.aliases(diff), {"Pz262plj20w"})
-        self.assertEqual(pr_overlap.aliases(diff.replace('+APS5_EXPORT("Pz262plj20w", placeholder_nid_no_patch);\n', "")), set())
-
-    def test_explicit_alias_overlaps_named_export_with_the_same_nid(self):
-        named = {"author": "a", "exports": {"FT_Outline_GetInsideBorder"}, "aliases": set()}
-        explicit = {"author": "b", "exports": set(), "aliases": {"Pz262plj20w"}}
-        self.assertEqual(pr_overlap.same_exports(named, explicit, []), {"FT_Outline_GetInsideBorder"})
-        self.assertEqual(pr_overlap.same_exports(explicit, dict(explicit, author="c"), []), {"Pz262plj20w"})
-        self.assertEqual(pr_overlap.same_exports(named, explicit, [explicit]), set())
+    def test_added_assembly_export_matches_base_definition(self):
+        nid = nid_names.compute_nid("sceKernelRead")
+        added = pr_overlap.source_changed_exports(
+            'diff --git a/a.cpp b/a.cpp\n+APS5_ASM_FUNCTION("sceKernelRead")\n', "+")
+        self.assertIn(("", nid), added)
+
+    def test_added_raw_export_matches_base_definition(self):
+        nid = nid_names.compute_nid("sceKernelRead")
+        added = pr_overlap.source_changed_exports(
+            f'diff --git a/a.cpp b/a.cpp\n+APS5_EXPORT("{nid}", sceKernelReadAlias);\n', "+")
+        self.assertEqual(added[("", nid)], {"sceKernelReadAlias"})
+
+    def test_raw_main_export_is_preserved_as_literal_nid(self):
+        nid = "123456789ab"
+        self.assertEqual(pr_overlap.source_exports(f'APS5_EXPORT("{nid}", alias);'), {("", nid): {"alias"}})
+
+    def test_replacing_an_export_in_place_cancels_the_same_nid(self):
+        diff = """diff --git a/a.cpp b/a.cpp
+-int APS5_VABI sceKernelRead_nid_postfix(int fd) { throw std::runtime_error("not implemented"); }
++int APS5_VABI sceKernelRead(int fd) { return fd; }
+"""
+        added = pr_overlap.source_changed_exports(diff, "+")
+        removed = pr_overlap.source_changed_exports(diff, "-")
+        self.assertEqual(set(added) - set(removed), set())
+
+    def test_exports_with_same_nid_in_different_libraries_do_not_overlap(self):
+        nid = nid_names.compute_nid("sceKernelRead")
+        added = pr_overlap.source_changed_exports(
+            "diff --git a/core/libs/prx/libSceAmpr/a.cpp b/core/libs/prx/libSceAmpr/a.cpp\n"
+            "+int APS5_VABI sceKernelRead(int fd) { return fd; }\n", "+")
+        base = {("core/libs/prx/libkernel", nid): {"sceKernelRead"}}
+        self.assertFalse(set(added) & set(base))
+
+    def test_main_exports_have_a_separate_report_section_not_a_fake_pr(self):
+        nid = nid_names.compute_nid("sceKernelRead")
+        prs = {9: {"added_exports": {("core/libs/prx/libkernel", nid): {"sceKernelRead"}}, "removed_exports": {}}}
+        findings = pr_overlap.main_export_findings(prs, {("core/libs/prx/libkernel", nid): {"sceKernelRead"}})
+        rows = {9: {"added": set()}}
+        report = pr_overlap.report(rows, "https://example.invalid/blob/base", findings[9])
+        self.assertIn("Exports introduced here that are already present in `main`", report)
+        self.assertNotIn("PR 0", report)
+        self.assertNotIn("PR 9", report)
+        self.assertNotIn("Already on `main` |", report)
+        self.assertEqual(rows, {9: {"added": set()}})
+
+
+    def test_explicit_export_aliases_are_collected_by_library_and_nid(self):
+        source = ('int APS5_VABI placeholder_nid_no_patch() {\n'
+                  '    NotImplemented_nid_no_patch(__func__);\n'
+                  '}\n'
+                  'APS5_EXPORT("Pz262plj20w", placeholder_nid_no_patch);\n')
+        self.assertEqual(
+            pr_overlap.source_exports(source, "core/libs/prx/libSceExample"),
+            {("core/libs/prx/libSceExample", "Pz262plj20w"): {"placeholder_nid_no_patch"}},
+        )
+
+    def test_explicit_alias_and_named_export_share_a_key_in_the_same_library(self):
+        explicit = pr_overlap.source_exports(
+            'APS5_EXPORT("Pz262plj20w", placeholder_nid_no_patch);',
+            "core/libs/prx/libSceExample",
+        )
+        named = pr_overlap.source_changed_exports(
+            "diff --git a/core/libs/prx/libSceExample/a.cpp b/core/libs/prx/libSceExample/a.cpp\n"
+            "+int APS5_VABI FT_Outline_GetInsideBorder() { return 0; }\n",
+            "+",
+        )
+        self.assertEqual(set(explicit) & set(named), {("core/libs/prx/libSceExample", "Pz262plj20w")})
 
 
 if __name__ == "__main__":