From 4bfe5821f85cfa6543758b8f59e8408a9059c678 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 4 Apr 2019 12:12:48 +0200 Subject: [PATCH 1/2] feat(jsii): check that referenced @param's exist Verify integrity of the documentation by forcing the parameter names referred to in @param declarations to actually exist. Fixes #422. --- packages/jsii/lib/assembler.ts | 30 ++++++++++++------- packages/jsii/lib/docs.ts | 24 ++++++++++++++- .../test/negatives/neg.param-must-exist.ts | 12 ++++++++ 3 files changed, 54 insertions(+), 12 deletions(-) create mode 100644 packages/jsii/test/negatives/neg.param-must-exist.ts diff --git a/packages/jsii/lib/assembler.ts b/packages/jsii/lib/assembler.ts index 206d709ace..8ff3b3c68a 100644 --- a/packages/jsii/lib/assembler.ts +++ b/packages/jsii/lib/assembler.ts @@ -7,7 +7,7 @@ import log4js = require('log4js'); import path = require('path'); import ts = require('typescript'); import { JSII_DIAGNOSTICS_CODE } from './compiler'; -import { parseSymbolDocumentation } from './docs'; +import { getReferencedDocParams, parseSymbolDocumentation } from './docs'; import { Diagnostic, Emitter } from './emitter'; import literate = require('./literate'); import { ProjectInfo } from './project-info'; @@ -645,18 +645,10 @@ export class Assembler implements Emitter { } /** - * Register documentations on a ``spec.Documentable`` entry. - * - * @param sym the symbol holding the JSDoc information - * @param documentable the entity being documented - * - * @returns ``documentable`` + * Return docs for a symbol */ private _visitDocumentation(sym: ts.Symbol): spec.Docs | undefined { - const comment = ts.displayPartsToString(sym.getDocumentationComment(this._typeChecker)).trim(); - - // Right here we'll just guess that the first declaration site is the most important one. - const result = parseSymbolDocumentation(comment, sym.getJsDocTags()); + const result = parseSymbolDocumentation(sym, this._typeChecker); for (const diag of result.diagnostics || []) { this._diagnostic(sym.declarations[0], @@ -669,6 +661,20 @@ export class Assembler implements Emitter { return !allUndefined ? result.docs : undefined; } + /** + * Check that all parameters the doc block refers to with a @param declaration actually exist + */ + private _validateReferencedDocParams(method: spec.Method, methodSym: ts.Symbol) { + const params = getReferencedDocParams(methodSym); + const actualNames = new Set((method.parameters || []).map(p => p.name)); + for (const param of params) { + if (!actualNames.has(param)) { + this._diagnostic(methodSym.valueDeclaration, ts.DiagnosticCategory.Error, + `In doc block of '${method.name}', '@param ${param}' refers to a nonexistent parameter.`); + } + } + } + private async _visitInterface(type: ts.Type, namespace: string[]): Promise { if (LOG.isTraceEnabled()) { LOG.trace(`Processing interface: ${colors.gray(namespace.join('.'))}.${colors.cyan(type.symbol.name)}`); @@ -846,6 +852,8 @@ export class Assembler implements Emitter { }); } + this._validateReferencedDocParams(method, symbol); + type.methods = type.methods || []; if (type.methods.find(m => m.name === method.name && m.static === method.static) != null) { LOG.trace(`Dropping re-declaration of ${colors.green(type.fqn)}#${colors.cyan(method.name!)}`); diff --git a/packages/jsii/lib/docs.ts b/packages/jsii/lib/docs.ts index 29a6666de6..38af86df46 100644 --- a/packages/jsii/lib/docs.ts +++ b/packages/jsii/lib/docs.ts @@ -33,7 +33,29 @@ import spec = require('jsii-spec'); import ts = require('typescript'); -export function parseSymbolDocumentation(comments: string | undefined, tags: ts.JSDocTagInfo[]): DocsParsingResult { +export function parseSymbolDocumentation(sym: ts.Symbol, typeChecker: ts.TypeChecker): DocsParsingResult { + const comment = ts.displayPartsToString(sym.getDocumentationComment(typeChecker)).trim(); + const tags = sym.getJsDocTags(); + + // Right here we'll just guess that the first declaration site is the most important one. + return parseDocParts(comment, tags); +} + +/** + * Return the list of parameter names that are referenced in the docstring for this symbol + */ +export function getReferencedDocParams(sym: ts.Symbol): string[] { + const ret = new Array(); + for (const tag of sym.getJsDocTags()) { + if (tag.name === 'param') { + const parts = (tag.text || '').split(' '); + ret.push(parts[0]); + } + } + return ret; +} + +function parseDocParts(comments: string | undefined, tags: ts.JSDocTagInfo[]): DocsParsingResult { const diagnostics = new Array(); const docs: spec.Docs = {}; diff --git a/packages/jsii/test/negatives/neg.param-must-exist.ts b/packages/jsii/test/negatives/neg.param-must-exist.ts new file mode 100644 index 0000000000..2e95f03094 --- /dev/null +++ b/packages/jsii/test/negatives/neg.param-must-exist.ts @@ -0,0 +1,12 @@ +///!MATCH_ERROR: In doc block of 'helpMe', '@param benKenobi' refers to a nonexistent parameter. + +export class Foo { + /** + * Ask for help + * + * @param benKenobi The person you want to ask for help + */ + public helpMe(obiWanKenobi: string) { + Array.isArray(obiWanKenobi); + } +} \ No newline at end of file From 489dfe98b0429ea8aa4e6f23ed865f97861be4ef Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 4 Apr 2019 13:03:21 +0200 Subject: [PATCH 2/2] Make Error a Warning --- packages/jsii/lib/assembler.ts | 2 +- packages/jsii/test/negatives/neg.param-must-exist.ts | 12 ------------ 2 files changed, 1 insertion(+), 13 deletions(-) delete mode 100644 packages/jsii/test/negatives/neg.param-must-exist.ts diff --git a/packages/jsii/lib/assembler.ts b/packages/jsii/lib/assembler.ts index 8ff3b3c68a..9f0f6b1d7a 100644 --- a/packages/jsii/lib/assembler.ts +++ b/packages/jsii/lib/assembler.ts @@ -669,7 +669,7 @@ export class Assembler implements Emitter { const actualNames = new Set((method.parameters || []).map(p => p.name)); for (const param of params) { if (!actualNames.has(param)) { - this._diagnostic(methodSym.valueDeclaration, ts.DiagnosticCategory.Error, + this._diagnostic(methodSym.valueDeclaration, ts.DiagnosticCategory.Warning, `In doc block of '${method.name}', '@param ${param}' refers to a nonexistent parameter.`); } } diff --git a/packages/jsii/test/negatives/neg.param-must-exist.ts b/packages/jsii/test/negatives/neg.param-must-exist.ts deleted file mode 100644 index 2e95f03094..0000000000 --- a/packages/jsii/test/negatives/neg.param-must-exist.ts +++ /dev/null @@ -1,12 +0,0 @@ -///!MATCH_ERROR: In doc block of 'helpMe', '@param benKenobi' refers to a nonexistent parameter. - -export class Foo { - /** - * Ask for help - * - * @param benKenobi The person you want to ask for help - */ - public helpMe(obiWanKenobi: string) { - Array.isArray(obiWanKenobi); - } -} \ No newline at end of file