Use documentation comments from inherited properties when @inheritDoc is present #18804

sjbarag · 2017-09-27T18:55:27Z

The JSDoc @ineheritDoc tag "indicates that a symbol should inherit its documentation from its parent class". In the case of a TypeScript file, this also includes implemented interfaces and parent interfaces.

With this change, a class method or property (or an interface property) with the @inheritDoc tag in its JSDoc comment will automatically use the comments from its nearest ancestor that has no @inheritDoc tag. To prevent breaking backwards compatibility, Symbol.getDocumentationComment now accepts an optional TypeChecker instance to support this feature.

fixes #8912

msftclas · 2017-09-27T18:55:35Z

All CLA requirements met.

mhegazy · 2017-10-09T21:54:38Z

@Andy-MS can you please review this change.

sjbarag · 2017-10-17T16:02:39Z

For posterity: rebased on fcb48dd and re-ran gulp baseline-accept to accept changes to api/tsserverlibrary.d.ts and api/typescript.d.ts. I haven't changed the diff otherwise though 😄

ghost · 2017-10-17T17:44:12Z

src/services/services.ts

            if (this.documentationComment === undefined) {
                this.documentationComment = this.declaration ? JsDoc.getJsDocCommentsFromDeclarations([this.declaration]) : [];
+
+                if (this.declaration && this.documentationComment.length === 0 && hasJSDocInheritDocTag(this) && typeChecker) {


This might whole block might be neater as if (this.declaration) { ... } else { this.documentationComment = []; }

ghost · 2017-10-17T17:44:56Z

src/services/services.ts

+     * @returns `true` if `symbol` has a JSDoc "inheritDoc" tag on it, otherwise `false`.
+     */
+    function hasJSDocInheritDocTag(symbol: Signature | Symbol) {
+        return !!find(symbol.getJsDocTags(), tag => tag.name === "inheritDoc");


symbol.getJsDocTags().some(...)

ghost · 2017-10-17T17:47:35Z

src/services/jsDoc.ts

        forEachUnique(declarations, declaration => {
            for (const tag of getJSDocTags(declaration)) {
-                if (tag.kind === SyntaxKind.JSDocTag) {
+                if (tag.kind === SyntaxKind.JSDocTag || tag.kind === SyntaxKind.JSDocInheritDocTag) {


Don't think you need to modify this function for what you want; use ts.getJSDocTags (from compiler/utilities.ts) instead, which returns tags of all kinds.

ghost · 2017-10-17T17:49:10Z

src/services/services.ts

+    function findInheritedJSDocComments(declaration: Declaration, propertyName: string, typeChecker: TypeChecker): SymbolDisplayPart[] {
+        let documentationComment: SymbolDisplayPart[] = [];
+
+        if (isClassDeclaration(declaration.parent) || isInterfaceDeclaration(declaration.parent)) {


Prefer an early return. Also, use isClassLike.

ghost · 2017-10-17T17:49:58Z

src/services/services.ts

+        let documentationComment: SymbolDisplayPart[] = [];
+
+        if (isClassDeclaration(declaration.parent) || isInterfaceDeclaration(declaration.parent)) {
+            const container: ClassDeclaration | InterfaceDeclaration = declaration.parent;


You can just declare this outside (as a Node) and rely on isClassDeclaration and isInterfaceDeclaration working as type predicates.

ghost · 2017-10-17T17:50:52Z

src/services/services.ts

+            const container: ClassDeclaration | InterfaceDeclaration = declaration.parent;
+            const baseTypeNode = getClassExtendsHeritageClauseElement(container);
+
+            if (baseTypeNode) {


Early return

ghost · 2017-10-17T17:52:28Z

src/services/services.ts

+
+        if (isClassDeclaration(declaration.parent) || isInterfaceDeclaration(declaration.parent)) {
+            const container: ClassDeclaration | InterfaceDeclaration = declaration.parent;
+            const baseTypeNode = getClassExtendsHeritageClauseElement(container);


There's a function getBaseTypes in checker.ts that might be better suited. You could expose it internally by adding /* @internal */ getBaseTypes(): ReadonlyArray<BaseType>; to interface TypeChecker.
The advantages are that this gets you both classes and interfaces, and you don't have to call getTypeAtLocation.

ghost · 2017-10-17T17:56:05Z

tests/cases/fourslash/jsDocInheritDoc.ts

+////const p1 = b.property1/*4*/;
+////const p2 = b.property2/*5*/;
+
+verify.baselineQuickInfo();


This would be breifer using just verify.quickInfoAt -- baselineQuickInfo is more so we can check all the displayParts, but that's not what we're testing here.

ghost · 2017-10-17T17:57:08Z

What if we just inherited documentation by default without requiring users to add /** @inheritDoc */ to everything?

sjbarag · 2017-10-17T18:27:24Z

Thanks for the review, @Andy-MS! I'll get this fixed up today or tomorrow.

What if we just inherited documentation by default without requiring users to add /** @inheritDoc */ to everything?

I think it'd be pretty intuitive to be honest. I'd considered doing that here but didn't want to take too big of a step 😃 Should be relatively simple to get working, so I'll switch to that approach.

Thanks again! ❤️

sjbarag · 2017-10-26T18:11:34Z

Got a few updates here based on your feedback!

Rebased on 9c96eee
Added early returns
Used ts.getJSDocTags
Used verify.quickInfoAt in tests

I unfortunately don't see a way to get typeChecker.getBaseType to work, since it accepts an InterfaceType. Perhaps I'm missing something though?

Thanks for your patience while I updated this 😄

ghost · 2017-10-26T19:12:24Z

OK, I was wrong about getBaseTypes -- it will only return the extends type, not any interfaces.
I've managed to eliminate duplicate code between the extends and implements cases though:

function findInheritedJSDocComments(declaration: Declaration, propertyName: string, typeChecker: TypeChecker): SymbolDisplayPart[] {
    return flatMap(getAllSuperTypeNodes(declaration), superType => {
        const baseProperty = typeChecker.getPropertyOfType(typeChecker.getTypeAtLocation(superType), propertyName);
        return baseProperty && baseProperty.getDocumentationComment(typeChecker);
    });
}

function getAllSuperTypeNodes(declaration: Declaration): ReadonlyArray<TypeNode> {
    const container = declaration.parent;
    if (!container || (!isClassDeclaration(container) && !isInterfaceDeclaration(container))) {
        return emptyArray;
    }
    const extended = getClassExtendsHeritageClauseElement(container);
    const types = extended ? [extended] : emptyArray;
    return isClassLike(container) ? concatenate(types, getClassImplementsHeritageClauseElements(container)) : types;
}

ghost · 2017-10-26T18:30:08Z

src/services/services.ts

+                if (this.declarations) {
+                    this.documentationComment = JsDoc.getJsDocCommentsFromDeclarations(this.declarations);
+
+                    if (this.documentationComment.length === 0 || this.declarations.some(dec => hasJSDocInheritDocTag(dec))) {


Nit: dec => hasJSDocInheritDocTag(dec) -- just use hasJSDocInheritDocTag directly

ghost · 2017-10-26T18:37:19Z

src/services/services.ts

+                if (this.declaration) {
+                    this.documentationComment = JsDoc.getJsDocCommentsFromDeclarations([this.declaration]);
+
+                    if (this.documentationComment.length === 0) {


Here's a case where I think we should use both inherited and own doc comments:

///<reference path="fourslash.ts" /> ////interface I { //// /** Interface doc */ //// m(): void; ////} //// ////class C implements I { //// /** //// * Class doc //// * @inheritDoc //// */ //// m/**/() {} ////} verify.quickInfoAt("", "(method) C.m(): void", "Inferface doc\nClass doc");

Great point! I agree. This would make TypeScript's handling of @inheritDoc different than vanilla JSDoc's though, which ignores documentation on anything with @inheritDoc, e.g.:

/** @class */ class Foo { /** Foo docs. */ test() { } } /** * @class * @extends Foo */ class Bar { /** * Bar docs. * @inheritDoc */ test() { } }

results in JSDoc for Bar#test of "Foo docs" only.

That said, I think it's helpful for TypeScript users and I'd love to have it. As long as you're okay with differing from upstream here, I'm 100% fine with it 👍

You're right, http://usejsdoc.org/tags-inheritdoc.html says:

The @inheritdoc tag indicates that a symbol should inherit its documentation from its parent class. Any other tags that you include in the JSDoc comment will be ignored.
This tag is provided for compatibility with Closure Compiler. By default, if you do not add a JSDoc comment to a symbol, the symbol will inherit documentation from its parent.
The presence of the @inheritdoc tag implies the presence of the @OverRide tag.

This makes it a rather strange tag -- one whose sole purpose is to make all documentation on the current node ignored! Presumably people who don't want to see that comment in quick info should just make it a regular comment and not a doc comment. I think it would be a more useful tag if it meant to add docs from a parent in addition to anything on the child.

Makes perfect sense to me. I've updated the PR to support that :)

ghost · 2017-10-26T18:39:08Z

src/compiler/parser.ts

                            case "constructor":
                                tag = parseClassTag(atToken, tagName);
                                break;
+                            case "inheritDoc":


Doesn't seem to be needed as its own SyntaxKind any more?

ghost · 2017-10-26T19:13:56Z

src/services/types.ts

        getName(): string;
        getDeclarations(): Declaration[] | undefined;
-        getDocumentationComment(): SymbolDisplayPart[];
+        getDocumentationComment(typeChecker?: TypeChecker): SymbolDisplayPart[];


Based on #19455 (comment) maybe it would be better to just take a compile-time breaking change and make this typeChecker: TypeChecker | undefined. @mhegazy

sjbarag · 2017-10-27T03:04:32Z

Sorry! 424606c failed to build because I forgot to re-accept the baselines after switching from typeChecker?: TypeChecker to typeChecker: TypeChecker | undefined. a839a80 should build 🤞

sjbarag · 2017-10-27T16:02:39Z

Well this is embarrassing, I forgot to update APISample_jsdoc.ts to match the interface change. Should be fixed now (for real (really (I mean it 😅)))

…is present The JSDoc `@ineheritDoc` [tag](http://usejsdoc.org/tags-inheritdoc.html) "indicates that a symbol should inherit its documentation from its parent class". In the case of a TypeScript file, this also includes implemented interfaces and parent interfaces. With this change, a class method or property (or an interface property) with the `@inheritDoc` tag in its JSDoc comment will automatically use the comments from its nearest ancestor that has no `@inheritDoc` tag. To prevent breaking backwards compatibility, `Symbol.getDocumentationComment` now accepts an optional `TypeChecker` instance to support this feature. fixes microsoft#8912

sjbarag · 2017-11-01T23:51:40Z

Rebased on 1a7a587! Dang, master moves quick 😄

ghost · 2017-11-06T21:18:19Z

Thanks!

cliffkoh · 2017-11-07T00:35:11Z

Thank you @sjbarag, this is super awesome!

mhegazy requested a review from a user October 9, 2017 21:54

sjbarag force-pushed the support-jsdoc-inheritdoc branch from df96aba to 6ff0aed Compare October 17, 2017 16:12

ghost reviewed Oct 17, 2017

View reviewed changes

This was referenced Oct 20, 2017

completed-docs should enable @inheritdoc by default palantir/tslint#3356

Closed

Stop using the type checker in completed-docs palantir/tslint#3376

Closed

sjbarag force-pushed the support-jsdoc-inheritdoc branch from 6ff0aed to f3f6769 Compare October 26, 2017 18:09

ghost reviewed Oct 26, 2017

View reviewed changes

sjbarag force-pushed the support-jsdoc-inheritdoc branch 2 times, most recently from 351dcbf to 424606c Compare October 26, 2017 23:42

ghost mentioned this pull request Oct 27, 2017

FR: JSDocs should be inherited from an implemented interface/base class #19524

Closed

sjbarag added 8 commits November 1, 2017 16:46

Use ts.getJSDocTags as per @Andy-MS 's recommendation

632bbfd

Convert @inheritdoc tests to verify.quickInfoAt

37678c3

Concatenate inherited and local docs when @inheritdoc is present

7e6c026

Make typeChecker param explicitly TypeChecker | undefined

1a1ff55

Re-accept baseline after switch to explicit | undefined

3e1c39f

Update APISample_jsodc.ts to match new getDocumentationComment signature

733ece2

Re-accept baselines after rebasing

5ba044a

sjbarag force-pushed the support-jsdoc-inheritdoc branch from 23ac1ed to 5ba044a Compare November 1, 2017 23:50

ghost merged commit a46d270 into microsoft:master Nov 6, 2017

microsoft locked and limited conversation to collaborators Jun 14, 2018

This pull request was closed.

Use documentation comments from inherited properties when @inheritDoc is present #18804

Use documentation comments from inherited properties when @inheritDoc is present #18804

Uh oh!

Conversation

sjbarag commented Sep 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msftclas commented Sep 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhegazy commented Oct 9, 2017

Uh oh!

sjbarag commented Oct 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Oct 17, 2017

Uh oh!

sjbarag commented Oct 17, 2017

Uh oh!

sjbarag commented Oct 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Oct 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Oct 26, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjbarag commented Oct 27, 2017

Uh oh!

sjbarag commented Oct 27, 2017

Uh oh!

sjbarag commented Nov 1, 2017

Uh oh!

ghost commented Nov 6, 2017

Uh oh!

cliffkoh commented Nov 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sjbarag commented Sep 27, 2017 •

edited

Loading

msftclas commented Sep 27, 2017 •

edited

Loading

sjbarag commented Oct 26, 2017 •

edited

Loading

ghost Oct 26, 2017 •

edited by ghost

Loading