fix(r): keep S3 method names from colliding with bare names in simple-name indexes

The shared simple-name indexes key every def by the text after the last '.'
of its qualifiedName, so an S3 method `print.foo` is indexed under `foo`
next to a bare `foo`. Two measured defects followed:

- `library(prov)` bound `foo` to `print.foo` (a wrong 0.85 import-resolved
  edge) when the S3 method was declared before the bare def;
- a same-package cross-file bare call `foo(v)` saw two candidates under
  `foo` and got no CALLS edge at all.

R-only fix, no shared-core edit, no SCHEMA_BUMP (both hooks run at resolve
time over cached ParsedFiles):

- rScopeResolver.isCallableVisibleFromCaller refuses dotted candidates: a
  bare R call can never invoke `print.foo`.
- expandRWildcardNames drops a bare name whose file also defines a dotted
  top-level def with that tail (order-blind, like the importFrom guard).

Characterisation tests deliberately updated to the new behaviour (r.test.ts,
R NAMESPACE importFrom fixture):
- print.tidy before tidy / tally before print.tally: was no edge, now a
  0.5 global-name-fallback edge to the bare def;
- bare call to a name that only an R6 method carries: was a 0.5 guess to
  the method, now no edge (a bare call cannot reach an R6 member).

Residual: dotted names bind only through qualified-name lookup (0.35),
never import-resolved; a bare name sharing a tail with an S3 method in the
same file resolves at 0.5 (fallback), not 0.85; backticked names unchanged.
This commit is contained in:
Mick Cooney 2026-09-29 22:00:45 +01:00
parent 7c383a31f8
commit b7728e72b5
12 changed files with 323 additions and 9 deletions

View file

