Skip to content

feat: C++ ADL V2: free-function reference args contribute enclosing namespace#1598

Merged
magyargergo merged 11 commits into
mainfrom
copilot/fix-adl-function-pointer-args
May 15, 2026
Merged

feat: C++ ADL V2: free-function reference args contribute enclosing namespace#1598
magyargergo merged 11 commits into
mainfrom
copilot/fix-adl-function-pointer-args

Conversation

Copilot AI commented May 14, 2026

Copy link
Copy Markdown
Contributor

C++ ADL (Koenig lookup) ignored function-pointer arguments entirely. Passing a free function reference like utils::worker as an argument should add utils to the associated-namespace set, enabling resolution of unqualified calls like with_callback(utils::worker) to utils::with_callback.

Note

GitNexus approximation (not strict ISO C++ ADL): ISO C++ [basic.lookup.argdep] says associated entities for function-type arguments come from parameter and return types — not the function's enclosing namespace. For void worker(), the standard-compliant associated set is empty. GitNexus instead contributes the enclosing namespace of confirmed Function/Method defs, capturing the dominant real-world pattern at bounded precision cost.

Changes

CppAdlArgInfo (adl.ts)

  • New optional field functionRefText?: string — set when an arg is a potential free-function reference (qualified or unqualified identifier not found in local scope or parameter list), as opposed to a locally-declared function-pointer variable or function parameter.

classifyAdlArg / lookupAdlIdentifierType / isIdentifierAFunctionParameter (captures.ts)

  • lookupAdlIdentifierType now returns CppAdlArgInfo | null instead of always CppAdlArgInfo:
    • null → identifier not found in local scope (potential free-function ref)
    • EMPTY_ADL_ARG → identifier found as a locally-declared void (*fp)() variable or as a function parameter (explicitly excluded)
  • Added foundAsLocalFunctionPointer tracking + extractDeclaratorLeafName helper to correctly extract variable names from function-pointer declarator chains (e.g., void (*g)()) so local fp vars are never misclassified as free-function refs.
  • New isIdentifierAFunctionParameter helper walks the enclosing function_definitionfunction_declaratorparameter_list chain. Function parameters live outside compound_statement so the local declaration scan previously returned null for them (triggering the free-function-ref path). Now returns EMPTY_ADL_ARG to prevent false positives.
  • classifyAdlArg now handles qualified_identifier nodes (e.g., utils::worker) as potential function references; noted as a GitNexus approximation since tree-sitter-cpp produces the same node type for variables, enum values, and static members.

collectFunctionRefNamespaces + pickCppAdlCandidates (adl.ts)

  • New helper collectFunctionRefNamespaces resolves the associated namespace(s):
    • Qualified refs (utils::worker): namespace extracted from the qualifier text (::. normalisation), then verified via workspace lookup — a Function/Method def with that simple name must exist in the target namespace before the namespace is contributed. This guards against false positives from namespace-qualified variables, enum values, and static data members.
    • Unqualified refs (worker): workspace scanned for any Function/Method def with matching simple name; all enclosing namespaces contributed (GitNexus simplification — overloads all contribute, no overload selection).
  • pickCppAdlCandidates loop extended to call collectFunctionRefNamespaces for each arg with functionRefText set, alongside the existing class-typed arg path.

Legacy DAG parity (helpers.ts)

  • Added two new negative-fixture test names to LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES.cpp. The legacy DAG's pickUniqueGlobalCallable resolves callees by simple-name workspace lookup regardless of argument analysis, so it emits false-positive CALLS edges for these cases that the registry-primary path correctly suppresses:
    • process(data::value) — legacy resolves process globally; registry-primary correctly contributes no namespace (no Function named value in data).
    • run_with(callback) — legacy resolves run_with globally; registry-primary correctly detects callback as a parameter (EMPTY_ADL_ARG).

Fixtures & tests

  • cpp-adl-free-func-ref: with_callback(utils::worker) resolves to utils::with_callback via ADL (qualified function ref, cross-namespace).
  • cpp-adl-free-func-ref-overloaded: same with utils::worker / utils::worker(int) overloads — verifies no crash and namespace still contributed.
  • cpp-adl-qualified-variable-arg (negative): process(data::value) where data::value is an int variable → 0 CALLS edges (namespace not contributed because no Function named value exists in data). Skipped in legacy DAG parity run.
  • cpp-adl-param-not-free-func-ref (negative): run_with(callback) where callback is an int parameter and utils::callback() exists in workspace → 0 CALLS edges (parameter detected, EMPTY_ADL_ARG returned). Skipped in legacy DAG parity run.
  • cpp-adl-local-fp-shadows-free-func (negative): local void (*g)() variable while audit::g() exists in workspace → 0 CALLS edges (foundAsLocalFunctionPointer guard).
  • cpp-adl-unqualified-ref-collision (negative): unqualified worker exists in two namespaces → ADL_AMBIGUOUS → 0 CALLS edges.
