mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-09-22 00:31:17 +00:00
fix: address second review findings 1-4 — isFileLocalDef hook, singleton docs, fn-ptr typedef, static isolation test
Finding 1: Added `isFileLocalDef` hook to ScopeResolver contract + implementation in free-call-fallback.ts to filter C static functions from global free-call fallback. Threads caller filePath through pickUniqueGlobalCallable so static defs in other files are excluded. Finding 2: Documented single-invocation assumption on staticNames Map. Added clearStaticNames() call in loadResolutionConfig to prevent cross-repo contamination in server-mode scenarios. Finding 3: Added tree-sitter query pattern for function pointer typedef aliases (typedef void (*callback)(int, int)) in query.ts. Added unit test. Finding 4: Added c-static-isolation integration fixture (a.c with static helper, b.c with non-static helper, caller.c) and test asserting no CALLS edge from caller to a.c's static helper. Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/5b948327-9f59-4ca2-9d8c-8c8087feb510 Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>
This commit is contained in:
parent
7d8f885177
commit
e28260cf40
12 changed files with 151 additions and 6 deletions
|
|
@ -67,6 +67,13 @@ const C_SCOPE_QUERY = `
|
|||
(type_definition
|
||||
declarator: (type_identifier) @declaration.name) @declaration.typedef
|
||||
|
||||
;; Declarations — typedef for function pointers: typedef void (*callback)(int, int)
|
||||
(type_definition
|
||||
declarator: (function_declarator
|
||||
declarator: (parenthesized_declarator
|
||||
(pointer_declarator
|
||||
declarator: (type_identifier) @declaration.name)))) @declaration.typedef
|
||||
|
||||
;; Declarations — struct fields
|
||||
(field_declaration
|
||||
declarator: (field_identifier) @declaration.name) @declaration.field
|
||||
|
|
|
|||
|
|
@ -1,4 +1,4 @@
|
|||
import type { ParsedFile } from 'gitnexus-shared';
|
||||
import type { ParsedFile, SymbolDefinition } from 'gitnexus-shared';
|
||||
import { SupportedLanguages } from 'gitnexus-shared';
|
||||
import { buildMro, defaultLinearize } from '../../scope-resolution/passes/mro.js';
|
||||
import { populateClassOwnedMembers } from '../../scope-resolution/scope/walkers.js';
|
||||
|
|
@ -6,7 +6,7 @@ import type { ScopeResolver } from '../../scope-resolution/contract/scope-resolv
|
|||
import { cProvider } from '../c-cpp.js';
|
||||
import { cArityCompatibility, cMergeBindings, resolveCImportTarget } from './index.js';
|
||||
import { scanHeaderFiles } from './header-scan.js';
|
||||
import { expandCWildcardNames } from './static-linkage.js';
|
||||
import { expandCWildcardNames, isStaticName, clearStaticNames } from './static-linkage.js';
|
||||
|
||||
/**
|
||||
* C `ScopeResolver` registered in `SCOPE_RESOLVERS` and consumed by
|
||||
|
|
@ -24,7 +24,12 @@ export const cScopeResolver: ScopeResolver = {
|
|||
languageProvider: cProvider,
|
||||
importEdgeReason: 'c-scope: include',
|
||||
|
||||
loadResolutionConfig: (repoPath: string) => scanHeaderFiles(repoPath),
|
||||
loadResolutionConfig: (repoPath: string) => {
|
||||
// Clear stale static-linkage data from any previous invocation to
|
||||
// prevent cross-repo contamination in server-mode scenarios.
|
||||
clearStaticNames();
|
||||
return scanHeaderFiles(repoPath);
|
||||
},
|
||||
|
||||
resolveImportTarget: (targetRaw, fromFile, allFilePaths, resolutionConfig) => {
|
||||
// Augment allFilePaths with .h files discovered via loadResolutionConfig
|
||||
|
|
@ -59,4 +64,10 @@ export const cScopeResolver: ScopeResolver = {
|
|||
propagatesReturnTypesAcrossImports: false,
|
||||
// C #include brings in all symbols — enable global free call fallback
|
||||
allowGlobalFreeCallFallback: true,
|
||||
// C `static` functions have file-local (translation-unit) linkage —
|
||||
// exclude them from global free-call fallback cross-file resolution.
|
||||
isFileLocalDef: (def: SymbolDefinition) => {
|
||||
const simple = def.qualifiedName?.split('.').pop() ?? def.qualifiedName ?? '';
|
||||
return isStaticName(def.filePath, simple);
|
||||
},
|
||||
};
|
||||
|
|
|
|||
|
|
@ -5,6 +5,11 @@ import type { ParsedFile, ScopeId, SymbolDefinition } from 'gitnexus-shared';
|
|||
* Populated during `emitCScopeCaptures` and consumed by `expandCWildcardNames`
|
||||
* to exclude file-local symbols from cross-file wildcard import visibility.
|
||||
*
|
||||
* NOTE: module-level state, single-process-single-repo use only.
|
||||
* For server-mode or multi-repo-in-one-process use cases, call
|
||||
* `clearStaticNames()` at the start of each resolution pass to avoid
|
||||
* stale static-linkage data from a previous invocation.
|
||||
*
|
||||
* Key: filePath, Value: Set of static function names.
|
||||
*/
|
||||
const staticNames = new Map<string, Set<string>>();
|
||||
|
|
|
|||
|
|
@ -472,6 +472,18 @@ export interface ScopeResolver {
|
|||
*/
|
||||
readonly allowGlobalFreeCallFallback?: boolean;
|
||||
|
||||
/**
|
||||
* Optional predicate to identify definitions with file-local linkage
|
||||
* (e.g. C `static` functions). When provided, `pickUniqueGlobalCallable`
|
||||
* excludes defs where `isFileLocalDef(def) === true` and the def lives
|
||||
* in a different file from the caller. This prevents the global free-call
|
||||
* fallback from creating CALLS edges to file-local symbols that are
|
||||
* logically invisible from the caller's translation unit.
|
||||
*
|
||||
* Languages without file-local linkage semantics leave this undefined.
|
||||
*/
|
||||
readonly isFileLocalDef?: (def: SymbolDefinition) => boolean;
|
||||
|
||||
/**
|
||||
* Optional post-finalize hook to inject cross-file bindings that
|
||||
* aren't modeled via explicit imports. Runs after
|
||||
|
|
|
|||
|
|
@ -36,7 +36,10 @@ export function emitFreeCallFallback(
|
|||
handledSites: Set<string>,
|
||||
model: SemanticModel,
|
||||
workspaceIndex: WorkspaceResolutionIndex,
|
||||
options: { readonly allowGlobalFallback?: boolean } = {},
|
||||
options: {
|
||||
readonly allowGlobalFallback?: boolean;
|
||||
readonly isFileLocalDef?: (def: SymbolDefinition) => boolean;
|
||||
} = {},
|
||||
): number {
|
||||
let emitted = 0;
|
||||
const seen = new Set<string>();
|
||||
|
|
@ -73,7 +76,13 @@ export function emitFreeCallFallback(
|
|||
// the caller does not import the target package. Same-package calls are
|
||||
// caught by findCallableBindingInScope above before reaching here.
|
||||
if (fnDef === undefined && options.allowGlobalFallback === true) {
|
||||
fnDef = pickUniqueGlobalCallable(site.name, model, scopes);
|
||||
fnDef = pickUniqueGlobalCallable(
|
||||
site.name,
|
||||
model,
|
||||
scopes,
|
||||
parsed.filePath,
|
||||
options.isFileLocalDef,
|
||||
);
|
||||
}
|
||||
if (fnDef === undefined) continue;
|
||||
const callerGraphId = resolveCallerGraphId(site.inScope, scopes, nodeLookup);
|
||||
|
|
@ -107,6 +116,8 @@ function pickUniqueGlobalCallable(
|
|||
name: string,
|
||||
model: SemanticModel,
|
||||
scopes: ScopeResolutionIndexes,
|
||||
callerFilePath: string,
|
||||
isFileLocalDef?: (def: SymbolDefinition) => boolean,
|
||||
): SymbolDefinition | undefined {
|
||||
const scopeDefs: SymbolDefinition[] = [];
|
||||
const scopeSeen = new Set<string>();
|
||||
|
|
@ -114,6 +125,15 @@ function pickUniqueGlobalCallable(
|
|||
const simple = def.qualifiedName?.split('.').pop() ?? def.qualifiedName;
|
||||
if (simple !== name) continue;
|
||||
if (def.type !== 'Function' && def.type !== 'Method' && def.type !== 'Constructor') continue;
|
||||
// Skip file-local defs (e.g. C `static` functions) that live in a
|
||||
// different file from the caller — they are logically invisible.
|
||||
if (
|
||||
isFileLocalDef !== undefined &&
|
||||
def.filePath !== callerFilePath &&
|
||||
isFileLocalDef(def)
|
||||
) {
|
||||
continue;
|
||||
}
|
||||
const key = logicalCallableKey(def);
|
||||
if (scopeSeen.has(key)) continue;
|
||||
scopeSeen.add(key);
|
||||
|
|
|
|||
|
|
@ -261,7 +261,10 @@ export function runScopeResolution(
|
|||
handledSites,
|
||||
readonlyModel,
|
||||
workspaceIndex,
|
||||
{ allowGlobalFallback: provider.allowGlobalFreeCallFallback === true },
|
||||
{
|
||||
allowGlobalFallback: provider.allowGlobalFreeCallFallback === true,
|
||||
isFileLocalDef: provider.isFileLocalDef,
|
||||
},
|
||||
);
|
||||
const { emitted, skipped } = emitReferencesViaLookup(
|
||||
graph,
|
||||
|
|
|
|||
9
gitnexus/test/fixtures/lang-resolution/c-static-isolation/a.c
vendored
Normal file
9
gitnexus/test/fixtures/lang-resolution/c-static-isolation/a.c
vendored
Normal file
|
|
@ -0,0 +1,9 @@
|
|||
/* a.c — contains a static (file-local) helper function.
|
||||
* This function must NOT be resolvable from caller.c. */
|
||||
static int helper(void) {
|
||||
return 42;
|
||||
}
|
||||
|
||||
int public_a(void) {
|
||||
return helper();
|
||||
}
|
||||
11
gitnexus/test/fixtures/lang-resolution/c-static-isolation/b.c
vendored
Normal file
11
gitnexus/test/fixtures/lang-resolution/c-static-isolation/b.c
vendored
Normal file
|
|
@ -0,0 +1,11 @@
|
|||
/* b.c — contains a non-static (externally visible) helper function.
|
||||
* This function SHOULD be resolvable from caller.c. */
|
||||
#include "b.h"
|
||||
|
||||
int helper(void) {
|
||||
return 99;
|
||||
}
|
||||
|
||||
int public_b(void) {
|
||||
return helper();
|
||||
}
|
||||
8
gitnexus/test/fixtures/lang-resolution/c-static-isolation/b.h
vendored
Normal file
8
gitnexus/test/fixtures/lang-resolution/c-static-isolation/b.h
vendored
Normal file
|
|
@ -0,0 +1,8 @@
|
|||
/* b.h — public header for b.c */
|
||||
#ifndef B_H
|
||||
#define B_H
|
||||
|
||||
int helper(void);
|
||||
int public_b(void);
|
||||
|
||||
#endif
|
||||
9
gitnexus/test/fixtures/lang-resolution/c-static-isolation/caller.c
vendored
Normal file
9
gitnexus/test/fixtures/lang-resolution/c-static-isolation/caller.c
vendored
Normal file
|
|
@ -0,0 +1,9 @@
|
|||
/* caller.c — includes only b.h, calls helper().
|
||||
* Should resolve to b.c:helper, NOT a.c:static helper. */
|
||||
#include "b.h"
|
||||
|
||||
int main(void) {
|
||||
int x = helper();
|
||||
int y = public_b();
|
||||
return x + y;
|
||||
}
|
||||
|
|
@ -69,3 +69,45 @@ describe('C struct & include resolution', () => {
|
|||
expect(edges).toContain('destroy_service → free_user');
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// C static function isolation — static functions must NOT leak across files
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe('C static function isolation', () => {
|
||||
let result: PipelineResult;
|
||||
|
||||
beforeAll(async () => {
|
||||
result = await runPipelineFromRepo(path.join(FIXTURES, 'c-static-isolation'), () => {});
|
||||
}, 60000);
|
||||
|
||||
it('detects both static and non-static helper functions', () => {
|
||||
const fns = getNodesByLabel(result, 'Function');
|
||||
expect(fns).toContain('helper');
|
||||
expect(fns).toContain('public_a');
|
||||
expect(fns).toContain('public_b');
|
||||
expect(fns).toContain('main');
|
||||
});
|
||||
|
||||
it('caller.c calls b:helper via include, NOT a:static helper', () => {
|
||||
const calls = getRelationships(result, 'CALLS');
|
||||
const edges = edgeSet(calls);
|
||||
|
||||
// caller.c should call public_b (included via b.h)
|
||||
expect(edges).toContain('main → public_b');
|
||||
|
||||
// a.c's static helper calls itself locally
|
||||
expect(edges).toContain('public_a → helper');
|
||||
|
||||
// caller.c should NOT have a CALLS edge to a.c's static helper.
|
||||
// Filter edges to only those originating from main → helper to
|
||||
// verify the correct target file.
|
||||
const mainToHelper = calls.filter(
|
||||
(r) => r.source === 'main' && r.target === 'helper',
|
||||
);
|
||||
// If a main→helper edge exists, it should point to b.c, not a.c
|
||||
for (const edge of mainToHelper) {
|
||||
expect(edge.targetFilePath).not.toContain('a.c');
|
||||
}
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -159,6 +159,14 @@ describe('emitCScopeCaptures — other declarations', () => {
|
|||
expect(m!['@declaration.name'].text).toBe('MyInt');
|
||||
});
|
||||
|
||||
it('captures function pointer typedef as @declaration.typedef', () => {
|
||||
const m = findMatch('typedef void (*callback)(int, int);', (t) =>
|
||||
t.includes('@declaration.typedef'),
|
||||
);
|
||||
expect(m).toBeDefined();
|
||||
expect(m!['@declaration.name'].text).toBe('callback');
|
||||
});
|
||||
|
||||
it('captures struct field as @declaration.field', () => {
|
||||
const m = findMatch('struct P { int x; };', (t) => t.includes('@declaration.field'));
|
||||
expect(m).toBeDefined();
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue