feat(cpp): sfinae filter#1623
Conversation
…le_if_t / requires constraints fail (abhigyanpatwari#1579)
|
@zander-raycraft 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 9133 tests passed 1 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
|
Claude finished @magyargergo's task in 10m 40s —— View job Adversarial Production-Readiness Review — PR #1623 (
|
|
@zander-raycraft Can you please address claude's findings? 🙏 |
…d char support, an unqualified F1 test fixture, and parameter-lookup gap documentation (abhigyanpatwari#1579) -> claude feedback
…ith abhigyanpatwari#1606 conversion-rank scoring
|
Claude finished @magyargergo's task in 4m 59s —— View job Adversarial Production-Readiness Review — PR #1623 (
|
| Category | Details |
|---|---|
| Conforms | Mutually exclusive is_integral_v<T> / is_floating_point_v<T> via F2 enable_if_t and F4 requires clause. bool and char correctly treated as integral per ISO §21.3.4 Table 48 (fixed in ca26c3f). is_arithmetic_v correctly spans integral and floating. Kleene AND/OR/NOT logic matches ISO [temp.constr.normal]. |
| Approximate but acceptable | is_same_v strips cv/ref/pointer via normalizeCppParamType — is_same_v<const T, T> returns 'compatible' rather than ISO-correct false. Explicitly documented with JSDoc note in constraint-filter.ts:55–60, conservative direction (over-permits, never emits wrong edges). |
| Unsupported but conservative | F3 (void-default typename = enable_if_t<P>), F5 (trailing requires), requires_expression blocks, decltype, user-defined predicates, alias templates, named concepts, constrained template parameters. All return {kind:'unknown'} → filter keeps candidate. |
| Unsupported and risky | None found. The implementation never treats an unrecognized constraint as satisfied. |
Quantitative scope: ~60% of real-world SFINAE cases per the PR description. The remaining 40% degrade conservatively to ambiguity suppression.
Findings
Finding 1 — long double literals classified as 'floating' but only via 'double' normalization
Severity: Minor, approximation
Evidence: inferCppLiteralType (captures.ts:686) returns 'double' for any number_literal containing . — including 3.14L. The token therefore reaches classifyType as 'double', which maps to 'floating'. A bare type annotation long double (via a variable declaration) would reach classifyType as the string 'long double', which hits the default case → 'class' → is_floating_point_v returns 'incompatible' (ISO: true).
ISO C++ relevance: is_floating_point_v<long double> is true per ISO §21.3.5.
GitNexus production risk: Conservative false negative only (no wrong edges). For literals 3.14L, the literal path returns 'double' anyway. The gap only triggers for variable arguments declared as long double — which already has no type resolution per Finding 3 from the previous review.
Recommended fix: Add case 'long double': return 'floating' to classifyType. One-line follow-up.
Blocks merge: no.
Finding 2 — logicalCallableKey does not include templateConstraints in global fallback
Severity: Minor / non-regression
Evidence: free-call-fallback.ts:419–427 — logicalCallableKey builds a dedup key from [filePath, qualifiedName, type, parameterCount, parameterTypes]. Two constrained process<T> overloads (same parameterTypes=['T']) would be collapsed to 1 candidate in pickUniqueGlobalCallable. The global fallback is only reached when both overloads are unreachable via scope-chain walk (i.e., neither overload is included via #include) — an unlikely pattern for C++ SFINAE overloads, which always share a translation unit. Pre-existing behavior, not introduced by this PR.
ISO C++ relevance: N/A — global fallback is a GitNexus approximation layer.
GitNexus production risk: No wrong edges; at worst 0 edges instead of 2 in the degenerate case.
Recommended fix: Add templateConstraintsIdTag(def.templateConstraints) to logicalCallableKey. Follow-up.
Blocks merge: no — pre-existing limitation, same behavior as before this PR.
Finding 3 — settings.local.json contains development-time permissions
Severity: Informational
Evidence: gitnexus/.claude/settings.local.json adds WebFetch(domain:cppreference.net), Bash(gh issue *) etc. — entries that were used during the PR development session for researching cppreference and GitHub issues.
GitNexus production risk: None. This file governs Claude Code's local allowed-tools list, has no effect on the resolver, parser, or CI pipeline.
Recommended fix: Optionally strip development-session entries before merge to keep settings.local.json minimal. Not blocking.
Blocks merge: no.
PR-Specific Assessment Sections
ISO C++ SFINAE / Constraints Rule Compliance
The implementation correctly models SFINAE semantics for the supported subset. The Kleene 3-valued evaluator in constraint-filter.ts:86–130 follows ISO [temp.constr.normal]:
- AND: short-circuits on first
incompatible; returnscompatibleonly when all childrencompatible; otherwiseunknown✅ - OR: short-circuits on first
compatible; returnsincompatibleonly when all childrenincompatible; otherwiseunknown✅ - NOT: flips
compatible↔incompatible, passes throughunknown✅ - unknown predicate: returns
'unknown'(monotonicity safe) ✅
The 'unknown' verdict is monotonicity-safe: unrecognized predicates keep candidates, falling through to the existing isOverloadAmbiguousAfterNormalization suppression. No constraint is ever treated as "satisfied" when it is only unknown.
Curated GitNexus Approximation Boundary
The four-predicate Tier-A registry is precisely documented. The getRegistrySize() export + unit test pins it at 4. Unsupported predicates are explicitly listed in constraint-extractor.ts:12–17 as deferred items. The is_same_v cv-approximation is explicitly flagged in constraint-filter.ts:55–60. No overclaiming of ISO C++ concepts support anywhere in comments or test names. The requires-clause support is correctly labeled "F4 AST shape" throughout, not "full C++20 concepts support."
Constraint Extraction Correctness
Three AST shapes handled:
- F1 (unqualified
enable_if_t<P, int> = 0):template_typebranch inextractEnableIfPredicate✅ - F2 (
std::enable_if_t<P, int> = 0):qualified_identifierunwrap →template_type✅ - F4 (leading
requires_clause):childOfType(templateDecl, 'requires_clause')→parseRequiresClause✅
Both captures.ts and c-cpp.ts walk to the same template_declaration and call extractCppTemplateConstraints → identical JSON → identical FNV-1a hash → identical ~c:<hash> suffix.
Explicitly not supported (all return {kind:'unknown'}): F3 void-default, F5 trailing requires, requires_expression blocks, alias templates, decltype, user-defined traits, named concepts. All fall through conservatively. The F1 unit test (cpp-constraint.test.ts:36) now correctly uses genuinely unqualified enable_if_t (no std:: prefix), independently exercising the template_type-direct branch.
Constraint Evaluator and Type-Traits Registry Correctness
The registry truth tables are ISO-correct for the claimed subset:
is_integral_v:'integral' || 'bool' || 'char'→ true; others → false.boolandcharcorrectly widened viaisIntegralClass()helper (constraint-filter.ts:44–46), sois_same_v<bool, int>still returns'incompatible'(distinct TypeClass enum values) ✅is_floating_point_v: only'floating'→ true ✅is_arithmetic_v:isIntegralClass(c) || c === 'floating'✅is_same_v: coarse token equality, unknown when either side is'unknown', with cv-approximation documented ✅
The verdictFromBool guard (constraint-filter.ts:70–73) returns 'unknown' when cls[0] === 'unknown', preventing false 'incompatible' verdicts for unresolvable argument types.
Candidate Filtering Phase and Overload Integration
Phase ordering (overload-narrowing.ts:87–171):
- Arity filter
- Exact-type filter
- Conversion-rank scoring (step 4b — only fires when exact-type returns 0)
- Constraint filter (step 4c)
For SFINAE template overloads (parameterTypes=['T']): exact-type filter yields 0 matches (argType 'int' ≠ 'T'). Conversion-rank scoring also yields 0 viable candidates because cppConversionRank('int', 'T') returns Infinity (T not in ARITHMETIC). The code correctly falls through (if (ranked.length > 0) result = ranked — not taken), leaving result as the arity-filtered set. Constraint filter then correctly disambiguates to the single ISO-correct overload.
Coverage of all candidate paths:
pickImplicitThisOverload(free-call-fallback.ts:500) →narrowOverloadCandidateswith constraint hook ✅- No-ADL multi-overload path (
free-call-fallback.ts:192) → constraint hook threaded ✅ - Merged ordinary+ADL path (
free-call-fallback.ts:227) → constraint hook threaded ✅ pickOverloadinreceiver-bound-calls.ts:734) → constraint hook threaded ✅pickUniqueGlobalCallable→ no constraint filtering (Finding 2, non-regression)
Disabled candidates cannot reach OVERLOAD_AMBIGUOUS because constraint filter runs before isOverloadAmbiguousAfterNormalization in the caller flow.
Graph Identity / Constraint Fingerprint Safety
The FNV-1a 32-bit hash over JSON.stringify(payload) is deterministic for the same structured CppConstraintPayload. The JSON.stringify is applied to the structured typed payload (not raw source text or AST positions), so field-order stability matches the TypeScript object literal construction in extractCppTemplateConstraints — always the same three fields (templateParams, paramArgIndex, expr) in the same key order.
Fingerprinting is gated on templateConstraints !== undefined, so unconstrained functions are unaffected. resolveDefGraphId tries the constraint-fingerprinted key first only when def.templateConstraints !== undefined (ids.ts:93–103). Old nodes without constraints continue to resolve via the regular qualified or simple-name keys.
FNV-1a 32-bit: 4B possible values, collision probability among SFINAE overloads in one file is negligible in practice (2–3 overloads per name). No security concern; this is a disambiguation tag, not a cryptographic hash.
Shared Contract and Cross-Language Safety
templateConstraints?: unknown in SymbolDefinition is fully optional (symbol-definition.ts:41). constraintCompatibility is optional on both RegistryProviders (context.ts:45) and ScopeResolver (scope-resolver.ts). narrowOverloadCandidates gates the constraint step behind hookCtx?.constraintCompatibility !== undefined && argCount !== undefined — absent hook is a complete pass-through. Candidates without templateConstraints are unconditionally kept (def.templateConstraints === undefined → return true). No TypeScript structural typing forces non-C++ providers to add no-op stubs. The Rust/Swift/TypeScript forward-reference comments use "future" language, not "current."
Free-call Fallback, ADL, Receiver-bound, and #1606 Conversion-rank Interaction
#1606 preservation: conversionRankFn and constraintCompatibility are threaded as separate named fields in OverloadNarrowingHookCtx (overload-narrowing.ts:64–77). The conflict-resolution commit (828ac16) unified hook passing without dropping either field. The constraint filter step 4c is additive — it runs after step 4b (conversion-rank), and the two hooks are independent. All conversion-rank fixtures remain green (CI confirms).
Free-call fallback SFINAE path (free-call-fallback.ts:187–213): The hasConstraints guard correctly gates the targeted narrow path on constrained candidates only. For non-SFINAE overload sets (hasConstraints === false and no conversion-rank fn), the code falls through to fnDef = ordinary[0] — exactly pre-#1579 behavior.
ADL path (free-call-fallback.ts:225–243): merged ordinary+ADL candidates both receive constraint filtering. ADL blockers (nonCallableFound, blockScopeDeclFound) suppress ADL before merge — unaffected.
Receiver-bound (receiver-bound-calls.ts:734): pickOverload passes constraintCompatibility from the provider subset. Constrained method overloads on explicit receivers are filtered.
helpers.ts Legacy DAG Skip Correctness
Three new entries (helpers.ts:200–202):
'enable_if_t<is_integral_v<T>> overload binds only on integral call sites'
'enable_if_t<is_floating_point_v<T>> overload binds only on floating call sites'
'requires-clause overloads disambiguate same as enable_if_t (F4 AST shape)'
These match exactly the it(...) strings at cpp.test.ts:3114, 3125, 3149. Comments reference #1579. Skips apply only when REGISTRY_PRIMARY_CPP=0.
The fourth test ('emits zero CALLS edges when predicate is outside the Tier-A registry') is correctly not in expected failures — legacy also produces 0 edges for unknown-predicate overloads. The fifth test ('emits exactly 1 CALLS edge to the arity-matching overload...') is correctly not in expected failures — arity filter works in legacy mode too.
No stale entries from prior PRs visible. No duplicate or missing entries.
Tests and CI Coverage
Integration fixtures (4 new):
cpp-sfinae-golden: F2enable_if_t. Assertion:targetIds.size === 2proves distinct graph nodes;targetStartLines[0] < targetStartLines[1]proves distinct source positions. Strong identity check ✅cpp-sfinae-requires-clause: F4 shape.targetIds.size === 2✅cpp-sfinae-unknown-predicate:MyCustomTrait_vnot in registry →'unknown'× 2 → both survive →isOverloadAmbiguousAfterNormalization(identicalTparams) → 0 edges ✅cpp-sfinae-arity-survives-unknown: bad-arity overload dropped first → 1 edge ✅
Unit tests (17 in cpp-constraint.test.ts): extractor shapes (F1, F2, F4, unknown-bail), Kleene truth table (AND/OR/NOT), registry surface + is_integral_v bool/char coverage. Registry-size assertion getRegistrySize() === 4 pins the supported scope. 4 monotonicity rows in overload-narrowing.test.ts verify the language-agnostic contract.
CI (all 6 workflows green): CI (9025 tests), CodeQL, Gitleaks, Dependency Review, Docker Build & Push, PR Autofix all completed success.
Hidden Unicode / Hygiene
No bidi Unicode control characters (U+202A–U+202E, U+2066–U+2069) found in any changed file under gitnexus/src/, gitnexus-shared/src/, or gitnexus/test/. Non-ASCII characters in new files are visible punctuation (—, →, ↔) in JSDoc comments — consistent with codebase style. Clean. ✅
Final Verdict
production-ready
This PR correctly implements DoD option 2 (curated type-traits approximation) with honest scope declaration and consistent "degrade not lie" semantics. The golden fixture process(42) / process(3.14) is fixed end-to-end — constraint fingerprinting distinguishes the two overloads in the graph, the Kleene filter correctly marks the incompatible candidate, and the integration tests verify distinct targetId values. The filtering phase order (arity → exact-type → conversion-rank → constraint → ambiguity-suppression) is correct for template overloads with parameterTypes=['T']; the conversion-rank step falls through gracefully when template params produce Infinity rank, leaving the candidate set intact for constraint filtering. The two remaining minor gaps (long double classification, logicalCallableKey missing templateConstraints in global fallback) are both pre-existing conservative false-negatives that produce 0 edges rather than wrong edges. All DoD criteria are satisfied, CI is clean, and hidden Unicode is absent.
* feat(cpp): SFINAE-aware overload filter — drops candidates whose enable_if_t / requires constraints fail (abhigyanpatwari#1579) * fix(cpp): SFINAE follow-ups for is_integral_v/is_arithmetic_v bool and char support, an unqualified F1 test fixture, and parameter-lookup gap documentation (abhigyanpatwari#1579) -> claude feedback * revert: reverting all changes to .md files
SFINAE /
requires-clause aware overload filter (#1579)Summary
enable_if_t<>predicates or C++20requiresclauses now resolve to the correct overload at each call site instead of collapsing into a false multi-candidate ambiguity. The canonical case from the ticket (process(42)for an integral template andprocess(3.14)for a floating template) used to produce zero CALLS edges because both overloads looked viable. It now produces two CALLS edges, one to each overload, matching what a real C++ compiler would resolve.constraintCompatibilityhook on bothRegistryProvidersandScopeResolverthat mirrors the existingarityCompatibilitysurface, along with a new opaquetemplateConstraintsfield onSymbolDefinitionthat carries the per language constraint payload. The hook returns one of three verdicts (compatible, incompatible, or unknown) and the filter only drops a candidate when the verdict is incompatible. This Kleene 3 valued semantics is the monotonicity guarantee: adding a predicate to the registry can only narrow correctly and never produce a wrong edge.<type_traits>predicates (is_integral_v,is_floating_point_v,is_arithmetic_v,is_same_v) with truth tables hardcoded against a coarse type class enum. This satisfies DoD option 2 from the parent ticket and closes roughly 60 percent of real world SFINAE cases without a full constexpr evaluation engine. The remaining six Tier A predicates (is_void_v,is_enum_v, and similar) are scoped as one line follow up adds once thenormalizeCppParamTypecv / ref / pointer sidecar refactor lands.['T']would collapse to the same Function node and the constraint filter would never see two candidates to narrow between. The fingerprint follows the same~tagsuffix pattern thatparameterTypesandtemplateArgumentsalready use elsewhere in the pipeline.requiresclause shape (F4), the monotonicity contract for unrecognized predicates, and the filter ordering invariant that arity always runs before the constraint check.Changed files
gitnexus-shared/.../symbol-definition.tstemplateConstraints?: unknownfield that carries the per language constraint payloadgitnexus-shared/.../registries/context.tsconstraintCompatibilityhook toRegistryProvidersand the newConstraintContextinterfacegitnexus-shared/src/index.tsConstraintContextfrom the public surfacescope-resolution/contract/scope-resolver.tsconstraintCompatibilityhook to theScopeResolverinterfacescope-resolution/passes/overload-narrowing.tshookCtxparameterscope-resolution/passes/receiver-bound-calls.tshookCtxintopickOverloadand addsconstraintCompatibilityto the receiver bound provider subsetscope-resolution/passes/free-call-fallback.tshookCtxintopickImplicitThisOverloadand adds a targeted narrow path for the no ADL multi overload case, gated on whether any candidate has constraints so the local shadows import scope walk semantics are preserved for non SFINAE C++scope-resolution/pipeline/run.tsconstraintCompatibilityfrom the provider through the orchestrator options bag into the free call fallbackscope-resolution/graph-bridge/ids.tsresolveDefGraphIdso SFINAE overloads route to their distinct nodesscope-resolution/graph-bridge/node-lookup.tstemplateConstraintscore/ingestion/utils/template-arguments.tsconstraintsHashhelper (FNV 1a 32 bit) and thetemplateConstraintsIdTagID suffix buildercore/ingestion/parsing-processor.tscore/ingestion/scope-extractor.ts@declaration.template-constraintssynthetic capture and threads it onto the producedSymbolDefinitioncore/ingestion/language-provider.tsextractTemplateConstraints(definitionNode)hook so each language can supply its own constraint payload extractorcore/ingestion/languages/c-cpp.tsextractTemplateConstraintsfor C++ by walking from the function definition up to its enclosingtemplate_declarationand delegating to the constraint extractorlanguages/cpp/type-classifier.ts(new)TypeClassenum so the predicate registry can dispatch in closed formlanguages/cpp/constraint-extractor.ts(new)ConstraintExpr. Anything unrecognized falls back to a kind unknown node, which the filter keeps for monotonicitylanguages/cpp/constraint-filter.ts(new)ConstraintExpr, the 4 predicate Tier A registry, and thecppConstraintCompatibilityhook export. Includes a JSDoc note flagging theis_same_vcv stripping approximation as a known limitation tracked under the sidecar follow uplanguages/cpp/captures.ts@declaration.template-constraintssynthetic capture. Walks from the enclosingtemplate_declarationparent rather than the innerfunction_definitionso inline method templates pick up their constraints correctlylanguages/cpp/scope-resolver.tscppConstraintCompatibilityonto the C++ scope resolverlanguages/cpp/arity-metadata.tsnormalizeCppParamTypeso the constraint filter can reuse the same normalizationlanguages/cpp/arity.tstest/integration/resolvers/cpp.test.tscreateResolverParityIthelpertest/integration/resolvers/helpers.tsLEGACY_RESOLVER_PARITY_EXPECTED_FAILURES.cppfor the positive SFINAE assertions that the legacy DAG cannot satisfytest/unit/scope-resolution/cpp/cpp-constraint.test.ts(new)test/unit/scope-resolution/overload-narrowing.test.tstemplateConstraintsare never consultedtest/fixtures/lang-resolution/cpp-sfinae-golden/(new)process<T>overloads guarded byis_integral_v<T>andis_floating_point_v<T>. Asserts two CALLS edges to distinct overloadstest/fixtures/lang-resolution/cpp-sfinae-requires-clause/(new)requiresshape so the F4 AST path is exercised in integrationtest/fixtures/lang-resolution/cpp-sfinae-unknown-predicate/(new)test/fixtures/lang-resolution/cpp-sfinae-arity-survives-unknown/(new)Architecture Rational
Function templates guarded by mutually exclusive enable_if_t or requires constraints collapse into a false multi-candidate ambiguity in V1, which suppresses CALLS edges where ISO C++ would resolve cleanly. The canonical example is two process overloads, one guarded by is_integral_v, one by is_floating_point_v, where process(42) should resolve only to the integral overload, but V1 sees both as visible and emits zero edges. The root cause is that V1 does not model template constraints at all: every overload is always viable, and the existing OVERLOAD_AMBIGUOUS sentinel correctly refuses to pick arbitrarily, which is safe but blinds the graph to a very common C++ idiom. The fix follows DoD option 2 from the parent ticket, a curated registry of four <type_traits> predicates plus a Kleene 3-valued evaluator, and the plan estimated it as a narrow change focused on the overload narrowing step. Integration showed it had to be wider than that, because the constraint signal travels through four pipeline stages before it can disambiguate anything: extraction at parse time, graph-node ID generation, node lookup across resolver paths, and the narrowing filter itself. The plan assumed upstream stages would carry the constraint payload through transparently, but both process overloads end up with identical parameterTypes=['T'] after normalization, so the legacy parse phase generated the same graph node ID for both and merged them into one, the constraint filter never got two candidates to compare. Once that surfaced, the fix had to fingerprint the constraint payload into the node ID using the same ~tag suffix pattern that already disambiguates parameter types and template arguments, which meant coordinated edits at every site that participates in that pattern. The rest of the “extra” files are seam plumbing for the same root issue: a new language-provider hook so the parsing processor can compute the constraint fingerprint (it reads raw tree-sitter matches, not the synthetic captures used elsewhere), and a small extra narrow path in the free-call fallback because the SFINAE case takes the free-function route rather than the implicit-this route the original plan threaded through. The end result is 11 new files and 15 modified, but there are no new architectural concepts in any of them, every piece mirrors an existing pattern in the codebase, and the wider surface area is the same surface area the next constrained-overload language (Rust trait bounds, Swift where, TypeScript conditional types) will plug into.
Test plan
cpp-constraint.test.tspass across the extractor, Kleene, and registry sectionsoverload-narrowing.test.tspass and pin the language agnostic contractREGISTRY_PRIMARY_CPP=0the suite shows 188 passed and 32 expected skips, including the 3 new SFINAE positive assertions added to the failures list. The legacy DAG lacks the constraint filter and the scope resolver only path is expected to win those assertions exclusivelynpx tsc --noEmitis clean across the monorepoCloses the C++ SFINAE / concept constrained candidate filtering subtask of #1564.