mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-10-03 02:21:44 +00:00
fix: Codex Round 3 — METHOD_IMPLEMENTS soundness
1. Depth-based nearest-match in findInheritedMethod: level-order BFS stops at the first depth with matches. C extends B extends A where both B.foo and A.foo exist now correctly returns B.foo (nearest) instead of treating the chain as ambiguous. 2. Concrete-source guard: skip Interface/Trait nodes entirely in emitMethodImplementsEdges, and skip abstract methods when building candidate lists. Prevents false edges like interface B.foo → A.foo. 3. Add OVERRIDES to default impact traversal fallback arrays so pre-rename indexes are traversed without explicit relationTypes. 6 new tests: nearest-match chain, deep single-match, interface-extends guard, abstract method skip, abstract+concrete mix, concrete regression.
This commit is contained in:
Gergo Magyar
parent
7149001877
commit
d11130378b
3 changed files with 205 additions and 36 deletions
|
|
@ -479,6 +479,9 @@ function emitMethodImplementsEdges(
|
|||
const classNode = graph.getNode(classId);
|
||||
if (!classNode) continue;
|
||||
|
||||
// Interfaces and traits declare contracts — they don't implement them
|
||||
if (classNode.label === 'Interface' || classNode.label === 'Trait') continue;
|
||||
|
||||
// Get this class's own methods
|
||||
const ownMethodIds = methodMap.get(classId) ?? [];
|
||||
|
||||
|
|
@ -490,6 +493,8 @@ function emitMethodImplementsEdges(
|
|||
for (const methodId of ownMethodIds) {
|
||||
const methodNode = graph.getNode(methodId);
|
||||
if (!methodNode || methodNode.label === 'Property') continue;
|
||||
// Abstract methods don't satisfy interface contracts
|
||||
if (methodNode.properties.isAbstract === true) continue;
|
||||
const name = methodNode.properties.name as string;
|
||||
const parameterTypes = (methodNode.properties.parameterTypes as string[] | undefined) ?? [];
|
||||
const parameterCount = methodNode.properties.parameterCount as number | undefined;
|
||||
|
|
@ -632,48 +637,58 @@ function findInheritedMethod(
|
|||
}
|
||||
}
|
||||
|
||||
// Collect all matches across the full BFS, then check for ambiguity.
|
||||
// If the same methodId is reachable via multiple paths (diamond), it's not
|
||||
// ambiguous. Only distinct methodIds count as separate matches.
|
||||
const matches = new Map<string, { methodId: string; parameterTypes: string[] }>();
|
||||
// Level-order BFS: process all ancestors at the current depth before
|
||||
// advancing. Once any match is found at depth D, finish that depth and stop.
|
||||
// Diamond dedup: same methodId via two paths at the same depth = 1 match.
|
||||
let currentLevel = [...queue];
|
||||
|
||||
while (queue.length > 0) {
|
||||
const ancestorId = queue.shift()!;
|
||||
if (visited.has(ancestorId)) continue;
|
||||
visited.add(ancestorId);
|
||||
while (currentLevel.length > 0) {
|
||||
const matches = new Map<string, { methodId: string; parameterTypes: string[] }>();
|
||||
const nextLevel: string[] = [];
|
||||
|
||||
// Check this ancestor's methods
|
||||
const methods = methodMap.get(ancestorId) ?? [];
|
||||
for (const mid of methods) {
|
||||
const mNode = graph.getNode(mid);
|
||||
if (!mNode || mNode.label === 'Property') continue;
|
||||
if (mNode.properties.name !== methodName) continue;
|
||||
for (const ancestorId of currentLevel) {
|
||||
if (visited.has(ancestorId)) continue;
|
||||
visited.add(ancestorId);
|
||||
|
||||
const mParamTypes = (mNode.properties.parameterTypes as string[] | undefined) ?? [];
|
||||
const mParamCount = mNode.properties.parameterCount as number | undefined;
|
||||
if (parameterTypesMatch(mParamTypes, targetParamTypes, mParamCount, targetParamCount)) {
|
||||
matches.set(mid, { methodId: mid, parameterTypes: mParamTypes });
|
||||
// Check this ancestor's methods
|
||||
const methods = methodMap.get(ancestorId) ?? [];
|
||||
for (const mid of methods) {
|
||||
const mNode = graph.getNode(mid);
|
||||
if (!mNode || mNode.label === 'Property') continue;
|
||||
// Abstract inherited methods don't count as concrete implementations
|
||||
if (mNode.properties.isAbstract === true) continue;
|
||||
if (mNode.properties.name !== methodName) continue;
|
||||
|
||||
const mParamTypes = (mNode.properties.parameterTypes as string[] | undefined) ?? [];
|
||||
const mParamCount = mNode.properties.parameterCount as number | undefined;
|
||||
if (parameterTypesMatch(mParamTypes, targetParamTypes, mParamCount, targetParamCount)) {
|
||||
matches.set(mid, { methodId: mid, parameterTypes: mParamTypes });
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Continue walking EXTENDS parents of this ancestor
|
||||
const grandparents = parentMap.get(ancestorId) ?? [];
|
||||
const ancestorEdges = parentEdgeType.get(ancestorId);
|
||||
for (const gp of grandparents) {
|
||||
if (visited.has(gp)) continue;
|
||||
const gpEdge = ancestorEdges?.get(gp);
|
||||
if (gpEdge === 'EXTENDS') {
|
||||
const gpNode = graph.getNode(gp);
|
||||
if (gpNode && gpNode.label !== 'Interface' && gpNode.label !== 'Trait') {
|
||||
queue.push(gp);
|
||||
// Collect EXTENDS parents for the next depth level
|
||||
const grandparents = parentMap.get(ancestorId) ?? [];
|
||||
const ancestorEdges = parentEdgeType.get(ancestorId);
|
||||
for (const gp of grandparents) {
|
||||
if (visited.has(gp)) continue;
|
||||
const gpEdge = ancestorEdges?.get(gp);
|
||||
if (gpEdge === 'EXTENDS') {
|
||||
const gpNode = graph.getNode(gp);
|
||||
if (gpNode && gpNode.label !== 'Interface' && gpNode.label !== 'Trait') {
|
||||
nextLevel.push(gp);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// If any matches found at this depth, decide and stop
|
||||
if (matches.size === 1) return matches.values().next().value!;
|
||||
if (matches.size > 1) return null; // ambiguous at same depth
|
||||
|
||||
currentLevel = nextLevel;
|
||||
}
|
||||
|
||||
// Ambiguous: 2+ distinct methods from different ancestors
|
||||
if (matches.size === 1) return matches.values().next().value!;
|
||||
return null; // 0 matches or ambiguous (2+)
|
||||
return null; // no matches found
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -1920,11 +1920,27 @@ export class LocalBackend {
|
|||
const rawRelTypes =
|
||||
mappedRelTypes && mappedRelTypes.length > 0
|
||||
? mappedRelTypes.filter((t: string) => VALID_RELATION_TYPES.has(t))
|
||||
: ['CALLS', 'IMPORTS', 'EXTENDS', 'IMPLEMENTS', 'METHOD_OVERRIDES', 'METHOD_IMPLEMENTS'];
|
||||
: [
|
||||
'CALLS',
|
||||
'IMPORTS',
|
||||
'EXTENDS',
|
||||
'IMPLEMENTS',
|
||||
'METHOD_OVERRIDES',
|
||||
'OVERRIDES',
|
||||
'METHOD_IMPLEMENTS',
|
||||
];
|
||||
const relationTypes =
|
||||
rawRelTypes.length > 0
|
||||
? rawRelTypes
|
||||
: ['CALLS', 'IMPORTS', 'EXTENDS', 'IMPLEMENTS', 'METHOD_OVERRIDES', 'METHOD_IMPLEMENTS'];
|
||||
: [
|
||||
'CALLS',
|
||||
'IMPORTS',
|
||||
'EXTENDS',
|
||||
'IMPLEMENTS',
|
||||
'METHOD_OVERRIDES',
|
||||
'OVERRIDES',
|
||||
'METHOD_IMPLEMENTS',
|
||||
];
|
||||
const includeTests = params.includeTests ?? false;
|
||||
const minConfidence = params.minConfidence ?? 0;
|
||||
|
||||
|
|
@ -2474,11 +2490,27 @@ export class LocalBackend {
|
|||
const rawRelTypes =
|
||||
mappedRelTypes && mappedRelTypes.length > 0
|
||||
? mappedRelTypes.filter((t: string) => VALID_RELATION_TYPES.has(t))
|
||||
: ['CALLS', 'IMPORTS', 'EXTENDS', 'IMPLEMENTS', 'METHOD_OVERRIDES', 'METHOD_IMPLEMENTS'];
|
||||
: [
|
||||
'CALLS',
|
||||
'IMPORTS',
|
||||
'EXTENDS',
|
||||
'IMPLEMENTS',
|
||||
'METHOD_OVERRIDES',
|
||||
'OVERRIDES',
|
||||
'METHOD_IMPLEMENTS',
|
||||
];
|
||||
const relationTypes =
|
||||
rawRelTypes.length > 0
|
||||
? rawRelTypes
|
||||
: ['CALLS', 'IMPORTS', 'EXTENDS', 'IMPLEMENTS', 'METHOD_OVERRIDES', 'METHOD_IMPLEMENTS'];
|
||||
: [
|
||||
'CALLS',
|
||||
'IMPORTS',
|
||||
'EXTENDS',
|
||||
'IMPLEMENTS',
|
||||
'METHOD_OVERRIDES',
|
||||
'OVERRIDES',
|
||||
'METHOD_IMPLEMENTS',
|
||||
];
|
||||
|
||||
try {
|
||||
return await this._runImpactBFS(repo, sym, symType, dir, {
|
||||
|
|
|
|||
|
|
@ -1139,5 +1139,127 @@ describe('computeMRO', () => {
|
|||
const fooEdge = mi.find((e) => e.sourceId === gbFoo);
|
||||
expect(fooEdge).toBeDefined();
|
||||
});
|
||||
|
||||
it('C extends B extends A, B and A both have foo → returns B.foo (nearest)', () => {
|
||||
// I { foo() }, A { foo() }, B extends A { foo() }, C extends B implements I { no foo }
|
||||
const graph = createKnowledgeGraph();
|
||||
addClass(graph, 'I', 'java', 'Interface');
|
||||
addClass(graph, 'A', 'java');
|
||||
addClass(graph, 'B', 'java');
|
||||
addClass(graph, 'C', 'java');
|
||||
addImplements(graph, 'C', 'I');
|
||||
addExtends(graph, 'C', 'B');
|
||||
addExtends(graph, 'B', 'A');
|
||||
addMethod(graph, 'I', 'foo', 'Interface');
|
||||
addMethod(graph, 'A', 'foo');
|
||||
const bFoo = addMethod(graph, 'B', 'foo');
|
||||
// C has NO own foo — nearest is B.foo at depth 1
|
||||
|
||||
computeMRO(graph);
|
||||
const mi = graph.relationships.filter((r) => r.type === 'METHOD_IMPLEMENTS');
|
||||
const fooEdge = mi.find((e) => e.sourceId === bFoo);
|
||||
expect(fooEdge).toBeDefined();
|
||||
// A.foo should NOT be reached
|
||||
const aFooId = generateId('Method', 'A.foo');
|
||||
const aFooEdge = mi.find((e) => e.sourceId === aFooId);
|
||||
expect(aFooEdge).toBeUndefined();
|
||||
});
|
||||
|
||||
it('C extends B extends A, only A has foo → returns A.foo (single match at depth 2)', () => {
|
||||
// I { foo() }, A { foo() }, B extends A { no foo }, C extends B implements I { no foo }
|
||||
const graph = createKnowledgeGraph();
|
||||
addClass(graph, 'I', 'java', 'Interface');
|
||||
addClass(graph, 'A', 'java');
|
||||
addClass(graph, 'B', 'java');
|
||||
addClass(graph, 'C', 'java');
|
||||
addImplements(graph, 'C', 'I');
|
||||
addExtends(graph, 'C', 'B');
|
||||
addExtends(graph, 'B', 'A');
|
||||
addMethod(graph, 'I', 'foo', 'Interface');
|
||||
const aFoo = addMethod(graph, 'A', 'foo');
|
||||
// B has NO foo, C has NO foo — only A.foo at depth 2
|
||||
|
||||
computeMRO(graph);
|
||||
const mi = graph.relationships.filter((r) => r.type === 'METHOD_IMPLEMENTS');
|
||||
const fooEdge = mi.find((e) => e.sourceId === aFoo);
|
||||
expect(fooEdge).toBeDefined();
|
||||
});
|
||||
});
|
||||
|
||||
// ---- METHOD_IMPLEMENTS concrete-source guard ----------------------------
|
||||
describe('METHOD_IMPLEMENTS concrete-source guard', () => {
|
||||
it('interface B extends interface A, B redeclares foo → 0 METHOD_IMPLEMENTS', () => {
|
||||
const graph = createKnowledgeGraph();
|
||||
addClass(graph, 'A', 'java', 'Interface');
|
||||
addClass(graph, 'B', 'java', 'Interface');
|
||||
addInterfaceExtends(graph, 'B', 'A');
|
||||
addMethod(graph, 'A', 'foo', 'Interface');
|
||||
addMethod(graph, 'B', 'foo', 'Interface');
|
||||
|
||||
computeMRO(graph);
|
||||
|
||||
const mi = graph.relationships.filter((r) => r.type === 'METHOD_IMPLEMENTS');
|
||||
expect(mi).toHaveLength(0);
|
||||
});
|
||||
|
||||
it('abstract class C implements I, C has abstract foo → 0 METHOD_IMPLEMENTS for foo', () => {
|
||||
const graph = createKnowledgeGraph();
|
||||
addClass(graph, 'I', 'java', 'Interface');
|
||||
addClass(graph, 'C', 'java', 'Class');
|
||||
addImplements(graph, 'C', 'I');
|
||||
addMethod(graph, 'I', 'foo', 'Interface');
|
||||
|
||||
// Add abstract method manually with isAbstract flag
|
||||
const classId = generateId('Class', 'C');
|
||||
const methodId = generateId('Method', 'C.foo');
|
||||
graph.addNode({
|
||||
id: methodId,
|
||||
label: 'Method',
|
||||
properties: { name: 'foo', filePath: 'src/C.ts', isAbstract: true },
|
||||
});
|
||||
graph.addRelationship({
|
||||
id: generateId('HAS_METHOD', `${classId}->${methodId}`),
|
||||
sourceId: classId,
|
||||
targetId: methodId,
|
||||
type: 'HAS_METHOD',
|
||||
confidence: 1.0,
|
||||
reason: '',
|
||||
});
|
||||
|
||||
computeMRO(graph);
|
||||
|
||||
const mi = graph.relationships.filter((r) => r.type === 'METHOD_IMPLEMENTS');
|
||||
expect(mi).toHaveLength(0);
|
||||
});
|
||||
|
||||
it('abstract class C implements I, C has concrete bar → 1 METHOD_IMPLEMENTS for bar', () => {
|
||||
const graph = createKnowledgeGraph();
|
||||
addClass(graph, 'I', 'java', 'Interface');
|
||||
addClass(graph, 'C', 'java', 'Class');
|
||||
addImplements(graph, 'C', 'I');
|
||||
addMethod(graph, 'I', 'bar', 'Interface');
|
||||
const cBar = addMethod(graph, 'C', 'bar');
|
||||
|
||||
computeMRO(graph);
|
||||
|
||||
const mi = graph.relationships.filter((r) => r.type === 'METHOD_IMPLEMENTS');
|
||||
expect(mi).toHaveLength(1);
|
||||
expect(mi[0].sourceId).toBe(cBar);
|
||||
});
|
||||
|
||||
it('concrete class implements interface → 1 METHOD_IMPLEMENTS (regression)', () => {
|
||||
const graph = createKnowledgeGraph();
|
||||
addClass(graph, 'I', 'java', 'Interface');
|
||||
addClass(graph, 'C', 'java', 'Class');
|
||||
addImplements(graph, 'C', 'I');
|
||||
addMethod(graph, 'I', 'foo', 'Interface');
|
||||
const cFoo = addMethod(graph, 'C', 'foo');
|
||||
|
||||
computeMRO(graph);
|
||||
|
||||
const mi = graph.relationships.filter((r) => r.type === 'METHOD_IMPLEMENTS');
|
||||
expect(mi).toHaveLength(1);
|
||||
expect(mi[0].sourceId).toBe(cFoo);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue