From fc32e53967b5388ec6d12e4875ab4236390d0779 Mon Sep 17 00:00:00 2001 From: Mick Cooney Date: Fri, 16 Oct 2026 15:43:34 +0100 Subject: [PATCH] fix(r): qualify R6, RefClass and S4 member ids with their class Member ids carried no owner, so a second class in one file that shared a member name with an earlier one (push, items, a setMethod generic, a slot) minted the same id as the first and lost the member, along with its HAS_METHOD or HAS_PROPERTY edge. Register resolveDetachedOwnerName for R. It names the class declared by an ancestor R6Class assignment, setClass or setRefClass call, or the signature class of a setMethod call, so ids become Method::Stack.push#n and Property::Stack.items. The owner edge is still attached by postParse through ownerNameHint. The change is R-only: the hook is optional and undefined for every other language. The r-packages characterisation counts now include Child.greet, the member the id collision used to drop. --- docs/languages/r-provider.md | 2 +- .../src/core/ingestion/field-extractors/r.ts | 11 ++ gitnexus/src/core/ingestion/languages/r.ts | 7 +- .../core/ingestion/languages/r/owner-hooks.ts | 18 ++- .../r-class-member-ids/R/classes.R | 33 +++++ .../resolvers/r-class-member-ids.test.ts | 121 ++++++++++++++++++ gitnexus/test/integration/resolvers/r.test.ts | 11 +- gitnexus/test/unit/r-owner-hooks.test.ts | 42 ++++++ 8 files changed, 238 insertions(+), 7 deletions(-) create mode 100644 gitnexus/test/fixtures/lang-resolution/r-class-member-ids/R/classes.R create mode 100644 gitnexus/test/integration/resolvers/r-class-member-ids.test.ts diff --git a/docs/languages/r-provider.md b/docs/languages/r-provider.md index 6623ac29d..01eea34b3 100644 --- a/docs/languages/r-provider.md +++ b/docs/languages/r-provider.md @@ -8,7 +8,7 @@ The provider is covered by unit and integration tests, listed under [Tests](#tes GitNexus indexes `.r` and `.R` files with the `@eagleoutice/tree-sitter-r` grammar and resolves them through the scope-resolution pipeline (see ARCHITECTURE.md, Scope-Resolution Pipeline). R-specific logic lives in `gitnexus/src/core/ingestion/languages/r/`, with the per-language extractor and import-resolver files beside those of the other languages. Apart from the registrations every language has (the language enum, the file-extension and classification tables, the export-checker table, the grammar loader and parse-worker grammar map, and the query, provider and scope-resolver registries), shared ingestion code does not name R; the provider plugs in through `LanguageProvider` and `ScopeResolver` hooks. -R has no class syntax: classes are function calls (`R6Class(...)`, `setClass(...)`, `setRefClass(...)`) and package exports live in a NAMESPACE file rather than in the source. That is why the provider uses two optional hooks, `postParse` (attach each member to its owner and refine export status once every file is parsed) and `resolveMemberOwnerNode` (name the owner of a member from a call rather than an enclosing node). +R has no class syntax: classes are function calls (`R6Class(...)`, `setClass(...)`, `setRefClass(...)`) and package exports live in a NAMESPACE file rather than in the source. That is why the provider uses three optional hooks: `postParse` (attach each member to its owner and refine export status once every file is parsed), `resolveMemberOwnerNode` (find the owner of a member from a call rather than an enclosing node) and `resolveDetachedOwnerName` (qualify a member's id with its class, so two classes in one file that share a member name stay distinct nodes: `Method:R/a.R:Stack.push#0`, `Property:R/a.R:Stack.items`). ## Supported constructs diff --git a/gitnexus/src/core/ingestion/field-extractors/r.ts b/gitnexus/src/core/ingestion/field-extractors/r.ts index 3e6c38964..d2cc8a141 100644 --- a/gitnexus/src/core/ingestion/field-extractors/r.ts +++ b/gitnexus/src/core/ingestion/field-extractors/r.ts @@ -190,6 +190,17 @@ export function findRFieldOwnerNode(node: SyntaxNode): SyntaxNode | null { export function getRTopLevelPropertyOwnerName(node: SyntaxNode): string | null { const ownerNode = findRFieldOwnerNode(node); if (!ownerNode) return null; + return getRClassNameOfOwnerNode(ownerNode); +} + +/** + * The class name an R6 / S4 / RefClass owner node declares: the binding name of + * `Foo <- R6Class(...)`, or the `Class` argument of `setClass(...)` / + * `setRefClass(...)`. Null when `ownerNode` is not such a node. Looks at this node + * only (no ancestor walk), so a caller visiting every ancestor stays linear. + */ +export function getRClassNameOfOwnerNode(ownerNode: SyntaxNode): string | null { + if (!isR6Class(ownerNode) && !isS4Class(ownerNode)) return null; if (ownerNode.type === 'binary_operator') { return ownerNode.childForFieldName('lhs')?.text ?? null; diff --git a/gitnexus/src/core/ingestion/languages/r.ts b/gitnexus/src/core/ingestion/languages/r.ts index 306f1a521..fda94b502 100644 --- a/gitnexus/src/core/ingestion/languages/r.ts +++ b/gitnexus/src/core/ingestion/languages/r.ts @@ -17,7 +17,11 @@ import { rMethodExtractor } from '../method-extractors/r.js'; import { emitRScopeCaptures } from './r/captures.js'; import { interpretRImport, interpretRTypeBinding } from './r/interpret.js'; import { rPostParse } from './r/post-parse.js'; -import { rDefinitionProperties, rResolveMemberOwnerNode } from './r/owner-hooks.js'; +import { + rDefinitionProperties, + rResolveDetachedOwnerName, + rResolveMemberOwnerNode, +} from './r/owner-hooks.js'; import { isRNonNamingArgumentCapture } from './r/naming-argument.js'; import { createRCfgVisitor } from '../cfg/visitors/r.js'; import { rRenameProvider } from './r/rename.js'; @@ -190,6 +194,7 @@ export const rProvider = defineLanguage({ interpretTypeBinding: interpretRTypeBinding, postParse: rPostParse, resolveMemberOwnerNode: rResolveMemberOwnerNode, + resolveDetachedOwnerName: rResolveDetachedOwnerName, definitionPropertiesExtractor: rDefinitionProperties, // R_QUERIES captures every string argument of setClass/setGeneric/setMethod; // only the one that names the definition is kept. diff --git a/gitnexus/src/core/ingestion/languages/r/owner-hooks.ts b/gitnexus/src/core/ingestion/languages/r/owner-hooks.ts index 084b48809..efe771d5a 100644 --- a/gitnexus/src/core/ingestion/languages/r/owner-hooks.ts +++ b/gitnexus/src/core/ingestion/languages/r/owner-hooks.ts @@ -12,13 +12,29 @@ import type { SyntaxNode } from '../../utils/ast-helpers.js'; import type { DefinitionPropertiesContext } from '../../language-provider.js'; -import { findRFieldOwnerNode, getRTopLevelPropertyOwnerName } from '../../field-extractors/r.js'; +import { + findRFieldOwnerNode, + getRClassNameOfOwnerNode, + getRTopLevelPropertyOwnerName, +} from '../../field-extractors/r.js'; import { getRTopLevelMethodOwnerName } from '../../method-extractors/r.js'; /** `LanguageProvider.resolveMemberOwnerNode` for R. */ export const rResolveMemberOwnerNode = (definitionNode: SyntaxNode): SyntaxNode | null => findRFieldOwnerNode(definitionNode); +/** + * `LanguageProvider.resolveDetachedOwnerName` for R: the class name an ancestor + * call or assignment declares (`Foo <- R6Class(...)`, `setClass("Foo", ...)`, + * `setRefClass("Foo", ...)`) or the class a `setMethod("m", "Foo", ...)` call + * names in its signature. It qualifies the member's id (`Method::Foo.m`, + * `Property::Foo.x`), so two classes in one file that share a member name + * stay distinct nodes. It carries no graph id: the owner edge is still attached + * by `postParse` through `ownerNameHint`. + */ +export const rResolveDetachedOwnerName = (ancestor: SyntaxNode): string | undefined => + getRClassNameOfOwnerNode(ancestor) ?? getRTopLevelMethodOwnerName(ancestor) ?? undefined; + /** * `LanguageProvider.definitionPropertiesExtractor` for R: the deferred owner * name (`ownerNameHint`) of a `Method`/`Property` whose owning class is named by diff --git a/gitnexus/test/fixtures/lang-resolution/r-class-member-ids/R/classes.R b/gitnexus/test/fixtures/lang-resolution/r-class-member-ids/R/classes.R new file mode 100644 index 000000000..5a8e952d6 --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/r-class-member-ids/R/classes.R @@ -0,0 +1,33 @@ +# Several classes in one file whose members share names. Each member must be its own +# node, owned by its own class. + +Stack <- R6::R6Class("Stack", public = list( + items = list(), + initialize = function() { self$items <- list() }, + push = function(x) { self$items <- c(self$items, x); invisible(self) }, + twice = function(x) { self$push(x); self$push(x) } +)) + +Queue <- R6::R6Class("Queue", public = list( + items = list(), + initialize = function() { self$items <- list() }, + push = function(x) { self$items <- c(self$items, x); invisible(self) }, + twice = function(x) { self$push(x); self$push(x) } +)) + +Account <- setRefClass("Account", + fields = list(balance = "numeric"), + methods = list(deposit = function(x) { balance <<- balance + x }) +) + +Wallet <- setRefClass("Wallet", + fields = list(balance = "numeric"), + methods = list(deposit = function(x) { balance <<- balance + x }) +) + +setClass("Circle", slots = list(size = "numeric")) +setClass("Square", slots = list(size = "numeric")) + +setGeneric("area", function(shape) standardGeneric("area")) +setMethod("area", "Circle", function(shape) 1) +setMethod("area", "Square", function(shape) 2) diff --git a/gitnexus/test/integration/resolvers/r-class-member-ids.test.ts b/gitnexus/test/integration/resolvers/r-class-member-ids.test.ts new file mode 100644 index 000000000..a54ca6c2a --- /dev/null +++ b/gitnexus/test/integration/resolvers/r-class-member-ids.test.ts @@ -0,0 +1,121 @@ +/** + * R: classes are calls, so a member's id used to carry no owner. Two classes in one + * file that shared a member name (`push`, `items`, `deposit`, `balance`, a `setMethod` + * generic, an S4 slot) minted the same id, and the second class lost the member. The + * id now carries the owning class. + */ +import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; +import path from 'path'; +import { FIXTURES, getRelationships, runPipelineFromRepo } from './helpers.js'; +import type { PipelineResult } from './helpers.js'; + +// Cold worker-pool startups otherwise flake against the 5s default ready budget +// on a loaded runner, failing for reasons unrelated to R parsing or resolution. +// Safe under `pool: 'forks'`: the env stub is process-global, but each file gets its own process. +beforeAll(() => vi.stubEnv('GITNEXUS_WORKER_READY_TIMEOUT_MS', '60000')); +afterAll(() => vi.unstubAllEnvs()); + +const FILE = 'R/classes.R'; + +describe('R members of several classes in one file', () => { + let result: PipelineResult; + + beforeAll(async () => { + result = await runPipelineFromRepo(path.join(FIXTURES, 'r-class-member-ids'), () => {}); + }, 60000); + + /** Ids of the nodes with this label and name, sorted. */ + const idsOf = (label: string, name: string): string[] => { + const ids: string[] = []; + result.graph.forEachNode((n) => { + if (n.label === label && n.properties.name === name && n.properties.filePath === FILE) { + ids.push(n.id); + } + }); + return ids.sort(); + }; + + /** `ownerName -> memberName` for every edge of this type, sorted. */ + const owned = (type: 'HAS_METHOD' | 'HAS_PROPERTY'): string[] => + getRelationships(result, type) + .map((e) => `${e.source}->${e.target}`) + .sort(); + + it('keeps one node per class for a shared R6 method name', () => { + expect(idsOf('Method', 'push').map((id) => id.replace(/#\d+$/, ''))).toEqual([ + `Method:${FILE}:Queue.push`, + `Method:${FILE}:Stack.push`, + ]); + expect(idsOf('Method', 'initialize')).toHaveLength(2); + }); + + it('keeps one node per class for a shared R6 field name', () => { + expect(idsOf('Property', 'items')).toEqual([ + `Property:${FILE}:Queue.items`, + `Property:${FILE}:Stack.items`, + ]); + }); + + it('keeps one node per class for a shared RefClass method and field name', () => { + expect(idsOf('Method', 'deposit').map((id) => id.replace(/#\d+$/, ''))).toEqual([ + `Method:${FILE}:Account.deposit`, + `Method:${FILE}:Wallet.deposit`, + ]); + expect(idsOf('Property', 'balance')).toEqual([ + `Property:${FILE}:Account.balance`, + `Property:${FILE}:Wallet.balance`, + ]); + }); + + it('keeps one node per class for a shared S4 slot and setMethod generic', () => { + expect(idsOf('Property', 'size')).toEqual([ + `Property:${FILE}:Circle.size`, + `Property:${FILE}:Square.size`, + ]); + expect(idsOf('Method', 'area').map((id) => id.replace(/#\d+$/, ''))).toEqual([ + `Method:${FILE}:Circle.area`, + `Method:${FILE}:Square.area`, + ]); + }); + + it('gives every class its own HAS_METHOD edges', () => { + const methods = owned('HAS_METHOD'); + for (const owner of ['Stack', 'Queue']) { + for (const m of ['initialize', 'push', 'twice']) expect(methods).toContain(`${owner}->${m}`); + } + for (const owner of ['Account', 'Wallet']) expect(methods).toContain(`${owner}->deposit`); + for (const owner of ['Circle', 'Square']) expect(methods).toContain(`${owner}->area`); + expect(methods).toHaveLength(10); + }); + + it('gives every class its own HAS_PROPERTY edges', () => { + expect(owned('HAS_PROPERTY')).toEqual([ + 'Account->balance', + 'Circle->size', + 'Queue->items', + 'Square->size', + 'Stack->items', + 'Wallet->balance', + ]); + }); + + it('points each owner edge at its own class member node', () => { + const edges = getRelationships(result, 'HAS_METHOD').filter((e) => e.target === 'push'); + const pairs = edges.map((e) => `${e.rel.sourceId}->${e.rel.targetId.replace(/#\d+$/, '')}`); + expect(pairs.sort()).toEqual([ + `Class:${FILE}:Queue->Method:${FILE}:Queue.push`, + `Class:${FILE}:Stack->Method:${FILE}:Stack.push`, + ]); + }); + + it('resolves self$push() inside each class to that class own push', () => { + const calls = getRelationships(result, 'CALLS').filter((e) => e.target === 'push'); + const pairs = calls.map( + (e) => `${e.rel.sourceId.replace(/#\d+$/, '')}->${e.rel.targetId.replace(/#\d+$/, '')}`, + ); + expect([...new Set(pairs)].sort()).toEqual([ + `Method:${FILE}:Queue.twice->Method:${FILE}:Queue.push`, + `Method:${FILE}:Stack.twice->Method:${FILE}:Stack.push`, + ]); + }); +}); diff --git a/gitnexus/test/integration/resolvers/r.test.ts b/gitnexus/test/integration/resolvers/r.test.ts index 833b9d43e..86105e519 100644 --- a/gitnexus/test/integration/resolvers/r.test.ts +++ b/gitnexus/test/integration/resolvers/r.test.ts @@ -1402,6 +1402,9 @@ describe('R deferred-owner attach (r-packages)', () => { 'BareR6|value|HAS_PROPERTY', 'BaseA|a_field|HAS_PROPERTY', 'BaseB|b_field|HAS_PROPERTY', + // Child and Parent share a file and both define `greet`; member ids carry the owner, so + // Child keeps its own node and edge (the id used to collide and drop it). + 'Child|greet|HAS_METHOD', 'DataModel|name|HAS_PROPERTY', 'DataModel|validate|HAS_METHOD', 'DataModel|value|HAS_PROPERTY', @@ -1414,8 +1417,8 @@ describe('R deferred-owner attach (r-packages)', () => { 'ResultSet|items|HAS_PROPERTY', ]; - it('emits exactly 10 HAS_METHOD and 12 HAS_PROPERTY edges', () => { - expect(getRelationships(result, 'HAS_METHOD')).toHaveLength(10); + it('emits exactly 11 HAS_METHOD and 12 HAS_PROPERTY edges', () => { + expect(getRelationships(result, 'HAS_METHOD')).toHaveLength(11); expect(getRelationships(result, 'HAS_PROPERTY')).toHaveLength(12); }); @@ -1429,7 +1432,7 @@ describe('R deferred-owner attach (r-packages)', () => { expect(triples).toEqual(ownedTriples); }); - it('gives exactly the 22 attached R Method/Property nodes a string ownerId', () => { + it('gives exactly the 23 attached R Method/Property nodes a string ownerId', () => { const owned: string[] = []; result.graph.forEachNode((n) => { if ( @@ -1440,7 +1443,7 @@ describe('R deferred-owner attach (r-packages)', () => { owned.push(n.id); } }); - expect(owned).toHaveLength(22); + expect(owned).toHaveLength(23); }); it('leaves no R node with an unresolved ownerNameHint', () => { diff --git a/gitnexus/test/unit/r-owner-hooks.test.ts b/gitnexus/test/unit/r-owner-hooks.test.ts index f81c6b65b..7460616c2 100644 --- a/gitnexus/test/unit/r-owner-hooks.test.ts +++ b/gitnexus/test/unit/r-owner-hooks.test.ts @@ -4,6 +4,7 @@ import { createParserForLanguage } from '../../src/core/tree-sitter/parser-loade import type { SyntaxNode } from '../../src/core/ingestion/utils/ast-helpers.js'; import { rDefinitionProperties, + rResolveDetachedOwnerName, rResolveMemberOwnerNode, } from '../../src/core/ingestion/languages/r/owner-hooks.js'; import type { DefinitionPropertiesContext } from '../../src/core/ingestion/language-provider.js'; @@ -99,6 +100,47 @@ describe('resolveMemberOwnerNode provider wiring', () => { }); }); +describe('rResolveDetachedOwnerName (class-qualified member ids)', () => { + const callTo = (fn: string): SyntaxNode => + find((n) => n.type === 'call' && n.childForFieldName('function')?.text === fn); + + it('names the binding of an `R6Class(...)` assignment', () => { + const r6 = find( + (n) => n.type === 'binary_operator' && n.childForFieldName('lhs')?.text === 'Foo', + ); + expect(rResolveDetachedOwnerName(r6)).toBe('Foo'); + }); + + it('names the Class argument of a `setRefClass(...)` call', () => { + expect(rResolveDetachedOwnerName(callTo('setRefClass'))).toBe('Acc'); + }); + + it('names the signature class of a `setMethod(...)` call', () => { + expect(rResolveDetachedOwnerName(callTo('setMethod'))).toBe('Circle'); + }); + + it('is undefined for ancestors that declare no class', () => { + const plain = find( + (n) => n.type === 'binary_operator' && n.childForFieldName('lhs')?.text === 'plain', + ); + expect(rResolveDetachedOwnerName(plain)).toBeUndefined(); + expect(rResolveDetachedOwnerName(fnDefinitionOn(3))).toBeUndefined(); + expect(rResolveDetachedOwnerName(callTo('list'))).toBeUndefined(); + expect(rResolveDetachedOwnerName(root)).toBeUndefined(); + }); + + it('is registered on the R provider only', () => { + expect(rProvider.resolveDetachedOwnerName).toBe(rResolveDetachedOwnerName); + for (const lang of [ + SupportedLanguages.Python, + SupportedLanguages.TypeScript, + SupportedLanguages.Java, + ]) { + expect(getProvider(lang).resolveDetachedOwnerName).not.toBe(rResolveDetachedOwnerName); + } + }); +}); + describe('rDefinitionProperties (ownerNameHint)', () => { const ctxFor = ( nodeLabel: DefinitionPropertiesContext['nodeLabel'],