Skip to content

Commit

Permalink
More @fileoverview comments.
Browse files Browse the repository at this point in the history
It turns out that various tooling treats comments containing `@modName`,
`@mods`, and `@pintomodule` as a file overview comment.

This change also moves parsing `@suppress` and its value into
`jsdoc.ts`, which is the more appropriate place.
  • Loading branch information
mprobst committed May 17, 2017
1 parent b53e236 commit 8ebfd01
Show file tree
Hide file tree
Showing 155 changed files with 277 additions and 242 deletions.
160 changes: 73 additions & 87 deletions src/jsdoc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,22 +13,32 @@
* to their API will be easier. See e.g. ts.JSDocTag and ts.JSDocComment.
*/
export interface Tag {
// tagName is e.g. "param" in an @param declaration. It is the empty string
// for the plain text documentation that occurs before any @foo lines.
/**
* tagName is e.g. "param" in an @param declaration. It is the empty string
* for the plain text documentation that occurs before any @foo lines.
*/
tagName: string;
// parameterName is the the name of the function parameter, e.g. "foo"
// in
// @param foo The foo param.
/**
* parameterName is the the name of the function parameter, e.g. "foo"
* in `\@param foo The foo param`
*/
parameterName?: string;
/**
* The type of a JSDoc \@param, \@type etc tag, rendered in curly braces.
* Can also hold the type of an \@suppress.
*/
type?: string;
// optional is true for optional function parameters.
/** optional is true for optional function parameters. */
optional?: boolean;
// restParam is true for "...x: foo[]" function parameters.
/** restParam is true for "...x: foo[]" function parameters. */
restParam?: boolean;
// destructuring is true for destructuring bind parameters, which require
// non-null arguments on the Closure side. Can likely remove this
// once TypeScript nullable types are available.
/**
* destructuring is true for destructuring bind parameters, which require
* non-null arguments on the Closure side. Can likely remove this
* once TypeScript nullable types are available.
*/
destructuring?: boolean;
/** Any remaining text on the tag, e.g. the description. */
text?: string;
}

