diff --git a/docs/languages/r-provider.md b/docs/languages/r-provider.md index 929619d49..7e407de72 100644 --- a/docs/languages/r-provider.md +++ b/docs/languages/r-provider.md @@ -60,7 +60,7 @@ The scope query (`languages/r/query.ts`) captures three call shapes only: `f(x)` - Discovery may have missed a package (its limits were reached): the call keeps the name-based behaviour. - `::` and `:::` are not distinguished, so `pkg::name` binds to a definition whether or not the package exports it. - Package discovery is bounded to depth 3 and 200 directories (when the bound may have hidden a package this is flagged internally and nothing is logged), reads directories in code-unit order, ranks duplicate package names by shallowest then by whole path, and never searches the directories the indexer itself ignores (dot-directories, `vendor`, `build`, `dist`, `lib64`, `fixtures` and the rest of its built-in list) or those named exactly `renv`, `packrat`, `revdep` or `.Rcheck`. A `DESCRIPTION` with a `Built:` field is an installed library package (`lib/`, `library/`) and registers nothing. The shared file scanner has no `renv`, `packrat`, `revdep` or `.Rcheck` entries, so `.R` files inside those trees are still parsed even though discovery skips them. -- Export status: the language-level export check treats every symbol as public. The NAMESPACE pass then marks the remaining `R/` symbols of a NAMESPACE-bearing package non-exported. Symbols outside `R/` (`tests/`, `vignettes/`, `inst/`) are not package members and stay public; files in `R/` subdirectories are treated as `R/` files although R itself does not load them. A package without a NAMESPACE stays fully public, and a nested package without one does not inherit its enclosing package's. roxygen2 `@export` tags are not consulted. The refinement changes the graph nodes' `isExported` only; the definitions scope resolution works from carry no export verdict, so `library()`, `pkg::name` and the global-name fallback do not consult NAMESPACE (only `importFrom()` does). +- Export status: the language-level export check treats every symbol as public. The NAMESPACE pass then marks the remaining `R/` symbols of a NAMESPACE-bearing package non-exported. Symbols outside `R/` (`tests/`, `vignettes/`, `inst/`) are not package members and stay public; files in `R/` subdirectories are treated as `R/` files although R itself does not load them. A package without a NAMESPACE stays fully public, and a nested package without one does not inherit its enclosing package's. roxygen2 `@export` tags are not consulted by the index (`rename` does read them, see Rename). The refinement changes the graph nodes' `isExported` only; the definitions scope resolution works from carry no export verdict, so `library()`, `pkg::name` and the global-name fallback do not consult NAMESPACE (only `importFrom()` does). - `exportPattern()` text comes from the analysed repository, so patterns are compiled to a linear-time automaton (`linear-regex.ts`) with a work budget. Constructs with no linear-time algorithm (back-references, look-around) and oversized patterns are dropped with one logged warning per pattern per package. Character classes use ASCII semantics. A directive argument the NAMESPACE reader cannot read at all is dropped without a warning: an R 4.0 raw string (`exportPattern(r"(^[^.])")`, `export(r"(foo)")`) and any string that spans lines. It exports nothing, so the names it was meant to cover read as non-exported in the index. The reader records that it dropped something that may hide an export (this, a conditional `export()` below, or a malformed directive that swallowed one), and `rename` refuses on that record (see Rename). ## Limits and known gaps @@ -156,7 +156,8 @@ Not modelled: - a new name that the sources already read as a value or call (`name_collision`): `sapply(x, paste)`, `Reduce(max, x)`, `x <- letters`, a default `f = assist`, a formula term, `assist[1]`, `assist(1)`. After the rename the function is the global of that name and would capture the read. A parameter of an enclosing function of that name covers a read, and covers a call only when it has no default. Not reads: bound names, argument names (`f(assist = 1)`), members (`x$assist`, `x@assist`) and qualified names (`pkg::assist`); - a call of the name under a same-named parameter that may hold a function (`ambiguous_reference`): a function, `NULL` or computed default, or an assignment to the parameter in the body, because the call may reach the parameter or the function being renamed; - a function that is also assigned with `<<-` or `->>` anywhere in the indexed R sources (`ambiguous_reference`, as the top-level form already was): the global re-assignment is a second definition that a rename would leave behind; - - a package function under `R/` that is not proven private, because `NAMESPACE` is not an indexed source and its entry would not be renamed. Two checks must both pass. The persisted `isExported` flag must be false, which it is only for a function that a `NAMESPACE` file beside the package leaves out of `export()`, `exportPattern()` and `S3method()` (the third argument of `S3method(generic, class, method)` counts): a package with no `NAMESPACE` file, or an `R/` folder that belongs to no package, keeps the default public flag and is refused. The flag is not enough alone, because an edit of `NAMESPACE` with no `R/` file touched leaves it stale until a full analyse. So every rename plan also reads the package's `NAMESPACE` from disk again, through the parsing the indexer uses, and refuses (`unsupported_occurrence`) when it now exports the name by `export()`, `exportPattern()` or `S3method()`. It fails closed with `incomplete_semantics` when that `NAMESPACE` was deleted or cannot be read, when an `exportPattern()` of it was dropped or ran out of work budget (a name such a pattern was meant to cover is then not provably private), or when the reader skipped or could not read something that may hide an export: a conditional `if (...) export(x)`, a raw-string or multi-line argument, an argument that is an expression, an `S3method()` it cannot read, or a malformed directive that swallowed an export. A private function in a package whose `NAMESPACE` is unchanged is renamed; a script outside `R/` is not affected. The check does not need `--force`; re-run `analyze` to bring the flag itself up to date. + - a package function under `R/` that is not proven private, because `NAMESPACE` is not an indexed source and its entry would not be renamed. Two checks must both pass. The persisted `isExported` flag must be false, which it is only for a function that a `NAMESPACE` file beside the package leaves out of `export()`, `exportPattern()` and `S3method()` (the third argument of `S3method(generic, class, method)` counts): a package with no `NAMESPACE` file, or an `R/` folder that belongs to no package, keeps the default public flag and is refused. The flag is not enough alone, because an edit of `NAMESPACE` with no `R/` file touched leaves it stale until a full analyse. So every rename plan also reads the package's `NAMESPACE` from disk again, through the parsing the indexer uses, and refuses (`unsupported_occurrence`) when it now exports the name by `export()`, `exportPattern()` or `S3method()`. It fails closed with `incomplete_semantics` when that `NAMESPACE` was deleted or cannot be read, when an `exportPattern()` of it was dropped or ran out of work budget (a name such a pattern was meant to cover is then not provably private), or when the reader skipped or could not read something that may hide an export: a conditional `if (...) export(x)`, a raw-string or multi-line argument, an argument that is an expression, an `S3method()` it cannot read, or a malformed directive that swallowed an export. It also refuses (`unsupported_occurrence`) a function whose roxygen2 block (the `#'` lines directly above the definition) carries `@export`, `@exportMethod`, `@exportS3Method` or another `@export*` tag, because the next `document()` would export the new name; the tag is found by text, other roxygen2 tags (`@rdname`, `@inheritParams`, `@seealso`) are not read. A private function in a package whose `NAMESPACE` is unchanged is renamed; a script outside `R/` is not affected. The check does not need `--force`; re-run `analyze` to bring the flag itself up to date. +- **Not updated by a successful rename:** files that are not indexed R sources. R Markdown and Quarto documents (`.Rmd`, `.qmd`, vignettes, `README.Rmd`), `man/*.Rd` (`\alias`, `\usage`), roxygen2 tags that name the function (`@rdname`, `@inheritParams`, `@seealso`), `_pkgdown.yml`, `pkg:::name` uses in `inst/`, and C++ or other code that looks it up by string keep the old name. The result does not list them; search for the old name in those files after a rename. - **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). - **Invalid names (`invalid_name`):** anything that is not a nonreserved R identifier: the reserved words, `...` and `..1`, `..2`, ..., and any name with a non-ASCII character, because new names are limited to ASCII letters, digits, `.` and `_` (conservative by design; the message says so). An old name made only of non-ASCII characters (`λ`) is refused as not a supported identifier. Backtick-quoting a new name is never offered: a name that needs it is refused rather than quoted at every use. - **Not proven:** unqualified calls to a function defined in another file (including the package `R/` layout) fail with `ambiguous_reference`, because R's global-name fallback is not a provable binding. Renaming across files works only where the call is qualified or otherwise resolved. diff --git a/gitnexus/src/core/ingestion/languages/r/rename.ts b/gitnexus/src/core/ingestion/languages/r/rename.ts index 11364d95e..5dff76c17 100644 --- a/gitnexus/src/core/ingestion/languages/r/rename.ts +++ b/gitnexus/src/core/ingestion/languages/r/rename.ts @@ -509,6 +509,14 @@ export const rRenameProvider: RenameProvider = { // NAMESPACE the planner has just loaded from disk for this plan. const file = target.filePath; if (!/(^|\/)R\//.test(file)) return; + // roxygen2 regenerates NAMESPACE from `@export*` tags on the next `document()`, so a tag on + // the function's block would export the new name (and drop the old one) even when the + // NAMESPACE on disk says private now. + const tree = files.get(file)?.tree; + if (tree && hasRoxygenExportTag(tree.rootNode, name)) + return unsupported( + 'A roxygen2 @export tag is attached to this function; renaming it would change the exported name when NAMESPACE is next regenerated.', + ); let exported = false; graph.forEachNode((node) => { if ( @@ -528,6 +536,28 @@ export const rRenameProvider: RenameProvider = { }, }; +/** A roxygen2 tag that adds its object to NAMESPACE: `@export`, `@exportMethod`, `@exportS3Method`, ... */ +const ROXYGEN_EXPORT_TAG = /^#'[ \t]*@export\w*(?:[ \t]|$)/; + +/** + * Whether a top-level assignment of `name` in the file carries a roxygen2 `@export*` tag: a + * `#'` line in the run of comments that directly precedes it (a plain `#` comment in between + * does not break the run, as when roxygen2 collects the block). Text only; the tags are not parsed. + */ +function hasRoxygenExportTag(root: Parser.SyntaxNode, name: string): boolean { + for (const node of walkNamedNodes(root)) { + if (boundName(node) !== name || insideFunction(node)) continue; + for (let sibling = node.previousSibling; sibling; sibling = sibling.previousSibling) { + if (sibling.type !== 'comment') { + if (sibling.isNamed) break; + continue; + } + if (ROXYGEN_EXPORT_TAG.test(sibling.text)) return true; + } + } + return false; +} + /** * Whether the NAMESPACE read from disk for this plan leaves `name` (defined in `file`) private. * Fail-closed: anything short of a NAMESPACE that was read, parsed completely and does not export diff --git a/gitnexus/test/fixtures/rename-integration/r/namespace-roxygen-export/after/DESCRIPTION b/gitnexus/test/fixtures/rename-integration/r/namespace-roxygen-export/after/DESCRIPTION new file mode 100644 index 000000000..2eb67eae1 --- /dev/null +++ b/gitnexus/test/fixtures/rename-integration/r/namespace-roxygen-export/after/DESCRIPTION @@ -0,0 +1,3 @@ +Package: pk +Version: 0.1.0 +Title: Fixture diff --git a/gitnexus/test/fixtures/rename-integration/r/namespace-roxygen-export/after/NAMESPACE b/gitnexus/test/fixtures/rename-integration/r/namespace-roxygen-export/after/NAMESPACE new file mode 100644 index 000000000..d401a4917 --- /dev/null +++ b/gitnexus/test/fixtures/rename-integration/r/namespace-roxygen-export/after/NAMESPACE @@ -0,0 +1 @@ +export(pub) diff --git a/gitnexus/test/fixtures/rename-integration/r/namespace-roxygen-export/after/R/pub.R b/gitnexus/test/fixtures/rename-integration/r/namespace-roxygen-export/after/R/pub.R new file mode 100644 index 000000000..717c3d196 --- /dev/null +++ b/gitnexus/test/fixtures/rename-integration/r/namespace-roxygen-export/after/R/pub.R @@ -0,0 +1,5 @@ +pub <- function() 1 + +#' Tagged for export, but NAMESPACE has not been regenerated yet. +#' @export +newfun <- function() 2 diff --git a/gitnexus/test/fixtures/rename-integration/r/namespace-roxygen-export/before/DESCRIPTION b/gitnexus/test/fixtures/rename-integration/r/namespace-roxygen-export/before/DESCRIPTION new file mode 100644 index 000000000..2eb67eae1 --- /dev/null +++ b/gitnexus/test/fixtures/rename-integration/r/namespace-roxygen-export/before/DESCRIPTION @@ -0,0 +1,3 @@ +Package: pk +Version: 0.1.0 +Title: Fixture diff --git a/gitnexus/test/fixtures/rename-integration/r/namespace-roxygen-export/before/NAMESPACE b/gitnexus/test/fixtures/rename-integration/r/namespace-roxygen-export/before/NAMESPACE new file mode 100644 index 000000000..d401a4917 --- /dev/null +++ b/gitnexus/test/fixtures/rename-integration/r/namespace-roxygen-export/before/NAMESPACE @@ -0,0 +1 @@ +export(pub) diff --git a/gitnexus/test/fixtures/rename-integration/r/namespace-roxygen-export/before/R/pub.R b/gitnexus/test/fixtures/rename-integration/r/namespace-roxygen-export/before/R/pub.R new file mode 100644 index 000000000..717c3d196 --- /dev/null +++ b/gitnexus/test/fixtures/rename-integration/r/namespace-roxygen-export/before/R/pub.R @@ -0,0 +1,5 @@ +pub <- function() 1 + +#' Tagged for export, but NAMESPACE has not been regenerated yet. +#' @export +newfun <- function() 2 diff --git a/gitnexus/test/fixtures/rename-integration/r/namespace-roxygen-export/case.json b/gitnexus/test/fixtures/rename-integration/r/namespace-roxygen-export/case.json new file mode 100644 index 000000000..1673753d9 --- /dev/null +++ b/gitnexus/test/fixtures/rename-integration/r/namespace-roxygen-export/case.json @@ -0,0 +1,9 @@ +{ + "language": "r", + "description": "Refuse a private-looking package function whose roxygen2 block carries @export: the next document() would export the new name.", + "categories": ["namespace", "package-export", "roxygen", "negative", "unchanged-on-error"], + "target": { "name": "newfun", "kind": "Function", "file": "R/pub.R", "startLine": 4 }, + "newName": "newfun2", + "expect": { "status": "error", "code": "unsupported_occurrence", "planningStatus": "unsupported" }, + "edges": [] +} diff --git a/gitnexus/test/integration/rename-provider-r.test.ts b/gitnexus/test/integration/rename-provider-r.test.ts index 277a5e49e..faba603b2 100644 --- a/gitnexus/test/integration/rename-provider-r.test.ts +++ b/gitnexus/test/integration/rename-provider-r.test.ts @@ -5,7 +5,7 @@ import { defineRenameFixtureTests, renameCases } from '../helpers/rename-integra // would define no test and stay green. Pin the number of R cases: adding one means raising it. describe('R rename fixtures', () => { it('are all discovered', () => { - expect(renameCases.filter((fixture) => fixture.language === 'r')).toHaveLength(53); + expect(renameCases.filter((fixture) => fixture.language === 'r')).toHaveLength(54); }); }); diff --git a/gitnexus/test/unit/r-rename-roxygen-export.test.ts b/gitnexus/test/unit/r-rename-roxygen-export.test.ts new file mode 100644 index 000000000..00d4a4b31 --- /dev/null +++ b/gitnexus/test/unit/r-rename-roxygen-export.test.ts @@ -0,0 +1,98 @@ +/** + * R rename: a roxygen2 `@export*` tag on the function's block refuses the rename. roxygen2 + * rebuilds NAMESPACE from those tags on the next `document()`, so renaming a tagged function + * would change the package's public API even though the NAMESPACE on disk (and the persisted + * `isExported`) say it is private today. The graph and NAMESPACE here say "private" throughout, + * so every refusal below comes from the tag. + */ +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import fs from 'node:fs/promises'; +import os from 'node:os'; +import path from 'node:path'; +import Parser from 'tree-sitter'; +import R from '@eagleoutice/tree-sitter-r'; +import { SupportedLanguages } from 'gitnexus-shared'; +import { createKnowledgeGraph } from '../../src/core/graph/graph.js'; +import { + loadRPackageConfig, + recordRPackageConfig, +} from '../../src/core/ingestion/languages/r/package-config.js'; +import { rRenameProvider } from '../../src/core/ingestion/languages/r/rename.js'; + +const parser = new Parser(); +parser.setLanguage(R as Parameters[0]); + +let root: string; + +beforeAll(async () => { + root = await fs.mkdtemp(path.join(os.tmpdir(), 'gnx-r-rename-roxygen-')); + await fs.mkdir(path.join(root, 'pk', 'R'), { recursive: true }); + await fs.writeFile(path.join(root, 'pk', 'DESCRIPTION'), 'Package: pk\nVersion: 0.1.0\n'); + await fs.writeFile(path.join(root, 'pk', 'NAMESPACE'), 'export(pub)\n'); +}); + +afterAll(async () => { + await fs.rm(root, { recursive: true, force: true }); +}); + +/** The verdict of renaming `hidden` in `source`, a file of the package `pk`. */ +async function verdictFor(source: string, file = 'pk/R/a.R'): Promise { + const graph = createKnowledgeGraph(); + graph.addNode({ + id: `Function:${file}:hidden`, + label: 'Function', + properties: { + name: 'hidden', + filePath: file, + language: SupportedLanguages.R, + isExported: false, + }, + }); + const parsed = { localDefs: [] }; + recordRPackageConfig([parsed], await loadRPackageConfig(root)); + const issue = rRenameProvider.checkSafety?.({ + target: { type: 'Function', qualifiedName: 'hidden', filePath: file }, + newName: 'secret', + files: new Map([[file, { tree: parser.parse(source), parsed }]]), + graph, + } as never); + return issue ? issue.code : 'ok'; +} + +describe('R rename: roxygen @export tags', () => { + it('renames a documented function without an export tag', async () => { + expect( + await verdictFor("#' Hidden.\n#' @param x A value.\n#' @noRd\nhidden <- function(x) x\n"), + ).toBe('ok'); + expect(await verdictFor('hidden <- function(x) x\n')).toBe('ok'); + }); + + it.each([ + ['@export', "#' @export\nhidden <- function(x) x\n"], + [ + '@export after other tags', + "#' Hidden.\n#' @param x A value.\n#' @export\nhidden <- function(x) x\n", + ], + ['@exportMethod', "#' @exportMethod hidden\nhidden <- function(x) x\n"], + ['@exportS3Method', "#' @exportS3Method print foo\nhidden <- function(x) x\n"], + ['@export with a plain comment between', "#' @export\n# TODO tidy\nhidden <- function(x) x\n"], + ["@export written with no space after #'", "#'@export\nhidden <- function(x) x\n"], + ['an = assignment', "#' @export\nhidden = function(x) x\n"], + ])('refuses %s', async (_label, source) => { + expect(await verdictFor(source)).toBe('unsupported_occurrence'); + }); + + it('is not triggered by the tag of another function or by prose', async () => { + expect(await verdictFor("#' @export\nother <- function() 1\n\nhidden <- function(x) x\n")).toBe( + 'ok', + ); + expect(await verdictFor("#' Mention of @export in prose.\nhidden <- function(x) x\n")).toBe( + 'ok', + ); + expect(await verdictFor('# @export (plain comment)\nhidden <- function(x) x\n')).toBe('ok'); + }); + + it('leaves a script outside R/ alone', async () => { + expect(await verdictFor("#' @export\nhidden <- function(x) x\n", 'pk/scripts/a.R')).toBe('ok'); + }); +});