Skip to content

feat(cpp): Enable C++ ADL for class pointer arguments and exclude function pointers#1592

Merged
magyargergo merged 8 commits into
mainfrom
copilot/fix-pointers-adl-namespace
May 14, 2026
Merged

feat(cpp): Enable C++ ADL for class pointer arguments and exclude function pointers#1592
magyargergo merged 8 commits into
mainfrom
copilot/fix-pointers-adl-namespace

Conversation

Copilot AI commented May 14, 2026

Copy link
Copy Markdown
Contributor

C++ ADL was skipping pointer-typed class arguments entirely, so calls like record(p) where p is N::S* or N::S** failed to associate N. This change extends the current ADL capture/resolution path to unwrap class pointers while keeping function-pointer arguments out of ADL.

  • ADL candidate selection

    • allow pointer-typed class args to contribute their pointee class namespace
    • keep reference args excluded
    • preserve existing lookup/narrowing behavior once associated namespaces are collected
  • Capture-time type classification

    • treat any pointer_declarator chain as pointer participation for ADL (N::S*, N::S**, etc.)
    • reject function-pointer declarators early so void (*)() does not surface a false associated class from the return type
  • Fixture coverage

    • flip the existing pointer boundary fixture from negative to positive
    • add a negative fixture for void (*g)()
    • add a positive fixture for N::S**
  • Behavioral example

    namespace audit {
      struct Event {};
      void record(Event**);
    }
    
    namespace app {
      void run() {
        audit::Event** pp;
        record(pp);  // ADL now associates namespace audit
      }
    }

@vercel

vercel Bot commented May 14, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
gitnexus Ready Ready Preview, Comment May 14, 2026 4:27pm

Request Review

Copilot AI changed the title [WIP] Fix pointer-type handling in ADL resolution Enable C++ ADL for class pointer arguments and exclude function pointers May 14, 2026
Copilot AI requested a review from magyargergo May 14, 2026 14:27
@magyargergo magyargergo marked this pull request as ready for review May 14, 2026 15:19
@magyargergo magyargergo changed the title Enable C++ ADL for class pointer arguments and exclude function pointers feat(cpp): Enable C++ ADL for class pointer arguments and exclude function pointers May 14, 2026
@github-actions

github-actions Bot commented May 14, 2026

Copy link
Copy Markdown
Contributor

Claude finished @magyargergo's task in 6m 56s —— View job


Production Readiness Review — PR #1592

Tasks

  • Post initial comment
  • Read repo context files (AGENTS.md, GUARDRAILS.md, DoD.md, TESTING.md — all present; CONTRIBUTING.md, ARCHITECTURE.md skipped as not impacted by this change)
  • Inspect changed files (adl.ts, captures.ts, cpp.test.ts, fixtures)
  • Read related files (pointer-arg-boundary fixture, query path, overload-narrowing)
  • Run git diff analysis and Unicode hygiene checks
  • Check CI status
  • Compile and post final review

Review bar for this PR

  • N::S* and N::S** pointer-class arguments must associate the pointee class's namespace, not just value-class arguments.
  • Function pointer arguments (void (*g)(), N::S (*factory)()) must produce zero associated namespaces — no new CALLS edges.
  • The function-pointer guard must not accidentally suppress ADL for co-resident class-typed variables in the same scope.
  • findCppClassDefBySimpleName is workspace-wide first-match; no incorrect namespace must be associated for pointer-typed args beyond the pre-existing V1 limitation.
  • All existing overload-ambiguity, parenthesized-form, and reference-exclusion tests must continue to pass.
  • Changed integration tests must be wired into CI and actually run.
  • Branch diff must contain only intended ADL changes — no unrelated churn from merge commits.

Current PR state

  • Open, 6 commits, head copilot/fix-pointers-adl-namespacemain.
  • PR-specific commits verified: 1772928 (core logic), 5c703c0 (tighten function-pointer guard), 8ccdc18 (comment clarity).
  • Two merge-from-main commits: 0506b05, 008e7e8.
  • CI checks observed:
    • Dependency Review: ✅ success
    • Gitleaks: ✅ success
    • CodeQL: ✅ success
    • PR Autofix: ✅ success
    • Docker Build: 🔄 in_progress
    • CI (tests/typecheck/lint): ⏳ queued — not yet confirmed passing
    • PR Description Check: ⚠️ action_required (appears to be a label/template gate, not a test failure)

