Skip to content

fix(conventions): ignore non-code duplicate export matches - #5780

Merged
oneandonlydean merged 1 commit into
boykopovar:mainfrom
MrMha:fix/conventions-duplicate-source-tokens
Oct 18, 2026
Merged

oneandonlydean merged 1 commit into
boykopovar:mainfrom
MrMha:fix/conventions-duplicate-source-tokens

Conversation

@MrMha

@MrMha MrMha commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

What

The duplicate-export checker can match an APS5_VABI declaration inside a comment or string and report a real export as a duplicate. Mask non-code tokens before matching each source line and inspecting its function body. Keep line numbers intact, including Unicode separators, and cache the masked source per file.

The branch is based on main 47a266cf975f00f7fba94013467d03bfa5e355e9. The masking is applied inside main's existing hits loop after its batched self.read(...); the batch-read behavior is retained.

Tested

macOS 27.0.1 arm64, Python 3.12.14: all 109 tests in the convention suites pass, including batched-read coverage and all 10 wrapped-declaration cases. The 10 wrapped-declaration cases also pass on Python 3.9.6.

Negative control: with unchanged main's checker and this branch's tests, 5 wrapped-declaration cases fail and 5 pass. The failures reproduce the comment, raw-string and source-line bugs that this change fixes. The trusted main checker accepts the committed patch, title and template; whitespace checks pass. No runtime or GPU behavior changes.

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 where required
  • No notes, investigation .md files or images added to the repository
  • New third-party code is a submodule built from source (none added)
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Unblocks: none
  • GPUs tested: n/a
  • AI-assisted: yes (OpenAI Codex)

@adriadam10 adriadam10 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The approach looks right: cpp_text keeps newlines, so line numbers still match git grep, and the new cases in tools/tests/test_check_conventions_wrapped_declarations.py pass on this head (9 tests).

One blocker: the PR conflicts with main in tools/check_conventions.py. duplicates() on main now reads blobs in one batch (self.read(...) and hits), so the masking has to be re-applied on top of that loop. I can't push a rebase to your branch (maintainer edits are disabled), so please rebase and re-run the five convention suites, plus the negative control against main's checker.

@MrMha
MrMha force-pushed the fix/conventions-duplicate-source-tokens branch from 1421354 to 89db679 Compare October 15, 2026 13:20
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 15, 2026
@MrMha

MrMha commented Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

@adriadam10 Rebased onto main 2e5dd5aa144af1bf90a214a6341bf3e1b4e6d47a as 89db679b980ae372eb3d4ce2327f4baa1e281f8b. The fix retains main's hits collection and self.read(...) batch, then masks/caches each source within that loop.

Fresh macOS 27.0.1 arm64 / Python 3.12.14 checks:

  • ctest --test-dir <relinker-only-build> -R '^conventions_' --output-on-failure --parallel 2: 14/14 registrations pass, covering all current convention suites, including batch-read process counts and the 9 wrapped-declaration cases.
  • Negative control with this head's tests and unchanged main's checker: 5 failures / 4 passes in the wrapped-declaration suite. The five failures reproduce the original comment/raw-string/Unicode-separator bugs.
  • Trusted main checker and branch checker accept the committed change and revised PR template; whitespace checks pass.

The description now records the fresh results. Manually reviewed the bot's remaining shared checker patches: #6078's environment rule and #6268's Unknown-NID rule do not implement this masking; no combined-tree test is claimed. #5766 is already merged in the new base.

AI-assisted: yes (OpenAI Codex).

@MrMha
MrMha requested a review from adriadam10 October 15, 2026 13:24
@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 6447 tools/check_conventions.py

@MrMha
MrMha force-pushed the fix/conventions-duplicate-source-tokens branch from 89db679 to 26c3a82 Compare October 17, 2026 14:08
@oneandonlydean

Copy link
Copy Markdown
Collaborator

Picking this up while @adriadam10 is away: we will merge it once full CI and an independent review pass on this head.

@oneandonlydean oneandonlydean self-assigned this Oct 18, 2026
@oneandonlydean
oneandonlydean dismissed adriadam10’s stale review October 18, 2026 02:57

Stale: the requested change is in place on 26c3a82 (checked while @adriadam10 is away).

@oneandonlydean
oneandonlydean merged commit fdcc9f7 into boykopovar:main Oct 18, 2026
10 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.

3 participants