Repository navigation
fix(tools): detect cross-library export duplicates - #12784
Conversation
|
@xdCloudy Before: an export added by the PR at After: the finding only keeps names whose canonical name is exported by a different library ( Could you keep the same-key finding and add the cross-library one next to it (a union, or two lists), and keep a test for Two smaller points:
|
|
@oneandonlydean Thanks, you're right: my first change regressed the existing same-(library, NID) warning. I restored that check alongside the cross-library name warning and added a regression test that pins the same-key case. The report now distinguishes the exact same-key finding from cross-library matches and labels the latter as potentially intentional; the PR description calls out the known Agc/AgcVsh and SaveData/SaveData.native pairs. The focused suite now passes with 111 passed and 5 skipped, and the conventions check passes. I also inspected the overlap bot's #9539 warning: its patch changes the probe/scan_all section, while this PR changes main_export_findings/report. GitHub still marks #9539 unresolved against the current base, so I left that bot warning as a manual-review notice rather than claiming the PRs are merge-clean. |
rooklz
left a comment
rooklz
left a comment
There was a problem hiding this comment.
Checked on macOS arm64, head 3075dc6398. Merge-tree against main 8958e22ad9 is clean, check_conventions.py --head passes, and git diff --check passes. test_pr_overlap*.py passes on Python 3.9, 3.13 and 3.15. With pr_overlap.py from main, the new tests fail (1 error, 1 failure), so they do pin the change.
Regression: rewritten definition lines are reported as duplicates of main
On main, candidates = added - removed drops a (library, NID) that the PR both removes and adds. That is what happens when a PR edits the line that defines an export (new signature, splitting a one-line body, and so on). The new loop sets same_key whenever the key is in current_exports, without checking removed, so every in-place edit of an existing export is now reported under "same library/NID already present".
I compared main and head main_export_findings on the first 80 open PRs that touch core/libs/prx/*.cpp, each diffed against its merge base:
| PRs with a finding | |
|---|---|
| main | 3 (#12700, #12688, #12682: recvfrom/sendto added to libkernel while the old definitions are still there) |
| head | 19 |
The 16 extra findings are all edits of the definition line itself, for example #12681 scalbnf (one-liner split into a body), #12791 sceAudioOutSetPadSpkVolume (parameter names added), #12692 UCNV_*_CALLBACK_*, #12674 drawup texture (ABI change), #12730 and #12729 (40 and 32 SSL getters), and #12642 pthread_suspend_*. CI posts this report on every PR, so these warnings would show up as noise.
Same issue in the cross-library check: #12644 moves sem_setname from libkernel_sys (removes the stub) to libkernel (adds the implementation). canonical not in removed_names only looks at the same key, so the move is reported as "also exported by another library in main: libkernel_sys".
Suggested fix (18 lines including 2 tests)
--- a/tools/pr_overlap.py
+++ b/tools/pr_overlap.py
@@ -521,6 +521,10 @@
for number, pr in prs.items():
added = pr.get("added_exports", {})
removed = pr.get("removed_exports", {})
+ moved = {}
+ for (library, nid), removed_from in removed.items():
+ for name in removed_from:
+ moved.setdefault(NID_DISAMBIG.sub("", nid_names.strip_nid_postfix(name)), set()).add(library)
names = {}
for library, nid in added:
added_names = {
@@ -533,10 +537,10 @@
}
for canonical in added_names:
finding = names.setdefault(canonical, {"same_key": set(), "other_libraries": set()})
- if (library, nid) in current_exports:
+ if (library, nid) in current_exports and (library, nid) not in removed:
finding["same_key"].add((library, nid))
if canonical not in removed_names:
- finding["other_libraries"].update(by_name.get(canonical, set()) - {library})
+ finding["other_libraries"].update(by_name.get(canonical, set()) - {library} - moved.get(canonical, set()))
if not finding["same_key"] and not finding["other_libraries"]:
names.pop(canonical)
if names:
--- a/tools/tests/test_pr_overlap_exports.py
+++ b/tools/tests/test_pr_overlap_exports.py
@@ -140,6 +140,18 @@
self.assertEqual(pr_overlap.main_export_findings(prs, main), {})
+ def test_rewritten_definition_line_is_not_a_main_duplicate(self):
+ nid = nid_names.compute_nid("sceKernelRead")
+ key = ("core/libs/prx/libkernel", nid)
+ prs = {9: {"added_exports": {key: {"sceKernelRead"}}, "removed_exports": {key: {"sceKernelRead"}}}}
+ self.assertEqual(pr_overlap.main_export_findings(prs, {key: {"sceKernelRead"}}), {})
+
+ def test_export_moved_between_libraries_is_not_a_cross_library_duplicate(self):
+ nid = nid_names.compute_nid("sem_setname")
+ old, new = ("core/libs/prx/libkernel_sys", nid), ("core/libs/prx/libkernel", nid)
+ prs = {9: {"added_exports": {new: {"sem_setname"}}, "removed_exports": {old: {"sem_setname"}}}}
+ self.assertEqual(pr_overlap.main_export_findings(prs, {old: {"sem_setname"}}), {})
+
def test_explicit_export_aliases_are_collected_by_library_and_nid(self):What I checked with the fix applied:
- The diff applies with
git applyon3075dc6398. test_pr_overlap*.pypasses on Python 3.9, 3.13 and 3.15.- On the current head, both new tests fail.
- On the same 80 PRs, findings drop back to the 3 that main reports (#12700/#12688/#12682, recvfrom/sendto), and #12644 is no longer reported.
- The PR's own new tests (cross-library
FT_Stroker_New, same-keysceKernelRead, same library only) still pass.
Non-blocking:
- Over those 80 PRs, the head did not produce a single cross-library finding except the #12644 move above. The new warning is quiet in practice, which is good.
- The "may already be present" heading now also covers the exact same-key case, which is a certain duplicate. It might be worth keeping "already present" for the same-key lines.
AI-assisted: yes
Thanks @rooklz for the thorough review and detailed analysis! Both regressions have now been addressed in commits Fixes
Regression coverageAdded two tests:
Validation: 55 unit tests passed, including both new regression tests. The filesystem-dependent integration suite has not yet been rerun. The original cross-library duplicate detection and same-key checks remain intact. Thanks again for testing against 80 open PRs and identifying the additional false positives. Really appreciate the detailed review! AI-assisted: yes |
rooklz
left a comment
rooklz
left a comment
There was a problem hiding this comment.
Re-checked on macOS arm64 at head 5ed1aa4afe, which adds 19225aabc0 and 5ed1aa4afe on top of 3075dc6398 that I reviewed before. Main is e347ab1f95.
test_pr_overlap*.py: 113 tests pass on Python 3.9, 3.13 and 3.15. Withpr_overlap.pyfrom3075dc6398, the two new tests fail:test_rewritten_definition_line_is_not_a_main_duplicateandtest_export_moved_between_libraries_is_not_a_cross_library_duplicate.- Real-PR corpus: I ran
main_export_findingsfrom main and from this head on the first 80 open PRs that touchcore/libs/prx/*.cpp. Each PR was diffed against its merge base, withbase_exports(upstream/main)as the current exports. Both versions report the same 3 PRs: #12700 (recvfrom/sendto), #12688 and #12682 (recvfrom). All three are same-key findings and none is cross-library. The 16 extra findings from definition-line rewrites at3075dc6398are gone. - #12644 no longer touches
sem_setname, since that move is on main now. The new unit test covers the move case instead. - Merge-tree against main is clean.
check_conventions.py --headandgit diff --checkpass.
The heading now reads "Exports with a name or library/NID already present in main", which resolves my earlier wording nit. Approving.
AI-assisted: yes
|
@xdCloudy the same-library/NID finding and its regression are restored, and the cross-library warning now clearly says it may be intentional. Those requests are addressed. One new false-negative remains in the move suppression at
The A entry is correctly treated as a rewrite, but the B entry also loses its warning because Please suppress only libraries where the canonical export is actually removed after accounting for the PR's retained/re-added names. Add a regression combining the rewrite in A with the addition in B and require the cross-library finding. Keep the separate rewrite-only, genuine-move and same-key tests. |
|
@oneandonlydean Thanks for catching this! Addressed in commit The move-suppression logic now accounts for canonical exports retained or re-added in their original library.
Added The new regression fails against the previous implementation and passes with the fix. All 20 export unit tests pass on Python 3.12. The full GitHub Actions checks are still pending. AI-assisted: yes |
|
Windows run of this head
AI-assisted: yes (Claude Code) |
|
The same-key finding and the combined rewrite-in-A/add-in-B regression are fixed. The focused export suite passes at this head (20 tests) and with the current main tool/test source composed (22 tests). One integration update remains: head Please rebase onto current main and preserve its conventions-check guidance together with this PR's changes, then rerun the relevant checks on the new head. The earlier branch-only results do not cover that new integration. |
60a80d1 to
898fe32
Compare
|
@oneandonlydean The branch is rebased onto current Checks on the rebased source:
The updated branch is pushed; the new head is |
|
The integration fix is present at |
What
Add a preflight warning when a PR introduces an export whose canonical name is already exported by another PRX library in
main.The check distinguishes two cases:
(library, NID)inmain.main, even if the NIDs or library paths differ.Cross-library findings are advisory because some duplicated exports are intentional. Known examples include
libSceAgc/libSceAgcVshandlibSceSaveData/libSceSaveData.native. The warning explicitly states that these matches may be intentional.False-positive and false-negative prevention
The implementation accounts for additions and removals within the same PR:
(library, NID)does not trigger a false same-key duplicate warning.The latest change in
60a80d1fixes the remaining false negative identified by @oneandonlydean.Previously, the move-suppression logic included every removed definition, even if the same canonical export was retained or re-added in its original library. This could incorrectly suppress a cross-library warning.
The updated logic removes retained libraries from the move-suppression set before evaluating cross-library duplicates.
Existing same-key detection, genuine-move suppression and advisory cross-library reporting remain intact.
Overlap with #9539
I also reviewed the overlap bot's warning concerning #9539.
That PR modifies the
probe/scan_allarea oftools/pr_overlap.py, whereas this PR modifiesmain_export_findings, reporting logic and associated tests.The overlap bot cannot complete its comparison because #9539 conflicts with the current base. Its warning remains a manual-review notice rather than confirmation of a merge conflict between these changes.
Tested
Windows x86_64, Python 3.11.2, head
898fe3211ead:python -m unittest discover -s tools/tests -p 'test_pr_overlap*.py' -q: 127 tests run: 122 passed, 5 skipped.test_pr_overlap_exports.py: 22 tests passed.python tools/check_conventions.py --base upstream/main: passed.git diff --check upstream/main...HEAD: passed.Hosted relinker jobs for this head passed on Linux, Windows and both macOS architectures; relinker ASan passed. Full runtime builds are not required for this tools-only change and were skipped.
Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)