Expand All @@ -37,72 +47,47 @@ export interface Tag {
* The public Closure docs don't list all the tags it allows; this list comes
* from the compiler source itself.
* https://github.com/google/closure-compiler/blob/master/src/com/google/javascript/jscomp/parsing/Annotation.java
* https://github.com/google/closure-compiler/blob/master/src/com/google/javascript/jscomp/parsing/ParserConfig.properties
*/
const JSDOC_TAGS_WHITELIST = new Set([
'ngInject',
'abstract',
'argument',
'author',
'consistentIdGenerator',
'const',
'constant',
'constructor',
'copyright',
'define',
'deprecated',
'desc',
'dict',
'disposes',
'enum',
'export',
'expose',
'extends',
'externs',
'fileoverview',
'final',
'hidden',
'idGenerator',
'implements',
'implicitCast',
'inheritDoc',
'interface',
'record',
'jaggerInject',
'jaggerModule',
'jaggerProvidePromise',
'jaggerProvide',
'lends',
'license',
'meaning',
'modifies',
'noalias',
'nocollapse',
'nocompile',
'nosideeffects',
'override',
'owner',
'package',
'param',
'polymerBehavior',
'preserve',
'preserveTry',
'private',
'protected',
'public',
'return',
'returns',
'see',
'stableIdGenerator',
'struct',
'suppress',
'template',
'this',
'throws',
'type',
'typedef',
'unrestricted',
'version',
'wizaction',
'abstract', 'argument',
'author', 'consistentIdGenerator',
'const', 'constant',
'constructor', 'copyright',
'define', 'deprecated',
'desc', 'dict',
'disposes', 'enum',
'export', 'expose',
'extends', 'externs',
'fileoverview', 'final',
'hassoydelcall', 'hassoydeltemplate',
'hidden', 'id',
'idGenerator', 'ignore',
'implements', 'implicitCast',
'inheritDoc', 'interface',
'jaggerInject', 'jaggerModule',
'jaggerProvide', 'jaggerProvidePromise',
'lends', 'license',
'link', 'meaning',
'modifies', 'modName',
'mods', 'ngInject',
'noalias', 'nocollapse',
'nocompile', 'nosideeffects',
'override', 'owner',
'package', 'param',
'pintomodule', 'polymerBehavior',
'preserve', 'preserveTry',
'private', 'protected',
'public', 'record',
'requirecss', 'requires',
'return', 'returns',
'see', 'stableIdGenerator',
'struct', 'suppress',
'template', 'this',
'throws', 'type',
'typedef', 'unrestricted',
'version', 'wizaction',
'wizmodule',
]);

/**
Expand All @@ -111,23 +96,15 @@ const JSDOC_TAGS_WHITELIST = new Set([
* these will cause Closure Compiler issues and should not be used.
*/
const JSDOC_TAGS_BLACKLIST = new Set([
'constructor',
'enum',
'extends',
'implements',
'interface',
'lends',
'private',
'public',
'record',
'template',
'this',
'type',
'typedef',
'augments', 'class', 'constructs', 'constructor', 'enhance', 'enhanceable',
'enum', 'extends', 'field', 'function', 'implements', 'interface',
'lends', 'namespace', 'private', 'public', 'record', 'static',
'template', 'this', 'type', 'typedef',
]);

/**
* A list of JSDoc @tags that might include a {type} after them. Only banned when a type is passed.
* Note that this does not include tags that carry a non-type system type, e.g. \@suppress.
*/
const JSDOC_TAGS_WITH_TYPES = new Set([
'const',
Expand Down Expand Up @@ -166,6 +143,7 @@ export function parse(comment: string): {tags: Tag[], warnings?: string[]}|null
// A synonym for 'return'.
tagName = 'return';
}
let type: string|undefined;
if (JSDOC_TAGS_BLACKLIST.has(tagName)) {
warnings.push(`@${tagName} annotations are redundant with TypeScript equivalents`);
continue; // Drop the tag so Closure won't process it.
Expand All @@ -174,6 +152,13 @@ export function parse(comment: string): {tags: Tag[], warnings?: string[]}|null
`the type annotation on @${tagName} is redundant with its TypeScript type, ` +
`remove the {...} part`);
continue;
} else if (tagName === 'suppress') {
const suppressMatch = text.match(/^\{(.*)\}(.*)$/);
if (!suppressMatch) {
warnings.push(`malformed @suppress tag: "${text}"`);
} else {
[, type, text] = suppressMatch;
}
} else if (tagName === 'dict') {
warnings.push('use index signatures (`[k: string]: type`) instead of @dict');
continue;
Expand All @@ -189,6 +174,7 @@ export function parse(comment: string): {tags: Tag[], warnings?: string[]}|null
let tag: Tag = {tagName};
if (parameterName) tag.parameterName = parameterName;
if (text) tag.text = text;
if (type) tag.type = type;
tags.push(tag);
} else {
// Text without a preceding @tag on it is either the plain text
Expand Down
24 changes: 16 additions & 8 deletions src/tsickle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,9 @@ type HasTypeParameters =
// Matches common extensions of TypeScript input filenames
const extension = /(\.ts|\.d\.ts|\.js|\.jsx|\.tsx)$/;

const FILEOVERVIEW_COMMENTS: ReadonlySet<string> =
new Set(['fileoverview', 'externs', 'modName', 'mods', 'pintomodule']);

/** Annotator translates a .ts to a .ts with Closure annotations. */
class Annotator extends ClosureRewriter {
/**
Expand Down Expand Up @@ -779,10 +782,11 @@ class Annotator extends ClosureRewriter {
*/
private emitSuppressChecktypes(sf: ts.SourceFile) {
const comments = ts.getLeadingCommentRanges(sf.getFullText(), 0) || [];

let fileoverviewIdx = -1;
for (let i = comments.length - 1; i >= 0; i--) {
const parsed = jsdoc.parse(sf.getFullText().substring(comments[i].pos, comments[i].end));
if (parsed !== null && parsed.tags.some(t => t.tagName === 'fileoverview')) {
if (parsed !== null && parsed.tags.some(t => FILEOVERVIEW_COMMENTS.has(t.tagName))) {
fileoverviewIdx = i;
break;
}
Expand All @@ -797,7 +801,7 @@ class Annotator extends ClosureRewriter {
// No existing comment to merge with, just emit a new one.
this.emit(jsdoc.toString([
{tagName: 'fileoverview', text: 'added by tsickle'},
{tagName: 'suppress', text: '{checkTypes}'}
{tagName: 'suppress', type: 'checkTypes', text: 'checked by tsc'},
]));
this.emit('\n');
return;
Expand All @@ -815,14 +819,18 @@ class Annotator extends ClosureRewriter {
// only appear once and be merged.
const suppressIdx = tags.findIndex(t => t.tagName === 'suppress');
if (suppressIdx !== -1) {
const suppressions =
(tags[suppressIdx].text || '').replace(/^\{(.*)\}$/, '$1').split(',').map(s => s.trim());
if (suppressions.indexOf('checkTypes') === -1) {
suppressions.push('checkTypes');
const suppressions = tags[suppressIdx].type || '';
const suppressionsList = suppressions.split(',').map(s => s.trim());
if (suppressionsList.indexOf('checkTypes') === -1) {
suppressionsList.push('checkTypes');
}
tags[suppressIdx].text = `{${suppressions.join(',')}}`;
tags[suppressIdx].type = suppressionsList.join(',');
} else {
tags.push({tagName: 'suppress', text: '{checkTypes}'});
tags.push({
tagName: 'suppress',
type: 'checkTypes',
text: 'checked by tsc',
});
}
this.emit(jsdoc.toString(tags));
if (sf.getFullText().substring(comment.end, comment.end + 2) !== '\n\n') {
Expand Down
7 changes: 6 additions & 1 deletion test/jsdoc_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,12 @@ describe('jsdoc.parse', () => {
it('allows @suppress annotations', () => {
let source = `/** @suppress {checkTypes} I hate types */`;
expect(jsdoc.parse(source)).to.deep.equal({
tags: [{tagName: 'suppress', text: '{checkTypes} I hate types'}]
tags: [{tagName: 'suppress', type: 'checkTypes', text: ' I hate types'}]
});
let malformed = `/** @suppress malformed */`;
expect(jsdoc.parse(malformed)).to.deep.equal({
tags: [{tagName: 'suppress', text: 'malformed'}],
warnings: ['malformed @suppress tag: "malformed"'],
});
});
});
Expand Down
2 changes: 1 addition & 1 deletion test_files/abstract/abstract.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
goog.module('test_files.abstract.abstract');var module = module || {id: 'test_files/abstract/abstract.js'};/**
* @fileoverview added by tsickle
* @suppress {checkTypes}
* @suppress {checkTypes} checked by tsc
*/
/**
* @abstract
Expand Down
2 changes: 1 addition & 1 deletion test_files/abstract/abstract.tsickle.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @fileoverview added by tsickle
* @suppress {checkTypes}
* @suppress {checkTypes} checked by tsc
*/


Expand Down
2 changes: 1 addition & 1 deletion test_files/arrow_fn.untyped/arrow_fn.untyped.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
goog.module('test_files.arrow_fn.untyped.arrow_fn.untyped');var module = module || {id: 'test_files/arrow_fn.untyped/arrow_fn.untyped.js'};/**
* @fileoverview added by tsickle
* @suppress {checkTypes}
* @suppress {checkTypes} checked by tsc
*/
var /** @type {?} */ fn3 = (a) => 12;
2 changes: 1 addition & 1 deletion test_files/arrow_fn.untyped/arrow_fn.untyped.tsickle.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @fileoverview added by tsickle
* @suppress {checkTypes}
* @suppress {checkTypes} checked by tsc
*/

var /** @type {?} */ fn3 = (a: number): number => 12;
2 changes: 1 addition & 1 deletion test_files/arrow_fn/arrow_fn.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
goog.module('test_files.arrow_fn.arrow_fn');var module = module || {id: 'test_files/arrow_fn/arrow_fn.js'};/**
* @fileoverview added by tsickle
* @suppress {checkTypes}
* @suppress {checkTypes} checked by tsc
*/
var /** @type {function(number): number} */ fn3 = (a) => 12;
var /** @type {function(?): ?} */ fn4 = (a) => a + 12;
2 changes: 1 addition & 1 deletion test_files/arrow_fn/arrow_fn.tsickle.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @fileoverview added by tsickle
* @suppress {checkTypes}
* @suppress {checkTypes} checked by tsc
*/

var /** @type {function(number): number} */ fn3 = (a: number): number => 12;
Expand Down
2 changes: 1 addition & 1 deletion test_files/basic.untyped/basic.untyped.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
goog.module('test_files.basic.untyped.basic.untyped');var module = module || {id: 'test_files/basic.untyped/basic.untyped.js'};/**
* @fileoverview added by tsickle
* @suppress {checkTypes}
* @suppress {checkTypes} checked by tsc
*/
/**
* @param {?} arg1
Expand Down
2 changes: 1 addition & 1 deletion test_files/basic.untyped/basic.untyped.tsickle.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @fileoverview added by tsickle
* @suppress {checkTypes}
* @suppress {checkTypes} checked by tsc
*/


Expand Down
2 changes: 1 addition & 1 deletion test_files/class.untyped/class.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
goog.module('test_files.class.untyped.class');var module = module || {id: 'test_files/class.untyped/class.js'};/**
* @fileoverview added by tsickle
* @suppress {checkTypes}
* @suppress {checkTypes} checked by tsc
*/
/**
* @record
Expand Down
2 changes: 1 addition & 1 deletion test_files/class.untyped/class.tsickle.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @fileoverview added by tsickle
* @suppress {checkTypes}
* @suppress {checkTypes} checked by tsc
*/


Expand Down
2 changes: 1 addition & 1 deletion test_files/class/class.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
goog.module('test_files.class.class');var module = module || {id: 'test_files/class/class.js'};/**
* @fileoverview added by tsickle
* @suppress {checkTypes}
* @suppress {checkTypes} checked by tsc
*/
/**
* @record
Expand Down
2 changes: 1 addition & 1 deletion test_files/class/class.tsickle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ Warning at test_files/class/class.ts:125:1: type/symbol conflict for Zone, using
====
/**
* @fileoverview added by tsickle
* @suppress {checkTypes}
* @suppress {checkTypes} checked by tsc
*/


Expand Down
2 changes: 1 addition & 1 deletion test_files/clutz.no_externs/strip_clutz_type.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
goog.module('test_files.clutz.no_externs.strip_clutz_type');var module = module || {id: 'test_files/clutz.no_externs/strip_clutz_type.js'};/**
* @fileoverview added by tsickle
* @suppress {checkTypes}
* @suppress {checkTypes} checked by tsc
*/

var goog_some_name_space_1 = goog.require('some.name.space');
Expand Down
2 changes: 1 addition & 1 deletion test_files/clutz.no_externs/strip_clutz_type.tsickle.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @fileoverview added by tsickle
* @suppress {checkTypes}
* @suppress {checkTypes} checked by tsc
*/

import {ClutzedClass, clutzedFn} from 'goog:some.name.space';
Expand Down
Loading

0 comments on commit 8ebfd01

Please sign in to comment.