feat(cpp): rank user-defined conversions#1829
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 9974 tests passed 4 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
|
Claude finished @magyargergo's task in 8m 59s —— View job PR #1829 Adversarial Production-Readiness Reviewfeat(cpp): rank user-defined conversions Todo List
Review bar for this PRThis review is conducted against DoD.md (v2.0.0), GUARDRAILS.md, AGENTS.md, and CONTRIBUTING.md. The bar is: zero false-positive CALLS edges from namespace collision, correct handling of Problem being solvedPR #1631 identified that Current PR state
Merge status and mergeabilityCannot be fully verified — Repository history considered
Branch hygiene assessmentProvisional verdict: Merge-from-main commit is harmless. Confirmed by direct inspection: Understanding of the change
Fixtures / tests: FindingsFinding 1 — Namespace collision in UDC sidecar keySeverity: HIGH Evidence:
return def.qualifiedName?.split('.').pop() ?? def.qualifiedName ?? '';This strips any namespace prefix. At lookup time in Risk: Any real-world repo with two classes sharing the same simple name in different namespaces (e.g., Recommended fix: Key on Blocks merge: Yes — confirmed false-positive CALLS edge risk. Finding 2 —
|
| Claim | Status |
|---|---|
| Standard conversions beat UDCs (rank 2 < rank 4) | ✅ VERIFIED |
| Ellipsis worse than UDC (rank 5 > rank 4) | ✅ VERIFIED |
| Exact match and promotions remain best (ranks 0, 1) | ✅ VERIFIED |
| Tied UDCs return ambiguous, zero edges | ✅ VERIFIED by unit test and integration test h(42) |
| No UDC chaining (one-step only) | ✅ The sidecar registers A→B entries; hasCppUserDefinedConversion is only called once per slot, not recursively |
| No inheritance-based UDC lookup | ✅ No MRO walk in the UDC sidecar |
| No template conversion-operator expansion | ✅ The regex /^operator\s+(.+)$/ is non-template-aware |
explicit constructors excluded |
❌ CONFIRMED MISSING — no isExplicit flag exists in the AST metadata |
| Only safe subset of [over.ics.user] modeled |
C++ compiler-engineering assessment
Constructor vs method distinction: isCallableMember correctly accepts Constructor type. The constructor-name check (simpleName === className) is redundant if the type is Constructor, but harmless.
parameterTypes?.length === 1 handling: Works for single-arg constructors. For constructors with default parameters (e.g., Wrap(int x, int y = 0)), parameterTypes.length would be 2, so they are NOT registered as UDCs. This is conservative and correct — the PR does not claim default-parameter UDC support.
Copy/move constructor exclusion: Wrap(const Wrap&) would have parameterTypes[0] === 'Wrap' (normalized). registerCppUserDefinedConversion('Wrap', 'Wrap') would be rejected by the argType === paramType guard at line 44. ✅
Conversion operator extraction: Unverified against actual parser output. See Finding 3.
explicit constructors: No AST field available. See Finding 2.
Static-analysis/tooling assessment
Namespace collision (Finding 1): Key is "int\0Wrap" regardless of namespace. a::Wrap(int) and b::Wrap(int) both register int → Wrap. An overload f(a::Wrap) has normalized paramType === "Wrap", so hasCppUserDefinedConversion("int", "Wrap") returns true even when only b::Wrap has the constructor. False CALLS edge confirmed possible.
normalizeCppParamType collapse for constructor args: Wrap(long x) registers as int → Wrap since long normalizes to int. Wrap(unsigned int x) also registers as int → Wrap. These are consistent with the existing normalization model — not a new issue.
Conversion key construction: The null-byte delimiter \0 prevents "ab" + "cd" from colliding with "abc" + "d". Correct.
Resolver lifecycle assessment
Clear order in loadResolutionConfig (confirmed from scope-resolver.ts:64-68):
clearFileLocalNames()
clearCppDependentBases()
clearCppAdlState()
clearCppInlineNamespaces()
clearCppUserDefinedConversions() ← new, last in the clear block
Order matches all other clear calls. Correct.
Populate order in populateOwners (confirmed from scope-resolver.ts:103-122):
populateClassOwnedMembers(parsed)
populateCppInlineNamespaceScopes(parsed)
populateCppAnonymousNamespaceScopes(parsed)
populateCppNonGloballyVisible(parsed)
populateCppAssociatedNamespaces(parsed)
populateCppUserDefinedConversions(parsed) ← new, last
Runs after populateClassOwnedMembers which populates ownedDefs — required so classScope.ownedDefs is available. Correct.
populateWorkspaceOwners: Only calls populateCppDependentBases. Not touched. Ordering relative to UDC population is independent (dependent bases are about template lookup; UDC is about overload ranking). No interaction risk.
Multi-file behavior: UDC registration accumulates across all files. lib.h registers int → Wrap, int → WrapA, int → WrapB from in-class declarations. lib.cpp's out-of-class definitions are not in class scopes, so they don't re-register (deduplication via Set is also a safety net). ✅
Fixture and test assessment
Positive standard-over-UDC (f(42) → f(double)): Verified. Integration test uses target?.properties.parameterTypes assertion, not just count. ✅
Positive UDC-only (g(42) → g(Wrap)): Verified. Target parameterTypes === ['Wrap']. ✅
Negative tied-UDC (h(42) → 0 edges): Verified. expect(hCalls.length).toBe(0). ✅
Conversion operator coverage: ABSENT. lib.h has no conversion operators. Claimed in scope-resolver comment but untested.
explicit constructor negative fixture: ABSENT. The DoD explicitly requires this when explicit constructors cannot be excluded.
Unit test ranks: cppConversionRank('int', 'Wrap', ...) === 4 and cppConversionRank('int', 'double', ...) === 2 confirmed. UDC loses to standard conversion. ✅
Tied UDC ambiguity unit test: narrowOverloadCandidates([byWrapA, byWrapB], ...) returns both candidates. ✅
Helper expected failures (lines 311-315 of helpers.ts):
'f(42) resolves to f(double) because standard conversion beats constructor UDC',
'g(42) keeps a single constructor UDC viable when no standard conversion overload exists',
'h(42) emits zero CALLS edges when two single-step constructor UDCs tie',These are correctly scoped to the CPP legacy-parity section and tied to #1631. The legacy DAG has no UDC sidecar or ranking, so these should only pass under registry-primary mode. Correct and appropriately narrow. ✅
PR #1822 overlap assessment
PR #1822 (feat(cpp): strict function-type ADL) is open and visibly overlaps in cpp.test.ts and helpers.ts. Direct diff against the PR #1822 branch is unverifiable in this environment (requires git fetch approval).
From static inspection of the current branch:
- The helpers.ts CPP expected-failure entries for
process(data::value),run_with(callback),record(e), and ADL-blocker tests are already present. These match entries associated with PRs feat: C++ ADL V2: free-function reference args contribute enclosing namespace #1598/feat: C++ ADL V2: merge ordinary and ADL free-call candidates before overload selection #1599 (not refactor(cpp): Use function-type ADL entities #1822). - The integration test file ends with the two-phase fixture at the bottom (verified to ~line 3040).
- No
cpp-adl-free-func-ref-strictfixture was found in the fixtures directory.
Semantic interaction: ADL candidates feed into narrowOverloadCandidates. If ADL discovers a f(Wrap) candidate from an associated namespace and a UDC int → Wrap is registered, this PR's ranking would make the call viable. This is the intended behavior. No regression is expected for the existing ADL tests since they don't involve UDC-ranked overloads.
Risk: If PR #1822 adds tests to cpp.test.ts or helpers.ts that conflict with the new UDC expected-failure entries, a merge conflict would surface. This is a rebase concern, not a correctness concern.
Verdict: Unverified interaction — recommend verifying git diff review-pr-1822...review-pr-1829 before merge.
PR #1819 overlap assessment
PR #1819 touches scope-resolver.ts for dependent-base qualifier plumbing. This PR also touches scope-resolver.ts (adding clear and populate calls).
From the visible scope-resolver.ts, the populateCppUserDefinedConversions call is appended at the end of populateOwners — after all existing calls including populateCppAssociatedNamespaces. The populateCppDependentBases is in populateWorkspaceOwners and is not touched.
Risk: If PR #1819 added a call between populateCppAssociatedNamespaces and the end of populateOwners, this PR's addition would come after it — correct. If #1819 modified loadResolutionConfig, a merge conflict would surface. The dependent-base state and UDC state are orthogonal (different sidecars, different lifecycle phases).
Verdict: Low interaction risk. Unverified without fetch.
CI and validation assessment
CI report claims: all checks passed, 9859 tests passed, 0 failed, 3 skipped, head SHA a1d8116. The visible CI comment was posted at 2026-05-26T03:32:38Z; the merge commit 37d70d2 was authored at 2026-05-26T05:46:05Z. The CI report predates the merge commit and does not cover the current branch tip (37d70d2).
Since the merge commit only touches package-lock.json (vitest bump), CI re-running on 37d70d2 is expected to pass. But technically the visible CI passed on a1d8116, not the current branch tip.
Validation commands specified in the review prompt could not be run due to environment constraints (long-running test suite requiring npm install). The author's claimed test runs included the targeted vitest suites and prettier check.
Hidden Unicode/hygiene assessment
Python-based Unicode scan could not run (requires approval). File inspection via grep found no evidence of bidi control characters in the new files. The fixture files (lib.h, lib.cpp) contain only ASCII C++ source code. The TypeScript file user-defined-conversions.ts uses only standard ASCII. The null byte \0 delimiter in conversionKey is safe — it only appears inside a JavaScript string literal as a string separator, not injected from external input.
Back-and-forth avoided by verifying
- "CI is green so it's fine" — verified that the CI report covers SHA
a1d8116, not the current branch tip37d70d2. However the merge commit is benign. - "merge commit is unrelated" — verified directly via
git show 37d70d2 --stat: onlypackage-lock.json. - "namespace qualified name in
qualifiedName" — verified thatsimpleNameOfstrips namespace prefix via.split('.').pop(). - "SymbolDefinition has an
isExplicitflag" — verified directly ingitnexus-shared/src/scope-resolution/symbol-definition.ts: no such field. - "ellipsis rank tests all updated" — searched
conversion-rank.ts,cpp-overload-ranking.test.ts: consistent5.
Open questions
-
Does tree-sitter-cpp produce a
simpleNameof"operator int"for conversion functions? The conversion-operator code path inconversionOperatorTargetcannot be verified without a fixture or parser trace. -
What does PR refactor(cpp): Use function-type ADL entities #1822 add to
cpp.test.tsandhelpers.ts? Cannot verify withoutgit fetch origin pull/1822/head. -
Is there a plan to add
isExplicittoSymbolDefinition? If not, the approximation must be explicitly documented as a known false-positive source in the PR body.
Final verdict
not production-ready
Two confirmed false-positive CALLS edge risks block this merge against the project's graph-safety posture documented in #1564 and the DoD's requirement that UDC recognition be conservative.
Finding 1 (namespace collision): The global UDC sidecar is keyed by normalized simple names without namespace context. Any repo where two classes share a simple name but differ in namespace — and only one has a converting constructor — will produce incorrect CALLS edges for overloads taking the other class. This is not a theoretical edge case: same-simple-name classes in different namespaces (e.g., detail::Result, api::Result) are common in production C++.
Finding 2 (explicit constructor exclusion): SymbolDefinition carries no isExplicit field. The implementation registers every single-arg constructor as a UDC, including explicit ones. explicit constructors are a best practice in modern C++ and common in real codebases. A call f(42) where only f(explicit_Wrap) exists should produce zero CALLS edges; this PR produces one false edge. The DoD requires either exclusion or a documented, tested approximation with a negative fixture.
Additionally, Finding 3 (conversion operator support) is claimed but unverified by any fixture — this is at minimum a documentation gap and potentially dead code. The branch hygiene is clean, the lifecycle ordering is correct, the ellipsis renumbering is correct, and the three covered test cases (standard-wins, UDC-only, tied-UDC) are well-structured.
Recommended path: Address namespace qualification in the conversion key (use qualifiedName prefix where available, or suppress same-simple-name collisions), add an isExplicit gate or a documented negative fixture for explicit constructors, and add a conversion-operator fixture or remove that code path to a follow-up.
| Branch
|
Addressed the production-readiness findings in commit f028a43. Summary:
Validation before push:
|
|
Claude finished @magyargergo's task in 20m 36s —— View job Production-Readiness Review — PR #1829
|
| Job | Result | Root Cause |
|---|---|---|
scope-parity / scope-resolution parity |
FAIL | 2 test assertion errors in legacy-mode run (Finding M1 below) |
quality / typecheck |
FAIL | ETIMEDOUT downloading microsoft.ml.onnxruntime.gpu from api.nuget.org during npm ci — infrastructure flakiness, not a code defect |
| All other jobs | PASS | — |
Primary mode (REGISTRY_PRIMARY_CPP=1): 255/255 tests pass. Every new test — including e(42) (explicit exclusion) and the namespace collision guard — is green.
Legacy mode (REGISTRY_PRIMARY_CPP=0): 2 failures:
e(42) ignores the explicit-constructor overload and keeps the implicit UDC viable→AssertionError: expected +0 to be 1(line 2025 ofcpp.test.ts)does not let beta::Token(int) tie the valid alpha::Other(int) conversion→AssertionError: expected +0 to be 1(line 2045 ofcpp.test.ts)
Both failures are expected: without the UDC sidecar, f(42) where f accepts a user type cannot resolve at all → 0 edges. The tests are correct; the parity configuration is incomplete.
3. Production Graph Safety (false-positive CALLS edges)
Overall posture: ✅ Correct — suppress rather than guess is maintained throughout.
| Guard | Mechanism | Verdict |
|---|---|---|
| Tied UDC candidates | narrowOverloadCandidates returns all when >1 share the same minimum rank |
✅ 0 edges (ambiguous) |
explicit constructors |
def.isExplicit === true check in populateCppUserDefinedConversions:46 |
✅ excluded from implicit UDC |
| Namespace-collision same simple name | isAmbiguousClassName gate in rebuildCppUserDefinedConversions:100 |
✅ suppressed |
| UDC chains (two-step) | Not modeled — only one-step constructors registered | ✅ conservative |
Conversion operators (operator T()) |
Not modeled | ✅ conservative |
| Cross-file name collision guard | classIdentitiesBySimpleName accumulates across files; rebuildCppUserDefinedConversions re-scans all pending on each file → late-arriving same-name classes retroactively suppress conversions |
✅ correct |
| Empty/self conversion guard | `if (argType === '' |
The rebuild pattern (clear-and-re-add from pendingUserDefinedConversions) correctly handles the cross-file case: if alpha::Token is in file A and beta::Token in file B, after file B is processed the ambiguity is detected and the UDC is suppressed in the rebuilt sidecar.
4. ISO C++ Overload-Resolution Correctness
[over.match.best] / [over.ics.user] / [class.conv.ctor] compliance:
| Rule | PR Behaviour | Verdict |
|---|---|---|
| Standard conv (rank 2) beats UDC (rank 4) | f(42) where f(double) and f(Wrap) exist → resolves to f(double) |
✅ |
| UDC (rank 4) beats ellipsis (rank 5) | Rank table enforces this; ellipsis shifted from 4→5 to make room | ✅ |
| Tied UDC → ambiguous | h(42) where h(WrapA) and h(WrapB) both rank 4 → 0 edges |
✅ |
explicit ctor excluded from implicit conversion |
explicit ExplicitWrap(int) → isExplicit = true → skipped in populateCppUserDefinedConversions:46 |
✅ |
| One-step UDC only | Only converting constructors registered; no chaining; no conversion operators | ✅ (conservative) |
| Integral promotion (rank 1) still beats standard (rank 2) | Unchanged from #1637 | ✅ |
| Null→pointer (rank 2) beats null→bool (rank 3) | Unchanged from #1637 | ✅ |
| Pointer→bool, pointer→void* (rank 2) | Unchanged from #1637 | ✅ |
Ellipsis rank change (4→5): correct — [over.ics.ellipsis] places ellipsis worst in the ICS ranking hierarchy, after user-defined conversions ([over.ics.user]). The PR correctly shifts ellipsis rank to 5 to insert UDC at 4.
Not modeled (deliberate and documented): conversion operators (operator T()), UDC chaining, conversion function templates. All omissions are conservative (under-approximate). No false-positive edges introduced.
5. State Management & Pipeline Isolation
The module-level sidecar (userDefinedConversions, pendingUserDefinedConversions, classIdentitiesBySimpleName) is correct:
clearCppUserDefinedConversions()is called inloadResolutionConfig(scope-resolver.ts:68) before every pipeline run — prevents cross-run contamination ✅populateCppUserDefinedConversions(parsed)is called at the end ofpopulateOwners(scope-resolver.ts:121) for every file — sidecar is fully populated before resolution starts ✅- Unit test isolation:
afterEach(() => { clearCppUserDefinedConversions(); })atcpp-overload-ranking.test.ts:47— prevents cross-test contamination ✅ rebuildCppUserDefinedConversionsis O(N pending conversions) and called once per file. For repos with many classes this could accumulate work, but is not a correctness issue. (Non-blocking observation.)
registerCppUserDefinedConversion (the direct-registration public API at line 55) bypasses the ambiguity guard and goes straight to userDefinedConversions. This is only called from unit tests, never from the pipeline. The separation is fine, but the function's JSDoc could note that it does not enforce the collision guard (non-blocking).
6. hasExplicitSpecifier — Explicit Constructor Detection
function hasExplicitSpecifier(node: SyntaxNode): boolean {
for (let i = 0; i < node.childCount; i++) {
const child = node.child(i);
if (child !== null && child.text === 'explicit') return true;
}
return /\bexplicit\b/.test(node.text.slice(0, 128));
}Analysis: Primary mode passes the e(42) test, confirming detection works for the explicit ExplicitWrap(int value); form in a class body. The dual-strategy (direct child walk → text prefix regex) is resilient to minor tree-sitter grammar shape variations. The 128-char prefix cap prevents scanning into function bodies for large constructors. No false-positive risk from the regex because the \b word boundaries prevent substring matches.
Correctness: ✅ verified by primary CI.
The synthetic @declaration.is-explicit capture flows correctly through scope-extractor.ts:577–592 via parseBooleanCapture → isExplicit: true on SymbolDefinition. The capture name is registered in KNOWN_SUB_TAGS (scope-extractor line 1091) to prevent the unknown-sub-tag warning.
7. Namespace Collision Guard
function normalizedQualifiedClassName(def: SymbolDefinition): string {
const qualifiedName = def.qualifiedName ?? simpleNameOf(def);
if (qualifiedName === '' || !qualifiedName.includes('.')) return `${def.filePath}:${def.nodeId}`;
return qualifiedName.split('.').map((part) => normalizeCppParamType(part)).join('.');
}For the cpp-overload-udc-namespace-collision fixture:
alpha::Token→qualifiedName = 'alpha.Token'→ identity'alpha.Token'beta::Token→qualifiedName = 'beta.Token'→ identity'beta.Token'classIdentitiesBySimpleName['Token']={'alpha.Token', 'beta.Token'}→ size 2 →isAmbiguousClassName('Token')= truebeta::Token(int)pending conversion is suppressed → 0 UDC forTokenalpha::Other(int)pending conversion:'Other'has size 1 → not ambiguous → UDC registeredf(42)→f(Other)rank 4,f(Token)Infinity → 1 edge ✅
Correctness: ✅ verified by primary CI.
Fallback to ${filePath}:${nodeId} for classes without namespace qualification (no dot in qualifiedName) produces unique identities per class definition — correct for same-file distinct classes.
8. Type Normalization Interactions
normalizeCppParamType (from arity-metadata.ts) maps arithmetic/built-in types but preserves user type names case-as-is. UDC registration stores normalized arg and param types. Because normalizeCppParamType('Wrap') === 'Wrap', normalizeCppParamType('int') === 'int', and call-site arg types are also normalized before hasCppUserDefinedConversion is invoked, there is no normalization mismatch.
The conversionKey(argType, paramType) uses a null-byte delimiter (${argType}\0${paramType}), which cannot appear in normalized type strings — no false key collisions possible.
Risk: None identified.
9. Test Coverage Assessment
| Scenario | Covered | Location |
|---|---|---|
| Standard conv beats UDC | ✅ | cpp.test.ts:1996 |
| Single UDC, no competitor | ✅ | cpp.test.ts:2005 |
| Two UDCs tie → 0 edges | ✅ | cpp.test.ts:2014 |
explicit ctor excluded |
✅ | cpp.test.ts:2021 |
| Namespace collision guard | ✅ | cpp.test.ts:2041 |
| UDC rank (unit) | ✅ | cpp-overload-ranking.test.ts:73 |
Tied UDC via narrowOverloadCandidates (unit) |
✅ | cpp-overload-ranking.test.ts:81 |
| Ellipsis rank 5 (unit) | ✅ | cpp-overload-ranking.test.ts:67 |
Test isolation (afterEach clear) |
✅ | cpp-overload-ranking.test.ts:47 |
Gaps (non-blocking):
- Conversion operator (
operator T()) negative test (i.e., assert it does NOT produce a UDC edge) — acceptable given the explicit "not modeled" stance - Cross-file UDC where the converting class is in a separate header — the sidecar rebuild pattern handles it, but no explicit test
registerCppUserDefinedConversionself-conversion guard (argType === paramType) is covered implicitly by the function contract but has no dedicated unit test
10. Branch & Code Hygiene
git diff --check HEAD~1..HEAD: clean — no trailing whitespace or mixed-indent issues- No suspicious Unicode bidi control characters in new source files
- Branch has 4 merge-from-main commits in recent history — squash before landing to keep graph clean (non-blocking recommendation)
f028a43 Fix C++ UDC review findingsis the topmost feature commit. The branch is current with main.- New
user-defined-conversions.tsfile: well-structured, JSDoc present on all exports, internal helpers are unexported
11. Specific Issue Verification (7 items from review brief)
-
explicitdetection works? ✅ Primary CI passese(42)—hasExplicitSpecifiercorrectly detectsexplicit ExplicitWrap(int value);in class body. -
Namespace collision guard works? ✅ Primary CI passes namespace collision test —
isAmbiguousClassName('Token')returns true foralpha::Token+beta::Token. -
rebuildCppUserDefinedConversionscross-file ordering? ✅ Correct — rebuild re-scans all pending conversions using the cumulativeclassIdentitiesBySimpleNamemap, so late-file name collisions retroactively suppress earlier registrations. -
clearCppUserDefinedConversionscalled before each pipeline? ✅ Called inloadResolutionConfig(scope-resolver.ts:68). -
pendingUserDefinedConversionsaccumulation across pipeline runs? ✅ Safe —pendingUserDefinedConversions.length = 0inclearCppUserDefinedConversions(line 17) resets it. -
Ellipsis rank shift (4→5) regresses existing tests? ✅ No regression — the existing ellipsis unit test was updated to
toBe(5)and passes; all 255 primary integration tests pass. -
LEGACY_RESOLVER_PARITY_EXPECTED_FAILUREScomplete? ❌ MISSING —e(42)and namespace collision tests not listed. Root cause of CI failure. See M1 below.
12. Findings & Final Verdict
M1 — MERGE-BLOCKING: Two UDC-dependent tests missing from LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES
File: gitnexus/test/integration/resolvers/helpers.ts
Section: LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES['cpp'] (currently ends around line 321)
The following two test names must be added:
// e(42) and namespace-collision tests require the UDC sidecar (rank 4
// path). Legacy DAG has no UDC support — int→UserType conversions never
// register, so the only viable candidates are absent → 0 edges. These are
// scope-resolver-only correctness wins; backporting UDC to legacy is out
// of scope.
'e(42) ignores the explicit-constructor overload and keeps the implicit UDC viable',
'does not let beta::Token(int) tie the valid alpha::Other(int) conversion',Root cause: Both tests call into code that resolves f(42) → f(UserType). Without the UDC sidecar, the int → UserType conversion doesn't exist, so neither overload is viable → 0 edges. The tests correctly expect 1 edge (primary mode produces it). Legacy mode cannot produce this without UDC support.
Fix: Add both strings to the cpp expected-failures set with the comment above. This is a two-line fix.
M2 — MERGE-BLOCKING (rerun required): Typecheck CI failure is transient
quality/typecheck ETIMEDOUT on api.nuget.org during npm ci downloading microsoft.ml.onnxruntime.gpu.linux-x64 native binaries. This is infrastructure flakiness unrelated to this PR. A clean re-run of that job is required to confirm no actual typecheck errors exist.
NB1 — Non-blocking: registerCppUserDefinedConversion bypasses collision guard
The public direct-registration function (user-defined-conversions.ts:55) adds directly to userDefinedConversions without ambiguity checking. Only used in unit tests. Add a JSDoc note: "Does not enforce the namespace-collision guard. Use populateCppUserDefinedConversions for pipeline use."
NB2 — Non-blocking: Cross-file UDC integration test gap
No integration test covers the scenario where the converting class (e.g., Wrap) is defined in a separate header included into the service file. The rebuild-pattern handles it correctly by design, but there is no explicit test.
Final Verdict
🔴 NOT PRODUCTION-READY
Two merge-blocking issues must be resolved:
- M1 (code change): Add 2 entries to
LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES['cpp']inhelpers.ts - M2 (CI rerun): Re-run
quality/typecheckafter M1 fix to confirm green
The underlying C++ UDC logic — ranking, explicit exclusion, collision guard, pipeline isolation — is correct and sound. This is a configuration-only gap in the parity test harness. Once M1 and M2 are resolved and CI is fully green on f028a43's successor, the PR may proceed to merge.
|
@azizur100389 can you please look into these findings? 🙏 |
|
@magyargergo looked into the bot findings and pushed commit 4c01c84. What changed:
Validation:
The previous |
|
Thanks @azizur100389 ! 🙏 |
2 participants
Fixes #1631
Summary
Tests
Note: local npx tsc --noEmit still reports pre-existing local shared-contract errors around Callsite/scopeResolution typing; this matched the earlier local environment behavior while CI typecheck was green on recent branches.