@ -20,7 +20,7 @@
* heritage target this plan's fixtures name but never declare.
*/
import type { ParsedFile, ScopeId } from 'gitnexus-shared';
import type { ParsedFile, ScopeId, SymbolDefinition } from 'gitnexus-shared';
import { SupportedLanguages } from 'gitnexus-shared';
import { buildMro, defaultLinearize } from '../../scope-resolution/passes/mro.js';
import {
@ -35,7 +35,11 @@ import type { KnowledgeGraph } from '../../../graph/types.js';
import { generateId } from '../../../../lib/utils.js';
import { loadRPackageConfig } from '../../language-config.js';
import { resolveRImportTarget } from '../../import-resolvers/r.js';
import { isRGlobalNameFallbackPlausible, populateRNamespaceImports } from './namespace-imports.js';
import {
isRGlobalNameFallbackPlausible,
populateRNamespaceImports,
rFileTopLevel,
} from './namespace-imports.js';
import { rProvider } from '../r.js';
import { rArityCompatibility, rMergeBindings } from './simple-hooks.js';
@ -106,18 +110,29 @@ function emitRVirtualHeritageEdges(
* scope regardless of what NAMESPACE declares as exported — verified during
* the plan's stress-test round: `pkgA/NAMESPACE` does not export `ResultSet`,
* yet `require("pkgA")` must still resolve it).
*
* A bare name is dropped when the same file also defines a dotted top-level
* name with that tail (`foo` next to `print.foo`). The shared finalize indexes
* every def by the text after its last `.`, so `foo` would be bound to
* whichever of the two comes first in the file, and `print.foo` (an S3 method,
* never callable as `foo()`) can win. The dropped name resolves through the
* free-call fallback instead, where {@link rIsCallableVisibleFromCaller}
* removes the dotted def from the candidates. Order-blind, like the matching
* guard in `import-resolvers/r.ts`.
*/
function expandRWildcardNames(
export function expandRWildcardNames(
targetModuleScope: ScopeId,
parsedFiles: readonly ParsedFile[],
): readonly string[] {
const target = parsedFiles.find((p) => p.moduleScope === targetModuleScope);
if (target === undefined) return [];
const { dottedTails } = rFileTopLevel(target);
const seen = new Set<string>();
const names: string[] = [];
for (const def of target.localDefs) {
const qn = def.qualifiedName;
if (qn === undefined || qn.length === 0 || qn.includes('.')) continue; // top-level only
if (dottedTails.has(qn)) continue; // shares its tail with an S3 method: see above
if (seen.has(qn)) continue;
seen.add(qn);
names.push(qn);
@ -125,6 +140,22 @@ function expandRWildcardNames(
return names;
}
/**
* A bare R call `foo(x)` can never invoke a def whose name contains a `.`
* (`print.foo` is only reachable as `print.foo(...)` or through S3 dispatch),
* yet the free-call fallback's simple-name index keys it under `foo`. Refuse
* such candidates so `foo(x)` cannot collide with `print.foo`.
*
* Applies to every candidate the hook sees, including R6/R5 members whose
* qualified name is `Class.member`: a bare `member(x)` never reaches those
* either.
*/
export function rIsCallableVisibleFromCaller(ctx: {
readonly candidate: SymbolDefinition;
}): boolean {
return !(ctx.candidate.qualifiedName ?? '').includes('.');
}
export const rScopeResolver: ScopeResolver = {
language: SupportedLanguages.R,
languageProvider: rProvider,
@ -184,4 +215,10 @@ export const rScopeResolver: ScopeResolver = {
// in R (see `isRGlobalNameFallbackPlausible` for the exact rule and the
// documented `pkg::name()` / `import()` limitations).
isGlobalNameFallbackPlausible: isRGlobalNameFallbackPlausible,
// The free-call fallback keys defs by the text after the last `.` of their
// qualified name, so an S3 method `print.foo` sits next to a bare `foo` and
// makes the bare call ambiguous (no edge). A bare call can never invoke a
// dotted def, so drop those candidates.
isCallableVisibleFromCaller: rIsCallableVisibleFromCaller,
};

View file

@ -0,0 +1,3 @@
Package: cons
Title: Consumer
Version: 0.1.0

View file

@ -0,0 +1 @@
export(use_foo)

View file

@ -0,0 +1,7 @@
library(prov)
use_foo <- function(v) foo(v)
use_bar <- function(v) bar(v)
use_baz <- function(v) baz(v)
use_dotted <- function(x) print.foo(x)
use_qux <- function(v) qux(v)
use_odd <- function(v) `odd name`(v)

View file

@ -0,0 +1,3 @@
Package: prov
Title: Provider
Version: 0.1.0

View file

@ -0,0 +1,7 @@
export(foo)
export(bar)
export(baz)
S3method(print, foo)
S3method(print, bar)
S3method(summary, baz)
export(qux)

View file

@ -0,0 +1,6 @@
same_pkg_foo <- function(v) foo(v)
same_pkg_bar <- function(v) bar(v)
same_pkg_baz <- function(v) baz(v)
dotted_call <- function(x) print.foo(x)
same_pkg_qux <- function(v) qux(v)
same_pkg_odd <- function(v) `odd name`(v)

View file

@ -0,0 +1,12 @@
print.foo <- function(x, ...) x
foo <- function(v) structure(list(v = v), class = "foo")
bar <- function(v) structure(list(v = v), class = "bar")
print.bar <- function(x, ...) x
baz <- function(v) v
summary.baz <- function(object, ...) object
qux <- function(v) v
`odd name` <- function(v) v

View file

@ -0,0 +1,4 @@
library(prov)
script_foo <- function(v) foo(v)
script_bar <- function(v) bar(v)
script_qux <- function(v) qux(v)

View file

@ -0,0 +1,111 @@
/**
* R: dotted (S3 method) names must not collide with same-tailed bare names
* in the shared simple-name indexes (kaybenleroll/GitNexus#11).
*
* The shared indexes key every def by the text after the last `.` of its
* qualifiedName, so `print.foo` is indexed under `foo` next to a bare `foo`.
* Two defects followed: a `library(prov)` wildcard bound `foo` to `print.foo`
* when the S3 method was declared first (a wrong 0.85 edge), and a bare
* cross-file call `foo(v)` saw two candidates and got no edge at all.
*
* Fixture layout: `prov/R/prov.R` declares `print.foo` BEFORE `foo`, `bar`
* BEFORE `print.bar`, `summary.baz` after `baz`, a control `qux` with no S3
* method and a backticked `odd name`; `cons/` and `scripts/run.R` reach them
* through `library(prov)`; `prov/R/other.R` calls them from the same package.
*/
import { describe, it, expect, beforeAll } from 'vitest';
import path from 'path';
import { FIXTURES, getRelationships, runPipelineFromRepo, type PipelineResult } from './helpers.js';
describe('R dotted (S3) names vs same-tailed bare names', () => {
let result: PipelineResult;
beforeAll(async () => {
result = await runPipelineFromRepo(path.join(FIXTURES, 'r-dotted-s3-collision'), () => {});
}, 60000);
/** `target:targetFile:reason:confidence` of every CALLS edge from `source` in `fileSuffix`. */
const edges = (source: string, fileSuffix: string): string[] =>
getRelationships(result, 'CALLS')
.filter((e) => e.source === source && e.sourceFilePath.endsWith(fileSuffix))
.map((e) => `${e.target}:${e.targetFilePath}:${e.rel.reason}:${e.rel.confidence}`)
.sort();
const PROV = 'prov/R/prov.R';
describe('library(prov) wildcard import', () => {
it('binds foo to the bare def, never to print.foo declared before it', () => {
const e = edges('use_foo', 'cons/R/cons.R');
expect(e.filter((x) => x.startsWith('print.foo:'))).toEqual([]);
expect(e).toEqual([`foo:${PROV}:global-name-fallback:0.5`]);
});
it('also keeps a script caller (no package NAMESPACE) off print.foo', () => {
expect(edges('script_foo', 'scripts/run.R')).toEqual([
`foo:${PROV}:global-name-fallback:0.5`,
]);
});
it('reversed declaration order (bare bar before print.bar) resolves to the bare def', () => {
expect(edges('use_bar', 'cons/R/cons.R')).toEqual([`bar:${PROV}:global-name-fallback:0.5`]);
expect(edges('script_bar', 'scripts/run.R')).toEqual([
`bar:${PROV}:global-name-fallback:0.5`,
]);
});
it('bare name with an S3 method declared after it (baz / summary.baz) resolves to the bare def', () => {
expect(edges('use_baz', 'cons/R/cons.R')).toEqual([`baz:${PROV}:global-name-fallback:0.5`]);
});
it('control: a name with no same-tailed S3 method is still import-resolved at 0.85', () => {
expect(edges('use_qux', 'cons/R/cons.R')).toEqual([`qux:${PROV}:import-resolved:0.85`]);
expect(edges('script_qux', 'scripts/run.R')).toEqual([`qux:${PROV}:import-resolved:0.85`]);
});
});
describe('same-package cross-file bare calls', () => {
const OTHER = 'prov/R/other.R';
it('foo(v) gets a fallback edge to the bare def (was: no edge, two tail candidates)', () => {
expect(edges('same_pkg_foo', OTHER)).toEqual([`foo:${PROV}:global-name-fallback:0.5`]);
});
it('bar(v) and baz(v) resolve to the bare defs', () => {
expect(edges('same_pkg_bar', OTHER)).toEqual([`bar:${PROV}:global-name-fallback:0.5`]);
expect(edges('same_pkg_baz', OTHER)).toEqual([`baz:${PROV}:global-name-fallback:0.5`]);
});
it('control: qux(v) keeps its fallback edge', () => {
expect(edges('same_pkg_qux', OTHER)).toEqual([`qux:${PROV}:global-name-fallback:0.5`]);
});
});
describe('dotted call sites', () => {
it('print.foo(x) in the same package resolves to the S3 def via qualified lookup (0.35)', () => {
expect(edges('dotted_call', 'prov/R/other.R')).toEqual([
`print.foo:${PROV}:scope-resolution: call:0.35`,
]);
});
it('print.foo(x) through library(prov) resolves to the S3 def (0.35)', () => {
expect(edges('use_dotted', 'cons/R/cons.R')).toEqual([
`print.foo:${PROV}:scope-resolution: call:0.35`,
]);
});
});
describe('backticked names (characterisation, unchanged by this fix)', () => {
it('a backticked def keeps its backticks in the name and still resolves like any bare name', () => {
// The qualifiedName keeps the backticks, so the wildcard binds the literal name and a
// same-package call falls back to a unique-name guess. There is no dotted tail involved;
// pinned so a future change to backtick handling is a deliberate decision.
expect({
cons: edges('use_odd', 'cons/R/cons.R'),
samePkg: edges('same_pkg_odd', 'prov/R/other.R'),
}).toEqual({
cons: [`\`odd name\`:${PROV}:import-resolved:0.85`],
samePkg: [`\`odd name\`:${PROV}:global-name-fallback:0.5`],
});
});
});
});

View file

@ -1067,12 +1067,19 @@ describe('R NAMESPACE importFrom() bindings to local packages', () => {
it('never binds tidy to print.tidy when print.tidy is defined first', () => {
// finalize would index `print.tidy` under `tidy`; the guard refuses the import instead, so
// there is no 0.85 edge to the wrong def (baseline: ambiguous, no edge).
expect(edgeSummaries('print_before_bare_user', 'analytics/R/dotted_use.R')).toEqual([]);
// there is no 0.85 edge to the wrong def. Since #11 the fallback no longer sees `print.tidy`
// as a tail-`tidy` candidate (a bare call cannot invoke it), so the call resolves to the
// bare def at 0.5 (was: ambiguous, no edge).
expect(edgeSummaries('print_before_bare_user', 'analytics/R/dotted_use.R')).toEqual([
'tidy:scorelib/R/dotted.R:global-name-fallback:0.5',
]);
});
it('also refuses the mirror layout (bare def first): the guard is order-blind', () => {
expect(edgeSummaries('bare_before_print_user', 'analytics/R/dotted_use.R')).toEqual([]);
// Same 0.5 fallback edge to the bare def since #11 (was: no edge).
expect(edgeSummaries('bare_before_print_user', 'analytics/R/dotted_use.R')).toEqual([
'tally:scorelib/R/dotted.R:global-name-fallback:0.5',
]);
});
it('does not fire for an underscore name (plain_fn binds by import)', () => {
@ -1093,9 +1100,10 @@ describe('R NAMESPACE importFrom() bindings to local packages', () => {
});
it('does not bind an import to an R6 method when no top-level def carries the name', () => {
expect(edgeSummaries('r6_method_only_user', 'analytics/R/r6_use.R')).toEqual([
'describe_run:scorelib/R/r6.R:global-name-fallback:0.5',
]);
// A bare `describe_run()` cannot invoke an R6 method (`Reporter.describe_run`, reached only
// as `obj$describe_run()`), so since #11 the fallback offers no candidate either (was: a
// 0.5 guess to the method).
expect(edgeSummaries('r6_method_only_user', 'analytics/R/r6_use.R')).toEqual([]);
});
it('documents residual O1: a nested function defined before the top-level one still wins the binding', () => {

View file

@ -0,0 +1,115 @@
/**
* R dotted (S3) names vs bare names in the shared simple-name indexes
* (kaybenleroll/GitNexus#11): the `isCallableVisibleFromCaller` hook and the
* `expandRWildcardNames` tail guard. ParsedFiles come from the real R extractor.
*/
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 { populateClassOwnedMembers } from '../../../../src/core/ingestion/scope-resolution/scope/walkers.js';
import {
expandRWildcardNames,
rIsCallableVisibleFromCaller,
rScopeResolver,
} from '../../../../src/core/ingestion/languages/r/scope-resolver.js';
function parse(src: string, filePath: string): ParsedFile {
const parsed = extractParsedFile(rProvider, src, filePath);
if (parsed === undefined) throw new Error(`no ParsedFile for ${filePath}`);
populateClassOwnedMembers(parsed);
return parsed;
}
const def = (parsed: ParsedFile, qualifiedName: string): SymbolDefinition => {
const found = parsed.localDefs.find((d) => d.qualifiedName === qualifiedName);
if (found === undefined) throw new Error(`no def ${qualifiedName}`);
return found;
};
const visible = (callerParsed: ParsedFile, candidate: SymbolDefinition): boolean =>
rIsCallableVisibleFromCaller({ callerParsed, candidate } as never);
describe('rIsCallableVisibleFromCaller', () => {
const p = parse(
[
'print.foo <- function(x, ...) x',
'foo <- function(v) v',
'`odd name` <- function(v) v',
'Scorer <- R6::R6Class("Scorer", public = list(score = function(d) d))',
].join('\n'),
'pkg/R/a.R',
);
it('is registered on rScopeResolver', () => {
expect(rScopeResolver.isCallableVisibleFromCaller).toBe(rIsCallableVisibleFromCaller);
});
it('refuses a dotted S3 def for a bare call', () => {
expect(visible(p, def(p, 'print.foo'))).toBe(false);
});
it('allows a bare def and a backticked non-dotted def', () => {
expect(visible(p, def(p, 'foo'))).toBe(true);
expect(visible(p, def(p, '`odd name`'))).toBe(true);
});
it('refuses a class member whose qualified name is dotted (never callable bare)', () => {
const member = p.localDefs.find(
(d) => d.type === 'Method' && d.qualifiedName?.endsWith('score'),
);
if (member === undefined) throw new Error('no R6 member def');
expect(member.qualifiedName).toContain('.');
expect(visible(p, member)).toBe(false);
});
it('allows a def with no qualified name (nothing to prove)', () => {
expect(visible(p, { ...def(p, 'foo'), qualifiedName: undefined })).toBe(true);
});
});
describe('expandRWildcardNames', () => {
const names = (src: string): readonly string[] => {
const parsed = parse(src, 'pkg/R/a.R');
return expandRWildcardNames(parsed.moduleScope, [parsed]);
};
it('drops a bare name whose file defines a dotted def with that tail (S3 method first)', () => {
expect(names('print.foo <- function(x) x\nfoo <- function(v) v\nqux <- function(v) v')).toEqual(
['qux'],
);
});
it('drops it in the mirror layout too (order-blind)', () => {
expect(names('bar <- function(v) v\nprint.bar <- function(x) x\nqux <- function(v) v')).toEqual(
['qux'],
);
});
it('keeps names that share no tail with a dotted def', () => {
expect(names('print.foo <- function(x) x\nplain_fn <- function(v) v')).toEqual(['plain_fn']);
});
it('keeps a bare name when the dotted def sits in a different file', () => {
const a = parse('foo <- function(v) v', 'pkg/R/a.R');
const b = parse('print.foo <- function(x) x', 'pkg/R/b.R');
expect(expandRWildcardNames(a.moduleScope, [a, b])).toEqual(['foo']);
});
it('keeps a bare name that only matches a class member (not a top-level dotted def)', () => {
expect(
names(
'Scorer <- R6::R6Class("Scorer", public = list(tidy = function(d) d))\ntidy <- function(d) d',
),
).toEqual(['Scorer', 'tidy']);
});
it('never lists dotted names themselves', () => {
expect(names('print.foo <- function(x) x')).toEqual([]);
});
it('returns nothing for an unknown module scope', () => {
const a = parse('foo <- function(v) v', 'pkg/R/a.R');
expect(expandRWildcardNames('scope:missing' as never, [a])).toEqual([]);
});
});