Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(jsii): check that referenced @param's exist #431

Merged
merged 2 commits into from
Apr 4, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 19 additions & 11 deletions packages/jsii/lib/assembler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -645,18 +645,10 @@ export class Assembler implements Emitter {
}

/**
* Register documentations on a ``spec.Documentable`` entry.
*
* @param sym the symbol holding the JSDoc information
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is sad that you're dropping rich documentation and making it less nice, while you're fixing the problems with it...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't feel like it was adding a lot of value. If the parameter is declared as sym: Symbol and the documentation basically says "this is the symbol", then I don't feel like the documentation is adding a lot of information.

Similarly for the return value. Declaration says: function(...): Docs, I don't feel the need to add @returns the docs.

* @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],
Expand All @@ -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.Warning,
`In doc block of '${method.name}', '@param ${param}' refers to a nonexistent parameter.`);
}
}
}

private async _visitInterface(type: ts.Type, namespace: string[]): Promise<spec.InterfaceType | undefined> {
if (LOG.isTraceEnabled()) {
LOG.trace(`Processing interface: ${colors.gray(namespace.join('.'))}.${colors.cyan(type.symbol.name)}`);
Expand Down Expand Up @@ -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!)}`);
Expand Down
24 changes: 23 additions & 1 deletion packages/jsii/lib/docs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string>();
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<string>();
const docs: spec.Docs = {};

Expand Down