diff --git a/docs/languages/r-provider.md b/docs/languages/r-provider.md index c9065a424..fab46b5b1 100644 --- a/docs/languages/r-provider.md +++ b/docs/languages/r-provider.md @@ -123,7 +123,7 @@ Not modelled: - **Refused (`unsupported_occurrence`, planning status `unsupported`):** - any target that is not a plain `Function` (S4 classes, R6 classes, members); - a function whose old or new name is the whole value of a string literal anywhere in the indexed R sources (`UseMethod("area")`, `do.call("f", ...)`, `match.fun`, `get`, `setGeneric`, ...), because string lookup is not resolved. The literal is decoded first, so escapes (`"f\x6fo"`, `"\u{66}oo"`, octal) and raw strings (`r"(foo)"`) count. A string that only contains the name (`"foo(1)"`), a name built at run time (`paste0("fo", "o")`), and strings in files that are not indexed R sources (R Markdown, `NAMESPACE`) are not detected; - - a new name `generic.class` whose prefix is an existing function, which would register an S3 method; + - a new name `generic.class` where any dot-separated prefix is a function defined in the indexed R sources or a common base generic (`print`, `format`, `summary`, `plot`, `as.character`, ...), which would register an S3 method (`to.json.foo` for a defined `to.json`, `print.foo`); - a package function under `R/` that `NAMESPACE` does not mark private (`isExported` is not false), because `NAMESPACE` is not an indexed source and its `export()` or `S3method()` entry would go stale. - **Refused by the shared engine (fail-closed, not provider code):** S3 methods and any dotted function name (`area.circle` reads as an owner-qualified member, so `stale_graph`); S4 class names (`unsupported_occurrence`, no exact source-name span); R6 methods (`stale_graph`). - **Invalid names (`invalid_name`):** anything that is not a nonreserved R identifier. diff --git a/gitnexus/src/core/ingestion/languages/r/rename.ts b/gitnexus/src/core/ingestion/languages/r/rename.ts index 6b2675420..f00272549 100644 --- a/gitnexus/src/core/ingestion/languages/r/rename.ts +++ b/gitnexus/src/core/ingestion/languages/r/rename.ts @@ -47,6 +47,40 @@ function rStringValue(text: string): string | null { ); } +/** + * Generics of base R and its default packages that an S3 method name `generic.class` can register + * against, whether or not the indexed sources define them. Not exhaustive: it covers the generics + * a project is most likely to meet by accident, and the repository's own functions are checked + * separately. + */ +const BASE_GENERICS: ReadonlySet = new Set( + ( + 'print format summary plot lines points text image hist barplot boxplot pairs persp ' + + 'as.character as.data.frame as.list as.vector as.numeric as.integer as.logical as.matrix ' + + 'as.Date as.POSIXct as.POSIXlt as.factor as.table as.function as.environment as.double ' + + 'length names dim dimnames levels rev sort unique duplicated anyDuplicated rep seq ' + + 'mean median quantile range max min round diff scale cut split merge subset transform ' + + 'with within head tail t c cbind rbind toString all.equal is.na labels str ' + + 'update predict residuals coef fitted anova simulate terms formula family weights ' + + 'vcov logLik AIC BIC nobs deviance confint profile model.frame row.names open close ' + + 'by aggregate droplevels relevel reorder xtfrm solve determinant julian months weekdays' + ).split(' '), +); + +/** + * Whether `newName` reads as an S3 method `generic.class`: some dot-separated prefix of it is a + * function the indexed sources define or a known base generic. Every dot is a possible split + * (`to.json.foo` is a method of `to.json`; `as.data.frame.foo` of `as.data.frame`). + */ +export function registersS3Method(newName: string, defined: ReadonlySet): boolean { + for (let dot = newName.indexOf('.'); dot >= 0; dot = newName.indexOf('.', dot + 1)) { + if (dot === 0) continue; + const prefix = newName.slice(0, dot); + if (defined.has(prefix) || BASE_GENERICS.has(prefix)) return true; + } + return false; +} + const unsupported = (message: string): RenameIssue => ({ code: 'unsupported_occurrence', status: 'unsupported', @@ -114,8 +148,10 @@ export const rRenameProvider: RenameProvider = { ), ), ); - if (newName.includes('.') && defined.has(newName.slice(0, newName.indexOf('.')))) - return unsupported('The new name would register an S3 method of an existing function.'); + if (registersS3Method(newName, defined)) + return unsupported( + 'The new name would register an S3 method of an existing function or base generic.', + ); for (const source of files.values()) { if (!source.tree) continue; for (const node of walkNamedNodes(source.tree.rootNode)) { diff --git a/gitnexus/test/fixtures/rename-integration/r/s3-new-method-base-generic/after/helper.R b/gitnexus/test/fixtures/rename-integration/r/s3-new-method-base-generic/after/helper.R new file mode 100644 index 000000000..94d6da8d4 --- /dev/null +++ b/gitnexus/test/fixtures/rename-integration/r/s3-new-method-base-generic/after/helper.R @@ -0,0 +1,5 @@ +helper <- function(v) v + 1 + +use_helper <- function(v) { + helper(v) +} diff --git a/gitnexus/test/fixtures/rename-integration/r/s3-new-method-base-generic/before/helper.R b/gitnexus/test/fixtures/rename-integration/r/s3-new-method-base-generic/before/helper.R new file mode 100644 index 000000000..94d6da8d4 --- /dev/null +++ b/gitnexus/test/fixtures/rename-integration/r/s3-new-method-base-generic/before/helper.R @@ -0,0 +1,5 @@ +helper <- function(v) v + 1 + +use_helper <- function(v) { + helper(v) +} diff --git a/gitnexus/test/fixtures/rename-integration/r/s3-new-method-base-generic/case.json b/gitnexus/test/fixtures/rename-integration/r/s3-new-method-base-generic/case.json new file mode 100644 index 000000000..cc76f2783 --- /dev/null +++ b/gitnexus/test/fixtures/rename-integration/r/s3-new-method-base-generic/case.json @@ -0,0 +1,9 @@ +{ + "language": "r", + "description": "Refuse a new name that would register a method of a base generic (print.foo), although the repository defines no print().", + "categories": ["s3", "name-collision", "negative", "unchanged-on-error"], + "target": { "name": "helper", "kind": "Function", "file": "helper.R", "startLine": 0 }, + "newName": "print.foo", + "expect": { "status": "error", "code": "unsupported_occurrence", "planningStatus": "unsupported" }, + "edges": [] +} diff --git a/gitnexus/test/fixtures/rename-integration/r/s3-new-method-dotted-generic/after/generics.R b/gitnexus/test/fixtures/rename-integration/r/s3-new-method-dotted-generic/after/generics.R new file mode 100644 index 000000000..2c3fe2be6 --- /dev/null +++ b/gitnexus/test/fixtures/rename-integration/r/s3-new-method-dotted-generic/after/generics.R @@ -0,0 +1 @@ +to.json <- function(x) UseMethod("to.json") diff --git a/gitnexus/test/fixtures/rename-integration/r/s3-new-method-dotted-generic/after/helper.R b/gitnexus/test/fixtures/rename-integration/r/s3-new-method-dotted-generic/after/helper.R new file mode 100644 index 000000000..94d6da8d4 --- /dev/null +++ b/gitnexus/test/fixtures/rename-integration/r/s3-new-method-dotted-generic/after/helper.R @@ -0,0 +1,5 @@ +helper <- function(v) v + 1 + +use_helper <- function(v) { + helper(v) +} diff --git a/gitnexus/test/fixtures/rename-integration/r/s3-new-method-dotted-generic/before/generics.R b/gitnexus/test/fixtures/rename-integration/r/s3-new-method-dotted-generic/before/generics.R new file mode 100644 index 000000000..2c3fe2be6 --- /dev/null +++ b/gitnexus/test/fixtures/rename-integration/r/s3-new-method-dotted-generic/before/generics.R @@ -0,0 +1 @@ +to.json <- function(x) UseMethod("to.json") diff --git a/gitnexus/test/fixtures/rename-integration/r/s3-new-method-dotted-generic/before/helper.R b/gitnexus/test/fixtures/rename-integration/r/s3-new-method-dotted-generic/before/helper.R new file mode 100644 index 000000000..94d6da8d4 --- /dev/null +++ b/gitnexus/test/fixtures/rename-integration/r/s3-new-method-dotted-generic/before/helper.R @@ -0,0 +1,5 @@ +helper <- function(v) v + 1 + +use_helper <- function(v) { + helper(v) +} diff --git a/gitnexus/test/fixtures/rename-integration/r/s3-new-method-dotted-generic/case.json b/gitnexus/test/fixtures/rename-integration/r/s3-new-method-dotted-generic/case.json new file mode 100644 index 000000000..35d7968ae --- /dev/null +++ b/gitnexus/test/fixtures/rename-integration/r/s3-new-method-dotted-generic/case.json @@ -0,0 +1,9 @@ +{ + "language": "r", + "description": "Refuse a new name whose second dot split names an existing dotted generic (to.json.foo registers a method of to.json).", + "categories": ["s3", "name-collision", "negative", "unchanged-on-error"], + "target": { "name": "helper", "kind": "Function", "file": "helper.R", "startLine": 0 }, + "newName": "to.json.foo", + "expect": { "status": "error", "code": "unsupported_occurrence", "planningStatus": "unsupported" }, + "edges": [] +} diff --git a/gitnexus/test/unit/r-rename-s3-guard.test.ts b/gitnexus/test/unit/r-rename-s3-guard.test.ts new file mode 100644 index 000000000..0743fe06d --- /dev/null +++ b/gitnexus/test/unit/r-rename-s3-guard.test.ts @@ -0,0 +1,76 @@ +/** + * The R rename provider refuses a new name `generic.class` that would silently register an S3 + * method. Every dot is a possible split, so `to.json.foo` is a method of `to.json`, and a base + * generic counts although no indexed source defines it (`print.foo`). Each refusal is paired with + * a name that only looks similar. + */ +import { describe, expect, it } from 'vitest'; +import Parser from 'tree-sitter'; +import R from '@eagleoutice/tree-sitter-r'; +import { registersS3Method, rRenameProvider } from '../../src/core/ingestion/languages/r/rename.js'; + +const parser = new Parser(); +parser.setLanguage(R as Parameters[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'); + const files = new Map([ + [ + 'a.R', + { + tree: parser.parse(source), + parsed: { localDefs: defined.map((name) => ({ qualifiedName: name })) }, + }, + ], + ]); + const issue = rRenameProvider.checkSafety?.({ + target: { type: 'Function', qualifiedName: 'helper', filePath: 'a.R' }, + newName, + files, + graph: { forEachNode: () => undefined }, + } as never); + return issue ? issue.code : 'ok'; +} + +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'); + }); + + it('refuses a name whose later dot split is a defined dotted generic', () => { + expect(verdict('to.json.foo', ['to.json'])).toBe('unsupported_occurrence'); + expect(verdict('as.data.frame.foo', ['as.data.frame'])).toBe('unsupported_occurrence'); + }); + + it('refuses a name with one dot after a defined function of a shorter name', () => { + expect(verdict('to.json', ['to'])).toBe('unsupported_occurrence'); + }); + + it('refuses a method of a base generic that the sources do not define', () => { + for (const name of ['print.foo', 'format.foo', 'summary.foo', 'plot.foo', 'as.character.foo']) { + expect(verdict(name, [])).toBe('unsupported_occurrence'); + } + }); + + it('allows a dotted name when no prefix is a function or a base generic', () => { + expect(verdict('my.helper', [])).toBe('ok'); + expect(verdict('to.json.foo', [])).toBe('ok'); + expect(verdict('toJson.foo', ['to.json'])).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'); + expect(verdict('.print.x', [])).toBe('ok'); + }); + + it('tests every prefix of the name', () => { + const defined = new Set(['a.b']); + expect([ + registersS3Method('a.b.c.d', defined), + registersS3Method('a.x.b', defined), + registersS3Method('a.b', defined), + ]).toEqual([true, false, false]); + }); +});