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.
This commit is contained in:
Mick Cooney 2026-10-15 20:36:03 +01:00
parent 5aa257b8f6
commit 2a342d1619
2 changed files with 78 additions and 3 deletions

View file

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

View file

@ -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' },