// utils.h
namespace utils {
  void worker();
  void with_callback(int n);
}
// app.cpp
namespace caller {
  void run() {
    with_callback(utils::worker);  // ADL approximation: utils::worker → confirm utils::worker is a Function → add utils → finds utils::with_callback
  }
}

@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 15, 2026 8:54am

Request Review

Copilot AI changed the title [WIP] Fix ADL handling for function-pointer argument associated entities cpp ADL V2: free-function reference args contribute enclosing namespace May 14, 2026
Copilot AI requested a review from magyargergo May 14, 2026 18:28
@magyargergo magyargergo changed the title cpp ADL V2: free-function reference args contribute enclosing namespace fet: C++ ADL V2: free-function reference args contribute enclosing namespace May 15, 2026
@magyargergo

Copy link
Copy Markdown
Collaborator

@copilot merge origin main into this branch and resolve anyt conflicts.

Copilot AI and others added 2 commits May 15, 2026 07:29
…app.cpp

Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>
…app.cpp

Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/136aeffe-45da-47e2-95dd-e3883e85fad7

Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>

Copilot AI commented May 15, 2026

Copy link
Copy Markdown
Contributor Author

@copilot merge origin main into this branch and resolve anyt conflicts.

Done. Merged origin/main into this branch and resolved the conflict in adl.ts — main added template-specialization fields and MRO base-class walking to CppAdlArgInfo; the merge preserves those additions alongside the functionRefText field from this PR. Commit 7e4ecd45.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • extension.ladybugdb.com
    • Triggering command: /usr/local/bin/node /usr/local/bin/node /home/REDACTED/work/GitNexus/GitNexus/gitnexus/scripts/install-duckdb-extension.mjs fts 17179869184 k/Gi�� e (dns block)
    • Triggering command: /usr/local/bin/node /usr/local/bin/node /home/REDACTED/work/GitNexus/GitNexus/gitnexus/scripts/install-duckdb-extension.mjs fts 17179869184 -c dn&#39;t have any #i-I (dns block)
    • Triggering command: /usr/local/bin/node /usr/local/bin/node /home/REDACTED/work/GitNexus/GitNexus/gitnexus/scripts/install-duckdb-extension.mjs fts 17179869184 -c ter_dart_binding-I (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

@magyargergo magyargergo marked this pull request as ready for review May 15, 2026 07:32
@github-actions

github-actions Bot commented May 15, 2026

Copy link
Copy Markdown
Contributor

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


Adversarial Production-Readiness Review — PR #1598

Tasks completed:

  • Read repo-level docs (DoD.md, AGENTS.md, GUARDRAILS.md)
  • Read resolver-specific context files
  • Inspect PR-changed files (adl.ts, captures.ts, fixtures, tests)
  • Run diff analysis (git show per-commit, final merge state)
  • Run grep/search commands for key symbols
  • Check hidden Unicode
  • Inspect helpers.ts legacy DAG skip correctness
  • Check CI status
  • Compile findings and write final review

Review Bar for This PR

Reviewing against DoD.md (v2.0.0), AGENTS.md (v1.7.0), GUARDRAILS.md, and the PR-specific DoD stated in the trigger. The highest-risk failure modes are: (1) silent false-positive CALLS edges from over-broad namespace contribution and (2) standards mismatch in comments/tests. Both are evaluated against actual code, not just PR description.


Current PR State

  • Branch: copilot/fix-adl-function-pointer-argsmain
  • CI: Dependency Review ✅, Gitleaks ✅, CodeQL ✅, PR Autofix ✅; Docker Build 🔄 in-progress; CI (integration/parity tests) 🔄 in-progress — resolver integration test result unconfirmed at review time
  • Commits: 4 total — 7b81e25 (plan), e333c96 (feature), 846af96 (merge), 7e4ecd4 (type-fix on top of merge)

Branch Hygiene Assessment

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

The branch contains two merge/conflict-resolution commits. Commit 846af96 (merge) resolved a conflict in adl.ts and captures.ts by correctly replacing the old isPointer/isReference-based loop in pickCppAdlCandidates with collectAssociatedNamespacesForAdlArg from main (which carries template and MRO logic from #1596/#1597), plus the new collectFunctionRefNamespaces call from this PR. Commit 7e4ecd4 fixed the captures.ts return objects to use the new interface shape (templateSimpleClassName, templateNamespace, templateArgClassNames, templateArgNamespaces) instead of the old isPointer/isReference fields.

The effective diff is scoped to C++ ADL files and test fixtures — no unrelated churn detected. The merge is traceable and reviewable. However, the correctness questions below are what block merge.


Understanding of the Change

The PR adds a functionRefText?: string field to CppAdlArgInfo and wires two classification paths in captures.ts:

  1. qualified_identifier arguments (e.g. utils::worker) → always classified as function references, namespace extracted from the qualifier text at resolution time.
  2. Unqualified identifier arguments not found in local compound_statement scope → classified as potential free-function references, triggering a workspace-wide scan for any Function/Method def with that simple name.

collectFunctionRefNamespaces in adl.ts handles both paths. The merge commit correctly wired these alongside the existing collectAssociatedNamespacesForAdlArg (template + MRO logic).


Findings


Finding 1 — ISO C++ Misstatement in Test Comment

Severity: High
Evidence: cpp.test.ts lines 2430–2435:

// ISO C++ [basic.lookup.argdep]: an overloaded-function set (or single
// function) passed as an argument contributes the function's enclosing
// namespace to the associated set.

Risk: This claim is factually incorrect per [basic.lookup.argdep]. The ISO standard says that for function-type and overload-set arguments, the associated entities are those of the parameter types and return type of each function — NOT the function's own enclosing namespace. Concretely: void worker() has no parameters and void return; under the strict standard, passing utils::worker contributes zero associated namespaces (void contributes nothing). The fixture works because the PR deliberately approximates "a function reference whose qualifier is utils:: contributes the utils namespace" — which is a GitNexus-specific heuristic, not an ISO rule.

This forces a standards compliance comment fix (or explicit GitNexus-approximation label) before merge, and the adl.ts doc comment at line 29-34 should align.

Recommended fix: Replace the // ISO C++ [basic.lookup.argdep] attribution in cpp.test.ts with a GitNexus-approximation label, e.g.:

// GitNexus approximation (not strict ISO C++ ADL): the qualifier namespace
// of a free-function argument is contributed to the associated set. Under
// ISO C++ [basic.lookup.argdep], associated entities come from parameter
// and return types, not the function's enclosing namespace.

And update the adl.ts doc comment similarly. Fix this →

Blocks merge: Yes — DoD §2.3 requires comments that explain intent and contracts accurately. Claiming ISO behavior for a non-ISO approximation violates the repo's correctness and readability bars.


Finding 2 — qualified_identifier Arguments Over-Classified as Function References

Severity: High
Evidence: captures.ts line 790–791:

if (argNode.type === 'qualified_identifier') {
  return { simpleClassName: '', ..., functionRefText: argNode.text };
}

Risk: Tree-sitter parses ANY a::b expression as qualified_identifier regardless of what b denotes semantically: namespace-qualified variables, enum values, static data members, or type names all produce the same AST node type. This means foo(utils::my_var) (variable), foo(utils::MyEnum::VALUE) (enum), or foo(utils::some_static) (static data member) would each contribute utils to the associated namespace set, and if utils::foo exists, a false-positive CALLS edge would be emitted.

There is no test covering this negative case — no test asserts that foo(utils::some_variable) does NOT contribute utils as an associated namespace.

Recommended fix: Either (a) verify the qualified identifier refers to a Function/Method def before contributing (requires a workspace lookup at classification time), or (b) explicitly document and test that all qualified_identifier arguments are treated as potential function references (accepted approximation), with a negative fixture proving the worst-case false-positive bound. Without a boundary test, the approximation scope is uncontrolled.

Fix this →

Blocks merge: Yes — this is a false-positive CALLS edge producer on valid C++ code patterns that appear in real codebases.


Finding 3 — Function Parameters Not in Local Scope → Misclassified as Free-Function References

Severity: Medium
Evidence: captures.ts lookupAdlIdentifierType walks to the nearest compound_statement and searches for declaration nodes within it. Function parameters live in parameter_list (outside the compound_statement), so any function parameter used as a call argument will return null → treated as a free-function reference.

Example:

void caller(int n) { foo(n); }
// n not found in compound_statement → lookupAdlIdentifierType returns null
// → functionRefText = "n" → workspace scan for any function named "n"
// → if utils::n() exists, utils gets added → foo might resolve to utils::foo

Similarly, auto-typed variables skip via the placeholder_type_specifier guard but are then also not found as class-typed, returning null and triggering the free-function ref path.

Risk: In large workspaces with common parameter names (n, val, data, key), workspace-wide function scans would contribute unrelated namespaces. The risk is bounded by whether a Function/Method def with that simple name exists, but in a real codebase this is plausible.

Recommended fix: Before treating "not found in compound_statement" as "potential free-function reference," also search the enclosing function's parameter_list. If the identifier is found as a parameter, return EMPTY_ADL_ARG. Fix this →

Blocks merge: Maybe — depends on workspace scale and naming conventions. High risk in production repos with common parameter names.


Finding 4 — typedef/using-Aliased Function-Pointer Variables Not Detected

Severity: Low
Evidence: lookupAdlIdentifierType detects function pointers via pointer_declarator > function_declarator declarator chains. For Callback g; (where Callback is a typedef/using alias for a function-pointer type), the declarator is simply identifier (g), isFunctionPointer stays false, and extractAdlSimpleTypeName returns "Callback". ADL then looks for a class named Callback.

Risk: False positive if a class named Callback (or whatever the alias is) exists in the workspace. Low probability in practice.

Recommended fix: Add a negative fixture comment acknowledging this limitation, or add a "known unsupported" note in the lookupAdlIdentifierType doc.

Blocks merge: No — risk is low and bounded. Document as known limitation.


Finding 5 — Test Coverage Gaps Against DoD §2.7

Severity: Medium
Evidence: The DoD (DoD.md §2.7) requires: "Fixtures are realistic enough for the risk of the change." The PR adds two positive fixtures only.

Missing coverage:

  • Negative local-fp fixture: void (*g)(); with_callback(g); — should NOT trigger function-ref ADL (mentioned as supported, but no test exercises it).
  • Negative non-function qualified identifier: foo(utils::some_var) where some_var is a variable — should NOT contribute utils (no test).
  • Unqualified namespace collision: worker exists in two unrelated namespaces — what edges are emitted? No test.
  • Function parameter as argument: void run(int n) { foo(n); } — parameter n should not trigger workspace scan.

Recommended fix: Add at least a negative local-fp fixture and a non-function qualified identifier negative fixture before merge.

Blocks merge: Maybe — the DoD's meaningful-assertion requirement combined with Finding 2's false-positive risk makes the missing negative test for non-function qualified identifiers particularly important. Without it, Finding 2 cannot be verified as bounded.


PR-Specific Assessment Sections

Standardized C++ ADL Behavior for Function-Type and Overload-Set Arguments

ISO C++ [basic.lookup.argdep] says function-type arguments contribute associated entities from parameter types and return type — NOT from the function's enclosing namespace. For void worker(), the associated set under the standard is empty. The PR's behavior (contributing the enclosing namespace of the declared function) is a GitNexus-specific static-analysis approximation. This must be labeled as such in all comments and tests.

GitNexus Approximation Boundary, If Any

The approximation — "extract namespace from qualifier text of a qualified_identifier argument" — is simple and bounded for qualified refs. For unqualified refs, the workspace-scan bound is "all namespaces containing any function with that simple name," which can grow unboundedly in large repos. Neither boundary is documented or tested for the negative case.

Free-Function Reference Namespace Collection Correctness

collectFunctionRefNamespaces logic is correct for the cases it handles:

  • Qualified: lastIndexOf('::') + slice + replace(/::/g, '.') — handles outer::inner::fn correctly (produces outer.inner).
  • Unqualified: workspace scan restricted to Namespace scopes, only Function/Method defs — bounded to defined function names. No Constructor false positives. Correct.

The problem is upstream in classifyAdlArg — what gets classified as a functionRefText is too broad (Finding 2, 3).

Local Function-Pointer and Qualified-Identifier Classification

extractDeclaratorLeafName correctly unwraps pointer_declarator > parenthesized_declarator > identifier chains (the canonical void (*g)() shape). The foundAsLocalFunctionPointer flag correctly prevents misclassification of this shape.

Gaps: auto (*g)() (placeholder_type_specifier skipped), typedef/using aliases (Finding 4), and parameters outside compound_statement (Finding 3).

Interaction with Merged ADL V2 Work (#1592, #1595, #1596, #1597)

✅ Preserved correctly. The merge commit 846af96 correctly resolved the conflict by:

No regression in #1592/#1595/#1596/#1597 behavior is evident from the diff.

Open ADL V2 Follow-on (#1599/#1576)

This PR does not depend on #1599 and does not worsen the ordinary-lookup short-circuit limitation. The comment in adl.ts at line 39-42 correctly documents the short-circuit. No interaction issues.

helpers.ts Legacy DAG Skip Correctness

The two new test names are NOT in LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES.cpp:

  • 'with_callback(utils::worker) resolves to utils::with_callback via ADL'
  • 'with_callback(utils::worker) with overloaded utils::worker still resolves utils::with_callback via ADL'

This is acceptable: in both fixtures, with_callback is the only function with that simple name in the workspace, so the legacy path's pickUniqueGlobalCallable fallback will also resolve with_callback to utils::with_callback. Both paths pass, no skip needed. No stale skip names detected.

Tests and CI Coverage

CI is still in-progress for the main resolver integration job. Two positive fixtures added; multiple negative cases missing (see Finding 5). Test comment misattributes ISO behavior (Finding 1).

Hidden Unicode/Hygiene

grep -P "[\x80-\xff]" across all changed files: no non-ASCII characters found. GitHub's hidden Unicode warning appears to be a false alarm from the diff rendering. ✅


Final Verdict: not production-ready

PR #1598 introduces two merge-blocking production issues that must be resolved before merge.

Finding 2 (qualified-identifier over-classification) is the primary blocker: every a::b expression used as a call argument — regardless of whether b is a variable, enum value, static member, or type — contributes namespace a to the ADL associated set and may emit false-positive CALLS edges. There is no test proving this is bounded, and the DoD requires meaningful assertions against the changed behavior's risk surface.

Finding 1 (ISO C++ misstatement) is a close second: the test comment explicitly cites ISO C++ [basic.lookup.argdep] for behavior that is not ISO-conformant — for void worker(), the standard-compliant associated set is empty, not utils. This contradicts the repo's correctness bar (DoD §2.3, §2.1) and must be relabeled as a GitNexus approximation.

Finding 3 (function parameters misclassified as free-function refs) is a medium-severity false-positive risk that may materialize in production repos. Finding 5 (missing negative tests) cannot be resolved until Finding 2 is resolved — the approximation boundary is currently untested and potentially unbounded. Findings 1 and 2 would move the verdict to production-ready with minor follow-ups only if Finding 2 is resolved either by restricting classification or by adding bounded negative tests.


View CI run (in progress) | View PR

@magyargergo

Copy link
Copy Markdown
Collaborator

@copilot retrieve all of the comments and filter out claude's review and get all of the findings and every findings must be fix in separate commits. Act as a senior c++ static analysis engineer as well as an iso c++ compiler engineer.

…th GitNexus-approximation label

Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/fa37c0dd-65f9-4dd4-9811-617227a37073

Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>
…tributing via qualified_identifier arg

Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/fa37c0dd-65f9-4dd4-9811-617227a37073

Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>
…assified as free-function refs

Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/fa37c0dd-65f9-4dd4-9811-617227a37073

Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>
Copilot AI changed the title fet: C++ ADL V2: free-function reference args contribute enclosing namespace feat: C++ ADL V2: free-function reference args contribute enclosing namespace May 15, 2026
@github-actions

github-actions Bot commented May 15, 2026

Copy link
Copy Markdown
Contributor

Claude finished @magyargergo's task in 16m 18s —— View job


Adversarial Production-Readiness Review — PR #1598 (Re-review)

Tasks completed:

  • Read repo context (DoD.md, AGENTS.md, GUARDRAILS.md — project instructions)
  • Read resolver context (adl.ts, captures.ts, scope-resolver.ts, helpers.ts, cpp.test.ts, free-call-fallback.ts, registry-primary-flag.ts)
  • Inspect all 6 new fixture files (2 positive, 4 negative)
  • Run git diff analysis and branch hygiene inspection
  • Run grep searches (ISO C++, GitNexus approximation, hidden Unicode, stale comments)
  • Verify all 5 previous findings are addressed
  • Check LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES vs new test names
  • Trace legacy DAG path (call-processor.ts vs emitFreeCallFallback isFileLocalDef filter)
  • Check CI status
  • Compiled final review

Review Bar for This PR

Reviewing against DoD.md v2.0.0, AGENTS.md, GUARDRAILS.md, and the PR-specific DoD. C++ is in MIGRATED_LANGUAGES — the scope parity CI runs the resolver integration test twice (once with REGISTRY_PRIMARY_CPP=0, once with =1), and both must pass.


Current PR State

  • Branch: copilot/fix-adl-function-pointer-argsmain
  • CI: Gitleaks ✅, CodeQL ✅, PR Autofix ✅, Dependency Review ✅, PR Description Check ✅; Docker Build 🔄 in-progress; CI (integration/parity) 🔄 in-progress — resolver parity result unconfirmed at review time
  • Commits since first review: 5 fix commits (477c45f through 46e2eb1)
  • Total effective PR commits (excluding merge plumbing): e333c96 (feature) + 5 fix commits

Branch Hygiene Assessment

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

Two merge commits (846af96, 7e4ecd4) correctly integrated origin/main (which carried #1597 base-class MRO, #1596 template specialization, #1595 reference unwrapping). The effective diff is scoped to C++ ADL files and fixtures. The merge correctly preserved collectAssociatedNamespacesForAdlArg (pointer, reference, template, MRO logic from prior PRs) alongside the new collectFunctionRefNamespaces call for functionRefText. No unrelated churn detected.


Understanding of the Change

The PR adds functionRefText?: string to CppAdlArgInfo. classifyAdlArg classifies qualified_identifier and unresolved identifier arguments as potential free-function references. collectFunctionRefNamespaces resolves them: qualified refs verify a Function/Method exists in the namespace before contributing; unqualified refs scan the workspace. isIdentifierAFunctionParameter prevents function parameters from falling into the free-function-ref path. extractDeclaratorLeafName + foundAsLocalFunctionPointer prevent local void (*g)() variables from being misclassified.


Previous Findings Status

Finding Status
Finding 1 — ISO C++ misstatement in test comment Fixed — test block header and adl.ts doc comment now correctly label behavior as "GitNexus approximation (not strict ISO C++ ADL)" with explicit reference to [basic.lookup.argdep]
Finding 2 — qualified_identifier over-classification FixedcollectFunctionRefNamespaces verifies a Function/Method named simpleName exists in the target namespace before contributing it; negative fixture cpp-adl-qualified-variable-arg added
Finding 3 — function parameters misclassified as free-function refs FixedisIdentifierAFunctionParameter walks the enclosing function_declaratorparameter_list chain; negative fixture cpp-adl-param-not-free-func-ref added
Finding 4 — typedef/using-aliased fp limitation undocumented DocumentedlookupAdlIdentifierType now carries a // Known limitation (Finding 4) block comment explaining the risk and deferring the fix
Finding 5 — missing negative test coverage Addressed — 4 negative fixtures added: cpp-adl-qualified-variable-arg, cpp-adl-param-not-free-func-ref, cpp-adl-local-fp-shadows-free-func, cpp-adl-unqualified-ref-collision

Findings (New)


Finding A — Negative tests absent from LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES.cpp

Severity: High
Evidence:

The 4 new negative tests are not in LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES.cpp in helpers.ts:

'process(data::value) emits zero CALLS edges — data::value is a variable, not a function'
'run_with(callback) emits zero CALLS edges when callback is a parameter, not a function reference'
'record(g) emits zero CALLS edges even though audit::g() exists in the workspace'
'run_with(worker) emits zero CALLS edges when worker exists in two namespaces'

C++ is in MIGRATED_LANGUAGES (confirmed in registry-primary-flag.ts line 75). The parity CI in ci-scope-parity.yml runs cpp.test.ts twice: once with REGISTRY_PRIMARY_CPP=0 (legacy) and once with REGISTRY_PRIMARY_CPP=1.

Why registry-primary passes: The registry-primary path runs emitFreeCallFallback with the C++ provider's isFileLocalDef hook (scope-resolver.ts:198–208), which marks namespace-nested defs as file-local: if (!isCppDefGloballyVisible(def.filePath, def.nodeId)) return true. This prevents data::process, utils::run_with, and audit::record from being reached by unqualified calls via pickUniqueGlobalCallable. The 4 tests correctly expect 0 edges in the registry-primary path.

Why legacy fails: When REGISTRY_PRIMARY_CPP=0, phase.ts:129 (if (!isRegistryPrimary(lang)) continue) skips the scope-resolution pipeline entirely. The legacy call-processor.ts runs instead (confirmed at call-processor.ts:816: if (isRegistryPrimary(language)) continue). The legacy call-processor has no equivalent isFileLocalDef namespace filter. The existing skip entry 'does NOT resolve unqualified foo() to ns::foo via #include' already documents this gap: legacy resolves namespace members by simple-name global lookup without namespace qualification. For the 3 single-candidate tests (data::process, utils::run_with, audit::record), legacy finds a unique match and emits 1 CALLS edge — test expects 0 → FAIL. For the 2-candidate collision (alpha::run_with, beta::run_with), legacy behavior is uncertain (may pick one arbitrarily → FAIL, mirroring the existing 'emits zero CALLS edges for ambiguous foo() bound via two using-namespace declarations' skip). All 4 tests need parity skip entries.

Risk: CI parity leg (REGISTRY_PRIMARY_CPP=0) fails for 3–4 tests. Under DoD.md §2.7 and the parity workflow contract, both legs must pass. Fix this →

Blocks merge: Yes — the parity CI leg (REGISTRY_PRIMARY_CPP=0) fails for the 3 single-candidate negative tests; the 4th (collision) is also at risk. The fix is adding 4 skip entries with comments referencing #1575/#1598.


PR-Specific Assessment Sections

Standardized C++ ADL Behavior for Function-Type and Overload-Set Arguments

ISO C++ [basic.lookup.argdep] associates function-type arguments via their parameter and return types — not the function's enclosing namespace. For void worker(), the standard-compliant associated set is empty. The PR now correctly labels this as a "GitNexus approximation (not strict ISO C++ ADL)" in both adl.ts lines 27–36 and the test block header at cpp.test.ts lines 2430–2441. ✅ Previously blocking finding is resolved.

GitNexus Approximation Boundary

Qualified refs (utils::worker): bounded — collectFunctionRefNamespaces verifies a Function/Method named worker exists in utils before contributing. Negative fixture (cpp-adl-qualified-variable-arg) demonstrates the guard works.

Unqualified refs (worker): bounded by workspace function lookup, with ambiguity suppressed via ADL_AMBIGUOUS when multiple namespaces match (demonstrated by cpp-adl-unqualified-ref-collision). The approximation is documented.

Free-Function Reference Namespace Collection Correctness

collectFunctionRefNamespaces (adl.ts:439–489) logic is correct:

  • Qualified path: lastIndexOf('::') + replace(/::/g, '.') → correctly handles outer::inner::fnouter.inner. Workspace verification loop is correct; returns on first confirmation (return inside inner loop).
  • Unqualified path: scans namespace scopes only; contributes nsQName for any Function/Method def with matching simple name. nsQName !== '' guard prevents anonymous-namespace false contributions.

No issues found in this logic.

Local Function-Pointer and Qualified-Identifier Classification

extractDeclaratorLeafName (captures.ts:1146–1160) correctly unwraps parenthesized_declarator → identifier chains via namedChild(0) and childForFieldName('declarator'). Handles void (*g)(), void (*g)(int), and initialized variants. The foundAsLocalFunctionPointer flag correctly catches these before they fall through to the free-function-ref path.

isIdentifierAFunctionParameter (captures.ts:824–856) correctly walks function_definition → function_declarator → parameter_list. The break after finding the first enclosing function prevents climbing further and matches only the immediately enclosing function.

Known limitation (Finding 4 — typedef/using-aliased fp) is correctly documented in captures.ts:972–983. Risk is low and bounded.

Interaction with Merged ADL V2 Work (#1592, #1595, #1596, #1597)

All prior merged ADL V2 work is preserved and unaffected:

The merge commits correctly resolved the conflict by keeping collectAssociatedNamespacesForAdlArg from main and adding the collectFunctionRefNamespaces call for functionRefText alongside it. No regression detected.

Open ADL V2 Follow-on (#1599/#1576)

PR #1598 does not worsen the short-circuit limitation. The adl.ts:44–47 comment correctly documents that ADL fires only when ordinary lookup is empty, and that ISO C++ would merge ADL with ordinary-lookup candidates (deferred to #1599). No dependency on #1599 detected. The free-function-ref path behaves consistently with the existing short-circuit constraint.

helpers.ts Legacy DAG Skip Correctness

The 2 positive tests ('with_callback(utils::worker) resolves to utils::with_callback via ADL' and the overloaded variant) are correctly absent from LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES.cpp: with_callback is unique in each fixture's workspace, so legacy's simple-name global lookup also finds it → both legs pass → no skip needed. ✅

The 4 negative tests are incorrectly absent — they will fail in legacy mode. This is Finding A above.

No stale skip names detected. The existing 18 C++ skip entries are all tied to documented, correct issues.

Tests and CI Coverage

The 6 new tests cover:

  • Positive qualified ref ✅ (cpp-adl-free-func-ref)
  • Overloaded positive ref ✅ (cpp-adl-free-func-ref-overloaded)
  • Negative qualified variable ✅ (cpp-adl-qualified-variable-arg)
  • Negative function parameter ✅ (cpp-adl-param-not-free-func-ref)
  • Negative local fp shadow ✅ (cpp-adl-local-fp-shadows-free-func)
  • Negative unqualified collision ✅ (cpp-adl-unqualified-ref-collision)

CI status: Gitleaks ✅, CodeQL ✅, PR Autofix ✅, Dependency Review ✅; CI and Docker Build in-progress. The resolver parity sub-job result is unconfirmed — but analysis shows the parity leg with REGISTRY_PRIMARY_CPP=0 will fail for the 4 negative tests unless they are added to LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES.cpp.

Hidden Unicode / Hygiene

Em dashes (, U+2014) and right arrows (, U+2192) appear in adl.ts comment text. These are visible, semantically meaningful Unicode characters used throughout the existing codebase's doc-comment style. No bidi control characters (\u202A\u202E, \u2066\u2069) found. No Unicode hygiene concern.


Final Verdict: not production-ready

A single merge-blocking issue remains: the 4 new negative tests (process(data::value), run_with(callback), record(g), run_with(worker)) are absent from LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES.cpp in helpers.ts. All 5 findings from the previous review were correctly addressed in separate, well-scoped commits. The ADL logic itself (collectFunctionRefNamespaces, isIdentifierAFunctionParameter, extractDeclaratorLeafName, foundAsLocalFunctionPointer) is correct and the negative fixture guards are well-chosen. However, the C++ parity CI leg (REGISTRY_PRIMARY_CPP=0) will fail for the 3 single-candidate negative tests because the legacy call-processor has no equivalent of the isFileLocalDef namespace filter that the registry-primary scope-resolver applies — the same gap already documented in the existing 'does NOT resolve unqualified foo() to ns::foo via #include' skip entry. The fix is a targeted addition of 4 skip entries with issue-linked comments to LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES.cpp; the entire rest of the PR is merge-ready.

@magyargergo

Copy link
Copy Markdown
Collaborator

/autofix

@github-actions

Copy link
Copy Markdown
Contributor

✅ Applied autofix and pushed a commit. (apply run)

@magyargergo

Copy link
Copy Markdown
Collaborator

@copilot we need to skip tests from the legacy DAG

…AG parity run

Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/64ffbaf1-f442-4a2b-8542-4afa500d9182

Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>

Copilot AI commented May 15, 2026

Copy link
Copy Markdown
Contributor Author

@copilot we need to skip tests from the legacy DAG

Done in commit 1f5ce77. Ran the full cpp.test.ts suite with REGISTRY_PRIMARY_CPP=0 to identify which of the new negative-fixture tests fail under the legacy DAG — two do, because the legacy path's pickUniqueGlobalCallable resolves the callee by simple-name workspace lookup regardless of argument analysis:

  • 'process(data::value) emits zero CALLS edges — data::value is a variable, not a function'
  • 'run_with(callback) emits zero CALLS edges when callback is a parameter, not a function reference'

Both are now listed in LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES.cpp in helpers.ts. The other two new fixtures (cpp-adl-local-fp-shadows-free-func, cpp-adl-unqualified-ref-collision) pass on the legacy path too, so they don't need skipping. 205 registry-primary tests pass, 0 legacy DAG failures.

@magyargergo magyargergo merged commit 99b8c7b into main May 15, 2026
35 checks passed
@magyargergo magyargergo deleted the copilot/fix-adl-function-pointer-args branch May 15, 2026 09:55
azizur100389 added a commit to azizur100389/GitNexus that referenced this pull request May 15, 2026
Keep both expected-failure entries: inline-namespace ambiguity (abhigyanpatwari#1564)
and ADL reference-arg negative fixtures (abhigyanpatwari#1598).
azizur100389 added a commit to azizur100389/GitNexus that referenced this pull request May 15, 2026
Keep both expected-failure entries: conversion-rank scoring (abhigyanpatwari#1578)
and ADL reference-arg negative fixtures (abhigyanpatwari#1598).
hohaivu pushed a commit to hohaivu/GitNexus that referenced this pull request May 19, 2026
…amespace (abhigyanpatwari#1598)

* Initial plan

* cpp ADL V2: free-function reference args contribute enclosing namespace

Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/24805583-c0c4-4ef8-978f-b874bd917947

Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>

* merge: resolve conflicts with origin/main and fix overloaded fixture app.cpp

Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/136aeffe-45da-47e2-95dd-e3883e85fad7

Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>

* fix(finding-1): replace ISO C++ [basic.lookup.argdep] misstatement with GitNexus-approximation label

Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/fa37c0dd-65f9-4dd4-9811-617227a37073

Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>

* fix(finding-2): verify Function/Method exists in namespace before contributing via qualified_identifier arg

Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/fa37c0dd-65f9-4dd4-9811-617227a37073

Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>

* fix(finding-3): function parameters in parameter_list no longer misclassified as free-function refs

Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/fa37c0dd-65f9-4dd4-9811-617227a37073

Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>

* doc(finding-4): document typedef/using-aliased function-pointer limitation in lookupAdlIdentifierType

Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/fa37c0dd-65f9-4dd4-9811-617227a37073

Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>

* test(finding-5): add negative fixtures for local-fp shadowing free-func and unqualified namespace collision

Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/fa37c0dd-65f9-4dd4-9811-617227a37073

Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>

* chore(autofix): apply prettier + eslint fixes via /autofix command

* fix(legacy-parity): skip two new negative-fixture tests from legacy DAG parity run

Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/64ffbaf1-f442-4a2b-8542-4afa500d9182

Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.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: function-pointer argument associated entities

2 participants