fix(r): refuse a rename of a function that a roxygen2 @export tag exports

roxygen2 rebuilds NAMESPACE from @export* tags, so renaming a tagged function
changes the package's public API on the next document() even when the NAMESPACE on
disk and the persisted isExported say it is private. checkSafety now refuses a
top-level function whose directly preceding #' block has an @export* tag. Documents
that a successful rename leaves .Rmd/.qmd/.Rd/pkgdown/roxygen cross-reference
consumers unchanged; the rename result has no provider-owned note field.
This commit is contained in:
Mick Cooney 2026-10-18 15:53:42 +01:00
parent 1f7f7135f5
commit fdbb64c920
11 changed files with 159 additions and 3 deletions

View file

@ -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 `<name>.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.

View file

@ -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

View file

@ -0,0 +1,3 @@
Package: pk
Version: 0.1.0
Title: Fixture

View file

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

View file

@ -0,0 +1,5 @@
pub <- function() 1
#' Tagged for export, but NAMESPACE has not been regenerated yet.
#' @export
newfun <- function() 2

View file

@ -0,0 +1,3 @@
Package: pk
Version: 0.1.0
Title: Fixture

View file

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

View file

@ -0,0 +1,5 @@
pub <- function() 1
#' Tagged for export, but NAMESPACE has not been regenerated yet.
#' @export
newfun <- function() 2

View file

@ -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": []
}

View file

@ -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);
});
});

View file

@ -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<Parser['setLanguage']>[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<string> {
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');
});
});