From dd4daefa3f710452b988cd6844fa274abfd3179b Mon Sep 17 00:00:00 2001 From: Mick Cooney Date: Sat, 17 Oct 2026 16:54:35 +0100 Subject: [PATCH] fix(r): refuse a new name that the sources already read as a value The engine protects the call form of a new name, but R's reference sites are calls, so a plain read was invisible: sapply(x, paste), Reduce(max, x), x <- letters or a default f = assist all kept compiling and silently started to mean the renamed global. The provider now refuses any identifier read of the new name that is not bound locally: a parameter of an enclosing function covers a read, and covers a call only when it has no default. Bound names, argument names, members ($, @) and qualified names are not reads. Four fixtures and unit tests cover values, callees, formulas and the allowed spellings. --- .../src/core/ingestion/languages/r/rename.ts | 45 ++++++++++++++ .../r/value-reference-constant/after/ref.R | 3 + .../r/value-reference-constant/before/ref.R | 3 + .../r/value-reference-constant/case.json | 9 +++ .../r/value-reference-default/after/ref.R | 3 + .../r/value-reference-default/before/ref.R | 3 + .../r/value-reference-default/case.json | 9 +++ .../r/value-reference-formula/after/ref.R | 3 + .../r/value-reference-formula/before/ref.R | 3 + .../r/value-reference-formula/case.json | 9 +++ .../after/ref.R | 3 + .../before/ref.R | 3 + .../case.json | 9 +++ .../test/unit/r-rename-collisions.test.ts | 58 +++++++++++++++++++ 14 files changed, 163 insertions(+) create mode 100644 gitnexus/test/fixtures/rename-integration/r/value-reference-constant/after/ref.R create mode 100644 gitnexus/test/fixtures/rename-integration/r/value-reference-constant/before/ref.R create mode 100644 gitnexus/test/fixtures/rename-integration/r/value-reference-constant/case.json create mode 100644 gitnexus/test/fixtures/rename-integration/r/value-reference-default/after/ref.R create mode 100644 gitnexus/test/fixtures/rename-integration/r/value-reference-default/before/ref.R create mode 100644 gitnexus/test/fixtures/rename-integration/r/value-reference-default/case.json create mode 100644 gitnexus/test/fixtures/rename-integration/r/value-reference-formula/after/ref.R create mode 100644 gitnexus/test/fixtures/rename-integration/r/value-reference-formula/before/ref.R create mode 100644 gitnexus/test/fixtures/rename-integration/r/value-reference-formula/case.json create mode 100644 gitnexus/test/fixtures/rename-integration/r/value-reference-function-argument/after/ref.R create mode 100644 gitnexus/test/fixtures/rename-integration/r/value-reference-function-argument/before/ref.R create mode 100644 gitnexus/test/fixtures/rename-integration/r/value-reference-function-argument/case.json diff --git a/gitnexus/src/core/ingestion/languages/r/rename.ts b/gitnexus/src/core/ingestion/languages/r/rename.ts index 7961ddd9b..bea8ce5c9 100644 --- a/gitnexus/src/core/ingestion/languages/r/rename.ts +++ b/gitnexus/src/core/ingestion/languages/r/rename.ts @@ -248,6 +248,43 @@ function insideFunction(node: Parser.SyntaxNode): boolean { return false; } +/** + * Whether `node`, an identifier spelled like the new name, is used to READ a binding: a value + * (`sapply(x, paste)`, `x <- letters`, a default `f = assist`) or a callee. After the rename + * the renamed function is the global of that name, so a read that reached something else + * (a base or attached object, another package's function) would be captured by it. Not reads: + * a bound name (`assist <- 3`, a parameter, a `for` variable), an argument name (`f(assist = 1)`), + * a member (`x$assist`, `x@assist`) and a qualified name (`pkg::assist`). + */ +function readsNewName(node: Parser.SyntaxNode, newName: string): boolean { + const parent = node.parent; + if (parent === null) return true; + switch (parent.type) { + case 'argument': + if (parent.childForFieldName('name')?.id === node.id) return false; + break; + case 'parameter': + if (parent.childForFieldName('name')?.id === node.id) return false; + break; + case 'extract_operator': + if (parent.childForFieldName('rhs')?.id === node.id) return false; + break; + case 'namespace_operator': + return false; + case 'for_statement': + if (parent.childForFieldName('variable')?.id === node.id) return false; + break; + case 'binary_operator': + if (assignmentTarget(parent)?.id === node.id) return false; + break; + } + // A parameter of the enclosing function binds the name locally: a read of it, or a call of it + // when it can only be a function (no default), never reaches the global. + const local = innermostParameterKind(node, newName); + if (local === null) return true; + return isCallee(node) && local !== 'function'; +} + /** * A name R also writes between its operands: an infix function (`%+%`, `%in%`) or an operator * (`+`, `[[`). Its uses are operator tokens, not `identifier` nodes, so a rename would edit the @@ -371,6 +408,14 @@ export const rRenameProvider: RenameProvider = { superassignedName(node) === newName ) return collision('The new name is already assigned as a variable at top level.'); + if ( + node.type === 'identifier' && + rBareName(node.text) === newName && + readsNewName(node, newName) + ) + return collision( + 'The new name is already read here as a value or called; the renamed function would capture it.', + ); // `name <<- value` (inside a function or not) is a global re-definition that the // scope graph keeps as a separate declaration, so a rename would leave it behind. if (node.type === 'identifier' && isCallee(node) && rBareName(node.text) === name) { diff --git a/gitnexus/test/fixtures/rename-integration/r/value-reference-constant/after/ref.R b/gitnexus/test/fixtures/rename-integration/r/value-reference-constant/after/ref.R new file mode 100644 index 000000000..5c037bc43 --- /dev/null +++ b/gitnexus/test/fixtures/rename-integration/r/value-reference-constant/after/ref.R @@ -0,0 +1,3 @@ +helper <- function(v) v + 1 + +alphabet <- letters diff --git a/gitnexus/test/fixtures/rename-integration/r/value-reference-constant/before/ref.R b/gitnexus/test/fixtures/rename-integration/r/value-reference-constant/before/ref.R new file mode 100644 index 000000000..5c037bc43 --- /dev/null +++ b/gitnexus/test/fixtures/rename-integration/r/value-reference-constant/before/ref.R @@ -0,0 +1,3 @@ +helper <- function(v) v + 1 + +alphabet <- letters diff --git a/gitnexus/test/fixtures/rename-integration/r/value-reference-constant/case.json b/gitnexus/test/fixtures/rename-integration/r/value-reference-constant/case.json new file mode 100644 index 000000000..e2202d568 --- /dev/null +++ b/gitnexus/test/fixtures/rename-integration/r/value-reference-constant/case.json @@ -0,0 +1,9 @@ +{ + "language": "r", + "description": "Refuse a new name that the sources read as a base constant (x <- letters): the renamed function would shadow it.", + "categories": ["name-collision", "value-reference", "negative", "unchanged-on-error"], + "target": { "name": "helper", "kind": "Function", "file": "ref.R", "startLine": 0 }, + "newName": "letters", + "expect": { "status": "error", "code": "name_collision", "planningStatus": "blocked" }, + "edges": [] +} diff --git a/gitnexus/test/fixtures/rename-integration/r/value-reference-default/after/ref.R b/gitnexus/test/fixtures/rename-integration/r/value-reference-default/after/ref.R new file mode 100644 index 000000000..79f7b005e --- /dev/null +++ b/gitnexus/test/fixtures/rename-integration/r/value-reference-default/after/ref.R @@ -0,0 +1,3 @@ +helper <- function(v) v + 1 + +use <- function(f = assist) f(1) diff --git a/gitnexus/test/fixtures/rename-integration/r/value-reference-default/before/ref.R b/gitnexus/test/fixtures/rename-integration/r/value-reference-default/before/ref.R new file mode 100644 index 000000000..79f7b005e --- /dev/null +++ b/gitnexus/test/fixtures/rename-integration/r/value-reference-default/before/ref.R @@ -0,0 +1,3 @@ +helper <- function(v) v + 1 + +use <- function(f = assist) f(1) diff --git a/gitnexus/test/fixtures/rename-integration/r/value-reference-default/case.json b/gitnexus/test/fixtures/rename-integration/r/value-reference-default/case.json new file mode 100644 index 000000000..218048e93 --- /dev/null +++ b/gitnexus/test/fixtures/rename-integration/r/value-reference-default/case.json @@ -0,0 +1,9 @@ +{ + "language": "r", + "description": "Refuse a new name that a parameter default reads (f = assist): the default would start to mean the renamed function.", + "categories": ["name-collision", "value-reference", "negative", "unchanged-on-error"], + "target": { "name": "helper", "kind": "Function", "file": "ref.R", "startLine": 0 }, + "newName": "assist", + "expect": { "status": "error", "code": "name_collision", "planningStatus": "blocked" }, + "edges": [] +} diff --git a/gitnexus/test/fixtures/rename-integration/r/value-reference-formula/after/ref.R b/gitnexus/test/fixtures/rename-integration/r/value-reference-formula/after/ref.R new file mode 100644 index 000000000..0ce07de00 --- /dev/null +++ b/gitnexus/test/fixtures/rename-integration/r/value-reference-formula/after/ref.R @@ -0,0 +1,3 @@ +helper <- function(v) v + 1 + +fit <- function(d) lm(y ~ assist, data = d) diff --git a/gitnexus/test/fixtures/rename-integration/r/value-reference-formula/before/ref.R b/gitnexus/test/fixtures/rename-integration/r/value-reference-formula/before/ref.R new file mode 100644 index 000000000..0ce07de00 --- /dev/null +++ b/gitnexus/test/fixtures/rename-integration/r/value-reference-formula/before/ref.R @@ -0,0 +1,3 @@ +helper <- function(v) v + 1 + +fit <- function(d) lm(y ~ assist, data = d) diff --git a/gitnexus/test/fixtures/rename-integration/r/value-reference-formula/case.json b/gitnexus/test/fixtures/rename-integration/r/value-reference-formula/case.json new file mode 100644 index 000000000..7a3ff3911 --- /dev/null +++ b/gitnexus/test/fixtures/rename-integration/r/value-reference-formula/case.json @@ -0,0 +1,9 @@ +{ + "language": "r", + "description": "Refuse a new name that a formula spells as a variable.", + "categories": ["name-collision", "value-reference", "negative", "unchanged-on-error"], + "target": { "name": "helper", "kind": "Function", "file": "ref.R", "startLine": 0 }, + "newName": "assist", + "expect": { "status": "error", "code": "name_collision", "planningStatus": "blocked" }, + "edges": [] +} diff --git a/gitnexus/test/fixtures/rename-integration/r/value-reference-function-argument/after/ref.R b/gitnexus/test/fixtures/rename-integration/r/value-reference-function-argument/after/ref.R new file mode 100644 index 000000000..5cac9ce35 --- /dev/null +++ b/gitnexus/test/fixtures/rename-integration/r/value-reference-function-argument/after/ref.R @@ -0,0 +1,3 @@ +helper <- function(v) v + 1 + +squares <- sapply(1:3, paste) diff --git a/gitnexus/test/fixtures/rename-integration/r/value-reference-function-argument/before/ref.R b/gitnexus/test/fixtures/rename-integration/r/value-reference-function-argument/before/ref.R new file mode 100644 index 000000000..5cac9ce35 --- /dev/null +++ b/gitnexus/test/fixtures/rename-integration/r/value-reference-function-argument/before/ref.R @@ -0,0 +1,3 @@ +helper <- function(v) v + 1 + +squares <- sapply(1:3, paste) diff --git a/gitnexus/test/fixtures/rename-integration/r/value-reference-function-argument/case.json b/gitnexus/test/fixtures/rename-integration/r/value-reference-function-argument/case.json new file mode 100644 index 000000000..59e8b3da8 --- /dev/null +++ b/gitnexus/test/fixtures/rename-integration/r/value-reference-function-argument/case.json @@ -0,0 +1,9 @@ +{ + "language": "r", + "description": "Refuse a new name that the sources pass as a function value (sapply(x, paste)): the renamed global would capture the base function.", + "categories": ["name-collision", "value-reference", "negative", "unchanged-on-error"], + "target": { "name": "helper", "kind": "Function", "file": "ref.R", "startLine": 0 }, + "newName": "paste", + "expect": { "status": "error", "code": "name_collision", "planningStatus": "blocked" }, + "edges": [] +} diff --git a/gitnexus/test/unit/r-rename-collisions.test.ts b/gitnexus/test/unit/r-rename-collisions.test.ts index 3e6868f4a..068deeb2c 100644 --- a/gitnexus/test/unit/r-rename-collisions.test.ts +++ b/gitnexus/test/unit/r-rename-collisions.test.ts @@ -65,3 +65,61 @@ describe('R rename: a new name that already exists', () => { } }); }); + +describe('R rename: a new name that the sources already read', () => { + it('refuses a value, a function argument or a callee of the new name', () => { + for (const source of [ + 'r <- sapply(1:3, assist)\n', + 'r <- Reduce(assist, 1:3)\n', + 'x <- assist\n', + 'x <- assist[1]\n', + 'x <- assist$a\n', + 'f <- function(f = assist) f(1)\n', + 'r <- assist(1)\n', + 'r <- 1 |> assist()\n', + 'r <- c(1, 2) |> sapply(assist)\n', + 'r <- y ~ assist\n', + 'print(`assist`)\n', + 'g <- function() { assist <- 3; assist }\n', + ]) { + expect(verdict(source), source).toBe('name_collision'); + } + }); + + it('allows a name that is not a read: argument name, member, qualified name, parameter', () => { + for (const source of [ + 'r <- f(assist = 1)\n', + 'r <- x$assist\n', + 'r <- x@assist\n', + 'r <- pkg::assist\n', + 'r <- assist::f(1)\n', + 'r <- pkg:::assist(1)\n', + 'f <- function(assist) assist * 2\n', + 'f <- function(assist, n = assist) n\n', + 'f <- function(assist) { g <- function() assist; g() }\n', + 'f <- function(assist) assist(1)\n', + '# assist\n', + ]) { + expect(verdict(source), source).toBe('ok'); + } + }); + + it('still refuses a call of a parameter that may not hold a function', () => { + for (const source of [ + 'f <- function(assist = 2) assist(1)\n', + 'f <- function(assist = g()) assist(1)\n', + 'f <- function(assist) { assist <- 3; assist(1) }\n', + ]) { + expect(verdict(source), source).toBe('name_collision'); + } + }); + + it('reads the new name through backticks and across files', () => { + expect( + rVerdict({ + files: { 'a.R': HELPER, 'b.R': 'x <- sapply(1:3, `assist`)\n' }, + newName: 'assist', + }), + ).toBe('name_collision'); + }); +});