feat(ingestion): Add C++ parameter type class sidecar#1642
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 9135 tests passed 1 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
|
Claude finished @magyargergo's task in 7m 46s —— View job Adversarial Production-Readiness Review — PR #1642Review completed. Adversarial inspection of all 9 changed files, graph identity code, resolver passes, converter rank, and ingestion threading. Todo List
Review Bar for This PRThis review applies the full multi-lane adversarial bar: (A) shared schema + graph identity, (B) C++ arity + type-shape classification, (C) ingestion/capture/worker threading, (D) resolver behavior preservation, (E) tests + CI + hidden Unicode. Each lane is independently merge-blocking. Current PR State
Branch Hygiene AssessmentClean feature/fix PR. Verified locally: Understanding of the ChangeThis PR adds an optional FindingsFinding 1 —
|
| Declarator | Expected | Actual |
|---|---|---|
int (*fp)(int) |
unknown (function pointer) |
indirection: 'pointer', pointerDepth: 2 |
void (*cb)() |
unknown (function pointer) |
indirection: 'pointer', pointerDepth: 1 |
std::vector<int*> v |
indirection: 'value', pointerDepth: 0 |
indirection: 'pointer', pointerDepth: 1 |
int C::* mp |
unknown (pointer-to-member) |
indirection: 'pointer', pointerDepth: 1 |
int (&arr)[3] |
unknown |
indirection: 'lvalue-ref', pointerDepth: 0 |
Issue #1630 explicitly states: "missing sidecar data for unsupported forms such as function pointers or arrays should fall back to unknown rather than misclassify."
Risk: Future #1629/#1637 consumers reading pointerDepth or indirection will trust falsely precise values and may emit wrong conversion ranks or type-trait predicates. This is the highest-risk finding because the sidecar is designed to be trusted by downstream consumers.
Recommended fix: Before setting indirection: 'pointer', detect parenthesized declarators (* suggesting function-pointer or pointer-to-member syntax (regex \(\s*\*) and return unknownTypeClass(base) for those forms. Similarly detect <[^>]*\*[^>]*> for template params containing * and exclude those from the pointer count. This restricts reliable classification to simple scalar/ref/pointer forms.
Blocks merge: No for today (no consumer reads this yet). Yes before #1629/#1637 land without a conservative fallback. Fix this →
Finding 4 — Missing test coverage for edge declarator shapes and invariants
Severity: Low
Evidence: cpp-arity.test.ts covers int, const int*, int&, int&&, int* p (via classifyCppParameterType direct call), and coarse parameterTypes stability. Missing:
- Function pointer parameter returning
unknown(critical given Finding 3) - Array parameter
int arr[]classification int**(double pointer depth 2)const int&(const on a reference — technically cv on the referenced object, not a reference)const int* const p(const pointer to const)std::vector<int*>or any template with embedded*- Explicit assertion:
parameterTypes.length === parameterTypeClasses.length - Variadic function:
parameterTypeClasseshas same count asparameterTypes(including the...slot)
Risk: Without a function-pointer test asserting unknown, the misclassification in Finding 3 cannot be caught by CI.
Recommended fix: Add test cases for the missing shapes listed above.
Blocks merge: No — but required alongside or before Finding 3 fix. Fix this →
PR-Specific Assessment Sections
Shared Schema and Graph Identity Safety — ✅ CLEAR
ParameterTypeClass is correctly exported from gitnexus-shared/src/index.ts:29 alongside SymbolDefinition. parameterTypeClasses is ? optional on SymbolDefinition at line 42. Non-C++ symbols require no migration. Old caches reading a SymbolDefinition without this field will simply have undefined — safe.
Graph identity audit: resolveDefGraphId (graph-bridge/ids.ts:72–115) constructs lookup keys using only qualifiedName, type, parameterTypes, and templateArguments. parameterTypeClasses is not referenced anywhere in graph-bridge/. The makeDefId function in scope-extractor.ts:701–708 uses filePath, range, type, and name — no sidecar. Confirmed: graph node IDs and edge target IDs are unchanged.
C++ parameterTypes Stability — ✅ CLEAR
No duplicate push. The GitHub-rendered diff gave the false impression of two types.push() calls in the non-variadic branch. The actual file at arity-metadata.ts:88–94 contains exactly one types.push(normalizeCppParamType(rawType)) per non-variadic parameter. The typeClasses.push(...) is the sidecar accumulator — a separate array.
normalizeCppParamType is unchanged (arity-metadata.ts:141–176). int, const int*, int&, and int&& all produce coarse int as before. Variadic and C-style ... handling is unchanged.
types.length === typeClasses.length is structurally maintained: every branch of the for (const p of params) loop pushes exactly one element to both types and typeClasses, including the hasEllipsis tail append at lines 97–100.
parameterTypeClasses Sidecar Correctness — ⚠️ MINOR ISSUES
See Findings 2 and 3. The core structure (ParameterTypeClass with base, cv, indirection, pointerDepth) is well-designed. base correctly reuses normalizeCppParamType so it is consistent with the coarse parameterTypes vocabulary. unknownTypeClass is correctly applied for variadic ... parameters. Parsing validation in parseJsonParameterTypeClassesCapture is strict — all enum values and numeric pointerDepth are validated before acceptance.
Classifier Conservatism for cv/ref/pointer forms — ⚠️ NOT CONSERVATIVE ENOUGH
See Findings 2 and 3. Simple forms (int, int*, int&, int&&, const int*) are classified correctly. Complex forms (function pointers, pointer-to-member, template args with *, arrays) produce overconfident results rather than unknown. The cv field cannot distinguish top-level vs. pointee cv. This is a documented gap in Issue #1630 itself (fallback to unknown requirement), not met by this PR for the listed complex forms.
Ingestion/Capture/Worker Data-Flow Completeness — ✅ CLEAR WITH OBSERVATION
Data flow fully traced:
computeCppDeclarationArity→typeClasses(arity-metadata.ts:78–106)emitCppScopeCaptures→syntheticCapture('@declaration.parameter-type-classes', fnNode, JSON.stringify(arity.parameterTypeClasses))(captures.ts:117–122)scope-extractor.ts:549–551→parseJsonParameterTypeClassesCapture(match['@declaration.parameter-type-classes'])→ stored inlocalDefsparsing-processor.ts:131→parameterTypeClasses: sym.parameterTypeClassesinmergeChunkResultsparse-worker.ts:2310→parameterTypeClasses: methodProps.parameterTypeClasses(observation: this isundefinedfor C++ becauseMethodInfodoesn't carryparameterTypeClasses, but this is acceptable — the sidecar lands inparsedFilesvia the scope-capture path, not in symbol table entries from the legacy DAG path)
Observation: The legacy DAG / methodExtractor symbol table path does not populate parameterTypeClasses. The sidecar is only available in ParsedFile.localDefs (scope-resolution path). This is architecturally intentional given the stated C++ migration to scope-resolution primary, but it means a consumer reading SymbolDefinition.parameterTypeClasses from the legacy symbol table index will see undefined. Future consumers must read from scope-resolution artifacts, not the legacy symbol table.
Resolver Behavior Preservation — ✅ CLEAR
overload-narrowing.ts, conversion-rank.ts, free-call-fallback.ts, receiver-bound-calls.ts, constraint-filter (absent), and scope-resolver.ts — none consume parameterTypeClasses. Confirmed by exhaustive grep across gitnexus/src/core/ingestion/scope-resolution/ and gitnexus/src/core/ingestion/languages/cpp/ with zero matches outside the producer files. ADL behavior, conversion sequence ranking (PR #1606), and legacy parity are unaffected.
Relationship to #1629, #1637, #1578/#1606, and #1579 — ADEQUATE FOUNDATION WITH CAVEATS
- cpp: type-class sidecar for normalizeCppParamType (cv/ref/pointer) #1630 (this PR): The sidecar schema and threading is implemented. The
unknownfallback requirement for unsupported forms is partially met (variadic/pack: ✅, function pointers/templates/arrays: ❌, see Finding 3). - cpp SFINAE: expand type_traits predicate registry (Tier A) #1629 (type-traits predicate registry): The
indirectionandcvfields provide the shape needed forstd::is_same_v<T, int*>vs.std::is_same_v<T, int&>discrimination. But before cpp SFINAE: expand type_traits predicate registry (Tier A) #1629 consumescv, the coarse-cv limitation (Finding 2) must be documented or fixed. - cpp overload resolution: pointer, nullptr, and ellipsis conversion ranks #1637 (pointer/nullptr/ellipsis conversion ranks):
pointerDepthis available. However,pointerDepthis unreliable for function pointers and templates (Finding 3) — cpp overload resolution: pointer, nullptr, and ellipsis conversion ranks #1637 should add conservative guards before trustingpointerDepth. - cpp overload resolution: standard-conversion-sequence ranking #1578/feat(cpp): standard-conversion-sequence ranking for overload resolution #1606 (standard-conversion-sequence ranking): Not affected. Conversion ranking does not read
parameterTypeClasses. No regression. - cpp overload resolution: SFINAE / concept-constrained candidate filtering #1579 (SFINAE/concept filtering): Not claimed and not affected.
Tests and CI Coverage — ✅ CI CLEAR; TESTS INCOMPLETE
CI: 9108/9108 tests passed (all platforms). Typecheck clean. The cpp parity scope is covered within the test suite.
Unit tests cover the essential happy-path shapes. Missing cases (Finding 4) mean Finding 3's misclassification is not caught by CI. The three most important additions before #1629/#1637 land: function-pointer → unknown, template-with-pointer-arg → value not misclassified, and the parameterTypes.length === parameterTypeClasses.length invariant assertion.
Hidden Unicode / Hygiene — ✅ CLEAR
No bidi control characters found in any changed file. Non-ASCII characters in comments (—, →) are em-dash and right-arrow in JSDoc comments, consistent with existing repository style throughout scope-extractor.ts and other files. The GitHub "hidden Unicode" warning appears to be triggered by these comment characters, not by any executable code or bidi controls.
Final Verdict
production-ready with minor follow-ups
The merge-blocking concerns from the DoD are all clear: (1) parameterTypes are not duplicated — the visible GitHub diff artifact is NOT present in the actual file; (2) graph node IDs are provably unchanged — resolveDefGraphId does not touch parameterTypeClasses; (3) no resolver consumes the sidecar; (4) no bidi controls; (5) CI is fully green at 9108/9108. The schema is additive, optional, and backward-compatible. The ingestion pipeline threading is complete for the scope-resolution path.
The non-blocking follow-ups that MUST land before #1629 or #1637 consume the sidecar are: (a) restrict classifier to return unknownTypeClass for function-pointer syntax ((*), templates with * inside angle-brackets, and pointer-to-member declarators (Finding 3); (b) clarify the cv field documentation to state it is a coarse "any-cv-present" signal, not a decomposed top-level vs. pointee cv (Finding 2); (c) add @declaration.parameter-type-classes to KNOWN_SUB_TAGS in scope-extractor.ts (Finding 1); and (d) add unit tests for function-pointer and template-pointer edge cases plus a length-parity assertion (Finding 4). None of these block today's metadata-only merge, but if any of findings 2–3 are unaddressed when #1629/#1637 land, they become merge-blocking at that point.
…i#1642) Co-authored-by: Gergő Magyar <gergomagyar@icloud.com>
Summary
parameterTypeClassessidecar onSymbolDefinitionparameterTypesunchangedFixes #1630
Tests
Note:
npx gitnexus impact .../detect_changes ...currently exit 1 with no output in this checkout because the local GitNexus index/registry is unavailable, so the change was kept narrowly scoped and validated manually with the C++ resolver suites.