feat(cpp): standard-conversion-sequence ranking for overload resolution

[azizur100389]

Summary

Closes #1578.

• Introduce ConversionRankFn type and cppConversionRank implementation to score argument-to-parameter conversions: exact match (0) beats standard arithmetic conversion (2) beats non-viable mismatch (∞)
• Thread conversion-rank scoring through narrowOverloadCandidates, pickImplicitThisOverload, pickOverload, receiver-bound Case 0.5, ADL, and pickUniqueGlobalCallable via the ScopeResolver.conversionRankFn contract property
• Add findAllCallableBindingsInScope scope walker to collect all callable overloads at the first binding scope (needed for rank-based disambiguation vs. first-match)
• Guard ambiguity suppression to same-file candidates only, preserving local-shadows-import semantics for cross-file bindings

Test plan

• f(2.5) → f(double) — exact match beats standard conversion (double → int)
• f(42) → f(int) — exact match beats standard conversion (int → double)
• g(42) → 0 edges — int/long normalize to same type, ambiguous after normalization
• Full C++ integration suite: 192/192 pass (registry-primary mode)
• Legacy resolver mode: 167 pass, 25 skipped (expected parity failures)
• Local-definition-shadows-import test: still passes (no regression)
• tsc --noEmit clean
• Prettier + ESLint clean

[vercel]

@azizur100389 is attempting to deploy a commit to the NexusCore Team on Vercel.

A member of the Team first needs to authorize it.

[github-actions]

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
9089	9088	0	1	444s

✅ All 9088 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.33%	28962/36972	N/A%	—	🟢 ███████████████░░░░░
Branches	66.76%	18379/27529	N/A%	—	🟢 █████████████░░░░░░░
Functions	83.08%	2888/3476	N/A%	—	🟢 ████████████████░░░░
Lines	81.61%	26129/32016	N/A%	—	🟢 ████████████████░░░░

---

_{📋 View full run · Generated by CI}

[magyargergo]

@azizur100389 Can you please merge origin/main and resolve the conflicts? Please be careful during the resolution as it might be non-trivial. Anyway, tests are there to protect. :)

[github-actions]

Claude finished @magyargergo's task in 9m 36s —— View job

---

Adversarial Production-Readiness Review — PR #1606

---

Review bar for this PR

DoD.md §2.7 (tests cover the real changed path), §2.1 (correct on real runtime path), and §2.3 (comments explain intent without overpromising). The PR-specific bar adds: ranker must not emit wrong CALLS edges for multi-argument overloads, conversion-rank path must be reachable by the test suite, and char/bool promotion limitations must be documented or tested.

---

Current PR state

CI is green (9074/9074 passing, 1 unrelated skip). TypeScript, format, and lint clean. 192 C++ registry-primary tests pass; 25 legacy-parity skipped as expected. The Vercel deployment request is irrelevant to resolver correctness.

---

Branch hygiene assessment

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