The main CI workflow (Vitest integration tests including cpp.test.ts) was still queued at review time. Integration test pass status cannot be confirmed.


Branch hygiene assessment

Classification: merge-from-main commit present but harmless and merge-safe

The full diff (a79c256~1..HEAD) includes files from main-branch merges (AGENTS.md, hooks, ESM extension resolver, setup tests). However, those files belong entirely to the two merge commits. The three PR-specific commits touch exactly 7 files — all scoped to C++ ADL runtime logic, fixtures, and integration tests. No unrelated churn was introduced into the ADL files by the merge commits.


Understanding of the change

Capture-time path (captures.ts):

  1. emitCppScopeCaptures sees a @reference.call.free node and calls inferCppCallAdlArgs(callNode).
  2. inferCppCallAdlArgs iterates argument nodes; for each identifier-typed argument it calls classifyAdlArglookupAdlIdentifierType.
  3. lookupAdlIdentifierType walks up to the enclosing compound_statement, iterates declaration statements, and unwraps the declarator chain to detect pointer_declarator, reference_declarator, function_declarator, and init_declarator wrappers. V2 adds: when a pointer_declarator wraps a function_declarator, return EMPTY_ADL_ARG; otherwise, set isPointer = true and continue unwrapping.
  4. The extracted simpleClassName (from the type field), isPointer, and isReference are stored via markCppAdlSiteArgs.

Resolution-time path (adl.ts):
5. pickCppAdlCandidates retrieves the stored CppAdlArgInfo[] for the call site.
6. V2 change: if (arg.isReference) continue;isPointer is NO LONGER in the exclusion filter. Pointer-class args now contribute their simpleClassName to associated namespace lookup.
7. findCppClassDefBySimpleName finds the class def (first-match, V1 limitation). classToNamespaceQualifiedName maps the class def's nodeId to its enclosing namespace qualified name.
8. The associated namespace set drives candidate collection across all parsed files. narrowOverloadCandidates + isOverloadAmbiguousAfterNormalization apply. A single surviving candidate emits a CALLS edge.


Findings


Finding 1 — Early return EMPTY_ADL_ARG in function-pointer guard abandons the entire lookupAdlIdentifierType scope scan

Severity: HIGH

Evidence:
captures.ts:807-808 (added by commit 5c703c0):

