fix review finding: an export list surfaces only the declarators it names

A name resolved through an export list (or a default-export identifier)
mapped back to its whole VariableStatement, and checkDecl walked every
declarator — so a private sibling sharing the statement with an exported
const was wrongly required to carry JSDoc.

The scope dispatch is now two-phase: requests accumulate per statement
(null = whole statement for a direct export modifier or ambient scope;
name sets union across lists, so two lists naming different declarators
of one statement both count), then each surfaced statement is checked
once with the declarator filter. Regressions pin the private-sibling
skip, the cross-list union, and the default-export sibling.
This commit is contained in:
Tianyi Cui
2026-07-07 17:05:32 +08:00
parent 3c572cf70a
commit 51b715433d
2 changed files with 64 additions and 8 deletions

View File

@@ -160,6 +160,36 @@ describe('verify-export-jsdoc export forms', () => {
))).toEqual([expect.stringMatching(/exported function 'f' .* has no JSDoc\./)])
})
it('does not treat a never-exported sibling declarator as surface (review round 2)', () => {
// `export { publicValue }` resolves to the whole variable statement; only
// the named declarator is surface — the gate must not demand JSDoc for
// the private sibling sharing the statement.
expect(collectExportJsdocViolations(make(
'/** The public knob. */\nconst publicValue = 1, privateHelper = 2\nexport { publicValue }\nvoid privateHelper\n',
))).toEqual([])
})
it('unions declarators across multiple export lists over one statement (review round 2)', () => {
// Two lists each name one declarator of the same undocumented statement:
// both are surface (deduplicating on first resolution would drop `b`),
// while the never-exported `c` stays out.
const violations = collectExportJsdocViolations(make(
'const a = 1, b = 2, c = 3\nexport { a }\nexport { b }\nvoid c\n',
))
expect(violations).toEqual([
expect.stringMatching(/exported const 'a' .* has no JSDoc\./),
expect.stringMatching(/exported const 'b' .* has no JSDoc\./),
])
})
it('scopes a default-export identifier to its own declarator (review round 2)', () => {
// `export default` of an identifier reaches the statement through the
// same name lookup as an export list; the sibling stays private.
expect(collectExportJsdocViolations(make(
'/** The app entry. */\nconst app = 1, scratch = 2\nexport default app\nvoid scratch\n',
))).toEqual([])
})
it('reports a re-exported module once, at its defining file', () => {
const violations = collectExportJsdocViolations(fixture({
'index.ts': "export * from './other.ts'\n",

View File

@@ -321,6 +321,11 @@ function checkClass(cls: ts.ClassDeclaration, name: string, w: Walk): void {
* @param byName - this scope's named declarations (for namespace/sibling-merge lookups).
* @param ambient - whether the enclosing scope is ambient (`declare`), where members export implicitly.
* @param w - the walk state violations append to.
* @param only - for a multi-declarator variable statement reached through an
* export list (or a default-export identifier), the declarator names that
* are actually exported; `null` means the whole statement is surface
* (direct `export` modifier or ambient scope). Non-variable statements
* declare exactly one name, so the filter never applies to them.
*/
function checkDecl(
stmt: ts.Statement,
@@ -329,6 +334,7 @@ function checkDecl(
byName: Map<string, ts.Statement[]>,
ambient: boolean,
w: Walk,
only: ReadonlySet<string> | null = null,
): void {
const at = (n: ts.Node): string => ` (${pointer(w.rel, w.sf, n)})`
if (ts.isFunctionDeclaration(stmt)) {
@@ -359,6 +365,7 @@ function checkDecl(
const raw = rawJsDoc(w.text, stmt) // JSDoc sits on the statement, not the declarator
for (const d of stmt.declarationList.declarations) {
const name = ts.isIdentifier(d.name) ? d.name.text : d.name.getText(w.sf)
if (only !== null && !only.has(name)) continue // sibling declarator the export list never named: not surface
if (prefix === '' && PROTOCOL_EXPORTS.has(name)) continue // cordis plugin-protocol slot
const where = `exported const '${prefix}${name}'${at(d)}`
const annotation = d.type !== undefined ? callableAnnotation(d.type) : null
@@ -462,11 +469,25 @@ function checkScope(statements: readonly ts.Statement[], prefix: string, w: Walk
}
}
}
const checked = new Set<ts.Statement>()
const check = (stmt: ts.Statement): void => {
if (checked.has(stmt)) return
checked.add(stmt)
checkDecl(stmt, prefix, overloadSigs, byName, ambient, w)
// Two-phase dispatch. Phase one accumulates WHICH statements are surface
// and, for a variable statement reached by name (an export list or a
// default-export identifier), which of its declarators the exports actually
// name — `null` marks the whole statement as surface (a direct `export`
// modifier, or an ambient scope). Requests for the same statement merge:
// `null` absorbs any name set, and name sets union, so
// `export { a }; export { b }` over one `const a = …, b = …` checks both
// declarators while a never-exported sibling stays out of the surface.
// Phase two runs each surfaced statement exactly once. (Checking a
// statement eagerly per request would either re-check on the second list or
// — deduplicated — silently drop the second list's declarators.)
const requested = new Map<ts.Statement, Set<string> | null>()
const request = (stmt: ts.Statement, name: string | null): void => {
const prior = requested.get(stmt)
if (name === null || prior === null) {
requested.set(stmt, null)
return
}
requested.set(stmt, prior === undefined ? new Set([name]) : prior.add(name))
}
for (const stmt of statements) {
if (ts.isModuleDeclaration(stmt)
@@ -477,7 +498,8 @@ function checkScope(statements: readonly ts.Statement[], prefix: string, w: Walk
if (stmt.moduleSpecifier) continue // re-export: the defining module is walked on its own
if (stmt.exportClause && ts.isNamedExports(stmt.exportClause)) {
for (const el of stmt.exportClause.elements) {
for (const decl of byName.get((el.propertyName ?? el.name).text) ?? []) check(decl)
const local = (el.propertyName ?? el.name).text
for (const decl of byName.get(local) ?? []) request(decl, local)
// a name with no local declaration is an imported binding re-exported
// without a specifier — its defining module is walked on its own
}
@@ -494,7 +516,7 @@ function checkScope(statements: readonly ts.Statement[], prefix: string, w: Walk
const where = `default export (${pointer(w.rel, w.sf, stmt)})`
const expr = unwrapExpression(stmt.expression)
if (ts.isIdentifier(expr)) {
for (const decl of byName.get(expr.text) ?? []) check(decl)
for (const decl of byName.get(expr.text) ?? []) request(decl, expr.text)
} else if (ts.isArrowFunction(expr) || ts.isFunctionExpression(expr)) {
checkFunctionLike(where, rawJsDoc(w.text, stmt), expr.parameters, expr.type, false, w)
} else {
@@ -502,7 +524,11 @@ function checkScope(statements: readonly ts.Statement[], prefix: string, w: Walk
}
continue
}
if (isExported(stmt) || (ambient && !ts.isImportDeclaration(stmt))) check(stmt)
if (isExported(stmt) || (ambient && !ts.isImportDeclaration(stmt))) request(stmt, null)
}
for (const stmt of statements) {
const only = requested.get(stmt)
if (only !== undefined) checkDecl(stmt, prefix, overloadSigs, byName, ambient, w, only)
}
}