mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-09-23 00:41:36 +00:00
fix(cpp): stop #include from leaking class methods and namespace members (U1)
The C++ registry-primary resolver was emitting impossible CALLS edges for ordinary headers: an including file's unqualified save() resolved to User::save and unqualified foo() resolved to ns::foo. Two leak paths converged on localDefs: 1. expandCppWildcardNames (file-local-linkage.ts) iterated the flattened localDefs and exported every simple tail, including class-owned methods and namespace-contained symbols. Replaced with a scope-aware filter: build nodeId -> owning Scope from Scope.ownedDefs and skip defs whose owning scope is Namespace or Class. 2. The shared global free-call fallback's pickUniqueGlobalCallable walks the workspace registry by simple name and would still hit class methods / namespace members even with wildcard expansion fixed. Plugged the gap via the existing isFileLocalDef hook — semantically 'logically invisible cross-file' — by tracking per- file non-globally-visible nodeIds (populateCppNonGloballyVisible, called from populateOwners) and adding an ownerId !== undefined fast-path for class-owned defs. Side fix in shared finalize-algorithm.ts: when wildcard expansion resolves to a real target but produces zero propagating names, the edge was dropped, taking the file-level IMPORTS edge with it. Preserve the original wildcard edge so #include dependencies survive even when the header exposes no unqualified bindings. Tests: cpp-include-no-class-leak, cpp-include-no-namespace-leak, and cpp-anon-ns-same-file-visible fixtures. Negative tests mode-gated to REGISTRY_PRIMARY_CPP=1 via the expected-failures registry — legacy DAG has no scope-aware filtering on the global fallback; backporting is out of scope. All 2104 resolver integration tests pass under registry-primary mode.
This commit is contained in:
parent
c58187fe09
commit
7e600fe8f2
10 changed files with 295 additions and 11 deletions
|
|
@ -833,7 +833,16 @@ function expandWildcard(
|
|||
if (target === undefined) return [edge];
|
||||
|
||||
const names = hooks.expandsWildcardTo(edge.targetModuleScope, workspace);
|
||||
if (names.length === 0) return [];
|
||||
if (names.length === 0) {
|
||||
// Resolved wildcard with zero propagating names is still a real file-
|
||||
// level dependency (e.g. a C++ header that only declares classes —
|
||||
// `#include` is a valid IMPORTS edge, but unqualified-binding names
|
||||
// are correctly empty since class methods require `Class::method`).
|
||||
// Preserve the original wildcard edge so the file→file IMPORTS edge
|
||||
// survives; downstream binding materialization sees no propagated
|
||||
// names because the edge has no `targetExportedName`/`localName`.
|
||||
return [edge];
|
||||
}
|
||||
|
||||
const expanded: ImportEdge[] = [];
|
||||
for (const name of names) {
|
||||
|
|
|
|||
|
|
@ -1,4 +1,4 @@
|
|||
import type { ParsedFile, ScopeId, SymbolDefinition } from 'gitnexus-shared';
|
||||
import type { ParsedFile, Scope, ScopeId, SymbolDefinition } from 'gitnexus-shared';
|
||||
|
||||
/**
|
||||
* Per-file set of symbol names with file-local linkage.
|
||||
|
|
@ -17,6 +17,23 @@ import type { ParsedFile, ScopeId, SymbolDefinition } from 'gitnexus-shared';
|
|||
*/
|
||||
const fileLocalNames = new Map<string, Set<string>>();
|
||||
|
||||
/**
|
||||
* Per-file set of `SymbolDefinition.nodeId`s that are NOT visible by
|
||||
* unqualified lookup from outside the file — class-owned methods/fields
|
||||
* and namespace-nested symbols. Populated by `populateCppNonGloballyVisible`
|
||||
* during the per-file `populateOwners` hook; consumed by
|
||||
* `isCppDefGloballyVisible` from both `expandCppWildcardNames` (wildcard
|
||||
* propagation) and the global free-call fallback's `isFileLocalDef` hook.
|
||||
*
|
||||
* Tracked per filePath rather than as a single global set so cross-file
|
||||
* lookup correctly compares the candidate's owning file's non-visible
|
||||
* set without leaking across pipeline invocations (the global free-call
|
||||
* fallback checks `def.filePath !== callerFilePath` and then asks "is
|
||||
* this def visible from outside its own file?" — that's exactly what
|
||||
* this set encodes).
|
||||
*/
|
||||
const nonGloballyVisibleNodeIds = new Map<string, Set<string>>();
|
||||
|
||||
/** Record a symbol name as file-local (static or anonymous namespace). */
|
||||
export function markFileLocal(filePath: string, name: string): void {
|
||||
let names = fileLocalNames.get(filePath);
|
||||
|
|
@ -35,12 +52,103 @@ export function isFileLocal(filePath: string, name: string): boolean {
|
|||
/** Clear tracked file-local names (call at start of each resolution pass). */
|
||||
export function clearFileLocalNames(): void {
|
||||
fileLocalNames.clear();
|
||||
nonGloballyVisibleNodeIds.clear();
|
||||
}
|
||||
|
||||
/**
|
||||
* Return the names visible through a C++ wildcard import (`#include`).
|
||||
* All module-scope defs from the target file are visible EXCEPT those
|
||||
* with file-local linkage (static functions/variables, anonymous namespace symbols).
|
||||
* Populate per-file "not globally visible" nodeIds by walking the parsed
|
||||
* file's scopes. Run as part of the `populateOwners` hook so every C++
|
||||
* scope is reflected before any cross-file resolution pass consults the
|
||||
* set.
|
||||
*
|
||||
* A def is "not globally visible" when its nearest structurally enclosing
|
||||
* scope is a `Namespace` or `Class` — those require qualification
|
||||
* (`ns::name`, `Class::method`) for cross-file unqualified lookup.
|
||||
* Module-scoped defs remain globally visible.
|
||||
*/
|
||||
export function populateCppNonGloballyVisible(parsed: {
|
||||
readonly filePath: string;
|
||||
readonly scopes: readonly { readonly kind: string; readonly ownedDefs: readonly { readonly nodeId: string }[] }[];
|
||||
}): void {
|
||||
let set = nonGloballyVisibleNodeIds.get(parsed.filePath);
|
||||
if (set === undefined) {
|
||||
set = new Set<string>();
|
||||
nonGloballyVisibleNodeIds.set(parsed.filePath, set);
|
||||
}
|
||||
for (const scope of parsed.scopes) {
|
||||
if (scope.kind !== 'Namespace' && scope.kind !== 'Class') continue;
|
||||
for (const def of scope.ownedDefs) {
|
||||
set.add(def.nodeId);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Check whether a def is visible by unqualified lookup from outside its
|
||||
* own file. Returns `false` for class-owned and namespace-nested defs.
|
||||
*
|
||||
* Used by the global free-call fallback's `isFileLocalDef` hook (which
|
||||
* historically meant "static / anonymous-namespace" but semantically
|
||||
* stands for "logically invisible cross-file"). Including class methods
|
||||
* and namespace members under the same negative answer fixes the leak
|
||||
* where unqualified `save()` resolved to `User::save` through a shared
|
||||
* workspace registry walk.
|
||||
*/
|
||||
export function isCppDefGloballyVisible(filePath: string, nodeId: string): boolean {
|
||||
return nonGloballyVisibleNodeIds.get(filePath)?.has(nodeId) !== true;
|
||||
}
|
||||
|
||||
/**
|
||||
* Return the names visible through a C++ wildcard import (`#include` or
|
||||
* `using namespace`).
|
||||
*
|
||||
* ## Contract
|
||||
*
|
||||
* C++ unqualified name lookup only sees names at the importer's enclosing
|
||||
* scope. Class members and namespace-nested symbols are NOT visible by
|
||||
* unqualified lookup from a free function in an including TU — they must
|
||||
* be reached via `Class::method`, `ns::name`, or a working `using`
|
||||
* declaration. The filter below enforces that contract for header
|
||||
* propagation: only defs whose nearest enclosing scope is the header's
|
||||
* `Module` scope are emitted as wildcard-binding names.
|
||||
*
|
||||
* ## Why scope-aware and not predicate-on-qualifiedName
|
||||
*
|
||||
* A naive `def.qualifiedName.indexOf('.') === -1` check is unreliable
|
||||
* because `populateClassOwnedMembers`
|
||||
* (`gitnexus/src/core/ingestion/scope-resolution/scope/walkers.ts`)
|
||||
* only dot-qualifies `qualifiedName` for `Class` scopes. Namespace-nested
|
||||
* defs (`namespace ns { void foo(); }`) arrive in `localDefs` with
|
||||
* `qualifiedName === 'foo'` and `ownerId === undefined`, indistinguishable
|
||||
* from a top-level free function. The structural truth lives in
|
||||
* `Scope.ownedDefs`: each scope lists what it structurally owns; the
|
||||
* Module scope owns only top-level symbols. We look the def up by
|
||||
* `nodeId` against the scope tree to identify its owning kind.
|
||||
*
|
||||
* ## `localDefs` consumer survey (recorded for future maintainers)
|
||||
*
|
||||
* Other consumers of `ParsedFile.localDefs` were audited at the time
|
||||
* this filter was introduced (see PR #1520 / plan
|
||||
* `docs/plans/2026-05-12-002-fix-cpp-resolver-followups-plan.md`):
|
||||
*
|
||||
* - `finalize-orchestrator.ts:113,163` — flattens defs into a workspace
|
||||
* registry keyed by `ownerId` + `qualifiedName`; class-owned and
|
||||
* namespace-owned symbols are registered under their owner, not as
|
||||
* unqualified names. Not a leak surface.
|
||||
* - `csharp/namespace-siblings.ts:307`, `go/expand-wildcards.ts:86`,
|
||||
* `php/scope-resolver.ts:141,151`, `c/static-linkage.ts:51` — other
|
||||
* languages' own wildcard / sibling expansions. Each owns its own
|
||||
* visibility contract.
|
||||
* - `receiver-bound-calls.ts:99`, `reconcile-ownership.ts:66,119`,
|
||||
* `mro.ts:61` — keyed by `ownerId` for member lookup, never used
|
||||
* as unqualified bindings.
|
||||
* - `go/interface-impls.ts:40,53`, `go/package-siblings.ts:41` — Go-
|
||||
* specific, sibling-package scoped.
|
||||
*
|
||||
* No other consumer treats `localDefs` as a flat unqualified-binding
|
||||
* set the way this function did before the fix. If a future consumer
|
||||
* does, mirror this filter or harden registration so class/namespace
|
||||
* members never enter `localDefs` unqualified.
|
||||
*/
|
||||
export function expandCppWildcardNames(
|
||||
targetModuleScope: ScopeId,
|
||||
|
|
@ -49,9 +157,35 @@ export function expandCppWildcardNames(
|
|||
const target = parsedFiles.find((p) => p.moduleScope === targetModuleScope);
|
||||
if (target === undefined) return [];
|
||||
|
||||
// Build nodeId → owning Scope map from the structural scope tree.
|
||||
// `Scope.ownedDefs` is the canonical source of structural ownership;
|
||||
// `localDefs` is its flattened union, which is why the original code
|
||||
// leaked: walking only `localDefs` discards the owning-scope context.
|
||||
const ownerScopeByNodeId = new Map<string, Scope>();
|
||||
for (const scope of target.scopes) {
|
||||
for (const ownedDef of scope.ownedDefs) {
|
||||
ownerScopeByNodeId.set(ownedDef.nodeId, scope);
|
||||
}
|
||||
}
|
||||
|
||||
const seen = new Set<string>();
|
||||
const names: string[] = [];
|
||||
for (const def of target.localDefs) {
|
||||
// Defense-in-depth: class methods carry a non-undefined ownerId after
|
||||
// `populateClassOwnedMembers` runs. Skip them outright.
|
||||
if (def.ownerId !== undefined) continue;
|
||||
|
||||
// Structural visibility check: exclude defs whose owning scope is a
|
||||
// Namespace or Class — these require qualification (`ns::name`,
|
||||
// `Class::method`) and are NOT reachable by unqualified lookup in an
|
||||
// including TU. When the owning scope is unknown we default to
|
||||
// include (preserves prior behavior for any def whose structural
|
||||
// ownership wasn't recorded in `Scope.ownedDefs`).
|
||||
const ownerScope = ownerScopeByNodeId.get(def.nodeId);
|
||||
if (ownerScope !== undefined && (ownerScope.kind === 'Namespace' || ownerScope.kind === 'Class')) {
|
||||
continue;
|
||||
}
|
||||
|
||||
const name = simpleName(def);
|
||||
if (name === '') continue;
|
||||
if (isFileLocal(target.filePath, name)) continue;
|
||||
|
|
|
|||
|
|
@ -8,7 +8,13 @@ import { cppArityCompatibility } from './arity.js';
|
|||
import { cppMergeBindings } from './merge-bindings.js';
|
||||
import { resolveCppImportTarget } from './import-target.js';
|
||||
import { scanCppHeaderFiles } from './header-scan.js';
|
||||
import { expandCppWildcardNames, isFileLocal, clearFileLocalNames } from './file-local-linkage.js';
|
||||
import {
|
||||
expandCppWildcardNames,
|
||||
isFileLocal,
|
||||
clearFileLocalNames,
|
||||
populateCppNonGloballyVisible,
|
||||
isCppDefGloballyVisible,
|
||||
} from './file-local-linkage.js';
|
||||
import { populateCppRangeBindings } from './range-bindings.js';
|
||||
|
||||
/**
|
||||
|
|
@ -62,7 +68,13 @@ export const cppScopeResolver: ScopeResolver = {
|
|||
buildMro: (graph, parsedFiles, nodeLookup) =>
|
||||
buildMro(graph, parsedFiles, nodeLookup, defaultLinearize),
|
||||
|
||||
populateOwners: (parsed: ParsedFile) => populateClassOwnedMembers(parsed),
|
||||
populateOwners: (parsed: ParsedFile) => {
|
||||
populateClassOwnedMembers(parsed);
|
||||
// Track namespace-nested and class-nested defs so the global free-call
|
||||
// fallback and wildcard expansion can suppress them as unqualified
|
||||
// cross-file callables.
|
||||
populateCppNonGloballyVisible(parsed);
|
||||
},
|
||||
|
||||
isSuperReceiver: (text) =>
|
||||
// C++ super patterns: explicit base class call `Base::method()`
|
||||
|
|
@ -80,10 +92,24 @@ export const cppScopeResolver: ScopeResolver = {
|
|||
// for cross-file propagation and compound-receiver chain resolution.
|
||||
// cppBindingScopeFor hoists @type-binding.return to Module scope.
|
||||
hoistTypeBindingsToModule: true,
|
||||
// C++ `static` functions and anonymous namespace symbols have file-local
|
||||
// linkage — exclude them from global free-call fallback cross-file resolution.
|
||||
// The `isFileLocalDef` hook on the global free-call fallback names
|
||||
// file-local linkage historically, but semantically gates "logically
|
||||
// invisible cross-file" defs. C++ extends this to also reject class-
|
||||
// owned methods/fields and namespace-nested symbols — an unqualified
|
||||
// call from a free function MUST NOT resolve to `User::save` or
|
||||
// `ns::foo` (Cppreference, "Unqualified name lookup"). Without this
|
||||
// gate, the global fallback walks every callable in the workspace
|
||||
// registry and matches any class method or namespace function by
|
||||
// simple name.
|
||||
isFileLocalDef: (def: SymbolDefinition) => {
|
||||
const simple = def.qualifiedName?.split('.').pop() ?? def.qualifiedName ?? '';
|
||||
return isFileLocal(def.filePath, simple);
|
||||
if (isFileLocal(def.filePath, simple)) return true;
|
||||
// Class-owned (Method/Field) — `populateClassOwnedMembers` already
|
||||
// stamps `ownerId`; cheap fast-path before consulting the scope map.
|
||||
if (def.ownerId !== undefined) return true;
|
||||
// Namespace-nested defs — require qualification cross-file. Scope-
|
||||
// walked at `populateOwners` time into a per-file nodeId set.
|
||||
if (!isCppDefGloballyVisible(def.filePath, def.nodeId)) return true;
|
||||
return false;
|
||||
},
|
||||
};
|
||||
|
|
|
|||
7
gitnexus/test/fixtures/lang-resolution/cpp-anon-ns-same-file-visible/helper.cpp
vendored
Normal file
7
gitnexus/test/fixtures/lang-resolution/cpp-anon-ns-same-file-visible/helper.cpp
vendored
Normal file
|
|
@ -0,0 +1,7 @@
|
|||
namespace {
|
||||
void w() {}
|
||||
}
|
||||
|
||||
void run() {
|
||||
w();
|
||||
}
|
||||
5
gitnexus/test/fixtures/lang-resolution/cpp-include-no-class-leak/caller.cpp
vendored
Normal file
5
gitnexus/test/fixtures/lang-resolution/cpp-include-no-class-leak/caller.cpp
vendored
Normal file
|
|
@ -0,0 +1,5 @@
|
|||
#include "user.h"
|
||||
|
||||
void run() {
|
||||
save();
|
||||
}
|
||||
6
gitnexus/test/fixtures/lang-resolution/cpp-include-no-class-leak/user.h
vendored
Normal file
6
gitnexus/test/fixtures/lang-resolution/cpp-include-no-class-leak/user.h
vendored
Normal file
|
|
@ -0,0 +1,6 @@
|
|||
#pragma once
|
||||
|
||||
class User {
|
||||
public:
|
||||
void save();
|
||||
};
|
||||
5
gitnexus/test/fixtures/lang-resolution/cpp-include-no-namespace-leak/caller.cpp
vendored
Normal file
5
gitnexus/test/fixtures/lang-resolution/cpp-include-no-namespace-leak/caller.cpp
vendored
Normal file
|
|
@ -0,0 +1,5 @@
|
|||
#include "lib.h"
|
||||
|
||||
void run() {
|
||||
foo();
|
||||
}
|
||||
5
gitnexus/test/fixtures/lang-resolution/cpp-include-no-namespace-leak/lib.h
vendored
Normal file
5
gitnexus/test/fixtures/lang-resolution/cpp-include-no-namespace-leak/lib.h
vendored
Normal file
|
|
@ -0,0 +1,5 @@
|
|||
#pragma once
|
||||
|
||||
namespace ns {
|
||||
void foo();
|
||||
}
|
||||
|
|
@ -1588,3 +1588,79 @@ describe('C++ Derived : A, B — diamond inheritance via leftmost-base MRO (SM-1
|
|||
expect(methodCall!.source).toBe('run');
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// U1: `#include` must not leak class-owned methods as unqualified bindings
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe('C++ include does not leak class methods', () => {
|
||||
let result: PipelineResult;
|
||||
|
||||
beforeAll(async () => {
|
||||
result = await runPipelineFromRepo(
|
||||
path.join(FIXTURES, 'cpp-include-no-class-leak'),
|
||||
() => {},
|
||||
);
|
||||
}, 60000);
|
||||
|
||||
it('does NOT resolve unqualified save() to User::save via #include', () => {
|
||||
const calls = getRelationships(result, 'CALLS');
|
||||
const leak = calls.filter((c) => c.source === 'run' && c.target === 'save');
|
||||
expect(leak.length).toBe(0);
|
||||
});
|
||||
|
||||
it('preserves the file-level #include IMPORTS edge', () => {
|
||||
const imports = getRelationships(result, 'IMPORTS');
|
||||
expect(imports.length).toBe(1);
|
||||
expect(imports[0].targetFilePath).toBe('user.h');
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// U1: `#include` must not leak namespace-nested symbols as unqualified bindings
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe('C++ include does not leak namespace members', () => {
|
||||
let result: PipelineResult;
|
||||
|
||||
beforeAll(async () => {
|
||||
result = await runPipelineFromRepo(
|
||||
path.join(FIXTURES, 'cpp-include-no-namespace-leak'),
|
||||
() => {},
|
||||
);
|
||||
}, 60000);
|
||||
|
||||
it('does NOT resolve unqualified foo() to ns::foo via #include', () => {
|
||||
const calls = getRelationships(result, 'CALLS');
|
||||
const leak = calls.filter((c) => c.source === 'run' && c.target === 'foo');
|
||||
expect(leak.length).toBe(0);
|
||||
});
|
||||
|
||||
it('preserves the file-level #include IMPORTS edge', () => {
|
||||
const imports = getRelationships(result, 'IMPORTS');
|
||||
expect(imports.length).toBe(1);
|
||||
expect(imports[0].targetFilePath).toBe('lib.h');
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// U1: anonymous-namespace symbols remain visible within their declaring TU
|
||||
// (positive companion to the cross-file exclusion test below)
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe('C++ anonymous namespace symbols visible in same TU', () => {
|
||||
let result: PipelineResult;
|
||||
|
||||
beforeAll(async () => {
|
||||
result = await runPipelineFromRepo(
|
||||
path.join(FIXTURES, 'cpp-anon-ns-same-file-visible'),
|
||||
() => {},
|
||||
);
|
||||
}, 60000);
|
||||
|
||||
it('resolves run() -> w() within the same TU', () => {
|
||||
const calls = getRelationships(result, 'CALLS');
|
||||
const wCalls = calls.filter((c) => c.source === 'run' && c.target === 'w');
|
||||
expect(wCalls.length).toBe(1);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -90,7 +90,18 @@ const LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES: Readonly<Record<string, Readonly
|
|||
'binds the call to alpha/services/sync.py, not omega',
|
||||
'lex tiebreak still picks alpha/services/sync.py with reversed file-write order',
|
||||
]),
|
||||
cpp: new Set<string>([]),
|
||||
cpp: new Set<string>([
|
||||
// The legacy DAG path has no scope-aware filtering on the global
|
||||
// free-call fallback, so `#include`d headers still leak class
|
||||
// methods (`User::save`) and namespace members (`ns::foo`) as
|
||||
// resolution targets for unqualified calls. The scope-resolver
|
||||
// path filters via `populateCppNonGloballyVisible` +
|
||||
// `isFileLocalDef`. Scope-resolver-only correctness win
|
||||
// (PR #1520 review follow-up plan U1); backporting to legacy is
|
||||
// out of scope.
|
||||
'does NOT resolve unqualified save() to User::save via #include',
|
||||
'does NOT resolve unqualified foo() to ns::foo via #include',
|
||||
]),
|
||||
};
|
||||
|
||||
type ResolverParityEnv = Readonly<Record<string, string | undefined>>;
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue