fix(r): refuse a closure-local function as the unique-name fallback target

A function bound inside another function lives in that call's frame, so no
other function in its file or another can call it by its bare name. The
unique-name fallback ignored that: when the name was unique in the repository,
a bare call elsewhere bound to the closure-local function at 0.5.

The R fallback veto now refuses a function or class that the file's scope
model holds below the top level. A candidate the scope model does not hold, and
every top-level definition, are judged as before.
This commit is contained in:
Mick Cooney 2026-10-17 17:39:52 +01:00
parent 23eb44dd13
commit d3e06d373d
7 changed files with 208 additions and 5 deletions

View file

@ -49,7 +49,7 @@ Exports:
The scope query (`languages/r/query.ts`) captures three call shapes only: `f(x)`, `pkg::f(x)` and `obj$f(x)`.
- A bare call binds lexically first. `library()` and `require()` of a local package make every top-level name of that package visible, unfiltered by NAMESPACE. This is wider than R's `library()` of an installed package, which attaches only its exports; it follows `devtools::load_all()`, which attaches every top-level name, exported or not. `source()` binds no name; a call into a sourced file, or into another file of the same package, binds through a workspace-wide unique-name fallback at confidence 0.5 (`global-name-fallback`). An ambiguous name yields no edge.
- A bare call binds lexically first. `library()` and `require()` of a local package make every top-level name of that package visible, unfiltered by NAMESPACE. This is wider than R's `library()` of an installed package, which attaches only its exports; it follows `devtools::load_all()`, which attaches every top-level name, exported or not. `source()` binds no name; a call into a sourced file, or into another file of the same package, binds through a workspace-wide unique-name fallback at confidence 0.5 (`global-name-fallback`). An ambiguous name yields no edge, and a function defined inside another function is never a candidate (no other function can reach it by name).
- A bare call never binds to a dotted definition, so `foo(x)` cannot reach `print.foo`. Dotted (S3-style) names are never import-resolved; a call that spells the full dotted name binds by qualified-name lookup. Dotted names are not bound through `importFrom()`.
- `pkg::name()` is decided by its qualifier:
- `pkg` is a package of the repository and defines `name` exactly once: the call binds to that definition at confidence 0.85 (`import-resolved`).

View file

