mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-10-04 02:31:36 +00:00
Fix C++ UDC review findings
This commit is contained in:
parent
442f5dbc5d
commit
f028a4388e
12 changed files with 193 additions and 20 deletions
|
|
@ -53,6 +53,10 @@ export interface SymbolDefinition {
|
|||
* `ScopeResolver.constraintCompatibility` hook during overload narrowing.
|
||||
* Absent for symbols that have no constraints (the common case). */
|
||||
templateConstraints?: unknown;
|
||||
/** True when the producing language marked this callable as explicit.
|
||||
* Currently used by C++ overload ranking to exclude explicit constructors
|
||||
* from implicit user-defined conversion candidates. */
|
||||
isExplicit?: boolean;
|
||||
/** Links Method/Constructor/Property to owning Class/Struct/Trait nodeId */
|
||||
ownerId?: string;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -126,6 +126,13 @@ export function emitCppScopeCaptures(
|
|||
JSON.stringify(arity.parameterTypeClasses),
|
||||
);
|
||||
}
|
||||
if (hasExplicitSpecifier(fnNode)) {
|
||||
grouped['@declaration.is-explicit'] = syntheticCapture(
|
||||
'@declaration.is-explicit',
|
||||
fnNode,
|
||||
'true',
|
||||
);
|
||||
}
|
||||
|
||||
// Detect static storage class (file-local linkage)
|
||||
if (hasStaticStorageClass(fnNode)) {
|
||||
|
|
@ -1542,6 +1549,20 @@ function extractDeclaratorLeafName(node: SyntaxNode): string | null {
|
|||
return null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Check if a C++ declaration has an `explicit` specifier. Tree-sitter-cpp
|
||||
* exposes `explicit` as a direct keyword child on constructor declarations in
|
||||
* current grammar builds; the bounded text prefix keeps this resilient across
|
||||
* small grammar shape differences without scanning whole function bodies.
|
||||
*/
|
||||
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));
|
||||
}
|
||||
|
||||
/**
|
||||
* Check if a C++ function_definition or declaration has `static` storage class.
|
||||
*/
|
||||
|
|
|
|||
|
|
@ -225,6 +225,12 @@ const CPP_SCOPE_QUERY = `
|
|||
declarator: (function_declarator
|
||||
declarator: (field_identifier) @declaration.name))) @declaration.method
|
||||
|
||||
;; Constructor prototype in class body: User(int id);
|
||||
(field_declaration_list
|
||||
(declaration
|
||||
declarator: (function_declarator
|
||||
declarator: (identifier) @declaration.name)) @declaration.method)
|
||||
|
||||
;; Method prototype with reference return: User& getRef();
|
||||
(field_declaration
|
||||
declarator: (reference_declarator
|
||||
|
|
|
|||
|
|
@ -116,8 +116,8 @@ export const cppScopeResolver: ScopeResolver = {
|
|||
// associated namespace for Koenig lookup.
|
||||
populateCppAssociatedNamespaces(parsed);
|
||||
// Build conservative one-step user-defined conversion facts for
|
||||
// overload ranking (#1631): converting constructors and conversion
|
||||
// operators only, with no chaining or explicit-constructor handling.
|
||||
// overload ranking (#1631): implicit converting constructors only,
|
||||
// with no chaining or conversion-operator handling.
|
||||
populateCppUserDefinedConversions(parsed);
|
||||
},
|
||||
|
||||
|
|
|
|||
|
|
@ -3,9 +3,19 @@ import type { ScopeId } from 'gitnexus-shared';
|
|||
import { normalizeCppParamType } from './arity-metadata.js';
|
||||
|
||||
const userDefinedConversions = new Set<string>();
|
||||
const pendingUserDefinedConversions: PendingUserDefinedConversion[] = [];
|
||||
const classIdentitiesBySimpleName = new Map<string, Set<string>>();
|
||||
|
||||
interface PendingUserDefinedConversion {
|
||||
readonly argType: string;
|
||||
readonly paramType: string;
|
||||
readonly ownerClassName: string;
|
||||
}
|
||||
|
||||
export function clearCppUserDefinedConversions(): void {
|
||||
userDefinedConversions.clear();
|
||||
pendingUserDefinedConversions.length = 0;
|
||||
classIdentitiesBySimpleName.clear();
|
||||
}
|
||||
|
||||
export function hasCppUserDefinedConversion(argType: string, paramType: string): boolean {
|
||||
|
|
@ -16,6 +26,12 @@ export function populateCppUserDefinedConversions(parsed: ParsedFile): void {
|
|||
const scopesById = new Map<ScopeId, (typeof parsed.scopes)[number]>();
|
||||
for (const scope of parsed.scopes) scopesById.set(scope.id, scope);
|
||||
|
||||
for (const classScope of parsed.scopes) {
|
||||
if (classScope.kind !== 'Class') continue;
|
||||
const classDef = classScope.ownedDefs.find(isClassLike);
|
||||
if (classDef !== undefined) recordClassIdentity(classDef);
|
||||
}
|
||||
|
||||
for (const classScope of parsed.scopes) {
|
||||
if (classScope.kind !== 'Class') continue;
|
||||
const classDef = classScope.ownedDefs.find(isClassLike);
|
||||
|
|
@ -27,16 +43,13 @@ export function populateCppUserDefinedConversions(parsed: ParsedFile): void {
|
|||
for (const def of methodDefs) {
|
||||
const simpleName = simpleNameOf(def);
|
||||
if (simpleName === className && def.parameterTypes?.length === 1) {
|
||||
registerCppUserDefinedConversion(def.parameterTypes[0], className);
|
||||
continue;
|
||||
}
|
||||
|
||||
const operatorTarget = conversionOperatorTarget(simpleName);
|
||||
if (operatorTarget !== undefined && def.parameterTypes?.length === 0) {
|
||||
registerCppUserDefinedConversion(className, operatorTarget);
|
||||
if (def.isExplicit === true) continue;
|
||||
registerPendingCppUserDefinedConversion(def.parameterTypes[0], className, className);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
rebuildCppUserDefinedConversions();
|
||||
}
|
||||
|
||||
export function registerCppUserDefinedConversion(argType: string, paramType: string): void {
|
||||
|
|
@ -67,17 +80,49 @@ function collectClassMethodDefs(
|
|||
return methods;
|
||||
}
|
||||
|
||||
function conversionOperatorTarget(simpleName: string): string | undefined {
|
||||
const match = /^operator\s+(.+)$/.exec(simpleName);
|
||||
if (match === null) return undefined;
|
||||
const target = normalizeCppParamType(match[1]);
|
||||
return target.length > 0 ? target : undefined;
|
||||
}
|
||||
|
||||
function conversionKey(argType: string, paramType: string): string {
|
||||
return `${argType}\0${paramType}`;
|
||||
}
|
||||
|
||||
function registerPendingCppUserDefinedConversion(
|
||||
argType: string,
|
||||
paramType: string,
|
||||
ownerClassName: string,
|
||||
): void {
|
||||
if (argType === '' || paramType === '') return;
|
||||
if (argType === paramType) return;
|
||||
pendingUserDefinedConversions.push({ argType, paramType, ownerClassName });
|
||||
}
|
||||
|
||||
function rebuildCppUserDefinedConversions(): void {
|
||||
userDefinedConversions.clear();
|
||||
for (const conversion of pendingUserDefinedConversions) {
|
||||
if (isAmbiguousClassName(conversion.ownerClassName)) continue;
|
||||
userDefinedConversions.add(conversionKey(conversion.argType, conversion.paramType));
|
||||
}
|
||||
}
|
||||
|
||||
function recordClassIdentity(def: SymbolDefinition): void {
|
||||
const simpleName = normalizedSimpleName(def);
|
||||
if (simpleName === '') return;
|
||||
const identities = classIdentitiesBySimpleName.get(simpleName) ?? new Set<string>();
|
||||
identities.add(normalizedQualifiedClassName(def));
|
||||
classIdentitiesBySimpleName.set(simpleName, identities);
|
||||
}
|
||||
|
||||
function isAmbiguousClassName(simpleName: string): boolean {
|
||||
return (classIdentitiesBySimpleName.get(simpleName)?.size ?? 0) > 1;
|
||||
}
|
||||
|
||||
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('.');
|
||||
}
|
||||
|
||||
function normalizedSimpleName(def: SymbolDefinition): string {
|
||||
return normalizeCppParamType(simpleNameOf(def));
|
||||
}
|
||||
|
|
|
|||
|
|
@ -574,6 +574,7 @@ function buildDefFromDeclarationMatch(
|
|||
const declaredType = match['@declaration.field-type']?.text;
|
||||
const returnType = match['@declaration.return-type']?.text;
|
||||
const templateConstraints = parseJsonCapture(match['@declaration.template-constraints']);
|
||||
const isExplicit = parseBooleanCapture(match['@declaration.is-explicit']);
|
||||
|
||||
return {
|
||||
nodeId: makeDefId(filePath, anchor.range, type, nameCap.text),
|
||||
|
|
@ -588,6 +589,7 @@ function buildDefFromDeclarationMatch(
|
|||
...(returnType !== undefined ? { returnType } : {}),
|
||||
...(templateArguments !== undefined ? { templateArguments } : {}),
|
||||
...(templateConstraints !== undefined ? { templateConstraints } : {}),
|
||||
...(isExplicit === true ? { isExplicit: true } : {}),
|
||||
};
|
||||
}
|
||||
|
||||
|
|
@ -610,6 +612,13 @@ function parseIntCapture(cap: { readonly text: string } | undefined): number | u
|
|||
return Number.isFinite(n) ? n : undefined;
|
||||
}
|
||||
|
||||
function parseBooleanCapture(cap: { readonly text: string } | undefined): boolean | undefined {
|
||||
if (cap === undefined) return undefined;
|
||||
if (cap.text === 'true') return true;
|
||||
if (cap.text === 'false') return false;
|
||||
return undefined;
|
||||
}
|
||||
|
||||
function parseJsonParameterTypeClassesCapture(
|
||||
cap: { readonly text: string } | undefined,
|
||||
): ParameterTypeClass[] | undefined {
|
||||
|
|
@ -1079,6 +1088,7 @@ const KNOWN_SUB_TAGS: ReadonlySet<string> = new Set<string>([
|
|||
'@declaration.parameter-types',
|
||||
'@declaration.parameter-type-classes',
|
||||
'@declaration.template-constraints',
|
||||
'@declaration.is-explicit',
|
||||
]);
|
||||
|
||||
/**
|
||||
|
|
|
|||
15
gitnexus/test/fixtures/lang-resolution/cpp-overload-udc-namespace-collision/lib.cpp
vendored
Normal file
15
gitnexus/test/fixtures/lang-resolution/cpp-overload-udc-namespace-collision/lib.cpp
vendored
Normal file
|
|
@ -0,0 +1,15 @@
|
|||
#include "lib.h"
|
||||
|
||||
namespace alpha {
|
||||
|
||||
Other::Other(int value) {}
|
||||
void Service::f(Token value) {}
|
||||
void Service::f(Other value) {}
|
||||
|
||||
} // namespace alpha
|
||||
|
||||
namespace beta {
|
||||
|
||||
Token::Token(int value) {}
|
||||
|
||||
} // namespace beta
|
||||
31
gitnexus/test/fixtures/lang-resolution/cpp-overload-udc-namespace-collision/lib.h
vendored
Normal file
31
gitnexus/test/fixtures/lang-resolution/cpp-overload-udc-namespace-collision/lib.h
vendored
Normal file
|
|
@ -0,0 +1,31 @@
|
|||
#pragma once
|
||||
|
||||
namespace alpha {
|
||||
|
||||
class Token {};
|
||||
|
||||
class Other {
|
||||
public:
|
||||
Other(int value);
|
||||
};
|
||||
|
||||
class Service {
|
||||
public:
|
||||
void f(Token value);
|
||||
void f(Other value);
|
||||
|
||||
void run() {
|
||||
f(42);
|
||||
}
|
||||
};
|
||||
|
||||
} // namespace alpha
|
||||
|
||||
namespace beta {
|
||||
|
||||
class Token {
|
||||
public:
|
||||
Token(int value);
|
||||
};
|
||||
|
||||
} // namespace beta
|
||||
|
|
@ -3,9 +3,12 @@
|
|||
Wrap::Wrap(int value) {}
|
||||
WrapA::WrapA(int value) {}
|
||||
WrapB::WrapB(int value) {}
|
||||
ExplicitWrap::ExplicitWrap(int value) {}
|
||||
|
||||
void Service::f(Wrap value) {}
|
||||
void Service::f(double value) {}
|
||||
void Service::g(Wrap value) {}
|
||||
void Service::h(WrapA value) {}
|
||||
void Service::h(WrapB value) {}
|
||||
void Service::e(Wrap value) {}
|
||||
void Service::e(ExplicitWrap value) {}
|
||||
|
|
|
|||
|
|
@ -15,6 +15,11 @@ public:
|
|||
WrapB(int value);
|
||||
};
|
||||
|
||||
class ExplicitWrap {
|
||||
public:
|
||||
explicit ExplicitWrap(int value);
|
||||
};
|
||||
|
||||
class Service {
|
||||
public:
|
||||
void f(Wrap value);
|
||||
|
|
@ -22,10 +27,13 @@ public:
|
|||
void g(Wrap value);
|
||||
void h(WrapA value);
|
||||
void h(WrapB value);
|
||||
void e(Wrap value);
|
||||
void e(ExplicitWrap value);
|
||||
|
||||
void run() {
|
||||
f(42);
|
||||
g(42);
|
||||
h(42);
|
||||
e(42);
|
||||
}
|
||||
};
|
||||
|
|
|
|||
|
|
@ -2017,6 +2017,35 @@ describe('C++ overload resolution — user-defined conversion rank (#1631)', ()
|
|||
|
||||
expect(hCalls.length).toBe(0);
|
||||
});
|
||||
|
||||
it('e(42) ignores the explicit-constructor overload and keeps the implicit UDC viable', () => {
|
||||
const calls = getRelationships(result, 'CALLS');
|
||||
const eCalls = calls.filter((c) => c.source === 'run' && c.target === 'e');
|
||||
|
||||
expect(eCalls.length).toBe(1);
|
||||
const target = result.graph.getNode(eCalls[0].rel.targetId);
|
||||
expect(target?.properties.parameterTypes).toEqual(['Wrap']);
|
||||
});
|
||||
});
|
||||
|
||||
describe('C++ overload resolution — UDC namespace collision guard (#1631)', () => {
|
||||
let result: PipelineResult;
|
||||
|
||||
beforeAll(async () => {
|
||||
result = await runPipelineFromRepo(
|
||||
path.join(FIXTURES, 'cpp-overload-udc-namespace-collision'),
|
||||
() => {},
|
||||
);
|
||||
}, 60000);
|
||||
|
||||
it('does not let beta::Token(int) tie the valid alpha::Other(int) conversion', () => {
|
||||
const calls = getRelationships(result, 'CALLS');
|
||||
const fCalls = calls.filter((c) => c.source === 'run' && c.target === 'f');
|
||||
|
||||
expect(fCalls.length).toBe(1);
|
||||
const target = result.graph.getNode(fCalls[0].rel.targetId);
|
||||
expect(target?.properties.parameterTypes).toEqual(['Other']);
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
|
|
|
|||
|
|
@ -1,4 +1,4 @@
|
|||
import { describe, expect, it } from 'vitest';
|
||||
import { afterEach, describe, expect, it } from 'vitest';
|
||||
import type { ParameterTypeClass, SymbolDefinition } from 'gitnexus-shared';
|
||||
import { cppConversionRank } from '../../../../src/core/ingestion/languages/cpp/conversion-rank.js';
|
||||
import {
|
||||
|
|
@ -44,6 +44,10 @@ const mkDef = (
|
|||
parameterTypeClasses: [...parameterTypeClasses],
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
clearCppUserDefinedConversions();
|
||||
});
|
||||
|
||||
describe('cppConversionRank pointer/nullptr/ellipsis ranks (#1637)', () => {
|
||||
it('ranks nullptr -> T* ahead of nullptr -> bool', () => {
|
||||
expect(cppConversionRank('null', 'int', value('null'), pointer('int'))).toBe(2);
|
||||
|
|
@ -72,8 +76,6 @@ describe('cppConversionRank user-defined conversion ranks (#1631)', () => {
|
|||
|
||||
expect(cppConversionRank('int', 'Wrap', value('int'), value('Wrap'))).toBe(4);
|
||||
expect(cppConversionRank('int', 'double', value('int'), value('double'))).toBe(2);
|
||||
|
||||
clearCppUserDefinedConversions();
|
||||
});
|
||||
|
||||
it('keeps tied user-defined conversion candidates ambiguous', () => {
|
||||
|
|
@ -90,7 +92,6 @@ describe('cppConversionRank user-defined conversion ranks (#1631)', () => {
|
|||
});
|
||||
|
||||
expect(result.map((d) => d.nodeId)).toEqual(['h:WrapA', 'h:WrapB']);
|
||||
clearCppUserDefinedConversions();
|
||||
});
|
||||
});
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue