Expose JSDoc tags through the language service #12856

minestarks · 2016-12-12T17:48:34Z

Expose JSDoc tags through the language service

The main motivation behind this PR is to fix UWP API documentation behavior in Dev15, which relies on a custom JSDoc tag (@xmldoc), and was broken between 2.0 and 2.1.

The behavior of getDocumentationComment() was changed by #10671 to not include JSDoc tags anymore. I added them back as a new property.

Note the "tags" property I added to these calls will only expose the unknown tags (i.e. things that are not specially handled like @returns, @param etc) ... I couldn't think of a justification to expose the known tags too (well, also I wouldn't have been able to do so without making more significant changes to the parser which I want to avoid at the moment).

Fixes #11956

You might be interested @sandersn @vladima

msftclas · 2016-12-12T17:48:37Z

Hi @minestarks, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Mine Yalcinalp). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

vladima · 2016-12-12T19:10:36Z

src/server/session.ts

                return {
                    kind: quickInfo.kind,
                    kindModifiers: quickInfo.kindModifiers,
                    start: scriptInfo.positionToLineOffset(quickInfo.textSpan.start),
                    end: scriptInfo.positionToLineOffset(ts.textSpanEnd(quickInfo.textSpan)),
                    displayString: displayString,
                    documentation: docString,
+                    tags: tags


nit: tags: quickInfo.tags || []

sandersn

I reviewed this on an iPad and it looks good but I'd like to take one more look with codeflow to make sure I don't miss anything before signing off.

sandersn · 2016-12-12T19:00:38Z

src/services/jsDoc.ts

+                return;
+            }
+            const jsDocTags: JSDocTag[] = [];
+            jsDocs.forEach(doc => {


Why not just for (const doc in jsDocs)?

sandersn · 2016-12-12T19:20:17Z

src/services/types.ts

@@ -492,12 +494,18 @@ namespace ts {
        kind: string;
    }

+    export interface JSDocTagInfo {


I'm not sure about the naming scheme in services, but I'd prefer a terser name, like JSDocTag.

Me too. But there's already a ts.JSDocTag type and it's not quite what I want....

sandersn · 2016-12-12T19:21:40Z

src/compiler/parser.ts

@@ -6468,7 +6468,7 @@ namespace ts {

                function parseTagComments(indent: number) {
                    const comments: string[] = [];
-                    let state = JSDocState.SawAsterisk;


Is this just a drive-by bug fix? I didn't see any real difference in parsing functionality.

Ah yes I should clarify. This isn't irrelevant. It fixes a bug I discovered while running the new quickinfo test I added. Consider this comment:

/** * @mytag1 * here all the comments are on a new line * @mytag2 */

When the comment parser starts out at "SawAsterisk" state I get this tag comment for mytag1: "* here all the comments are on a new line"

With my change it's fixed: "here all the comments are on a new line"

This is because we skip all the whitespace (including newlines) before we begin parsing so we start right at the *. This would have been handled in the AsteriskToken case below, but only if the last state had been BeginningOfLine.

Going over the whole logic I believe it just makes more sense this way. Starting at BeginningOfLine is more logical since we didn't actually see an asterisk so why start at SawAsterisk...

Makes sense. There was a similar bug in the start state for the main jsdoc parsing, so it makes sense that you ran into it here too.

vladima · 2016-12-12T19:30:05Z

src/services/completions.ts

@@ -759,13 +759,14 @@ namespace ts.Completions {
            const symbol = forEach(symbols, s => getCompletionEntryDisplayNameForSymbol(typeChecker, s, compilerOptions.target, /*performCharacterChecks*/ false, location) === entryName ? s : undefined);

            if (symbol) {
-                const { displayParts, documentation, symbolKind } = SymbolDisplay.getSymbolDisplayPartsDocumentationAndSymbolKind(typeChecker, symbol, sourceFile, location, location, SemanticMeaning.All);
+                const { displayParts, documentation, symbolKind, jsDocTags } = SymbolDisplay.getSymbolDisplayPartsDocumentationAndSymbolKind(typeChecker, symbol, sourceFile, location, location, SemanticMeaning.All);


nit: tags to enable shorthand property assignment below

vladima · 2016-12-12T19:31:00Z

src/services/jsDoc.ts

+            }
+            const jsDocTags: JSDocTag[] = [];
+            jsDocs.forEach(doc => {
+                const tagsForDoc = (doc as JSDoc).tags;


is type assertion necessary?

I guess not. I got confused by the 2.0 language service giving me an implicit any warning...

vladima · 2016-12-12T19:32:37Z

src/services/jsDoc.ts

+                    jsDocTags.push(...tagsForDoc.filter(tag => tag.kind === SyntaxKind.JSDocTag));
+                }
+            });
+            if (!jsDocTags) {


always false since jsDocTags array is pre-allocated

vladima · 2016-12-12T19:37:36Z

src/services/jsDoc.ts

+            if (!jsDocTags) {
+                return;
+            }
+            for (const tag of jsDocTags) {


silly question: do we need intermediate jsDocTags? Can we just put data into tags directly?

Sure I don't see why not. Takes care of your other comment below too.

vladima · 2016-12-12T19:38:23Z

src/services/jsDoc.ts

+                return;
+            }
+            for (const tag of jsDocTags) {
+                if (tag) {


can it ever be null or undefined? I think in this case line 83 should fail

mihailik

Suggestion: avoid empty array allocation in default case.

mihailik · 2016-12-13T08:49:13Z

src/services/services.ts

@@ -404,6 +416,10 @@ namespace ts {
        // symbol has no doc comment, then the empty string will be returned.
        documentationComment: SymbolDisplayPart[];

+        // Undefined is used to indicate the value has not been computed. If, after computing, the
+        // symbol has no doc comment, then the empty array will be returned.
+        jsDocTags: JSDocTagInfo[];


Empty array is expensive here.

Most of SignatureObject instances will have no JSDoc comments. Allocating empty arrays will add bunch of fluff to GC. Would be best to avoid that.

mihailik · 2016-12-13T08:51:55Z

src/services/services.ts

+        getJsDocTags(): JSDocTagInfo[] {
+            if (this.jsDocTags === undefined) {
+                this.jsDocTags = this.declaration ? JsDoc.getJsDocTagsFromDeclarations([this.declaration]) : [];
+            }


How about using undefined for storing un-computed state and null for computed and empty?

mihailik · 2016-12-13T08:54:42Z

src/services/jsDoc.ts

@@ -68,6 +68,28 @@ namespace ts.JsDoc {
        return documentationComment;
    }

+    export function getJsDocTagsFromDeclarations(declarations: Declaration[]) {
+        // Only collect doc comments from duplicate declarations once.
+        const tags: JSDocTagInfo[] = [];


Here too the array may be lazy-allocated when first tag is encountered -- meaning no allocation for the default case.

mihailik · 2016-12-13T08:56:58Z

src/services/services.ts

+        getJsDocTags(): JSDocTagInfo[] {
+            if (this.tags === undefined) {
+                this.tags = JsDoc.getJsDocTagsFromDeclarations(this.declarations);
+            }


Here too the check would need to change to avoid empty array allocation for the default no-JSDoc case.
(to mark the difference between un-computed value and empty value in this.tags -- undefined versus null can be used)

mihailik · 2016-12-13T09:00:47Z

src/services/services.ts

@@ -1315,7 +1339,8 @@ namespace ts {
                                kindModifiers: ScriptElementKindModifier.none,
                                textSpan: createTextSpan(node.getStart(), node.getWidth()),
                                displayParts: typeToDisplayParts(typeChecker, type, getContainerNode(node)),
-                                documentation: type.symbol ? type.symbol.getDocumentationComment() : undefined
+                                documentation: type.symbol ? type.symbol.getDocumentationComment() : undefined,
+                                tags: type.symbol ? type.symbol.getJsDocTags() : undefined


Note that the API may return undefined here, so returning null or undefined instead of an empty array shouldn't break expectations.

mihailik · 2016-12-13T09:02:35Z

tests/baselines/reference/quickInfoDisplayPartsArrowFunctionExpression.baseline

@@ -73,7 +73,8 @@
          "kind": "keyword"
        }
      ],
-      "documentation": []
+      "documentation": [],
+      "tags": []


And now look at those billions of empty arrays peppering the ***.baseline down from here.

Expose JSDoc tags through the language service

minestarks added 3 commits December 7, 2016 13:55

Add JSDoc tag support for QuickInfo

bd0893a

Add JSDoc tag support for Signature Help

cf2bfc0

Add JSDoc tag support for completion entry details

5130e95

msftclas added the cla-not-required label Dec 12, 2016

vladima reviewed Dec 12, 2016

View reviewed changes

sandersn reviewed Dec 12, 2016

View reviewed changes

vladima reviewed Dec 12, 2016

View reviewed changes

minestarks added 2 commits December 12, 2016 13:51

Clean up per CR comments

d7c4001

Fix whitespace

2875d5b

minestarks merged commit 17d7aa8 into microsoft:release-2.1 Dec 12, 2016

minestarks deleted the includejsdoctags branch December 12, 2016 23:29

mihailik reviewed Dec 13, 2016

View reviewed changes

aindlq mentioned this pull request Mar 6, 2017

getJsDocTags does not return @type annotations #13498

Closed

minestarks added a commit to minestarks/TypeScript that referenced this pull request Apr 1, 2017

Merge pull request microsoft#12856 from minestarks/includejsdoctags

dfde279

Expose JSDoc tags through the language service

minestarks mentioned this pull request Apr 1, 2017

Expose JSDoc tags through the language service #14960

Merged

minestarks added a commit to minestarks/TypeScript that referenced this pull request Apr 1, 2017

Merge pull request microsoft#12856 from minestarks/includejsdoctags

af0b2d9

Expose JSDoc tags through the language service

minestarks added a commit to minestarks/TypeScript that referenced this pull request Apr 3, 2017

Merge pull request microsoft#12856 from minestarks/includejsdoctags

5e98bcd

Expose JSDoc tags through the language service

minestarks mentioned this pull request Apr 3, 2017

Expose JSDoc tags through the language service (2.2) #14988

Merged

minestarks added a commit to minestarks/TypeScript that referenced this pull request Apr 3, 2017

Merge pull request microsoft#12856 from minestarks/includejsdoctags

e1c2757

Expose JSDoc tags through the language service

minestarks mentioned this pull request Apr 3, 2017

Expose JSDoc tags through the language service (2.3) #14989

Merged

This was referenced Apr 28, 2017

jsDoc tags are not available from the language service API #12863

Closed

changed behavior of symbol.getDocumentationComment() #11956

Closed

microsoft locked and limited conversation to collaborators Jun 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose JSDoc tags through the language service #12856

Expose JSDoc tags through the language service #12856

minestarks commented Dec 12, 2016 •

edited

Loading

msftclas commented Dec 12, 2016

vladima Dec 12, 2016

sandersn left a comment

sandersn Dec 12, 2016

sandersn Dec 12, 2016

minestarks Dec 12, 2016

sandersn Dec 12, 2016

minestarks Dec 12, 2016

sandersn Dec 12, 2016

vladima Dec 12, 2016 •

edited

Loading

vladima Dec 12, 2016

minestarks Dec 12, 2016

vladima Dec 12, 2016 •

edited

Loading

vladima Dec 12, 2016

minestarks Dec 12, 2016

vladima Dec 12, 2016

mihailik left a comment

mihailik Dec 13, 2016

mihailik Dec 13, 2016

mihailik Dec 13, 2016

mihailik Dec 13, 2016

mihailik Dec 13, 2016

mihailik Dec 13, 2016

Expose JSDoc tags through the language service #12856

Expose JSDoc tags through the language service #12856

Conversation

minestarks commented Dec 12, 2016 • edited Loading

msftclas commented Dec 12, 2016

Choose a reason for hiding this comment

sandersn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladima Dec 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladima Dec 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihailik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minestarks commented Dec 12, 2016 •

edited

Loading

vladima Dec 12, 2016 •

edited

Loading

vladima Dec 12, 2016 •

edited

Loading