feat(cpp): Add pointer nullptr ellipsis conversion ranks#1708
Conversation
|
@azizur100389 is attempting to deploy a commit to the NexusCore Team on Vercel. A member of the Team first needs to authorize it. |
CI Report✅ All checks passed Pipeline Status
Test Results
✅ All 9245 tests passed 1 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
|
Claude finished @magyargergo's task in 10m 53s —— View job Adversarial Production-Readiness Review — PR #1708
|
| Conversion | ISO Standard | PR Implementation | Status |
|---|---|---|---|
nullptr → T* |
rank: conversion (null pointer const) | rank 2 | ✅ Conforms for supported evidence |
nullptr → bool |
rank: conversion (but worse than nullptr→T*) |
rank 3 | ✅ Conforms (correctly worse) |
T* → bool |
rank: conversion (pointer-to-bool) | rank 2 | ✅ Conforms |
T* → void* |
rank: conversion (pointer-to-void) | rank 2 with dual pointer sidecar check | ✅ Conforms |
Ellipsis T → ... |
worst: always worse than any std conversion seq | rank 4 | ✅ Conforms |
| Fixed beats ellipsis | standard conversion seq > ellipsis conversion seq | pairwise dominance ensures this | ✅ Conforms |
char → int, bool → int |
promotion (rank 1, preserved) | rank 1 unchanged | ✅ Preserved |
int** → int* (pointer depth) |
distinct types — not interchangeable | both classified as isPointer → conservatively ambiguous |
|
| Member-pointer ranks | out of scope | Infinity | ✅ Conservative |
| Derived-to-base pointer | out of scope | Infinity | ✅ Conservative |
| Unknown sidecar | conservative path | exactShapeCompatible returns true → same-type strings decide |
Findings
Finding A — exactShapeCompatible / exactTypeSlotMatches fallback when sidecar is missing or unknown
Severity: Low-moderate
Evidence: exactShapeCompatible (conversion-rank.ts:67) and exactTypeSlotMatches (overload-narrowing.ts:221-222) both return true when either sidecar is undefined or indirection === 'unknown'. The concern is: if an argument's normalized type equals a pointer parameter's normalized type (both become 'int' for int vs int*) and the argument sidecar is missing/unknown, the call would be treated as exact match → wrong edge.
Mitigating evidence found: In captures.ts:224-231, inferCppCallArgTypeClasses is always called when argument types are inferred, populating @reference.parameter-type-classes for every call site. For identifier arguments (f(p)), lookupDeclaredTypeClassForIdentifier returns unknownTypeClass only when the variable cannot be found, but in that case lookupDeclaredTypeForIdentifier returns '' (empty string), which triggers argTypes[i] === '' → continue in the narrowing loop before the sidecar is consulted. The asymmetry is safe: you cannot have a non-empty argType with an unknown sidecar that reaches the sidecar comparison path in practice.
GitNexus production risk: Low in current code paths. However, the absence of a test proving the "unknown sidecar + same normalized type → no false exact match" invariant means future captures changes could regress this silently.
Recommended fix: Add a unit test row for: cppConversionRank('int', 'int', undefined, pointer('int')) → must not return 0 (or add an explicit sidecar-absent test in cpp-overload-ranking.test.ts that verifies the fallback is conservative for the shared narrowing path).
Blocks merge: No — the risk is theoretical and mitigated by the current extractor's invariants.
Finding B — Pointer depth not checked in exact-shape comparison
Severity: Low
Evidence: Both exactShapeCompatible and exactTypeSlotMatches compare isPointer(arg) === isPointer(param) — not pointerDepth. An int* argument (depth 1) vs int** parameter (depth 2) would both classify as isPointer → true → compatible. Result: both overloads get "exact match" → multi-survivor → suppressed edge (zero CALLS). No wrong edge, just a missing edge.
ISO C++ relevance: int* and int** are distinct types; exact match is wrong. But the "degrade not lie" invariant holds — suppressing is correct behavior.
GitNexus production risk: Missing edges (conservative), never wrong edges. Out of scope per #1637 ("multi-level pointer / void* qualification nuance").
Recommended fix: Document the limitation as a known approximation in the function JSDoc.
Blocks merge: No.
Finding C — T* → void* guard is correct and conservative
Severity: None (verification finding)
Evidence: cppConversionRank:55 — if (isPointer(argTypeClass) && isPointer(paramTypeClass) && paramType === 'void') return 2. The isPointer(paramTypeClass) check requires pointer sidecar on the param side. If paramTypeClass is undefined/unknown, isPointer(undefined) = false → condition fails → conservative (Infinity). The void* parameter has classifyCppParameterType('void', '* pv', ...) → pointerDepth=1, indirection='pointer' → isPointer=true. Non-pointer void (return types, etc.) would have indirection='value' → isPointer=false. No unsafe path found.
Blocks merge: No.
Finding D — Ellipsis slot mapping is correct; parameterTypeAt/parameterTypeClassAt are aligned
Severity: None (verification finding)
Evidence: parameterTypeAt(params, i) (overload-narrowing.ts:299-302) returns params[i] when i < params.length, otherwise '...' iff params[params.length-1] === '...'. parameterTypeClassAt (overload-narrowing.ts:304-311) mirrors this using base === '...' for the ellipsis slot. The two helpers are synchronized — they both fall back to the last slot and both key off the same ... marker. No desynchronization found. The unit test (keeps an ellipsis overload viable when it is the only match) directly exercises the extra-arg mapping path.
Blocks merge: No.
Finding E — Ranked ambiguity does not fall through to global fallback for the fixture calls
Severity: None (verification finding)
Evidence: For f(nullptr)→f(int*) (rank 2) vs f(bool) (rank 3): pairwise gives exactly 1 survivor → no fallthrough. For f(42)→f(bool) (rank 2) vs f(int*) (Infinity): only 1 viable → no fallthrough. For g(1,2) / h(1,'a'): both have fixed overloads that exact-match → single survivor → no fallthrough. For k(1,2,3): only one candidate → returned directly. The scope-chain fallback (narrowed.length === 0 → keep first-match) is pre-existing behavior not touched by this PR.
Blocks merge: No.
Finding F — Integration test target identity: pointer overload not distinguished from normalized-int overload ⚠️ MERGE-BLOCKING per DoD verdict rules
Severity: Moderate
Evidence: cpp.test.ts:1849-1862 — the f(nullptr)/f(p)/f(42) assertion checks:
const pointerTargets = fCalls.filter((c) => {
const tgt = result.graph.getNode(c.rel.targetId);
return tgt?.properties.parameterTypes?.[0] === 'int'; // f(int*) normalizes to 'int'
});
const boolTargets = ...parameterTypes?.[0] === 'bool';
expect(pointerTargets.length).toBe(1);
expect(boolTargets.length).toBe(1);The fixture has f(int*) and f(bool). Free-call dedup collapses all call sites to one edge per unique (caller, target) pair. Demonstrated wrong-edge scenario: if f(nullptr)→f(bool) (wrong) while f(p)→f(int*) (correct) and f(42)→f(bool) (correct):
run→f(int*): 1 edge (fromf(p)) → pointerTargets = 1 ✓run→f(bool): 1 edge (fromf(nullptr)+f(42)deduped) → boolTargets = 1 ✓- Test PASSES with wrong routing for
f(nullptr).
Per the explicit DoD verdict rule: "use not production-ready if the current fixture can pass with a wrong edge."
ISO C++ relevance: This is the primary correctness claim of the PR — that f(nullptr) prefers f(int*) over f(bool).
GitNexus production risk: A future regression in nullptr ranking (e.g., a wrong rank applied to the null literal) would silently pass this test.
Recommended fix: Strengthen the assertion to distinguish per-call-site identity, e.g.:
// After filtering fCalls to pointer targets, assert the pointer sidecar on the target node
const pointerTarget = fCalls.find((c) => {
const tgt = result.graph.getNode(c.rel.targetId);
return tgt?.properties.parameterTypes?.[0] === 'int';
});
expect(pointerTarget).toBeDefined();
// Prove it's actually the pointer overload, not a hypothetical f(int)
const ptc = pointerTarget!.rel.targetId;
const node = result.graph.getNode(ptc);
expect(node?.properties.parameterTypeClasses?.[0]?.indirection).toBe('pointer');Or restructure with two separate describe blocks — one for f(nullptr) and one for f(p) — each with its own runPipelineFromRepo result for a minimal single-call fixture, so dedup cannot mask wrong routing.
Blocks merge: YES — per DoD verdict rule for test-identity weakness.
Finding G — Unit test coverage gaps (minor)
Severity: Low
Evidence: cpp-overload-ranking.test.ts covers the primary DoD rows but is missing:
int* → int*exact match = rank 0 (no explicit test)int → int*non-exact when sidecars known (only implicitly tested via the "does not treat normalized value and pointer types as exact matches" case using narrowing, not cppConversionRank directly)int** → int*behavior (out of scope, but a documented-limitation test would be useful)- Tied pointer ranks suppressing → no edges (not tested directly; only an indirect fixture-level check)
Recommended fix: Add explicit cppConversionRank('int', 'int', pointer('int'), pointer('int')) → 0 and cppConversionRank('int', 'int', value('int'), pointer('int')) → Infinity unit rows.
Blocks merge: No — the missing rows are covered implicitly by the narrowing integration; the missing pointer-depth case is out of scope.
Finding H — Hidden Unicode
Severity: None
Evidence: Ran grep -Pn "[\x{202A}-\x{202E}\x{2066}-\x{2069}]" across all 8 changed files — no bidi/hidden Unicode controls found. The grep -Pn "[^\x00-\x7F]" in overload-narrowing.ts found only ordinary typographic characters (→, —) used throughout the codebase in JSDoc comments. GitHub's warning was triggered by these pre-existing em-dashes, not by actual bidi controls.
Blocks merge: No.
PR-Specific Assessment Sections
Pointer/nullptr/ellipsis rank ladder correctness
All five rank cases in cppConversionRank are ISO-correct for the bounded approximation: null literal argType === 'null' triggers nullptr rules; pointer shape is determined by isPointer(paramTypeClass) requiring indirection === 'pointer' && pointerDepth > 0; ellipsis paramType '...' is rank 4; arithmetic promotions and standard conversions are preserved. The rank ladder 0 < 1 < 2 < 3 < 4 < Infinity correctly encodes the ISO preference chain for the supported cases.
Sidecar propagation and shape compatibility
inferCppCallArgTypeClasses (captures.ts:698-717) is always invoked alongside inferCppCallArgTypes at call sites. For literals, valueTypeClass(litType) produces non-pointer shape. For identifier arguments, lookupDeclaredTypeClassForIdentifier walks the enclosing compound_statement and calls classifyCppParameterType which correctly detects * in the full statement text to classify pointer declarations. Parameter-side sidecar (parameterTypeClasses) is always populated in computeCppDeclarationArity for every non-variadic parameter. Ellipsis gets unknownTypeClass('...'). The sidecar lengths are aligned with parameterTypes/argumentTypes — no slot-shift bugs found.
narrowOverloadCandidates ranking and ambiguity behavior
exactTypeSlotMatches is the new exact-filter function used in step 4 — correct. rankByConversion passes sidecars to rankFn via argTypeClasses?.[i] and parameterTypeClassAt(d.parameterTypeClasses, i). Pairwise dominance logic is unchanged and correct. Multiple survivors → returned as-is → callers suppress. parameterTypeAt correctly maps extra args to the final '...' slot for ellipsis candidates. No Infinity-rank candidate escapes the viability filter.
Free-call fallback / global fallback safety
site.argumentTypeClasses is now threaded into:
pickImplicitThisOverloadpath (free-call-fallback.ts:115-118)- Scope-chain non-ADL narrowing path (
free-call-fallback.ts:140-144) - ADL no-contribution ordinary narrowing path (
free-call-fallback.ts:205-209) - ADL merged-candidate narrowing path (
free-call-fallback.ts:241-245) pickUniqueGlobalCallablescope-index narrowing (free-call-fallback.ts:390-392)pickUniqueGlobalCallablemodel-pool narrowing (free-call-fallback.ts:431-433)
All six call paths are covered. The narrowed.length === 0 → keep first-match branch in the non-ADL scope-chain path is pre-existing behavior and not changed. Global fallback does not receive pointer-ranked sidecars unless site.argumentTypeClasses is set.
Receiver-bound and ADL interaction
The fixture uses Service::run() calling overloaded members. pickImplicitThisOverload handles this correctly — it finds the enclosing class scope, calls narrowOverloadCandidates with site.argumentTypeClasses, and requires exactly 1 survivor (returns undefined otherwise, falling through to further resolution). receiver-bound-calls.ts:349 and :736 already thread argumentTypeClasses (unchanged). ADL candidates are merged before narrowing; pointer ranks apply over the merged set. No ADL regressions found.
Test target identity and fixture quality ⚠️
As documented in Finding F, the primary assertion for f(nullptr)/f(p)/f(42) does not distinguish per-call-site routing. The dedup semantics mean a wrong f(nullptr)→f(bool) routing would not fail the count assertion. The g/h/k tests are stronger (assert specific parameterTypes on the target) and use unique fixture function names, so they are not affected by this weakness. Only the first integration test has the identity gap.
helpers.ts legacy DAG skip correctness
The 4 new skip entries exactly match the integration test names:
'f(nullptr) and f(p) resolve to f(int*) while f(42) resolves to f(bool)'✅'g(1, 2) resolves to fixed-arity g(int, int), not g(int, ...)'✅"h(1, 'a') resolves to h(int, double), not h(int, ...)"✅'k(1, 2, 3) keeps the ellipsis overload viable when it is the only match'✅
Comment correctly attributes these to #1637. Previous expected-failure entries from #1598/#1599/#1600/#1606/#1623/#1648 are intact and unchanged. Merge-from-main did not introduce stale or duplicate skip names. Registry-primary mode runs all 4 new tests without skipping.
CI coverage
CI report (2026-05-19) shows: Typecheck ✅, Tests/3-platform (ubuntu+windows+macos) ✅, E2E ✅. 9240 tests passed, 0 failed. The CI comment aggregates to 3 stages and does not show individual quality/lint, quality/format, CodeQL, Gitleaks, Dependency Review, or scope-parity/cpp parity rows. The overall "All checks passed" claim covers these if they ran as sub-jobs of the pipeline. No evidence of skipped or re-running jobs. Author's note that npx gitnexus impact exits 1 with no output is an unrelated local tooling issue (the GitNexus index requires npx gitnexus analyze to be run first; this does not affect CI or correctness).
Hidden Unicode / hygiene
No bidi or hidden Unicode controls found in any of the 8 changed files. Ordinary typographic characters (→, —) in comments are present throughout the pre-existing codebase and are not security-relevant. Clean.
Final Verdict: not production-ready
The rank ladder implementation is ISO-correct for the supported subset, sidecar propagation is verified end-to-end, ellipsis slot mapping is correct, pairwise dominance preserves the "no wrong edge" invariant, legacy parity skips exactly match test names, and no hidden Unicode exists. However, the primary integration assertion for the f(nullptr)/f(p)/f(42) scenario uses aggregate counts bucketed by normalized parameterTypes[0] rather than per-call-site routing, and the free-call dedup semantics allow a wrong f(nullptr)→f(bool) routing to pass the test undetected while f(p)→f(int*) is correct. Per the explicit DoD verdict rule — "use not production-ready if the current fixture can pass with a wrong edge" — this is blocking. The fix is a one-test addition: assert parameterTypeClasses[0].indirection === 'pointer' on the pointer-target graph node, or split the fixture into per-call-site scenarios that cannot be masked by dedup.
· PR branch
|
@azizur100389 can you please look into claude's findings? 🙏 |
|
Addressed Claude's blocking test-identity finding. The fixture now has separate runNullptr, runPointer, and runBoolConversion callers, so CALLS dedup cannot hide wrong f(nullptr) routing; the integration assertion checks each caller independently. I also added unit coverage for pointer exact-match shape and known value-to-pointer non-match. Validation: cpp-overload-ranking unit test passed; focused pointer/nullptr/ellipsis integration passed; full cpp resolver integration passed 235/235; tsc passed; git diff --check passed. Note: the full legacy C++ run timed out locally after 10 minutes, while the focused legacy #1637 block is skipped by existing registry-primary-only expected-failure wiring. Local gitnexus impact/detect_changes still exit 1 with no output in this checkout. |
|
Nice! Thank you! |
2 participants
Fixes #1637
Summary:
Validation:
npx vitest run test/unit/scope-resolution/cpp/cpp-overload-ranking.test.tsnpx vitest run test/integration/resolvers/cpp.test.ts -t "pointer/nullptr/ellipsis ranks"npx vitest run test/integration/resolvers/cpp.test.tsREGISTRY_PRIMARY_CPP=0 npx vitest run test/integration/resolvers/cpp.test.tsnpx tsc --noEmitgit diff --checkNote:
npx gitnexus impact ...andnpx gitnexus detect_changes --scope stagedstill exit 1 with no output in this checkout, matching the local index issue seen on earlier PRs.