From 2a342d1619b4d8b464bbf907039078ebb93ebdd8 Mon Sep 17 00:00:00 2001 From: Mick Cooney Date: Thu, 15 Oct 2026 20:36:03 +0100 Subject: [PATCH] fix(r): skip the body of an `else if` in NAMESPACE as conditional skipConditionalBody returned just after the `if` of `else if (cond) body`, so the main scan then read `(cond)` as stray text and emitted the body's importFrom, S3method, export, exportClasses, exportMethods and exportPattern directives as unconditional (the same for `if (a) if (b) body`). Reproduction: 12 new cases in r-namespace-parser.test.ts failed on the unfixed scanner (e.g. `if (a) {} else if (b) importFrom(x, y)` returned x::y; `else if (b) export(hidden)` exported `hidden`). The nested `if (cond)` is now stepped over iteratively (no recursion, tested with 50,000 nested ifs) and the body that follows it is skipped. --- .../languages/r/namespace-imports.ts | 28 +++++++++- gitnexus/test/unit/r-namespace-parser.test.ts | 53 ++++++++++++++++++- 2 files changed, 78 insertions(+), 3 deletions(-) diff --git a/gitnexus/src/core/ingestion/languages/r/namespace-imports.ts b/gitnexus/src/core/ingestion/languages/r/namespace-imports.ts index ac0d86841..1c2823bc6 100644 --- a/gitnexus/src/core/ingestion/languages/r/namespace-imports.ts +++ b/gitnexus/src/core/ingestion/languages/r/namespace-imports.ts @@ -201,13 +201,37 @@ function normaliseArg(raw: string): string { return trimmed; } +/** + * When `text[i]` starts an `if (cond)`, the index just after its condition group; `-1` + * when the condition is malformed; `null` when no `if` starts here. + */ +function nestedIfCondition(text: string, i: number): number | null { + if (i >= text.length || !isIdentStart(text[i])) return null; + let j = i + 1; + while (j < text.length && isIdentPart(text[j])) j++; + if (text.slice(i, j) !== 'if') return null; + while (j < text.length && /\s/.test(text[j])) j++; + if (text[j] !== '(') return null; + const group = parseArgs(text, j); + return group ? group.end : -1; +} + /** * Skip one statement following an `if (…)` / `else`: either a `{ … }` block or a * single directive call. Returns the index to resume scanning from. */ function skipConditionalBody(text: string, from: number): number { let i = from; - while (i < text.length && /\s/.test(text[i])) i++; + for (;;) { + while (i < text.length && /\s/.test(text[i])) i++; + // `else if (cond) body` and `if (a) if (b) body`: the nested `if` owns the body that + // follows its condition, so step over `if (cond)` and skip that body instead + // (iteratively: an `if` chain can be arbitrarily long). + const nested = nestedIfCondition(text, i); + if (nested === null) break; + if (nested === -1) return resyncAfterMalformed(text, i); + i = nested; + } if (i >= text.length) return i; if (text[i] === '{') { const blockStart = i; @@ -237,7 +261,7 @@ function skipConditionalBody(text: string, from: number): number { let j = i + 1; while (j < text.length && isIdentPart(text[j])) j++; const head = text.slice(i, j); - if (CONDITIONAL_HEADS.has(head)) return j; // `else if (…)` — let the main loop handle the chain. + if (CONDITIONAL_HEADS.has(head)) return j; // a bare `else` here has no body to skip while (j < text.length && /\s/.test(text[j])) j++; if (text[j] === '(') { const group = parseArgs(text, j); diff --git a/gitnexus/test/unit/r-namespace-parser.test.ts b/gitnexus/test/unit/r-namespace-parser.test.ts index 12db4e002..408ff784d 100644 --- a/gitnexus/test/unit/r-namespace-parser.test.ts +++ b/gitnexus/test/unit/r-namespace-parser.test.ts @@ -1,7 +1,10 @@ import { describe, it, expect } from 'vitest'; import fs from 'node:fs'; import path from 'node:path'; -import { parseRNamespaceImportFrom } from '../../src/core/ingestion/languages/r/namespace-imports.js'; +import { + parseRNamespaceExports, + parseRNamespaceImportFrom, +} from '../../src/core/ingestion/languages/r/namespace-imports.js'; import { loadRPackageConfig } from '../../src/core/ingestion/languages/r/package-config.js'; import { createTempDirPool } from '../helpers/temp-dir-pool.js'; @@ -132,6 +135,54 @@ describe('parseRNamespaceImportFrom', () => { expect(parseRNamespaceImportFrom(src)).toEqual([{ pkg: 'dplyr', name: 'mutate' }]); }); + describe('an `else if` chain is conditional all the way down', () => { + const kept = 'importFrom(dplyr, mutate)'; + const chains: Array<[string, string]> = [ + ['bare else-if body', 'if (a) {\n} else if (b) importFrom(x, y)'], + ['braced else-if body', 'if (a) {\n} else if (b) {\n importFrom(x, y)\n}'], + ['unbraced chain', 'if (a) importFrom(p, q) else if (b) importFrom(x, y)'], + ['else-if then else', 'if (a) {\n} else if (b) {\n} else importFrom(x, y)'], + ['long chain', 'if (a) 1 else if (b) 2 else if (c) importFrom(x, y) else if (d) 3'], + [ + 'else-if with a call in the condition', + 'if (a) {\n} else if (is.na(f(b))) importFrom(x, y)', + ], + ['if directly under if', 'if (a) if (b) importFrom(x, y)'], + ]; + it.each(chains)('importFrom: %s is ignored and the next directive is kept', (_n, chain) => { + expect(parseRNamespaceImportFrom(`${chain}\n${kept}\n`)).toEqual([ + { pkg: 'dplyr', name: 'mutate' }, + ]); + }); + + it.each([ + ['export', 'export(hidden)', 'namedExports', ['hidden']], + ['exportClasses', 'exportClasses(Hidden)', 'namedExports', ['Hidden']], + ['exportMethods', 'exportMethods(hidden)', 'namedExports', ['hidden']], + ['S3method', 'S3method(print, hidden)', 'namedExports', ['print.hidden']], + ['S3method with method', 'S3method(print, hidden, impl)', 'namedExports', ['impl']], + ['exportPattern', 'exportPattern("^hidden")', 'exportPatterns', ['^hidden']], + ] as const)('%s inside an else-if is not exported', (_n, directive, field, leaked) => { + const src = `if (a) {\n} else if (b) ${directive}\nexport(visible)\n`; + const parsed = parseRNamespaceExports(src); + expect(parsed.namedExports).toEqual(['visible']); + expect(parsed.exportPatterns).toEqual([]); + // positive control: the same directive outside a conditional is read + const plain = parseRNamespaceExports(`${directive}\n`); + expect(plain[field]).toEqual(leaked); + }); + + it('a very long chain of nested ifs is skipped without recursion', () => { + const src = `${'if (a) '.repeat(50_000)}importFrom(x, y)\n${kept}\n`; + expect(parseRNamespaceImportFrom(src)).toEqual([{ pkg: 'dplyr', name: 'mutate' }]); + }); + + it('a plain else body and the directives after the chain are unaffected', () => { + const src = 'if (a) {\n} else if (b) {\n} else {\n importFrom(x, y)\n}\nimportFrom(p, q)\n'; + expect(parseRNamespaceImportFrom(src)).toEqual([{ pkg: 'p', name: 'q' }]); + }); + }); + it('returns a self-import entry like any other (dropping it is the consumer job)', () => { expect(parseRNamespaceImportFrom('importFrom(analytics, helper)\n')).toEqual([ { pkg: 'analytics', name: 'helper' },