fix(r): treat only functions as S3 generics in the rename guard

checkSafety collected every local definition into the set of possible
generics, so renaming a function to Widget.foo was refused when Widget was
an R6 or S4 class, or area.circle when area was only an R6 method. The
final NAMESPACE-export check likewise matched a member of the same name in
the same file. A class, method or property is not an S3 generic and has no
NAMESPACE export entry of its own; match functions only.
This commit is contained in:
Mick Cooney 2026-10-17 14:58:08 +01:00
parent 06ae82c4e3
commit bb68db7dbb
2 changed files with 68 additions and 6 deletions

View file

@ -277,10 +277,14 @@ export const rRenameProvider: RenameProvider = {
const name = rBareName(target.qualifiedName ?? '');
if (target.type !== 'Function')
return unsupported('Only plain R functions can be renamed; classes and members are refused.');
// Only a function can be an S3 generic: an R6 or S4 class, a member or a field of the same
// spelling is not one, so a name like `Widget.foo` beside `Widget <- R6Class(...)` is free.
const defined = new Set(
[...files.values()].flatMap((source) =>
source.parsed.localDefs.flatMap((d) =>
d.qualifiedName === undefined ? [] : [rBareName(d.qualifiedName)],
d.type !== 'Function' || d.qualifiedName === undefined
? []
: [rBareName(d.qualifiedName)],
),
),
);
@ -321,6 +325,7 @@ export const rRenameProvider: RenameProvider = {
let exported = false;
graph.forEachNode((node) => {
if (
node.label === 'Function' &&
node.properties.filePath === file &&
typeof node.properties.name === 'string' &&
rBareName(node.properties.name) === name &&

View file

@ -12,15 +12,21 @@ import { registersS3Method, rRenameProvider } from '../../src/core/ingestion/lan
const parser = new Parser();
parser.setLanguage(R as Parameters<Parser['setLanguage']>[0]);
/** Run the provider's safety check for renaming `helper` to `newName`, with these functions defined. */
function verdict(newName: string, defined: readonly string[]): string {
const source = defined.map((name) => `${name} <- function(x) x`).join('\n');
type Definition = { readonly name: string; readonly type: string };
/** Run the provider's safety check for renaming `helper` to `newName`, with these definitions. */
function verdictWith(
newName: string,
defs: readonly Definition[],
nodes: readonly Record<string, unknown>[] = [],
): string {
const source = defs.map((d) => `${d.name} <- function(x) x`).join('\n');
const files = new Map([
[
'a.R',
{
tree: parser.parse(source),
parsed: { localDefs: defined.map((name) => ({ qualifiedName: name })) },
parsed: { localDefs: defs.map((d) => ({ qualifiedName: d.name, type: d.type })) },
},
],
]);
@ -28,11 +34,21 @@ function verdict(newName: string, defined: readonly string[]): string {
target: { type: 'Function', qualifiedName: 'helper', filePath: 'a.R' },
newName,
files,
graph: { forEachNode: () => undefined },
graph: {
forEachNode: (visit: (node: { properties: Record<string, unknown> }) => void) =>
nodes.forEach((properties) => visit({ properties })),
},
} as never);
return issue ? issue.code : 'ok';
}
/** The same, with every name defined as a plain function. */
const verdict = (newName: string, defined: readonly string[]): string =>
verdictWith(
newName,
defined.map((name) => ({ name, type: 'Function' })),
);
describe('R rename: the S3 registration guard', () => {
it('refuses a name whose first dot split is a defined function', () => {
expect(verdict('area.circle', ['area'])).toBe('unsupported_occurrence');
@ -59,6 +75,47 @@ describe('R rename: the S3 registration guard', () => {
expect(verdict('toJson.foo', ['to.json'])).toBe('ok');
});
it('treats only functions as generics, not classes or members of the same name', () => {
// The positive control: the same name defined as a function is refused.
expect(verdictWith('Widget.foo', [{ name: 'Widget', type: 'Function' }])).toBe(
'unsupported_occurrence',
);
for (const type of ['Class', 'Method', 'Property']) {
expect(verdictWith('Widget.foo', [{ name: 'Widget', type }]), type).toBe('ok');
}
// A name with a Class and a Function of the same spelling is still refused.
expect(
verdictWith('Widget.foo', [
{ name: 'Widget', type: 'Class' },
{ name: 'Widget', type: 'Function' },
]),
).toBe('unsupported_occurrence');
});
it('refuses a rename only for the NAMESPACE export of a function, not of a member', () => {
const inPackage = (label: string) =>
rRenameProvider.checkSafety?.({
target: { type: 'Function', qualifiedName: 'helper', filePath: 'R/a.R' },
newName: 'helper2',
files: new Map([
['R/a.R', { tree: parser.parse('helper <- function(x) x'), parsed: { localDefs: [] } }],
]),
graph: {
forEachNode: (
visit: (node: { label: string; properties: Record<string, unknown> }) => void,
) =>
visit({
label,
properties: { filePath: 'R/a.R', name: 'helper', isExported: true },
}),
},
} as never)?.code ?? 'ok';
expect(inPackage('Function')).toBe('unsupported_occurrence');
expect(inPackage('Method')).toBe('ok');
expect(inPackage('Property')).toBe('ok');
expect(inPackage('Class')).toBe('ok');
});
it('allows a name with no dot, and a name that merely starts with a dot', () => {
expect(verdict('print_foo', [])).toBe('ok');
expect(verdict('.print', [])).toBe('ok');