Skip to content

fix(tools): detect NID alias overlaps between exports - #8961

Merged
oneandonlydean merged 2 commits into
boykopovar:mainfrom
xdCloudy:fix/pr-overlap-nid-postfix-aliases
Oct 21, 2026
Merged

oneandonlydean merged 2 commits into
boykopovar:mainfrom
xdCloudy:fix/pr-overlap-nid-postfix-aliases

Conversation

@xdCloudy

@xdCloudy xdCloudy commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

What

Normalize _nid_postfix and _nid_disambigN export names in PR overlap comparisons so aliases that resolve to the same NID are detected.

Tested

Windows local Python: python -m unittest tools.tests.test_pr_overlap_exports tools.tests.test_pr_overlap_wrapped_exports passed (10 tests). git diff --check passed. The normalized export check covered 813 open PRs; 3 unrelated diffs could not be retrieved.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw; silent stubs are documented (not applicable)
  • No notes, investigation .md files or images added to the repository
  • New third-party code is a submodule built from source (not applicable)
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Fixes: none
  • Unblocks: none
  • GPUs tested: n/a
  • AI-assisted: yes

@MarcZX10

Copy link
Copy Markdown
Contributor

The current Conventions job fails for two fixable reasons: the three-line explanatory comment above the canonicalization in tools/pr_overlap.py violates the repository rule that code comments belong only to technical debt, and the PR body is missing the required ### What, ### Tested, and ### Checklist sections. The code change itself is focused; moving the rationale into the PR description and filling the template should clear these findings. relinker-asan and the Ubuntu relinker job pass; macOS/Windows relinker jobs are still pending.

@xdCloudy

Copy link
Copy Markdown
Contributor Author

I found a gap in my overlap checker: it compared export names literally and did not normalize _nid_postfix / _nid_disambigN aliases, which can resolve to the same NID as a plain name. I updated the checker to compare normalized names and checked this PR against current main and the available open PR diffs; no remaining normalized-name overlap turned up for this PR.

@MarcZX10

Copy link
Copy Markdown
Contributor

I independently ran the focused tests from exact head 3a1acb576b: python3 -m unittest tools.tests.test_pr_overlap_exports tools.tests.test_pr_overlap_wrapped_exports passes (10 tests). The remaining red check is the conventions/template failure above; I used an isolated temporary worktree and removed it afterward.

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The author removed the disallowed implementation comment and completed the required PR sections; the latest Conventions check passes. I independently ran the focused overlap tests on the previous head and all 10 passed; the implementation changes since then are documentation/comment cleanup. The cross-platform relinker jobs are still pending, so this approval reflects the focused code and convention review, not those unfinished jobs.

@MarcZX10

Copy link
Copy Markdown
Contributor

Thanks for fixing the template and comment issue. The current Conventions check is green. My 10-test focused run was on the preceding code-equivalent head; the remaining platform jobs are still running.

@oneandonlydean

Copy link
Copy Markdown
Collaborator

@xdCloudy thanks, the alias check reads right and its normalisation matches tools/nid_names.py. A non-blocking idea for a follow-up, not this PR: pr_overlap.py compares open pull requests with each other, but the duplicates we have been finding in the stub PRs are against main itself, where a plain-name stub exports the same NID as an existing _nid_postfix implementation. A check of each PR's new exports against main's (including APS5_ASM_FUNCTION and raw APS5_EXPORT NIDs) would catch those automatically. Full CI on this head is next.

@xdCloudy

Copy link
Copy Markdown
Contributor Author

Thanks, agreed. This PR stays focused on normalizing aliases between open PRs. I separately checked this batch against current main, including APS5_VABI definitions, APS5_ASM_FUNCTION exports, and named APS5_EXPORT aliases; no matches turned up. An automated PR-versus-main export inventory check would be a useful separate follow-up.

@github-actions

Copy link
Copy Markdown

Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.

Pull request Shared files not compared
PR 988 tools/pr_overlap.py

@xdCloudy

Copy link
Copy Markdown
Contributor Author

I manually reviewed #988's change to tools/pr_overlap.py. It adds end-of-run summary logging near the script entry point; this PR changes export-name normalization in exports(). The edits are independent, and a direct three-way merge check found no conflict in tools/pr_overlap.py. The bot warning reflects #988's conflict with its base branch, so this file should combine cleanly after the branches are rebased.

@THEMANnt THEMANnt left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at 39b0c5cb4, with both commits cherry-picked onto main 44e3389c8 on Windows.

  • Full test_pr_overlap*.py suite: 61 passed, 4 platform-specific skips (65 run).
  • Compared 36 alias/suffix cases against the production C++ Nid::Internal::StripNidPostfix, compiled with GCC 15.2; all matched.
  • Three overlaps() probes reproduce missed collisions on main and detect them with this change. Distinct names and no-patch helpers remain excluded.
  • Conventions and whitespace checks pass.

No actionable findings. Linux-only cases and a live exhaustive PR audit were not run.

AI-assisted: yes (Codex); results above were run locally.

@oneandonlydean
oneandonlydean merged commit f52cb13 into boykopovar:main Oct 21, 2026
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants