From bd4a048148b3b168a5c5f287a5f1e46415933d35 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Sat, 23 Feb 2019 16:12:11 -0500 Subject: [PATCH] Removed use of type checker from completed-docs (#3557) --- src/rules/completedDocsRule.ts | 130 ++++++++++++++---- .../completed-docs/accessors/test.ts.lint | 122 +++++++++++++++- .../completed-docs/defaults/test.ts.lint | 4 - .../completed-docs/tags/content/test.ts.lint | 13 ++ .../completed-docs/variables/test.ts.lint | 10 ++ 5 files changed, 244 insertions(+), 35 deletions(-) diff --git a/src/rules/completedDocsRule.ts b/src/rules/completedDocsRule.ts index 102d5159221..efffcd3330a 100644 --- a/src/rules/completedDocsRule.ts +++ b/src/rules/completedDocsRule.ts @@ -15,6 +15,7 @@ * limitations under the License. */ +import * as tsutils from "tsutils"; import * as ts from "typescript"; import * as Lint from "../index"; @@ -78,7 +79,7 @@ export type Privacy = export type Visibility = All | typeof VISIBILITY_EXPORTED | typeof VISIBILITY_INTERNAL; -export class Rule extends Lint.Rules.TypedRule { +export class Rule extends Lint.Rules.AbstractRule { public static FAILURE_STRING_EXIST = "Documentation must exist for "; public static defaultArguments: IInputExclusionDescriptors = { @@ -268,17 +269,16 @@ export class Rule extends Lint.Rules.TypedRule { `, type: "style", typescriptOnly: false, - requiresTypeInfo: true, }; /* tslint:enable:object-literal-sort-keys */ private readonly exclusionFactory = new ExclusionFactory(); - public applyWithProgram(sourceFile: ts.SourceFile, program: ts.Program): Lint.RuleFailure[] { + public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { const options = this.getOptions(); const exclusionsMap = this.getExclusionsMap(options.ruleArguments); - return this.applyWithFunction(sourceFile, walk, exclusionsMap, program.getTypeChecker()); + return this.applyWithFunction(sourceFile, walk, exclusionsMap); } private getExclusionsMap( @@ -296,7 +296,7 @@ const modifierAliases: { [i: string]: string } = { export: "exported", }; -function walk(context: Lint.WalkContext, typeChecker: ts.TypeChecker) { +function walk(context: Lint.WalkContext) { return ts.forEachChild(context.sourceFile, cb); function cb(node: ts.Node): void { @@ -364,7 +364,7 @@ function walk(context: Lint.WalkContext, typeChecker: ts.TypeChec case ts.SyntaxKind.GetAccessor: case ts.SyntaxKind.SetAccessor: if (node.parent.kind !== ts.SyntaxKind.ObjectLiteralExpression) { - checkNode(node as ts.AccessorDeclaration, ARGUMENT_PROPERTIES); + checkAccessorNode(node as ts.AccessorDeclaration); } } @@ -376,45 +376,58 @@ function walk(context: Lint.WalkContext, typeChecker: ts.TypeChec nodeType: DocType, requirementNode: ts.Node = node, ): void { + if (!nodeIsExcluded(node, nodeType, requirementNode) && !nodeHasDocs(node)) { + addDocumentationFailure(node, describeNode(nodeType), requirementNode); + } + } + + function checkAccessorNode(node: ts.AccessorDeclaration): void { + if (nodeIsExcluded(node, ARGUMENT_PROPERTIES, node) || nodeHasDocs(node)) { + return; + } + + const correspondingAccessor = getCorrespondingAccessor(node); + + if (correspondingAccessor === undefined || !nodeHasDocs(correspondingAccessor)) { + addDocumentationFailure(node, ARGUMENT_PROPERTIES, node); + } + } + + function nodeIsExcluded( + node: ts.NamedDeclaration, + nodeType: DocType, + requirementNode: ts.Node, + ): boolean { const { name } = node; if (name === undefined) { - return; + return true; } const exclusions = context.options.get(nodeType); if (exclusions === undefined) { - return; + return true; } for (const exclusion of exclusions) { if (exclusion.excludes(requirementNode)) { - return; + return true; } } - const symbol = typeChecker.getSymbolAtLocation(name); - if (symbol === undefined) { - return; - } - - const comments = symbol.getDocumentationComment(typeChecker); - checkComments(node, describeNode(nodeType), comments, requirementNode); + return false; } - function checkComments( - node: ts.Node, - nodeDescriptor: string, - comments: ts.SymbolDisplayPart[], - requirementNode: ts.Node, - ) { - if ( - comments - .map((comment: ts.SymbolDisplayPart) => comment.text) - .join("") - .trim() === "" - ) { - addDocumentationFailure(node, nodeDescriptor, requirementNode); + function nodeHasDocs(node: ts.Node): boolean { + const docs = getApparentJsDoc(node); + if (docs === undefined) { + return false; } + + const comments = docs + .map(doc => doc.comment) + .filter(comment => comment !== undefined && comment.trim() !== ""); + + return comments.length !== 0; } function addDocumentationFailure( @@ -430,6 +443,57 @@ function walk(context: Lint.WalkContext, typeChecker: ts.TypeChec } } +function getCorrespondingAccessor(node: ts.AccessorDeclaration) { + const propertyName = tsutils.getPropertyName(node.name); + if (propertyName === undefined) { + return undefined; + } + + const parent = node.parent as ts.ClassDeclaration | ts.ClassExpression; + const correspondingKindCheck = + node.kind === ts.SyntaxKind.GetAccessor ? isSetAccessor : isGetAccessor; + + for (const member of parent.members) { + if (!correspondingKindCheck(member)) { + continue; + } + + if (tsutils.getPropertyName(member.name) === propertyName) { + return member; + } + } + + return undefined; +} + +/** + * @remarks See https://github.com/ajafff/tsutils/issues/16 + */ +function getApparentJsDoc(node: ts.Node): ts.JSDoc[] | undefined { + if (ts.isVariableDeclaration(node)) { + if (variableIsAfterFirstInDeclarationList(node)) { + return undefined; + } + + node = node.parent; + } + + if (ts.isVariableDeclarationList(node)) { + node = node.parent; + } + + return tsutils.getJsDoc(node); +} + +function variableIsAfterFirstInDeclarationList(node: ts.VariableDeclaration): boolean { + const parent = node.parent; + if (parent === undefined) { + return false; + } + + return ts.isVariableDeclarationList(parent) && node !== parent.declarations[0]; +} + function describeDocumentationFailure(node: ts.Node, nodeType: string): string { let description = Rule.FAILURE_STRING_EXIST; @@ -451,3 +515,11 @@ function describeModifier(kind: ts.SyntaxKind) { function describeNode(nodeType: DocType): string { return nodeType.replace("-", " "); } + +function isGetAccessor(node: ts.Node): node is ts.GetAccessorDeclaration { + return node.kind === ts.SyntaxKind.GetAccessor; +} + +function isSetAccessor(node: ts.Node): node is ts.SetAccessorDeclaration { + return node.kind === ts.SyntaxKind.SetAccessor; +} diff --git a/test/rules/completed-docs/accessors/test.ts.lint b/test/rules/completed-docs/accessors/test.ts.lint index 41cf985c126..be5805310ef 100644 --- a/test/rules/completed-docs/accessors/test.ts.lint +++ b/test/rules/completed-docs/accessors/test.ts.lint @@ -45,7 +45,7 @@ export class CommentsOnGetOnly { */ get myAccessor(): string { return this.myField; } - set myAccessor(value: string) { this.myField = value; } // Comments for the setter are inherited from the getter. + set myAccessor(value: string) { this.myField = value; } } @@ -56,7 +56,7 @@ export class CommentsOnSetOnly { */ private myField: string; - get myAccessor(): string { return this.myField; } // Comments from the getter are inherited from the setter. + get myAccessor(): string { return this.myField; } /** * The set accessor. @@ -116,3 +116,121 @@ export class OnlyHasSetAccessorWithNoComments { ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Documentation must exist for properties.] } + +export class BothAccessorWithDifferentNamesNoComments { + + /** + * ... + */ + private myField: string; + + get myGetAccessor() { return this.myField; } + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Documentation must exist for properties.] + + set mySetAccessor(value: string) { this.myField = value; } + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Documentation must exist for properties.] + +} + +const computedName = "myComputedName"; + +export class ComputedNamesWithCommentsOnGetOnly { + + /** + * ... + */ + private myField: string; + + /** + * ... + */ + get [computedName](): string { return this.myField; } + + set [computedName](value: string) { this.myField = value; } + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Documentation must exist for properties.] + +} + +export class ComputedNamesWithCommentsOnSetOnly { + + /** + * ... + */ + private myField: string; + + get [computedName](): string { return this.myField; } + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Documentation must exist for properties.] + + /** + * ... + */ + set [computedName](value: string) { this.myField = value; } + +} + +export class OnlyComputedGetAccessorNameWithNoComments { + + /** + * ... + */ + private myField: string; + + get [computedName](): string { return this.myField; } + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Documentation must exist for properties.] + +} + +export class OnlyComputedGetAccessorNameWithComments { + + /** + * ... + */ + private myField: string; + + /** + * ... + */ + get [computedName](): string { return this.myField; } + +} + +export class OnlyComputedSetAccessorNameWithNoComments { + + /** + * ... + */ + private myField: string; + + set [computedName](value: string) { this.myField = value; } + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Documentation must exist for properties.] + +} + +export class OnlyComputedSetAccessorNameWithComments { + + /** + * ... + */ + private myField: string; + + /** + * ... + */ + set [computedName](value: string) { this.myField = value; } + +} + +export class SameComputedNamesWithNoComments { + + /** + * ... + */ + private myField: string; + + get [computedName](): string { return this.myField; } + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Documentation must exist for properties.] + + set [computedName](value: string) { this.myField = value; } + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Documentation must exist for properties.] + +} diff --git a/test/rules/completed-docs/defaults/test.ts.lint b/test/rules/completed-docs/defaults/test.ts.lint index aca3361617f..011989a4537 100644 --- a/test/rules/completed-docs/defaults/test.ts.lint +++ b/test/rules/completed-docs/defaults/test.ts.lint @@ -12,12 +12,8 @@ export class Aaa { public set prop(value) { this.bbb = value; } ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Documentation must exist for public properties.] - // TypeScript API doesn't give a symbol for this before version 2.8.0 - // https://github.com/Microsoft/TypeScript/issues/14257 [Symbol.iterator]() {} -#if typescript >= 2.8.0 ~~~~~~~~~~~~~~~~~~~~~~ [Documentation must exist for methods.] -#endif } export enum Ddd { } diff --git a/test/rules/completed-docs/tags/content/test.ts.lint b/test/rules/completed-docs/tags/content/test.ts.lint index d0bbbc68cc3..2e8b5dfe556 100644 --- a/test/rules/completed-docs/tags/content/test.ts.lint +++ b/test/rules/completed-docs/tags/content/test.ts.lint @@ -42,7 +42,20 @@ const ContentSingleLineValidVariable = 4; /** * ... */ +const CommentBodyVariableDots = 5; + +/** + * ...content + */ +const CommentBodyVariableDotsAndContent = 6; + +/** content */ +const CommentBodyVariableSingleLineContent = 7; + +/** ...content */ +const CommentBodyVariableSingleLineDotsAndContent = 8; const CommentBodyVariableRofl = 5; + ~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Documentation must exist for variables.] /** ... */ const CommentBodyVariableRofl = 5; diff --git a/test/rules/completed-docs/variables/test.ts.lint b/test/rules/completed-docs/variables/test.ts.lint index 33dcdf5d900..fd162a287af 100644 --- a/test/rules/completed-docs/variables/test.ts.lint +++ b/test/rules/completed-docs/variables/test.ts.lint @@ -4,6 +4,16 @@ export const exportedVariable = 1; const internalVariable = 2; ~~~~~~~~~~~~~~~~~~~~ [Documentation must exist for variables.] +/** + * (documentation here will be read) + */ +const firstVariableInList = true, + /** + * (documentation here will not be read) + */ + secondVariableInList = false; + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Documentation must exist for variables.] + // Variables in `catch` clauses should not be checked. try { } catch (ex) {