diff --git a/review-enrichment/src/analyzers/doc-comment-drift.ts b/review-enrichment/src/analyzers/doc-comment-drift.ts index 32b021bdd3..9030912b69 100644 --- a/review-enrichment/src/analyzers/doc-comment-drift.ts +++ b/review-enrichment/src/analyzers/doc-comment-drift.ts @@ -103,11 +103,45 @@ export function parseDocParams(jsdoc: string): string[] { return names; } -/** Split a parameter-list source on top-level commas, tracking ()/{}/[] depth and string literals. Angle brackets - * are intentionally NOT tracked: `<`/`>` are ambiguous between generics and comparison/arrow operators, so a - * default like `n = max > 0 ? max : 1` would be mis-balanced. A comma inside a generic (`Map`) therefore - * splits, but the resulting type-argument fragment is dropped by `parseFunctionParams`. Returns null only if the - * unambiguous brackets never balance. */ +/** If `src[open]` is `<` opening a balanced generic argument list, return the index of its matching `>`; otherwise + * −1 (a comparison operator). Tracks nested `<…>` and skips string literals; it does NOT reject `{`/`=>`/etc. + * because TS type arguments legitimately contain object types (`Result`) and function + * types (`Map void>`). Generic vs comparison is decided by the char after the matching `>`: a + * comparison's `>` is followed by an operand (digit/identifier/string — `removed>0`); a generic close is followed + * by a type terminator (`,` `)` `(` `[` `>` `|` `&` whitespace or end). */ +function matchAngle(src: string, open: number): number { + let angle = 0; + let quote: string | null = null; + for (let i = open; i < src.length; i++) { + const ch = src[i]!; + if (quote) { + if (ch === quote && src[i - 1] !== "\\") quote = null; + continue; + } + if (ch === '"' || ch === "'" || ch === "`") quote = ch; + else if (ch === "<") { + if (src[i + 1] === "=") i += 1; // `<=` is a comparison operator, not a generic open + else angle += 1; + } else if (ch === ">" && src[i - 1] !== "=") { + // (a `=>` arrow inside a function type — e.g. `Map void>` — is not an angle close.) + if (src[i + 1] === "=") return -1; // `>=` is a comparison operator, never a generic close + angle -= 1; + if (angle === 0) { + let j = i + 1; + while (j < src.length && /\s/.test(src[j]!)) j += 1; // the next NON-whitespace token decides + const next = src[j]; + return next !== undefined && /[\w$"'`]/.test(next) ? -1 : i; + } + } + } + return -1; +} + +/** Split a parameter-list source on top-level commas, tracking ()/{}/[] depth, balanced generic `<…>` regions, and + * string literals. A `<` that abuts an identifier (`Map<`, `makeMap<`) is probed by `matchAngle`, which accepts it + * as a generic only when the closing `>` is followed by a type terminator — covering type annotations + * (`a: Map`), generic calls/constructors (`makeMap()`, `new Map()`), and casts (`x as Map`) + * — and rejects comparison operators (`x < y`, `z > q`). Returns null only if the unambiguous brackets never balance. */ function splitParams(src: string): string[] | null { const parts: string[] = []; let depth = 0; @@ -124,6 +158,9 @@ function splitParams(src: string): string[] | null { else if (ch === ")" || ch === "}" || ch === "]") { depth -= 1; if (depth < 0) return null; + } else if (ch === "<" && i > 0 && /[A-Za-z0-9_$]/.test(src[i - 1]!)) { + const close = matchAngle(src, i); + if (close !== -1) i = close; // skip the whole generic argument list, including its commas } else if (ch === "," && depth === 0) { parts.push(src.slice(start, i)); start = i + 1; @@ -134,12 +171,11 @@ function splitParams(src: string): string[] | null { return parts; } -/** Parameter names of a function from its parenthesised source, or null when not confidently enumerable. - * Each comma-separated segment yields its leading identifier (after an optional rest marker). A destructured - * segment (`{…}`/`[…]`) makes the set ambiguous → null. A segment whose identifier is immediately followed by - * `<`/`>` is a generic type-argument fragment left over from splitting a generic (`Map`), not a real - * parameter, and is dropped — so comparison defaults and callback params stay enumerable without ever inventing - * a name. Pure. */ +/** Parameter names of a function from its parenthesised source, or null when not confidently enumerable. Each + * comma-separated segment (commas inside generics/brackets/strings don't split) yields its leading identifier + * after an optional rest marker. A destructured segment (`{…}`/`[…]`) or a segment without a leading identifier + * makes the set ambiguous → null. Generic-typed and defaulted params (`cache = new Map()`) enumerate + * cleanly because the generic's comma no longer splits the list. Pure. */ export function parseFunctionParams(paramSrc: string): string[] | null { const trimmed = paramSrc.trim(); if (!trimmed) return []; @@ -154,9 +190,6 @@ export function parseFunctionParams(paramSrc: string): string[] | null { if (!name) return null; // not a plain identifier — ambiguous const id = name[1]!; // After the name a real parameter has only a type (`:`), an optional marker (`?`), a default (`=`), or nothing. - // Anything else means this segment is a generic type-argument fragment left over from splitting a generic - // (e.g. `readonly V[]>` from `Map`) — fail closed and skip the whole function rather than - // invent a parameter name. const rest = part.slice(id.length).trimStart(); if (rest && !/^[:?=]/.test(rest)) return null; if (id === "this") continue; // a TS `this` pseudo-parameter is not a real argument diff --git a/review-enrichment/test/doc-comment-drift.test.ts b/review-enrichment/test/doc-comment-drift.test.ts index 6d6e217d34..1dc18aad1d 100644 --- a/review-enrichment/test/doc-comment-drift.test.ts +++ b/review-enrichment/test/doc-comment-drift.test.ts @@ -93,12 +93,43 @@ test("parseFunctionParams: comparison defaults and callback/arrow params stay en assert.deepEqual(parseFunctionParams("a, cb = () => a"), ["a", "cb"]); }); -test("parseFunctionParams: fails closed (null) on destructuring, unbalanced, or generic-comma fragments", () => { +test("parseFunctionParams: generic types and generic-comma defaults enumerate (a comma inside <…> doesn't split)", () => { + assert.deepEqual(parseFunctionParams("a: Map, b"), ["a", "b"]); + assert.deepEqual(parseFunctionParams("a: Map, b"), ["a", "b"]); + assert.deepEqual(parseFunctionParams("cache = new Map(), b"), ["cache", "b"]); + assert.deepEqual(parseFunctionParams("a: Record>, b"), ["a", "b"]); // nested generics + assert.deepEqual(parseFunctionParams("a: Result, b"), ["a", "b"]); // object-type arg + assert.deepEqual(parseFunctionParams("a: Map void>, b"), ["a", "b"]); // function-type arg + // a function-type arg followed by a further type arg: the `=>` arrow must not close the generic early. + assert.deepEqual(parseFunctionParams("a: Foo void, Extra>, b"), ["a", "b"]); + assert.deepEqual(parseFunctionParams("a = items as Map, b"), ["a", "b"]); // generic via `as` keyword + assert.deepEqual(parseFunctionParams("removed, cache = makeMap()"), ["removed", "cache"]); // generic call +}); + +test("parseFunctionParams: comparison operators are not mistaken for generics (commas still split)", () => { + // A comparison `<` must not pair with a later comparison `>` and swallow a real comma — spaced OR not, with or + // without an `=` between them (the `>` is followed by an operand, which a generic close never is). + assert.deepEqual(parseFunctionParams("a = x < y, b = z > 0, removed"), ["a", "b", "removed"]); + assert.deepEqual(parseFunctionParams("a = x0, removed"), ["a", "b", "removed"]); + assert.deepEqual(parseFunctionParams("a = x q, removed"), ["a", "b", "removed"]); // spaced `>` comparison + // a comparison default `<…>` is in expression position (after `=`), so it is never a generic — the `>` may be + // followed by `?`, `,`, `)` or any operator without swallowing the next parameter's comma. + assert.deepEqual(parseFunctionParams("a = x q ? 1 : 0, b"), ["a", "removed", "b"]); + assert.deepEqual(parseFunctionParams("a = x w), b"), ["a", "removed", "b"]); + assert.deepEqual(parseFunctionParams("a = x=0"), ["a", "b"]); // `>=` is a comparison, not a generic close + assert.deepEqual(parseFunctionParams("a = x0` is not a valid parameter, but the comma after `y` must still split so it is not silently merged + // into `a`'s segment and returned as ["a","b"]; the malformed segment then fails closed (null), never a wrong enum. + assert.equal(parseFunctionParams("a = x0, b"), null); +}); + +test("parseFunctionParams: fails closed (null) on destructuring or unbalanced brackets", () => { assert.equal(parseFunctionParams("{ a, b }"), null); assert.equal(parseFunctionParams("[a, b]"), null); assert.equal(parseFunctionParams("a, (b"), null); - assert.equal(parseFunctionParams("a: Map, b"), null); - assert.equal(parseFunctionParams("a: Map, b"), null); }); test("findDocCommentDrift: flags a @param that was a real OLD parameter and is now gone (rename)", () => { @@ -169,6 +200,26 @@ test("scanDocCommentDrift: a parameter the PR actually removed IS reported", asy assert.deepEqual(findings[0].staleParams, ["removed"]); }); +test("scanDocCommentDrift: a removed param is still reported when a sibling param has a generic-comma default", async () => { + // Regression for the false negative: `cache`'s `new Map()` default must not make the function + // unparseable and silently hide the removed-and-still-documented `removed` parameter. + const content = `/**\n * @param [removed=1] the removed one\n * @param cache the cache\n */\nexport function f(cache = new Map()) {}\n`; + const patch = `@@ -1,5 +1,5 @@\n /**\n * @param [removed=1] the removed one\n * @param cache the cache\n */\n-export function f(removed, cache = new Map()) {}\n+export function f(cache = new Map()) {}`; + const findings = await scanDocCommentDrift(baseReq([{ path: "src/a.ts", patch }]), fileWith(content)); + assert.equal(findings.length, 1); + assert.deepEqual(findings[0].staleParams, ["removed"]); +}); + +test("scanDocCommentDrift: a removed param is still reported beside a sibling with a generic-CALL default", async () => { + // Regression for the false negative: `cache`'s `makeMap()` (a generic call, not `new`) default + // must not make the function unparseable and hide the removed-and-documented `removed` parameter. + const content = `/**\n * @param [removed=1] the removed one\n * @param cache the cache\n */\nexport function f(cache = makeMap()) {}\n`; + const patch = `@@ -1,5 +1,5 @@\n /**\n * @param [removed=1] the removed one\n * @param cache the cache\n */\n-export function f(removed, cache = makeMap()) {}\n+export function f(cache = makeMap()) {}`; + const findings = await scanDocCommentDrift(baseReq([{ path: "src/a.ts", patch }]), fileWith(content)); + assert.equal(findings.length, 1); + assert.deepEqual(findings[0].staleParams, ["removed"]); +}); + test("scanDocCommentDrift: fetches the file at headSha and reports drift", async () => { const findings = await scanDocCommentDrift(baseReq([{ path: "src/a.ts", patch: DRIFT_PATCH }]), fileWith(DRIFTED)); assert.equal(findings.length, 1);