The local git log confirms aed370b (feat: C++ ADL V2: merge ordinary and ADL free-call candidates before overload selection (#1599)) is a reachable ancestor of HEAD. walkers.ts is not in the PR's 11 changed files, which is only possible if the findAllCallableBindingsInScope and findCallableBindingsAndAdlBlocker functions are already on origin/main (absorbed when #1599 merged). The conflict-resolution commits (7a85f9e, 06b0cf5) are noisy history, but the effective diff is correctly scoped to conversion-rank wiring and tests. #1599 is in main; this PR is not stacked. The multi-merge commit history does not require rebase before review.

---

Understanding of the change

The PR introduces cppConversionRank in a new conversion-rank.ts file and threads an optional conversionRankFn hook through narrowOverloadCandidates, pickImplicitThisOverload, pickOverload, Case 0.5 (explicit this receiver), the ADL merged-candidate path, and pickUniqueGlobalCallable. The intent: when exact normalized-type matching can't choose between overloads, score by conversion cost and pick the unique best. Only C++ sets the hook; non-C++ resolvers are unaffected.

The key design choice: argument types are inferred from the tree-sitter AST at capture time (inferCppCallArgTypes / inferCppLiteralType) and stored in site.argumentTypes. Parameter types are normalized (normalizeCppParamType) and stored in def.parameterTypes. The ranker compares these post-normalization strings.

---

Findings

---

Finding 1 — Integration tests do not exercise the conversion ranker

Severity: HIGH

Evidence:

inferCppLiteralType maps 2.5 → 'double' and 42 → 'int'. normalizeCppParamType maps double params → 'double' and int params → 'int'. For the three test cases:

• f(2.5): argTypes=['double'], f(double).parameterTypes=['double'] → exact-type filter returns [f(double)]. typed.length > 0 → returned before step 4b.
• f(42): argTypes=['int'], f(int).parameterTypes=['int'] → exact match. Returned before step 4b.
• g(42): argTypes=['int'], both g(int) and g(long) normalize to parameterTypes=['int'] → both match in exact filter. Returned before step 4b with 2 candidates; pickImplicitThisOverload sees candidates.length !== 1 → zero edges.

rankByConversion is never called by any of the three new test cases. Removing conversionRankFn: cppConversionRank from cppScopeResolver or deleting the step-4b block in narrowOverloadCandidates would leave all three tests green. No unit tests for cppConversionRank or rankByConversion exist (confirmed by search).

Risk: The new code path (rankByConversion, step-4b in narrowOverloadCandidates) is untested. Bugs in the ranker (wrong cost, off-by-one, empty-ARITHMETIC set miss) are invisible to CI.

Recommended fix: Add a test case that requires the ranker to resolve. The canonical trigger is a variable (not a literal) argument: lookupDeclaredTypeForIdentifier returns the raw declared type (e.g., 'unsigned' or a non-normalized type), while the stored paramTypes are normalized. OR add a fixture where argType = 'char' (from a char literal) calls f(int) vs f(double) — exact filter misses both (char ≠ int, char ≠ double), forcing the ranker. The result should be a tie (both rank 2) and zero edges, but this at least proves step 4b is reached.

Alternatively, add unit tests directly for cppConversionRank and rankByConversion in a dedicated test file — these are pure functions that are trivial to unit-test.

Blocks merge: yes. DoD §2.7: "Tests would fail if behavior, wiring, or contracts were broken." Removing the ranker does not break any test.

---

Finding 2 — Multi-argument total-score tie picks first-match in non-ADL and receiver-bound paths

Severity: HIGH

Evidence:

rankByConversion sums per-slot conversion ranks and returns all candidates sharing the minimum total. For h(int_var, int_var) against h(int, double) and h(double, int):

• Candidate A: rank('int','int') + rank('int','double') = 0 + 2 = 2
• Candidate B: rank('int','double') + rank('int','int') = 2 + 0 = 2

Both score 2 → rankByConversion returns [A, B].

isOverloadAmbiguousAfterNormalization([A, B]) returns false (A.parameterTypes ['int','double'] ≠ B.parameterTypes ['double','int']). In three paths:

1. Non-ADL free-call (free-call-fallback.ts:128–142): narrowed.length > 1, not normalized-ambiguous → comment says "keep first-match fnDef" → emits CALLS edge to A. Wrong.
2. Case 0.5 receiver-bound (receiver-bound-calls.ts:362): narrowed.length > 0, not normalized-ambiguous, not zero → memberDef = narrowed[0] → emits CALLS edge to A. Wrong.
3. pickOverload Case 4 (receiver-bound-calls.ts:736): candidates[0] ?? overloads[0] → picks A. Wrong.

C++ would report this call as ambiguous. GitNexus emits a false CALLS edge to the first-registered overload.

The ADL merged-candidate path is correct: when narrowed.length > 1 and not normalized-ambiguous it suppresses unconditionally (handledSites.add(siteKey); continue). This asymmetry is unintentional.

pickImplicitThisOverload is correct: if (candidates.length !== 1) return undefined.

Risk: Any C++ codebase with overloaded functions using cross-argument type tradeoffs (e.g., void process(int, double) / void process(double, int)) and a call site with mixed int/double variables will receive a false CALLS edge pointing at whichever overload was registered first.

Recommended fix: In the non-ADL fallback (after conversion ranking), when narrowed.length > 1 and isOverloadAmbiguousAfterNormalization returns false, suppress the edge (mark handled and continue) instead of falling back to first-match. Mirror the ADL path's behaviour. In pickOverload, return OVERLOAD_AMBIGUOUS when candidates.length > 1. In Case 0.5, set ambiguous = true when narrowed.length > 1 after the normalized-ambiguity check. Add a guard test with h(int,double) vs h(double,int) called with h(int_var, int_var) that asserts zero CALLS edges.

Blocks merge: yes. The review criteria is explicit: "Block if a simple summed rank can emit wrong CALLS edges for common multi-argument overloads."

---

Finding 3 — char/bool promotions misranked as standard conversions; not tested

Severity: MEDIUM

Evidence:

conversion-rank.ts header comment says "Promotion (rank 1) — int→long, float→double, char→int, bool→int", correctly identifying these as C++ promotions. The post-normalization table then assigns them rank 2 ("arithmetic cross-type (int↔double, char→int, bool→int) → rank 2"). The cppConversionRank implementation confirms: ARITHMETIC = Set(['int','double','char','bool']) → cppConversionRank('char','int') = 2, cppConversionRank('bool','int') = 2.

Consequence for f(char_literal) with f(int) and f(double): inferCppLiteralType returns 'char'; both f(int) and f(double) have different paramTypes; exact filter returns empty; ranker scores both rank 2 → TIE → rankByConversion returns both → candidates.length !== 1 in pickImplicitThisOverload → zero edges. C++ picks f(int) (integral promotion beats standard conversion). GitNexus produces a false silence.

No test covers f(char_literal) or f(true) against overloads.

Risk: False silence (missing CALLS edges) for calls using char or bool literals/variables to overloaded functions, which is a common C++ pattern.

Recommended fix: Either (a) add rank-1 special cases for char→int, bool→int in cppConversionRank and test them; or (b) add explicit tests showing f('a') and f(true) produce zero edges, paired with a comment "known limitation: char/bool promotions not distinguished from standard conversions — both rank as 2." The current state has the documentation for the limitation but no test to lock in the (limited) behaviour, which means a future "fix" could silently regress. DoD criterion 4 requires either correct handling or documented+tested limitation.

Blocks merge: maybe. The omission creates missing edges (false silences), not false positives. It becomes yes if the PR description or test names continue claiming "standard-conversion-sequence ranking" as equivalent to ISO C++ overload resolution.

---

Finding 4 — Documentation contradiction: rank-1 promotions listed but not implemented

Severity: MEDIUM

Evidence:

The top-level comment of conversion-rank.ts presents a four-tier ISO C++ ranking table including "Promotion (rank 1) — int→long, float→double, char→int, bool→int". This creates the impression that the function implements rank-1 promotions. The implementation does not — it skips from rank 0 to rank 2 with no rank-1 branch. The ConversionRankFn type JSDoc in overload-narrowing.ts says "1 = promotion (e.g. float→double in C++)" — implying the contract supports rank 1 — but cppConversionRank never returns 1.

Risk: Future contributors reading the comment will believe promotions are already ranked correctly and won't add the missing cases. Alternatively they'll read the second table and be confused about which table the implementation actually follows. The test name "standard-conversion-sequence ranking" further overstates the approximation boundary.

Recommended fix: Remove the ISO C++ ranking table from the top of the comment, or clearly label it "C++ standard (not implemented)" and keep only the post-normalization table ("what this function actually implements"). Rename the PR / test suite description to "GitNexus normalized arithmetic conversion ranking (approximation)" to set honest expectations.

Blocks merge: maybe. Non-blocking on its own but compounds Finding 3's impact by creating misleading documentation that will slow down future fixes.

---

Finding 5 — Same-file ambiguity guard is a brittle proxy for overload-set identity

Severity: LOW

Evidence:

In the non-ADL free-call path (free-call-fallback.ts:131–138), when conversion ranking returns multiple normalized-ambiguous candidates, suppression is conditional on all candidates sharing the same filePath. The comment says "True overload ambiguity: suppress only when all candidates share the same file (real overloads). Cross-file candidates are shadowing; keep first-match fnDef for those."

C++ allows a single overload set to span header declarations and multiple .cpp definitions. For a class method declared in lib.h and defined in lib_a.cpp (f(int)) and lib_b.cpp (f(long)), after normalization both have parameterTypes=['int']. The resolver would see two candidates with different file paths → skips suppression → emits first-match CALLS edge.

Risk: Multi-file overload sets (split definitions, template specializations in separate TUs) can produce a false CALLS edge where suppression is the correct behaviour.

Recommended fix: For the V1 approximation, document this limitation explicitly in a comment. Longer term, use overload-set identity (e.g., same qualified name, same owner, same namespace scope) rather than file path.

Blocks merge: maybe. The fixture's overloads are all in the same file, so the test doesn't catch this. The pattern of header-declared + multiple-TU-defined overloads is not common enough to block, but warrants a comment.

---

Finding 6 — Unicode in source files is visible punctuation; no bidi controls found

Severity: INFO

The non-ASCII characters in changed files are → (U+2192), ↔ (U+2194), and — (U+2014), used in code comments as directional arrows and em dashes. A check for bidi control characters (U+202A–U+202E, U+2066–U+2069) returned no matches. The GitHub "hidden Unicode" warning appears to be a false positive triggered by these visible punctuation characters.

Blocks merge: no.

---

PR-specific assessment sections

C++ standard-conversion ranking compliance and approximation boundary

The ranker is a deliberately simplified 3-tier approximation (0/2/∞) over normalized type strings. This is architecturally sound and correctly isolated behind conversionRankFn. However, the approximation is understated in tests (exact-type filter handles all tested calls) and overstated in comments (ISO C++ rank table header). The boundary needs an honest description and test coverage that exercises the ranker path.

cppConversionRank correctness

The function's ARITHMETIC set ('int','double','char','bool') is correctly bounded — 'unsigned', 'string', user types, pointers, and enums all return Infinity. The rank 0/2/∞ assignments are correct for the stated normalized approximation, with the exception that char→int and bool→int should be rank 1 (promotion) per ISO C++.

narrowOverloadCandidates ranking and ambiguity behavior

Step 4b is correctly unreachable when exact-type filter succeeds. The conversion ranking correctly filters Infinity candidates. The ambiguity detection after ranking (rankByConversion returning >1 tied candidates) is correct in principle but not acted on consistently: the ADL path suppresses, pickImplicitThisOverload suppresses, but the non-ADL free-call path and pickOverload/Case 0.5 fall through to first-match on tied multi-arg results. This is the core of Finding 2.

Free-call fallback behavior

Local-shadows-import semantics are preserved: findAllCallableBindingsInScope stops at the first scope that binds the name. When conversion ranking produces a single winner, it correctly overrides first-match. The narrowed.length === 0 fallback to first-match is pre-existing behaviour (no regression). The cross-file same-file guard for normalized-ambiguous candidates is correct in the common case.

ADL merged-candidate interaction and #1599 dependency

#1599 is confirmed in origin/main. The ADL path correctly deduplicates ordinary+ADL candidates before ranking. The ADL path correctly suppresses when narrowed.length > 1 regardless of isOverloadAmbiguousAfterNormalization, which is the safer policy and should be adopted by the non-ADL path too. No regressions to ADL fixtures observed.

Receiver-bound call behavior

Case 0.5 (explicit this) threads conversionRankFn correctly. The normalized-ambiguity check (ambiguous = true) correctly suppresses when both overloads collapse to the same normalized type. The hiddenByName guard for C++ name-hiding semantics is correct. The failure mode is the tied multi-arg case (Finding 2): narrowed[0] is returned when narrowed.length > 1 and types differ.

Shared ScopeResolver contract and cross-language safety

conversionRankFn is readonly ... ?: in ScopeResolver — fully optional, no default value. Only cppScopeResolver sets it. All non-C++ resolver code paths see undefined and skip conversion ranking. The ReceiverBoundProviderSubset Pick includes conversionRankFn as optional. pipeline/run.ts passes provider.conversionRankFn (which is undefined for non-C++ providers). Cross-language safety is sound.

helpers.ts legacy DAG skip correctness

The three new skip entries exactly match the test names in cpp.test.ts:

• 'f(2.5) resolves to f(double) — exact match beats standard conversion'
• 'f(42) resolves to f(int) — exact match beats standard conversion'
• 'g(42) emits zero CALLS edges — int/long normalize to same type, ambiguous'

The \u2014 escape in the two #1598 entries matches the literal — in the test names (same Unicode code point). Skip comments reference #1578/#1606 as required. No stale or duplicate entries found. Registry-primary tests are not skipped.

Tests and CI coverage

192/192 C++ registry-primary tests pass. Three new tests pass. However, all three new tests are resolved via the exact-type filter before the conversion ranker is reached (see Finding 1). No unit tests exist for cppConversionRank or rankByConversion. No tests for char/bool literal promotion cases. No guard test for multi-argument equal-total-score ambiguity. The test suite proves the outcome of the fixture is correct but does not prove the conversion ranker is on the live path.

Hidden Unicode/hygiene

No bidi controls. Visible arrows/em dashes in comments only. Clean.

---

Final verdict

not production-ready

The integration tests pass but do so via the pre-existing exact-type filter — none of the three new test cases exercise rankByConversion or the step-4b code path. Removing conversionRankFn from the C++ resolver leaves all tests green, which means the core changed behaviour is unverified. Separately, when rankByConversion returns multiple tied candidates with incomparable per-slot distributions (multi-argument equal-total-score case), the non-ADL free-call path and pickOverload pick candidates[0] instead of suppressing, emitting a false CALLS edge to whichever overload was registered first — an outcome C++ would call ambiguous. The ADL merged-candidate path correctly suppresses in this scenario, creating an inconsistency that needs to be unified. Finally, char→int and bool→int promotions are ranked as rank-2 standard conversions with no test coverage, meaning calls like f(char_literal) to f(int)/f(double) overloads produce false silences that are completely invisible to CI. Each of these three issues independently meets the merge-blocking threshold defined in the review criteria.

[github-actions]

Claude finished @magyargergo's task in 12m 36s —— View job

---

Adversarial Production-Readiness Review — PR #1606

Tasks

• Post initial comment with task list
• Read repo guidelines (DoD.md, AGENTS.md, GUARDRAILS.md, CONTRIBUTING.md, TESTING.md, ARCHITECTURE.md)
• Inspect git log and effective diff vs origin/main
• Read all changed files (conversion-rank.ts, scope-resolver.ts, contract, free-call-fallback.ts, overload-narrowing.ts, receiver-bound-calls.ts, run.ts, fixtures, tests, helpers.ts)
• Read targeted related files (captures.ts, walkers.ts, adl.ts, etc.)
• Run hidden Unicode checks
• Analyze: ISO C++ compliance, cppConversionRank correctness, multi-arg ranking, narrowOverloadCandidates algorithm
• Analyze: free-call fallback, ADL integration, receiver-bound calls, global fallback safety
• Analyze: shared contract/cross-language safety, helpers.ts parity, tests/CI coverage
• Compose and post final verdict

---

Review bar for this PR

DoD §2.7 (tests cover the real changed path), §2.1 (correct on real runtime path), and §2.3 (comments explain intent without overpromising). PR-specific bar (from trigger): ranker must not emit wrong CALLS edges for multi-argument overloads, conversion-rank path must be reachable by the test suite, char/bool promotion limitations must be documented or tested, multi-arg incomparable candidates must suppress rather than pick by summed cost.

---

Current PR state

This review is based on the current HEAD (aa047c4 fix: address Claude review findings on conversion-rank PR) which incorporates all changes from the previous Claude review cycle. CI is green (9074/9074). TypeScript, format, and lint clean. 192 C++ registry-primary tests pass.

The previous review found six findings. The author has addressed five of the six. One finding persists and is merge-blocking.

---

Branch hygiene assessment

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

The branch contains merge commits from main (35beb8f, 6b31e74, 06b0cf5) that absorbed #1598, #1599, and #1600 as they landed. Those PRs are confirmed in main. GitHub's changed-files list (11 files) is correctly scoped to conversion-rank behavior, shared hook wiring, and tests — no unrelated ADL/inline-namespace churn appears in the effective diff. The multi-merge history is noisy but the review surface is clean.

---

Understanding of the change

cppConversionRank in conversion-rank.ts implements a 3-tier post-normalization scoring table (0=exact, 1=integral promotion, 2=arithmetic conversion, ∞=mismatch). rankByConversion in overload-narrowing.ts sums per-slot ranks and returns candidates sharing the minimum total. This scoring is threaded as an optional conversionRankFn hook through narrowOverloadCandidates (step 4b, fallback after exact-type filter misses), pickImplicitThisOverload, receiver-bound Case 0.5, pickOverload, and pickUniqueGlobalCallable. Only cppScopeResolver sets the hook; all other resolvers see undefined and skip the path.

---

Findings

---

Finding 1 — Summed-cost ranking emits wrong CALLS edges for asymmetric multi-arg overloads

Severity: HIGH

Evidence:

rankByConversion (overload-narrowing.ts:130–158) returns the candidate(s) sharing the lowest summed per-slot rank. ISO C++ overload resolution uses pairwise comparison: F1 is better than F2 only when F1's ICS is not worse for every argument and better for at least one. These are not equivalent for asymmetric overload sets.

Concrete trigger using the existing fixture types (cppConversionRank returns 0 for exact, 1 for char→int promotion, 2 for arithmetic cross-type):

overloads: h(int,int), h(double,double)
call:      h('a', 2.5)   →   argTypes = ['char', 'double']

  h(int,int):    rank('char','int')  + rank('double','int')  = 1 + 2 = 3
  h(double,double): rank('char','double') + rank('double','double') = 2 + 0 = 2

rankByConversion returns [h(double,double)] (unique lowest sum).
GitNexus emits:  CALLS → h(double,double)

ISO C++ check for the same call:

• Arg 0 (char): h(int,int) uses promotion (rank 1), h(double,double) uses conversion (rank 2). h(int,int) is better for arg 0.
• Arg 1 (double): h(int,int) uses conversion (rank 2), h(double,double) uses exact (rank 0). h(double,double) is better for arg 1.

Neither overload is "not worse for all arguments" → ISO C++ calls this ambiguous. The correct result is zero CALLS edges. GitNexus emits a false edge to h(double,double).

This affects all five call paths that invoke rankByConversion:

1. pickImplicitThisOverload (free-call-fallback.ts:480–486): candidates.length === 1 → returns h(double,double). ✗
2. Case 0.5 (receiver-bound-calls.ts:363–368): narrowed.length > 1 check is false (length=1) → memberDef = narrowed[0]. ✗
3. pickOverload (receiver-bound-calls.ts:730–748): candidates.length > 1 check is false → returns candidates[0]. ✗
4. ADL ordinary-only path (free-call-fallback.ts:183–184): narrowed.length === 1 → fnDef = narrowed[0]. ✗
5. pickUniqueGlobalCallable (free-call-fallback.ts:338–339): narrowed.length === 1 → returns narrowed[0]. ✗

The trigger requires mixed literal/promotion arguments — h('a', 2.5) with the existing lib.h fixture is a concrete reproduction case. The h(42, 2.5) test that was added covers only the symmetric-tie case (both cost 2) where both paths agree. The asymmetric case is untested.

Risk: Any C++ codebase with arithmetic overloads and mixed literal arguments (e.g. compute('A', 3.14) against compute(int,int) / compute(double,double)) receives a false CALLS edge to whichever overload has the lower total conversion cost, when ISO C++ would report the call as ambiguous.

Recommended fix: Two options:

(A) Document and test the limitation: Add a test asserting that h('a', 2.5) against h(int,int) / h(double,double) emits zero CALLS edges (or explicitly assert the current wrong behaviour and mark it as a known limitation — but this requires documenting and skipping in parity). Add a fixture/test comment that explains the asymmetric case is a known gap in the summed-cost approximation. This is the minimal path to making the test suite catch regressions.

(B) Implement pairwise comparison: Replace the summed-cost loop with a pairwise filter: for each candidate F1, check whether any other candidate F2 is "not worse for all args and better for at least one" (i.e., F2 dominates F1). Return only non-dominated candidates. If more than one remains, they are genuinely ambiguous. This is the ISO-correct fix.

Blocks merge: yes. DoD criterion 5 and final verdict rule: "If ranking can emit a wrong CALLS edge for multi-argument incomparable overloads, verdict must be not production-ready."

Fix this →

---

Finding 2 — Non-ADL cross-file path still emits first-match when narrowed.length > 1 and !sameFile

Severity: LOW

Evidence:

free-call-fallback.ts:128–143 (non-ADL path):

} else if (narrowed.length > 1) {
  const sameFile = narrowed.every((d) => d.filePath === narrowed[0]!.filePath);
  if (sameFile) { handledSites.add(...); continue; }
}
// narrowed.length === 0: keep first-match fnDef

When narrowed.length > 1 and !sameFile, the code falls through and emits fnDef (first-match).

However, for C++ — the only language currently setting conversionRankFn — this code path is dead. cppScopeResolver always defines resolveAdlCandidates, so the runtime always goes through the ADL path (lines 145–233), never the non-ADL path (lines 111–143). This issue would only affect a future language that sets conversionRankFn but not resolveAdlCandidates.

Risk: Low for current codebase. Potential footgun for future language adopters.

Recommended fix: Document that the non-ADL path suppresses for sameFile but falls back to first-match for cross-file, or unify the behavior with the ADL path (always suppress when narrowed.length > 1).

Blocks merge: no. Dead code for current C++ usage.

---

PR-specific assessment sections

ISO C++ overload-resolution rule compliance

The implementation is honestly bounded. conversion-rank.ts documents "post-normalization ranking" with a table that accurately reflects what the code does (not an ISO C++ ranking table). The comment in scope-resolver.ts says "standard-conversion-sequence ranking for overload resolution" with an example that is correct. The term is slightly over-broad (ISO SCS includes reference binding, cv-qualification, derived-to-base conversions, etc. that are not implemented), but the description is accompanied by a specific example that is correct. The PR title uses the same term; this is an approximation boundary claim, not a correctness claim.

However, the summed-cost vs. pairwise divergence (Finding 1) makes the approximation incorrect for the asymmetric multi-arg case, which is within the stated scope of "arithmetic primitive conversion ranking."

GitNexus approximation boundary

The approximation is correctly documented: ARITHMETIC = Set(['int','double','char','bool']), returns ∞ for everything else. User-defined types, pointers, references, enums, templates, and non-primitive types conservatively return ∞ (no false-positive edges). Reference binding, cv-qualification, derived-to-base, user-defined conversions, and ellipsis are all correctly out-of-scope (∞). The boundary is conservative for unknown types.

The gap is Finding 1: within the claimed arithmetic primitive scope, the asymmetric multi-arg case produces a false positive. This is not a limitation at the boundary but a bug within the stated scope.

cppConversionRank correctness

• argType === paramType → 0 ✓
• char→int and bool→int → 1 (integral promotion) ✓
• char→char, bool→bool → 0 (exact, not promotion) ✓
• char→double → 2 (arithmetic, not rank 1) ✓
• int↔double → 2 ✓
• string, user types, pointers → ∞ ✓
• char→double > char→int correctly (2 > 1) ✓
• ARITHMETIC set is closed: no accidental membership from normalization artifacts ✓

The function itself is correct. The bug is in how rankByConversion aggregates its results.

narrowOverloadCandidates ranking and ambiguity behavior

Step 4b correctly runs only when the exact-type filter returns empty. The ∞ exclusion (viable=false on any ∞ slot) is correct. The isOverloadAmbiguousAfterNormalization call for the normalization-collapse case works correctly.

The issue is in rankByConversion: it returns the unique lowest-sum candidate. For the symmetric-tie case (both cost 2), it returns both candidates → callers see length > 1 and suppress. For the asymmetric case (one costs 3, one costs 2), it returns a single candidate and callers emit an edge — but ISO C++ would suppress because neither candidate is pairwise-better.

Free-call fallback behavior

Local-shadows-import semantics preserved ✓. findAllCallableBindingsInScope stops at the first binding scope ✓. ADL merged-candidate path always suppresses when narrowed.length > 1 (correct, more conservative than non-ADL path) ✓. When narrowed.length === 0, falls back to first-match (pre-existing behavior, no regression) ✓. The handledSites mark on suppress-and-continue paths is correct per Contract Invariant I2 ✓.

ADL merged-candidate interaction and #1599 baseline

#1599 is confirmed in origin/main. The ADL path correctly deduplicates ordinary+ADL candidates before ranking (seenMerge set, lines 200–209). conversionRankFn is threaded through the merged narrowOverloadCandidates call. When narrowed.length > 1 in the merged path, the code always suppresses (not sameFile-gated), which is the correct conservative behavior ✓. No regressions to ADL fixtures observed from the changed files.

Receiver-bound call behavior

Case 0.5 (this->m() fallback): narrowed.length > 1 → ambiguous = true → suppress ✓. This correctly handles the symmetric-tie case. For the asymmetric case (Finding 1), narrowed.length === 1 → emits wrong edge ✗.

pickOverload: candidates.length > 1 → OVERLOAD_AMBIGUOUS ✓. Again, asymmetric case gives length === 1 → wrong edge ✗.

isOverloadAmbiguousAfterNormalization correctly detects the int/long normalization-collapse case ✓. hiddenByName guard for C++ name-hiding semantics is correct ✓.

Shared ScopeResolver contract and cross-language safety

conversionRankFn?: ConversionRankFn is readonly and optional in ScopeResolver ✓. Only cppScopeResolver sets it ✓. All shared paths treat it as undefined-guarded (if (conversionRankFn !== undefined)) ✓. Non-C++ resolvers see undefined and skip conversion ranking entirely ✓. TypeScript types do not require the property ✓. pipeline/run.ts:385 passes provider.conversionRankFn which is undefined for non-C++ resolvers ✓.

helpers.ts legacy DAG skip correctness

Five new conversion-rank entries exactly match the test names in cpp.test.ts:

• 'f(2.5) resolves to f(double) — exact match beats standard conversion' ✓
• 'f(42) resolves to f(int) — exact match beats standard conversion' ✓
• 'g(42) emits zero CALLS edges — int/long normalize to same type, ambiguous' ✓
• "p('a') resolves to p(int) — char promotion (rank 1) beats char→double conversion (rank 2)" ✓
• 'h(42, 2.5) emits zero CALLS edges — multi-arg tied total score, ambiguous' ✓

Skip comments reference #1578/#1606 ✓. \u2014 escape in #1598 entries matches literal — in test names ✓. #1599 entries ('swap(a,b)...', 'record(e)...') are present and correct ✓. No duplicate or stale entries found. Registry-primary tests are not skipped ✓.

Tests and CI coverage

The aa047c4 commit added two tests that address the previous review's Finding 1:

p('a') resolves to p(int) — This test does exercise step 4b: argTypes=['char'], exact filter returns empty (char≠int, char≠double), rankByConversion is called, cppConversionRank('char','int')=1 < cppConversionRank('char','double')=2, returns [p(int)], length=1 ✓. Target identity verified via parameterTypes?.[0] === 'int' ✓.

h(42, 2.5) emits zero CALLS edges — Exercises step 4b for the symmetric-tie case: argTypes=['int','double'], both overloads cost 2, both returned, narrowed.length > 1, sameFile=true → suppress ✓.

Gap: No test for the asymmetric case (h('a', 2.5) with h(int,int) vs h(double,double)) that would expose Finding 1.

Existing f(2.5) and f(42) tests are still resolved via exact-type filter (step 4, not step 4b) — they prove the outcome but not the ranker path. The promotion and tied-total tests now cover step 4b.

Target identity verified in: f(double) via parameterTypes?.[0]==='double', f(int) via parameterTypes?.[0]==='int', p(int) via parameterTypes?.[0]==='int' ✓. Simple name-only assertions avoided ✓.

Hidden Unicode/hygiene

Checked conversion-rank.ts, overload-narrowing.ts, free-call-fallback.ts, helpers.ts, cpp.test.ts. No bidi control characters (U+202A–U+202E, U+2066–U+2069) found. Non-ASCII characters are visible punctuation only: → (U+2192), ↔ (U+2194), — (U+2014) in code comments. GitHub's warning was a false positive on visible Unicode punctuation. Clean.

---

Progress since previous review

Previous Finding	Status
Finding 1 — Ranker never called (no step-4b test)	ADDRESSED — p('a') exercises step 4b via promotion, h(42,2.5) exercises via tied total
Finding 2 — Multi-arg tie picks first-match in pickOverload/Case 0.5	ADDRESSED — pickOverload returns OVERLOAD_AMBIGUOUS when candidates.length > 1; Case 0.5 sets ambiguous = true when narrowed.length > 1
Finding 3 — char/bool promotions misranked	ADDRESSED — p('a') test proves char→int rank 1 beats char→double rank 2
Finding 4 — Documentation contradiction (ISO C++ table header)	ADDRESSED — ISO C++ table removed; only post-normalization table remains in comment
Finding 5 — Same-file guard is brittle proxy for overload-set identity	Acknowledged — low risk for C++ (dead path), documented in code comments
Finding 6 — Unicode (INFO)	Clean — no bidi controls
NEW — Summed vs. pairwise divergence for asymmetric multi-arg	Not addressed — merge-blocking

---

Final verdict

not production-ready

The aa047c4 fix commit resolved five of the six previous findings, including adding two tests (p('a') and h(42,2.5)) that genuinely exercise the conversion ranker's step 4b path. The shared-contract threading, cross-language safety, helpers.ts parity, char/bool promotion handling, and symmetric-tie suppression are all correct.

The remaining blocking issue is that rankByConversion uses summed per-slot conversion costs, which diverges from ISO C++ pairwise "not worse for all, better for at least one" comparison for asymmetric overload sets. For a call like h('a', 2.5) against h(int,int) and h(double,double), the summed scoring picks h(double,double) (cost 2 < 3) while ISO C++ would find the call ambiguous because h(int,int) is better for argument 0 via char promotion. All five resolution paths (pickImplicitThisOverload, Case 0.5, pickOverload, ADL ordinary-only, pickUniqueGlobalCallable) would emit a wrong CALLS edge in this scenario. The existing h(42,2.5) test covers the symmetric-tie case (both cost 2) but not the asymmetric case. Either a pairwise comparison implementation or a test/documented limitation for the asymmetric case is required before merge.

[magyargergo]

@azizur100389 can you please implement option B for the 1st finding? 🙏

[magyargergo]

@azizur100389 Thank you 🙏