From 7e600fe8f2133188efa4fb152808d8d514da36e0 Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Wed, 13 May 2026 08:42:58 +0100 Subject: [PATCH] fix(cpp): stop #include from leaking class methods and namespace members (U1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../scope-resolution/finalize-algorithm.ts | 11 +- .../languages/cpp/file-local-linkage.ts | 142 +++++++++++++++++- .../ingestion/languages/cpp/scope-resolver.ts | 36 ++++- .../cpp-anon-ns-same-file-visible/helper.cpp | 7 + .../cpp-include-no-class-leak/caller.cpp | 5 + .../cpp-include-no-class-leak/user.h | 6 + .../cpp-include-no-namespace-leak/caller.cpp | 5 + .../cpp-include-no-namespace-leak/lib.h | 5 + .../test/integration/resolvers/cpp.test.ts | 76 ++++++++++ .../test/integration/resolvers/helpers.ts | 13 +- 10 files changed, 295 insertions(+), 11 deletions(-) create mode 100644 gitnexus/test/fixtures/lang-resolution/cpp-anon-ns-same-file-visible/helper.cpp create mode 100644 gitnexus/test/fixtures/lang-resolution/cpp-include-no-class-leak/caller.cpp create mode 100644 gitnexus/test/fixtures/lang-resolution/cpp-include-no-class-leak/user.h create mode 100644 gitnexus/test/fixtures/lang-resolution/cpp-include-no-namespace-leak/caller.cpp create mode 100644 gitnexus/test/fixtures/lang-resolution/cpp-include-no-namespace-leak/lib.h diff --git a/gitnexus-shared/src/scope-resolution/finalize-algorithm.ts b/gitnexus-shared/src/scope-resolution/finalize-algorithm.ts index a94012a0b..f5d3dd0bf 100644 --- a/gitnexus-shared/src/scope-resolution/finalize-algorithm.ts +++ b/gitnexus-shared/src/scope-resolution/finalize-algorithm.ts @@ -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) { diff --git a/gitnexus/src/core/ingestion/languages/cpp/file-local-linkage.ts b/gitnexus/src/core/ingestion/languages/cpp/file-local-linkage.ts index e617fbc44..6ab3bbdaf 100644 --- a/gitnexus/src/core/ingestion/languages/cpp/file-local-linkage.ts +++ b/gitnexus/src/core/ingestion/languages/cpp/file-local-linkage.ts @@ -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>(); +/** + * 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>(); + /** 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(); + 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(); + for (const scope of target.scopes) { + for (const ownedDef of scope.ownedDefs) { + ownerScopeByNodeId.set(ownedDef.nodeId, scope); + } + } + const seen = new Set(); 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; diff --git a/gitnexus/src/core/ingestion/languages/cpp/scope-resolver.ts b/gitnexus/src/core/ingestion/languages/cpp/scope-resolver.ts index d5b9542c2..6e0371b95 100644 --- a/gitnexus/src/core/ingestion/languages/cpp/scope-resolver.ts +++ b/gitnexus/src/core/ingestion/languages/cpp/scope-resolver.ts @@ -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; }, }; diff --git a/gitnexus/test/fixtures/lang-resolution/cpp-anon-ns-same-file-visible/helper.cpp b/gitnexus/test/fixtures/lang-resolution/cpp-anon-ns-same-file-visible/helper.cpp new file mode 100644 index 000000000..3d992bfe5 --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/cpp-anon-ns-same-file-visible/helper.cpp @@ -0,0 +1,7 @@ +namespace { + void w() {} +} + +void run() { + w(); +} diff --git a/gitnexus/test/fixtures/lang-resolution/cpp-include-no-class-leak/caller.cpp b/gitnexus/test/fixtures/lang-resolution/cpp-include-no-class-leak/caller.cpp new file mode 100644 index 000000000..8de016602 --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/cpp-include-no-class-leak/caller.cpp @@ -0,0 +1,5 @@ +#include "user.h" + +void run() { + save(); +} diff --git a/gitnexus/test/fixtures/lang-resolution/cpp-include-no-class-leak/user.h b/gitnexus/test/fixtures/lang-resolution/cpp-include-no-class-leak/user.h new file mode 100644 index 000000000..089fa6c59 --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/cpp-include-no-class-leak/user.h @@ -0,0 +1,6 @@ +#pragma once + +class User { +public: + void save(); +}; diff --git a/gitnexus/test/fixtures/lang-resolution/cpp-include-no-namespace-leak/caller.cpp b/gitnexus/test/fixtures/lang-resolution/cpp-include-no-namespace-leak/caller.cpp new file mode 100644 index 000000000..90b1f2aff --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/cpp-include-no-namespace-leak/caller.cpp @@ -0,0 +1,5 @@ +#include "lib.h" + +void run() { + foo(); +} diff --git a/gitnexus/test/fixtures/lang-resolution/cpp-include-no-namespace-leak/lib.h b/gitnexus/test/fixtures/lang-resolution/cpp-include-no-namespace-leak/lib.h new file mode 100644 index 000000000..11f71286f --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/cpp-include-no-namespace-leak/lib.h @@ -0,0 +1,5 @@ +#pragma once + +namespace ns { + void foo(); +} diff --git a/gitnexus/test/integration/resolvers/cpp.test.ts b/gitnexus/test/integration/resolvers/cpp.test.ts index 5a1edc11f..83b7f3919 100644 --- a/gitnexus/test/integration/resolvers/cpp.test.ts +++ b/gitnexus/test/integration/resolvers/cpp.test.ts @@ -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); + }); +}); diff --git a/gitnexus/test/integration/resolvers/helpers.ts b/gitnexus/test/integration/resolvers/helpers.ts index 8c83228ec..f753ba317 100644 --- a/gitnexus/test/integration/resolvers/helpers.ts +++ b/gitnexus/test/integration/resolvers/helpers.ts @@ -90,7 +90,18 @@ const LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES: Readonly([]), + cpp: new Set([ + // 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>;