@ -650,6 +650,39 @@ export function rFileTopLevel(parsed: ParsedFile): RFileTopLevel {
return result;
}
interface RDefIds {
/** Ids of the function and class definitions the file binds at top level. */
readonly topLevel: ReadonlySet<string>;
/** Ids of every definition the file's scope model holds, nested or not. */
readonly all: ReadonlySet<string>;
}
const defIdsCache = new WeakMap<ParsedFile, RDefIds>();
/**
* The function and class definitions of `parsed` that are bound at top level: the `local`
* bindings of its Module scope. A function written inside another function is owned by that
* function's scope (and `localDefs` lists it all the same), so its id is in `all` only.
*/
function rDefIds(parsed: ParsedFile): RDefIds {
const cached = defIdsCache.get(parsed);
if (cached !== undefined) return cached;
const topLevel = new Set<string>();
const moduleScope = parsed.scopes.find((s) => s.id === parsed.moduleScope);
if (moduleScope !== undefined) {
for (const refs of moduleScope.bindings.values()) {
for (const ref of refs) {
if (ref.origin === 'local' && TOP_LEVEL_BINDABLE_TYPES.has(ref.def.type)) {
topLevel.add(ref.def.nodeId);
}
}
}
}
const result = { topLevel, all: new Set(parsed.localDefs.map((d) => d.nodeId)) };
defIdsCache.set(parsed, result);
return result;
}
// ─── Import synthesis (populateWorkspaceReferences) ─────────────────────────
/**
@ -880,15 +913,38 @@ export function rQualifiedDefinitionCount(
*/
export interface RGlobalNameFallbackContext {
readonly callerParsed: { readonly filePath: string };
readonly candidate: { readonly filePath: string };
readonly candidate: {
readonly filePath: string;
readonly nodeId?: string;
readonly type?: string;
};
/** Reaches the candidate's parse; absent in hand-built contexts, which read as "cannot decide". */
readonly parsedFileOf?: (filePath: string) => ParsedFile | undefined;
/** Opaque `loadResolutionConfig` result; an {@link RPackageConfig} for R. */
readonly resolutionConfig?: unknown;
readonly site: { readonly name: string; readonly rawQualifiedName?: string };
}
/**
* Refuse a `global-name-fallback` guess that the caller package's NAMESPACE
* contradicts. Returns `false` (veto) only when ALL of these hold:
* True when the candidate is a function or class that its file's scope model holds and binds
* below the top level. A candidate the scope model does not hold (the semantic model's own
* entry for a dotted name carries a graph id, not a scope definition id) is not judged.
*/
function isClosureLocalCandidate(ctx: RGlobalNameFallbackContext): boolean {
const { candidate } = ctx;
if (candidate.nodeId === undefined || candidate.type === undefined) return false;
if (!TOP_LEVEL_BINDABLE_TYPES.has(candidate.type)) return false;
const parsed = ctx.parsedFileOf?.(candidate.filePath);
if (parsed === undefined) return false;
const ids = rDefIds(parsed);
return ids.all.has(candidate.nodeId) && !ids.topLevel.has(candidate.nodeId);
}
/**
* Refuse a `global-name-fallback` guess that R cannot make. Returns `false` (veto) when the
* candidate is a function or class bound inside another function (not at top level), or when
* the caller package's NAMESPACE contradicts the guess. The NAMESPACE veto applies only when
* ALL of these hold:
*
* - the caller file lives under `<pkgDir>/R/**` of a package with a NAMESPACE
* that has an `importFrom(P, <site.name>)` entry (the LAST such entry over
@ -901,7 +957,7 @@ export interface RGlobalNameFallbackContext {
* re-exports the name from a third package and the candidate may be the
* real origin (unknown per-package names = cannot decide = allow).
*
* Everything else — no config, caller outside a package `R/` (`tests/`,
* Everything else — no config, no parse of the candidate's file, caller outside a package `R/` (`tests/`,
* `scripts/`, root `plumber.R`), no entry, candidate outside any package
* `R/` — answers `true`, so `source()`/`library()` script flows are untouched.
*
@ -916,6 +972,11 @@ export interface RGlobalNameFallbackContext {
* tokenised, so a later `import(pkg)` that R would prefer is invisible.
*/
export function isRGlobalNameFallbackPlausible(ctx: RGlobalNameFallbackContext): boolean {
// A function or class bound inside another function lives in that call's frame: no other
// function, in this file or another, can reach it by its bare name. Only top-level
// definitions are candidates for a unique-name guess.
if (isClosureLocalCandidate(ctx)) return false;
const cfg = ctx.resolutionConfig as RPackageConfig | null | undefined;
if (
typeof cfg !== 'object' ||

View file

@ -0,0 +1,6 @@
make_counter <- function() {
count_fn <- function() 2
count_fn
}
use_local <- function() count_fn()

View file

@ -0,0 +1,8 @@
use_other <- function() count_fn()
use_top <- function() top_helper()
wrap <- function() {
shared_name <- function() 2
shared_name()
}

View file

@ -0,0 +1,9 @@
top_helper <- function() 1
other <- function() {
inner_only <- function() top_helper()
inner_only()
}
reach_inner <- function() inner_only()
reach_top <- function() top_helper()

View file

@ -0,0 +1,44 @@
/**
* R: a function bound inside another function lives in that call's frame, so no other function
* (in its file or another) can call it by its bare name. The unique-name fallback used to bind
* such calls to it at 0.5 whenever the name was unique in the repository.
*/
import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest';
import path from 'path';
import { FIXTURES, getRelationships, runPipelineFromRepo } from './helpers.js';
import type { PipelineResult } from './helpers.js';
// Safe under `pool: 'forks'`: the env stub is process-global, but each file gets its own process.
beforeAll(() => vi.stubEnv('GITNEXUS_WORKER_READY_TIMEOUT_MS', '60000'));
afterAll(() => vi.unstubAllEnvs());
describe('R unique-name fallback and closure-local functions', () => {
let result: PipelineResult;
beforeAll(async () => {
result = await runPipelineFromRepo(path.join(FIXTURES, 'r-closure-local-fallback'), () => {});
}, 60000);
/** `caller -> callee (reason)` for every CALLS edge, sorted. */
const calls = (): string[] =>
getRelationships(result, 'CALLS')
.map((e) => `${e.source}->${e.target}`)
.sort();
it('does not bind a bare call to a closure-local function of another function', () => {
expect(calls()).not.toContain('use_local->count_fn');
expect(calls()).not.toContain('use_other->count_fn');
expect(calls()).not.toContain('reach_inner->inner_only');
});
it('keeps the unique-name guess to a top-level function in another file (positive control)', () => {
expect(calls()).toContain('use_top->top_helper');
expect(calls()).toContain('reach_top->top_helper');
});
it('keeps the lexical edges inside a function that defines the closure itself', () => {
expect(calls()).toContain('wrap->shared_name');
expect(calls()).toContain('inner_only->top_helper');
expect(calls()).toContain('other->inner_only');
});
});

View file

@ -0,0 +1,75 @@
/**
* The R unique-name fallback veto refuses a candidate bound inside another function, and only that:
* a top-level definition, a candidate it cannot inspect, and every non-function kind stay plausible.
*/
import { describe, expect, it } from 'vitest';
import type { ParsedFile, SymbolDefinition } from 'gitnexus-shared';
import { extractParsedFile } from '../../../../src/core/ingestion/scope-extractor-bridge.js';
import { rProvider } from '../../../../src/core/ingestion/languages/r.js';
import { isRGlobalNameFallbackPlausible } from '../../../../src/core/ingestion/languages/r/namespace-imports.js';
const SOURCE = [
'make_counter <- function() {',
' inner_fn <- function() 2',
' Inner <- setRefClass("Inner", fields = list(a = "numeric"))',
' inner_fn',
'}',
'top_fn <- function() 1',
].join('\n');
const parsed = (): ParsedFile => {
const file = extractParsedFile(rProvider, SOURCE, 'R/a.R');
if (file === undefined) throw new Error('no ParsedFile');
return file;
};
const defNamed = (file: ParsedFile, name: string): SymbolDefinition => {
const def = file.localDefs.find((d) => d.qualifiedName?.split('.').pop() === name);
if (def === undefined) throw new Error(`no def ${name}`);
return def;
};
const verdict = (
file: ParsedFile,
candidate: Pick<SymbolDefinition, 'nodeId' | 'filePath' | 'type'>,
withParse = true,
): boolean =>
isRGlobalNameFallbackPlausible({
callerParsed: { filePath: 'R/b.R' },
candidate,
parsedFileOf: withParse ? (p) => (p === file.filePath ? file : undefined) : undefined,
site: { name: 'x' },
});
describe('isRGlobalNameFallbackPlausible: closure-local candidates', () => {
it('refuses a function bound inside another function', () => {
const file = parsed();
expect(verdict(file, defNamed(file, 'inner_fn'))).toBe(false);
expect(verdict(file, defNamed(file, 'Inner'))).toBe(false);
});
it('keeps a top-level function', () => {
const file = parsed();
expect(verdict(file, defNamed(file, 'top_fn'))).toBe(true);
expect(verdict(file, defNamed(file, 'make_counter'))).toBe(true);
});
it('does not judge a candidate the scope model does not hold (a graph node id)', () => {
const file = parsed();
expect(
verdict(file, { nodeId: 'Function:R/a.R:inner_fn', filePath: 'R/a.R', type: 'Function' }),
).toBe(true);
});
it('cannot decide without the candidate parse, and allows', () => {
const file = parsed();
expect(verdict(file, defNamed(file, 'inner_fn'), false)).toBe(true);
expect(
isRGlobalNameFallbackPlausible({
callerParsed: { filePath: 'R/b.R' },
candidate: { filePath: 'R/a.R' },
site: { name: 'x' },
}),
).toBe(true);
});
});