if (inner.type === 'pointer_declarator') {
  if (findFirstDescendantOfType(inner, 'function_declarator') !== null) {
    return EMPTY_ADL_ARG;  // ← exits the ENTIRE function, not just this statement
  }

Same pattern at captures.ts:837-838:

if (inner.type === 'function_declarator') {
  return EMPTY_ADL_ARG;
}

The outer loop at captures.ts:783-848 iterates ALL declaration statements in the enclosing scope looking for the target variable. When either guard fires on any statement — regardless of whether that statement declares the target variable — the entire function returns immediately. The name-match check if (nameText !== varName) continue; at line 844 is never reached.

Concrete failing scenario (not covered by any existing fixture):

void run() {
  void (*fp)();          // declaration A — function pointer
  audit::Event e;        // declaration B — class-typed variable
  record(e);             // should resolve via ADL to audit::record
}

When lookupAdlIdentifierType is called for e (varName = 'e'):

  1. Iterates scope statements; hits declaration A (void (*fp)()) first.
  2. declarator = pointer_declarator; findFirstDescendantOfType finds function_declaratorreturn EMPTY_ADL_ARG.
  3. Declaration B is never scanned. e returns EMPTY_ADL_ARG. ADL silently fails.

The same issue occurs for audit::Event (*factory)() declared before a class-typed argument in the same scope.

Risk: Silent ADL miss (under-resolution) for any function scope where a function pointer variable precedes a class-typed variable. No wrong edges are created, but valid CALLS edges are not emitted.

Regression status: This is a regression introduced by this PR. Pre-PR, there was no function-pointer guard in lookupAdlIdentifierType — all pointer args were excluded at the pickCppAdlCandidates level via arg.isPointer || arg.isReference. The pre-PR code would correctly scan all declarations before returning.

Recommended fix: Inside the declarator-walking while loop, replace return EMPTY_ADL_ARG with a flag + break, so the outer for loop's continue is reached:

let isFunctionPointer = false;
while (safety-- > 0) {
  if (inner.type === 'pointer_declarator') {
    if (findFirstDescendantOfType(inner, 'function_declarator') !== null) {
      isFunctionPointer = true;
      break; // exit while; nameText stays null; outer `continue` fires
    }
    // ...
  }
  if (inner.type === 'function_declarator') {
    isFunctionPointer = true;
    break;
  }
  // ...
}
if (isFunctionPointer || nameText !== varName) continue; // outer for loop

Alternatively, add a fixture that places a function pointer declaration before a class-typed argument in the same scope and expects 1 CALLS edge.

Blocks merge: yes — this is an untested correctness regression that silently disables ADL for valid C++ patterns.

Fix this →


Finding 2 — Stale docstring on inferCppCallAdlArgs directly contradicts current behavior

Severity: MEDIUM

Evidence: captures.ts:722-731:

/**
 * Per-argument ADL classification: walk each argument of a free call and
 * decide whether it's a directly-named class type (V1 ADL fires) or
 * something V1 excludes (pointer, reference, primitive, literal, function
 * pointer, template specialization).
 *
 * V1 only fires for value class-typed args: `void f(N::S); N::S s; f(s);`.
 * Pointer args (`N::S* p; f(p);`) intentionally return `simpleClassName=''`
 * to lock the V1 boundary — the `cpp-adl-pointer-arg-boundary` fixture
 * regression-tests this.
 */

This docstring says pointer args "intentionally return simpleClassName='' to lock the V1 boundary" and references the boundary fixture as regression-testing that exclusion. After this PR: (a) pointer args DO return a non-empty simpleClassName, (b) the pointer-arg-boundary fixture now ASSERTS 1 CALLS edge, not 0. The docstring is directly wrong on both counts.

Risk: A future contributor reading this comment will believe pointer args are excluded and may incorrectly revert or guard against them. The DoD.md section 2.3 explicitly prohibits stale comments.

Recommended fix: Update the docstring to describe the current behavior:

/**
 * Per-argument ADL classification: walk each argument of a free call and
 * decide whether it's a directly-named class type or pointer-to-class type
 * (ADL fires) vs. something excluded: function-pointer, reference,
 * primitive, literal, or template specialization.
 *
 * Class-typed values and class pointers (`N::S`, `N::S*`, `N::S**`)
 * contribute the pointee class's enclosing namespace to associated
 * namespaces. Function pointers and references are excluded.
 */

Blocks merge: yes/maybe — per DoD.md §2.3, comments that contradict executable behavior are a "Not Done" signal.

Fix this →


Finding 3 — Function-pointer negative fixture tests only void (*)() — class-returning variant untested

Severity: MEDIUM

Evidence: cpp-adl-function-pointer-arg/app.cpp uses void (*g)(). The stated concern (PR description + review prompt) is whether audit::Event (*factory)() — a function pointer whose return type is a class — would incorrectly contribute ADL associated namespaces.

Implementation analysis: The guard is pointer_declarator → check for function_declarator descendant. For audit::Event (*factory)():

  • type field = audit::Event (qualified_identifier)
  • declarator = pointer_declarator → contains function_declarator
  • Guard fires → EMPTY_ADL_ARGimplementation is correct

However, there is no test that proves this. If Finding 1 is fixed (early return replaced with break/continue), audit::Event (*factory)() would still be correctly excluded. But without a fixture, a future refactor could silently break this.

Recommended fix: Add a fixture cpp-adl-function-pointer-class-return-arg with audit::Event (*factory)() as the argument; assert 0 CALLS edges. This also exercises Finding 1's fix: if factory is declared in a scope alongside a legitimate class-typed arg, the class-typed arg should still resolve.

Blocks merge: maybe — the implementation correctly handles this case, but test coverage for the issue's stated pitfall is incomplete.


Finding 4 — Duplicate simple-class-name collision: pre-existing V1 limitation expanded to pointer call sites without a boundary test

Severity: MEDIUM

Evidence: adl.ts:326-336findCppClassDefBySimpleName iterates all defs and returns the first match:

// V1 arbitrary-pick on collisions (multiple classes share the simple name);
// C++ ADL strictness would require full type-driven lookup
function findCppClassDefBySimpleName(simpleName, scopes): SymbolDefinition | undefined {
  for (const def of scopes.defs.byId.values()) {
    if (def.type !== 'Class' ...) continue;
    const simple = def.qualifiedName?.split('.').pop() ?? ...;
    if (simple === simpleName) return def;  // arbitrary first match
  }
}

When A::Event and B::Event both exist and the argument is B::Event*, the resolver may associate namespace A instead of B, emitting a CALLS edge to A::record rather than B::record.

Comparison to V1 value-arg path: This limitation pre-exists for value-class args (the cpp-adl-basic fixture uses single-namespace setups). This PR extends the same risk to pointer call sites without adding a multi-namespace collision test for the pointer path.

Risk: False positive CALLS edges for pointer-typed args when two namespaces contain identically-named classes.

Recommended fix: Add a fixture with two namespaces (A::Event, B::Event) and a pointer arg typed B::Event*; assert the edge targets namespace B. If the V1 limitation causes the wrong association, document it explicitly with a // known V1 limitation comment on the fixture.

Blocks merge: maybe — existing value-arg path has the same issue; pointer args don't introduce a new mechanism, only expand the blast radius. A documented limitation test is preferable to a silent gap.


Finding 5 — CppAdlArgInfo.isPointer is now documentation-only

Severity: LOW

isPointer is set correctly in lookupAdlIdentifierType but is never consulted in pickCppAdlCandidates after this PR (the arg.isPointer check was removed from the exclusion filter). The field's updated JSDoc is accurate: "True when the variable's declarator contained one or more pointer_declarator wrappers."

No action needed. Retaining it preserves declarator-shape information for future V3+ pointer work and documents what the unwrapper detected. lint/typecheck should accept an unused-for-dispatch field on an exported interface.

Blocks merge: no


PR-specific assessment sections

Runtime ADL semantics

The core change in adl.ts:181if (arg.isReference) continue; (was if (arg.isPointer || arg.isReference) continue;) — is correct and minimal. Pointer-class args now contribute associated namespaces; references remain excluded (consistent with the documented boundary). isPointer is retained for documentation. Overload narrowing and ambiguity suppression paths are unchanged.

Declarator-chain and function-pointer classification

The declarator unwrapping in lookupAdlIdentifierType correctly handles:

  • N::S* pisPointer=true, simpleClassName='Event'
  • N::S** ppisPointer=true (after two pointer_declarator unwraps), simpleClassName='Event'
  • void (*g)()EMPTY_ADL_ARG ✓ (returns before name check — see Finding 1)
  • N::S (*factory)()EMPTY_ADL_ARG ✓ (same guard)
  • N::S& risReference=true, excluded at ADL selection ✓

Unverified shapes (no fixture):

  • N::S *p, *q — second declarator never scanned (pre-existing childForFieldName('declarator') returns first only)
  • using Fn = void(*)(); Fn g; — type would be type_identifier (Fn), extractAdlSimpleTypeName returns 'Fn', a simple name that won't match any class def → resolves to EMPTY_ADL_ARG safely, but not tested
  • const N::S* pconst is a type qualifier; tree-sitter-cpp puts it in the type specifier, not the declarator; extractAdlSimpleTypeName would see qualified_identifier → correct

Namespace association / duplicate simple-name risk

See Finding 4. The V1 arbitrary-pick is a pre-existing limitation, clearly documented. No new mechanism was introduced. The pointer path uses exactly the same code path as value-class args.

Fixture and integration-test adequacy

  • cpp-adl-pointer-arg-boundary: flipped from .toBe(0) to .toBe(1) with targetFilePath assertion ✓
  • cpp-adl-pointer-to-pointer: new positive fixture, count + file assertion ✓
  • cpp-adl-function-pointer-arg: new negative fixture, count-only assertion ✓ (but fixture scope is too narrow — see Findings 1 and 3)

Missing tests:

  1. Function pointer + class-typed arg in same scope (the interaction that exposes Finding 1)
  2. Class-returning function pointer negative test
  3. Multi-namespace same-simple-name collision for pointer args

CI validation

Gitleaks, CodeQL, Dependency Review: all green. Docker: in_progress. The critical "CI" workflow (vitest with coverage, ubuntu + macOS + Windows) is queued and unconfirmed. Per TESTING.md, ci-tests.yml runs npm test in gitnexus/ which includes all integration tests. The new test descriptions are correctly named and structured. Once CI runs, the three new describe blocks should be picked up automatically.

Hidden Unicode / hygiene

All non-ASCII characters in the changed files are (U+2192 RIGHT ARROW) used as visible arrow notation in comments and test assertions ('Duck → Flyer'). These are semantically appropriate visible characters consistent with the rest of the test file. No bidirectional control characters (U+202A–U+202E, U+2066–U+2069) were found in any of the changed files. GitHub's "hidden Unicode text" warnings are false positives from the arrow notation.

Cross-domain impact

Only the C++ ADL runtime path, fixtures, and integration tests were changed by the PR-specific commits. The merge commits brought unrelated changes (hook skip behavior, ESM extension fallback) that do not interact with the ADL path. No web UI, schema, shared contracts, or cross-language pipeline code was touched.


Final verdict

not production-ready

Finding 1 is a correctness regression introduced by this PR: the return EMPTY_ADL_ARG inside the function-pointer guard in lookupAdlIdentifierType exits the entire function rather than continuing the declaration scan, silently disabling ADL for any class-typed argument that is declared after a function pointer in the same scope. This behavior is untested, would affect real C++ codebases, and represents a step backward from the pre-PR behavior (where all pointer args were uniformly excluded at the selection level). Finding 2 (stale docstring directly contradicting behavior) violates DoD.md §2.3 and would mislead future contributors about the actual boundary. Finding 3 (weak negative fixture scope) leaves the key pitfall — class-returning function pointer — formally untested. The main CI workflow is still queued and cannot confirm integration tests pass. These issues together mean the change cannot be considered done by the repo's own DoD bar.


| PR branch

@magyargergo

Copy link
Copy Markdown
Collaborator

Findings

Finding 1 — Early return EMPTY_ADL_ARG in function-pointer guard abandons the entire lookupAdlIdentifierType scope scan

Severity: HIGH

Evidence: captures.ts:807-808 (added by commit 5c703c0):

if (inner.type === 'pointer_declarator') {
  if (findFirstDescendantOfType(inner, 'function_declarator') !== null) {
    return EMPTY_ADL_ARG;  // ← exits the ENTIRE function, not just this statement
  }

Same pattern at captures.ts:837-838:

if (inner.type === 'function_declarator') {
  return EMPTY_ADL_ARG;
}

The outer loop at captures.ts:783-848 iterates ALL declaration statements in the enclosing scope looking for the target variable. When either guard fires on any statement — regardless of whether that statement declares the target variable — the entire function returns immediately. The name-match check if (nameText !== varName) continue; at line 844 is never reached.

Concrete failing scenario (not covered by any existing fixture):

void run() {
  void (*fp)();          // declaration A — function pointer
  audit::Event e;        // declaration B — class-typed variable
  record(e);             // should resolve via ADL to audit::record
}

When lookupAdlIdentifierType is called for e (varName = 'e'):

  1. Iterates scope statements; hits declaration A (void (*fp)()) first.
  2. declarator = pointer_declarator; findFirstDescendantOfType finds function_declaratorreturn EMPTY_ADL_ARG.
  3. Declaration B is never scanned. e returns EMPTY_ADL_ARG. ADL silently fails.

The same issue occurs for audit::Event (*factory)() declared before a class-typed argument in the same scope.

Risk: Silent ADL miss (under-resolution) for any function scope where a function pointer variable precedes a class-typed variable. No wrong edges are created, but valid CALLS edges are not emitted.

Regression status: This is a regression introduced by this PR. Pre-PR, there was no function-pointer guard in lookupAdlIdentifierType — all pointer args were excluded at the pickCppAdlCandidates level via arg.isPointer || arg.isReference. The pre-PR code would correctly scan all declarations before returning.

Recommended fix: Inside the declarator-walking while loop, replace return EMPTY_ADL_ARG with a flag + break, so the outer for loop's continue is reached:

let isFunctionPointer = false;
while (safety-- > 0) {
  if (inner.type === 'pointer_declarator') {
    if (findFirstDescendantOfType(inner, 'function_declarator') !== null) {
      isFunctionPointer = true;
      break; // exit while; nameText stays null; outer `continue` fires
    }
    // ...
  }
  if (inner.type === 'function_declarator') {
    isFunctionPointer = true;
    break;
  }
  // ...
}
if (isFunctionPointer || nameText !== varName) continue; // outer for loop

Alternatively, add a fixture that places a function pointer declaration before a class-typed argument in the same scope and expects 1 CALLS edge.

Blocks merge: yes — this is an untested correctness regression that silently disables ADL for valid C++ patterns.

Fix this →

Finding 2 — Stale docstring on inferCppCallAdlArgs directly contradicts current behavior

Severity: MEDIUM

Evidence: captures.ts:722-731:

/**
 * Per-argument ADL classification: walk each argument of a free call and
 * decide whether it's a directly-named class type (V1 ADL fires) or
 * something V1 excludes (pointer, reference, primitive, literal, function
 * pointer, template specialization).
 *
 * V1 only fires for value class-typed args: `void f(N::S); N::S s; f(s);`.
 * Pointer args (`N::S* p; f(p);`) intentionally return `simpleClassName=''`
 * to lock the V1 boundary — the `cpp-adl-pointer-arg-boundary` fixture
 * regression-tests this.
 */

This docstring says pointer args "intentionally return simpleClassName='' to lock the V1 boundary" and references the boundary fixture as regression-testing that exclusion. After this PR: (a) pointer args DO return a non-empty simpleClassName, (b) the pointer-arg-boundary fixture now ASSERTS 1 CALLS edge, not 0. The docstring is directly wrong on both counts.

Risk: A future contributor reading this comment will believe pointer args are excluded and may incorrectly revert or guard against them. The DoD.md section 2.3 explicitly prohibits stale comments.

Recommended fix: Update the docstring to describe the current behavior:

/**
 * Per-argument ADL classification: walk each argument of a free call and
 * decide whether it's a directly-named class type or pointer-to-class type
 * (ADL fires) vs. something excluded: function-pointer, reference,
 * primitive, literal, or template specialization.
 *
 * Class-typed values and class pointers (`N::S`, `N::S*`, `N::S**`)
 * contribute the pointee class's enclosing namespace to associated
 * namespaces. Function pointers and references are excluded.
 */

Blocks merge: yes/maybe — per DoD.md §2.3, comments that contradict executable behavior are a "Not Done" signal.

Fix this →

Finding 3 — Function-pointer negative fixture tests only void (*)() — class-returning variant untested

Severity: MEDIUM

Evidence: cpp-adl-function-pointer-arg/app.cpp uses void (*g)(). The stated concern (PR description + review prompt) is whether audit::Event (*factory)() — a function pointer whose return type is a class — would incorrectly contribute ADL associated namespaces.

Implementation analysis: The guard is pointer_declarator → check for function_declarator descendant. For audit::Event (*factory)():

  • type field = audit::Event (qualified_identifier)
  • declarator = pointer_declarator → contains function_declarator
  • Guard fires → EMPTY_ADL_ARGimplementation is correct

However, there is no test that proves this. If Finding 1 is fixed (early return replaced with break/continue), audit::Event (*factory)() would still be correctly excluded. But without a fixture, a future refactor could silently break this.

Recommended fix: Add a fixture cpp-adl-function-pointer-class-return-arg with audit::Event (*factory)() as the argument; assert 0 CALLS edges. This also exercises Finding 1's fix: if factory is declared in a scope alongside a legitimate class-typed arg, the class-typed arg should still resolve.

Blocks merge: maybe — the implementation correctly handles this case, but test coverage for the issue's stated pitfall is incomplete.

Finding 4 — Duplicate simple-class-name collision: pre-existing V1 limitation expanded to pointer call sites without a boundary test

Severity: MEDIUM

Evidence: adl.ts:326-336findCppClassDefBySimpleName iterates all defs and returns the first match:

// V1 arbitrary-pick on collisions (multiple classes share the simple name);
// C++ ADL strictness would require full type-driven lookup
function findCppClassDefBySimpleName(simpleName, scopes): SymbolDefinition | undefined {
  for (const def of scopes.defs.byId.values()) {
    if (def.type !== 'Class' ...) continue;
    const simple = def.qualifiedName?.split('.').pop() ?? ...;
    if (simple === simpleName) return def;  // arbitrary first match
  }
}

When A::Event and B::Event both exist and the argument is B::Event*, the resolver may associate namespace A instead of B, emitting a CALLS edge to A::record rather than B::record.

Comparison to V1 value-arg path: This limitation pre-exists for value-class args (the cpp-adl-basic fixture uses single-namespace setups). This PR extends the same risk to pointer call sites without adding a multi-namespace collision test for the pointer path.

Risk: False positive CALLS edges for pointer-typed args when two namespaces contain identically-named classes.

Recommended fix: Add a fixture with two namespaces (A::Event, B::Event) and a pointer arg typed B::Event*; assert the edge targets namespace B. If the V1 limitation causes the wrong association, document it explicitly with a // known V1 limitation comment on the fixture.

Blocks merge: maybe — existing value-arg path has the same issue; pointer args don't introduce a new mechanism, only expand the blast radius. A documented limitation test is preferable to a silent gap.

Finding 5 — CppAdlArgInfo.isPointer is now documentation-only

Severity: LOW

isPointer is set correctly in lookupAdlIdentifierType but is never consulted in pickCppAdlCandidates after this PR (the arg.isPointer check was removed from the exclusion filter). The field's updated JSDoc is accurate: "True when the variable's declarator contained one or more pointer_declarator wrappers."

No action needed. Retaining it preserves declarator-shape information for future V3+ pointer work and documents what the unwrapper detected. lint/typecheck should accept an unused-for-dispatch field on an exported interface.

Blocks merge: no

@copilot address these findings and act as a senior c++ static analysis tool engineer and iso c++ compiler expert and follow DoD.md

Copilot AI commented May 14, 2026

Copy link
Copy Markdown
Contributor Author

Findings

Finding 1 — Early return EMPTY_ADL_ARG in function-pointer guard abandons the entire lookupAdlIdentifierType scope scan

Severity: HIGH
...

Addressed in 1897a2f. The function-pointer guard now breaks out of the declarator walk instead of returning from the whole scope scan, so earlier void (*)() declarations no longer suppress ADL for later class-typed args. I also updated the stale inferCppCallAdlArgs docstring and added regression coverage for both the mixed-scope case and audit::Event (*)() staying excluded. I left the simple-name collision behavior unchanged here because it is the pre-existing findCppClassDefBySimpleName limitation, not a regression from this fix.

@github-actions

github-actions Bot commented May 14, 2026

Copy link
Copy Markdown
Contributor

CI Report

All checks passed

Pipeline Status

Stage Status Details
✅ Typecheck success tsc --noEmit
✅ Tests success unit tests, 3 platforms
✅ E2E success gitnexus-web changes only

Test Results

Tests Passed Failed Skipped Duration
9035 9034 0 1 427s

✅ All 9034 tests passed

1 test(s) skipped — expand for details
  • buildTypeEnv > known limitations (documented skip tests) > Ruby block parameter: users.each { |user| } — closure param inference, different feature

Code Coverage

Tests

Metric Coverage Covered Base Delta Status
Statements 78.18% 28434/36368 N/A% 🟢 ███████████████░░░░░
Branches 66.55% 18024/27082 N/A% 🟢 █████████████░░░░░░░
Functions 83.02% 2857/3441 N/A% 🟢 ████████████████░░░░
Lines 81.47% 25682/31522 N/A% 🟢 ████████████████░░░░

📋 View full run · Generated by CI

@magyargergo magyargergo merged commit c219331 into main May 14, 2026
31 checks passed
@magyargergo magyargergo deleted the copilot/fix-pointers-adl-namespace branch May 14, 2026 16:56
hohaivu pushed a commit to hohaivu/GitNexus that referenced this pull request May 19, 2026
…ction pointers (abhigyanpatwari#1592)

* Initial plan

* fix: unwrap cpp adl pointer argument types

Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/2e9c8549-e062-410c-9ce3-66ba0a181590

* chore: tighten cpp adl function-pointer guard

Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/2e9c8549-e062-410c-9ce3-66ba0a181590

* docs: clarify cpp adl implementation comments

Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/2e9c8549-e062-410c-9ce3-66ba0a181590

* fix: avoid aborting cpp adl declaration scan

Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/e54f1d4b-9aac-407c-9b5e-b5f3ea0534ea

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Gergő Magyar <gergomagyar@icloud.com>
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.

cpp ADL V2: pointer-type unwrapping

2 participants