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

Do not emit @type if @define is specified #1003

Merged
merged 7 commits into from
Mar 20, 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
26 changes: 15 additions & 11 deletions src/jsdoc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -151,11 +151,13 @@ const JSDOC_TAGS_INPUT_BLACKLIST = new Set([
]);

/**
* 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.
* JSDoc \@tags that might include a {type} after them. Specifying a type is forbidden, since it
* would collide with TypeScript's type information. If a type *is* given, the entire tag will be
* ignored.
*/
const JSDOC_TAGS_WITH_TYPES = new Set([
'const',
'define',
'export',
'param',
'return',
Expand Down Expand Up @@ -234,17 +236,19 @@ export function parseContents(commentText: string): ParsedJSDocComment|null {
// Drop it without any warning. (We also don't ensure its correctness.)
continue;
}
} else if (JSDOC_TAGS_WITH_TYPES.has(tagName) && text[0] === '{') {
warnings.push(
`the type annotation on @${tagName} is redundant with its TypeScript type, ` +
`remove the {...} part`);
continue;
} else if (JSDOC_TAGS_WITH_TYPES.has(tagName)) {
if (text[0] === '{') {
warnings.push(
`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}"`);
const typeMatch = text.match(/^\{(.*)\}(.*)$/);
if (typeMatch) {
[, type, text] = typeMatch;
} else {
[, type, text] = suppressMatch;
warnings.push(`malformed @${tagName} tag: "${text}"`);
}
} else if (tagName === 'dict') {
warnings.push('use index signatures (`[k: string]: type`) instead of @dict');
Expand Down
11 changes: 9 additions & 2 deletions src/jsdoc_transformer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -602,7 +602,8 @@ export function jsdocTransformer(
tags = null;
}
// Add an @type for plain identifiers, but not for bindings patterns (i.e. object or array
// destructuring) - those do not have a syntax in Closure.
// destructuring - those do not have a syntax in Closure) or @defines, which already
shicks marked this conversation as resolved.
Show resolved Hide resolved
// declare their type.
if (ts.isIdentifier(decl.name)) {
// For variables that are initialized and use a blacklisted type, do not emit a type at
// all. Closure Compiler might be able to infer a better type from the initializer than
Expand All @@ -615,7 +616,13 @@ export function jsdocTransformer(
// getOriginalNode(decl) is required because the type checker cannot type check
// synthesized nodes.
const typeStr = moduleTypeTranslator.typeToClosure(ts.getOriginalNode(decl));
localTags.push({tagName: 'type', type: typeStr});
// If @define is present then add the type to it, rather than adding a normal @type.
const defineTag = localTags.find(({tagName}) => tagName === 'define');
if (defineTag) {
defineTag.type = typeStr;
} else {
localTags.push({tagName: 'type', type: typeStr});
}
}
}
const newStmt = ts.createVariableStatement(
Expand Down
11 changes: 11 additions & 0 deletions test_files/jsdoc/jsdoc.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
// test_files/jsdoc/jsdoc.ts(77,1): warning TS0: @extends annotations are redundant with TypeScript equivalents
// @implements annotations are redundant with TypeScript equivalents
// test_files/jsdoc/jsdoc.ts(84,3): warning TS0: @constructor annotations are redundant with TypeScript equivalents
// test_files/jsdoc/jsdoc.ts(132,1): warning TS0: the type annotation on @define is redundant with its TypeScript type, remove the {...} part
/**
* @fileoverview added by tsickle
* @suppress {checkTypes,extraRequire,missingOverride,missingReturn,unusedPrivateMembers,uselessCode} checked by tsc
Expand Down Expand Up @@ -166,3 +167,13 @@ Polymer({ behaviors: [(/** @type {?} */ ('test'))] });
*/
class Foo {
}
/** @type {number} */
const DEFINE_WITH_JSDOC_TYPE = 42;
/**
* @define {boolean}
*/
const DEFINE_WITH_INFERRED_TYPE = false;
/**
* @define {string}
*/
const DEFINE_WITH_DECLARED_TYPE = 'y';
15 changes: 15 additions & 0 deletions test_files/jsdoc/jsdoc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -128,3 +128,18 @@ Polymer({behaviors: ['test' as any]});
* @template T2 Another user comment.
*/
class Foo<T1, T2> {}

/**
* @define {boolean}
*/
const DEFINE_WITH_JSDOC_TYPE = 42;

/**
* @define
*/
const DEFINE_WITH_INFERRED_TYPE = false;

/**
* @define
*/
const DEFINE_WITH_DECLARED_TYPE: string = 'y';
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks, this looks good!