Merge existing JSDoc comments #27978

sandersn · 2018-10-18T21:36:42Z

This PR is a followup to the infer-from-usage codefix PR. Instead of generating comment text in textChanges.ts, it uses the existing infrastructure in the emitter and formatter. That means that:

The emitter can now emit JSDoc.
The formatter is slightly better (probably) at emitting multiline comments. Because I didn't want to take the time to completely understand and restructure the formatter, I left the existing code for single-line comments intact.
The factory functions now include a few JSDoc tags, but only the ones I needed for this PR. It's easy to add these, so I think it's fine to leave them out.

It additionally means that
4. inferFromUsage.ts constructs JSDoc tags instead of its own data structures.

As a result, you'll see a lot of new, very predictable code in the emitter and lots of deletes in textChanges.ts.

I also wrote code to merge with existing JSDoc comments. The biggest change to existing comments will be the merging of multiple JSDoc comments into one, eg

/** @param x one */
/** @param y two */

Will become

/**
 * @param {number} x one
 * @param {number} y two
 */

Note that part of the code, in formatting.ts, is cloned but should be extracted to a function instead.

But I see 4 failures with whitespace, so perhaps not.

The indentation code is very complex so I'm just going to avoid breaking our single-line tests for now, plus add a simple jsdoc test to show that multiline jsdoc indentation isn't destroyed in the common case.

Still doesn't merge correctly though

@param

(not for @param yet)

Not all of them; I got cold feet since I'll have to write tests for them. I'll do that tomorrow.

And single tests (at least) for all tags

(Plus a little more in textChanges.ts)

sandersn · 2018-10-18T21:37:28Z

@amcasey you are not required for this review, but your comments, combined with @Andy-MS' noImplicitThis codefix, led to it, so I thought you'd be interested.

src/compiler/emitter.ts

tests/cases/fourslash/codeFixInferFromUsageExistingJSDoc.ts

instead of space.

tests/cases/fourslash/codeFixInferFromUsageExistingJSDoc.ts

amcasey · 2018-10-22T18:09:21Z

src/services/codefixes/inferFromUsage.ts

+            }
+        }
+        syntheticTags.forEach(v => tags.push(v));
+        const tag = createJSDocComment(comments.join("\n"), createNodeArray(tags));


"\n" [](start = 53, length = 4)

Some later step will replace this with a platform-specific line break?

Yes, the formatting code re-breaks lines and reindents them as well.

amcasey · 2018-10-22T18:10:43Z

tests/cases/fourslash/codeFixInferFromUsageExistingJSDoc.ts

+     * 1
+     * 2
+     * 3
+     * @returns {number} First one


@returns {number} First one [](start = 7, length = 27)

Based on our experience with imports, it seems likely that users will expect the merged JS Doc to be sorted (e.g. all of the @returnss together at the end).

Good idea. I filed #28090 to track that.

amcasey · 2018-10-22T18:11:02Z

I skimmed it and it seems sensible. Thanks!

In reply to: 431172674 [](ancestors = 431172674)

ghost

Should be a test where the codefix infers types for multiple parameters / return type of the same function, since that can cause issues with codeFixAll.

src/compiler/emitter.ts

ghost · 2018-10-22T17:49:59Z

src/compiler/emitter.ts

+        }
+
+        function emitJSDocAugmentsTag(tag: JSDocAugmentsTag) {
+            write("@extends");


Might be better to always use tagName since this might be @augments or @extends. Also you could share more code between these functions since you can always unconditionally emit tagName first rather than needing a different string literal for each tag kind.

src/compiler/emitter.ts

src/services/codefixes/inferFromUsage.ts

ghost · 2018-10-22T18:06:08Z

src/services/codefixes/inferFromUsage.ts

+                }
+                const typeTags = createMap<JSDocTypeTag | JSDocReturnTag>();
+                const typeTag = isGetAccessorDeclaration(declaration) ? createJSDocReturnTag(typeNode, "") : createJSDocTypeTag(typeNode, "");
+                typeTags.set("TAG", typeTag);


See earlier comment at "UNUSED"

src/services/utilities.ts

tests/cases/fourslash/codeFixInferFromUsageExistingJSDoc.ts

1. Refactor emit code into smaller functions. 2. Preceding-whitespace utility is slightly easier to use. 3. Better casts and types in inferFromUsage make it easier to read.

sandersn · 2018-10-24T17:17:45Z

I think this is ready for another look now.

src/compiler/emitter.ts

ghost · 2018-10-24T18:03:22Z

src/services/codefixes/inferFromUsage.ts

+                typeTags.set("TAG" as __String, typeTag);
+                insertMergedJSDoc(
+                    typeTags, changes, sourceFile, parent,
+                    t => t.kind === typeTag.kind && "TAG" as __String,


IMO using the type __String | false is just confusing -- I was beginning to question my knowledge of operator precedence. Would be more clear if getKey took __String | undefined and this were t.kind === typeTag.kind ? "TAG" : undefined.

ghost · 2018-10-24T18:04:31Z

src/services/codefixes/inferFromUsage.ts

+            return;
+        }
+        const paramTags = createUnderscoreEscapedMap<JSDocParameterTag>();
+        forEach(parameterInferences, inference => {


This is typed as a non-nullable array so this could just be a for loop? Also, this function never writes to parameterInferences so that could be a ReadonlyArray.

ghost · 2018-10-24T18:07:18Z

src/services/codefixes/inferFromUsage.ts

+                        const key = getKey(existing);
+                        const synthetic = key && syntheticTags.get(key);
+                        if (synthetic) {
+                            merge(synthetic, existing as T);


This might be simpler as tags.push(merge(synthetic, existing as T)); where we create a new synthetic node instead of mutating an existing one -- I believe there were plans to make nodes immutable in the future.

Also might want to comment the reason we push to tags here instead of doing it at the bottom -- looks like we're trying to preserve the original order of tags as much as possible.

ghost · 2018-10-24T18:09:14Z

src/services/codefixes/inferFromUsage.ts

+                        if (synthetic) {
+                            merge(synthetic, existing as T);
+                            tags.push(synthetic);
+                            syntheticTags.delete(key as __String);


I'm pretty sure we can avoid a cast here. See my earlier comment on getKey.

ghost · 2018-10-24T19:16:59Z

Here's my take on improving jsdoc comment merging:

function annotateJSDocParameters(changes: textChanges.ChangeTracker, sourceFile: SourceFile, parameterInferences: ReadonlyArray<ParameterInference>, program: Program, host: LanguageServiceHost): void {
    const signature = parameterInferences.length && parameterInferences[0].declaration.parent;
    if (!signature) {
        return;
    }
    const paramTags = mapDefined(group(parameterInferences, i => String(getNodeId(i.declaration))), inferences => {
        const param = first(inferences).declaration;
        // only infer parameters that have (1) no type and (2) an accessible inferred type
        if (param.initializer || getJSDocType(param) || !isIdentifier(param.name)) return;

        // TODO: improve combining inferences
        const type = firstDefined(inferences, i => i.type);
        const isOptional = inferences.some(i => !!i.isOptional);
        const typeNode = type && getTypeNodeIfAccessible(type, param, program, host);
        return typeNode && createJSDocParamTag(createJSDocTypeExpression(typeNode), param.name, isOptional, "");
    });
    addJsdocTags(changes, sourceFile, signature, paramTags);
}

function addJsdocTags(changes: textChanges.ChangeTracker, sourceFile: SourceFile, parent: HasJSDoc, newTags: ReadonlyArray<JSDocTag>): void {
    const comments = mapDefined(parent.jsDoc, j => j.comment);
    const oldTags = flatMap(parent.jsDoc, j => j.tags);
    const unmergedNewTags = newTags.filter(newTag => !oldTags || !oldTags.some((tag, i) => {
        const merged = tryMergeJsdocTags(tag, newTag);
        if (merged) oldTags[i] = merged;
        return !!merged;
    }));
    const tag = createJSDocComment(comments.join("\n"), createNodeArray([...(oldTags || emptyArray), ...unmergedNewTags]));
    changes.insertJsdocCommentBefore(sourceFile, parent, tag);
}

function tryMergeJsdocTags(oldTag: JSDocTag, newTag: JSDocTag): JSDocTag | undefined {
    if (oldTag.kind !== newTag.kind) return undefined;

    switch (oldTag.kind) {
        case SyntaxKind.JSDocParameterTag: {
            const oldParam = oldTag as JSDocParameterTag, newParam = newTag as JSDocParameterTag;
            return isIdentifier(oldParam.name) && isIdentifier(newParam.name) && oldParam.name.escapedText === newParam.name.escapedText
                ? createJSDocParamTag(newParam.typeExpression, newParam.name, newParam.isBracketed, oldParam.comment)
                : undefined;
        }
        case SyntaxKind.JSDocReturnTag:
            return createJSDocReturnTag((newTag as JSDocReturnTag).typeExpression, oldTag.comment);
        default:
            return undefined;
    }
}

addJSDocTags replaces insertMergedJSDoc with no map or callbacks needed. The caller just passes in the new tags as an array and the function is responsible for looking for matching existing tags and merging them, so it only needs 4 parameters. It also avoids modifying existing tags unless a new tag matches it. This is technically O(oldTags * newTags) but I expect the number of new tags is rarely more than 10.
This does break the test codeFixInferFromUsageMultipleParametersJS, but that test has two different parameters with the same name so there's no reason we should merge their tags.

src/compiler/factory.ts

sandersn · 2018-10-24T21:43:55Z

I like your suggestion, although I note that the group(parameterInferences, i => getNodeId(i.declaration), ...) doesn't make much sense, since each parameter maps to exactly 0 or 1 parameterInference. I think you can mapDefined(parameterInferences, ...) directly. I'll try that.

(Edit: And the order isn't changed from the original parameter list)

sandersn added 12 commits October 15, 2018 14:34

Correct indentation, using correct (I hope) indentation code

40cc33e

Note that part of the code, in formatting.ts, is cloned but should be extracted to a function instead.

Remove some possibly-superfluous code

87940b5

But I see 4 failures with whitespace, so perhaps not.

Restrict indentation change to avoid breaking baselines

edbe1c6

The indentation code is very complex so I'm just going to avoid breaking our single-line tests for now, plus add a simple jsdoc test to show that multiline jsdoc indentation isn't destroyed in the common case.

Switched over to construction for @return/@type

e18a4df

Still doesn't merge correctly though

Add @return tags to emitter

2ad4fa9

Merge multiple jsdocs

c917708

(not for @param yet)

Merge multiple jsdoc for parameters too

35b74d8

Emit more jsdoc tags

bba6538

Not all of them; I got cold feet since I'll have to write tests for them. I'll do that tomorrow.

Many fixes to JSDoc emit

a617cf5

And single tests (at least) for all tags

Cleanup in textChanges.ts

7593a48

Cleanup in formatting.ts

4f417d0

(Plus a little more in textChanges.ts)

Cleanup in inferFromUsage.ts

e1a2dfc

sandersn requested review from a user and amcasey October 18, 2018 21:36

sandersn added 2 commits October 18, 2018 14:44

Fix minor omissions

f69826c

Merge branch 'master' into infer-from-usage/existing-jsdoc-comments

1c56ab6

weswigham reviewed Oct 18, 2018

View reviewed changes

src/compiler/emitter.ts Show resolved Hide resolved

weswigham reviewed Oct 19, 2018

View reviewed changes

tests/cases/fourslash/codeFixInferFromUsageExistingJSDoc.ts Outdated Show resolved Hide resolved

Separate merged top-level JSDoc comments with \n

e513f17

instead of space.

amcasey reviewed Oct 22, 2018

View reviewed changes

tests/cases/fourslash/codeFixInferFromUsageExistingJSDoc.ts Show resolved Hide resolved

amcasey reviewed Oct 22, 2018

View reviewed changes

ghost reviewed Oct 22, 2018

View reviewed changes

Merge branch 'master' into infer-from-usage/existing-jsdoc-comments

24fd7ec

sandersn mentioned this pull request Oct 23, 2018

JSDoc generated by infer-from-usage codefix should be sorted #28090

Open

sandersn added 3 commits October 23, 2018 16:31

Don't delete intrusive non-jsdoc comments

54df72f

Cleanup from PR comments

236eec3

1. Refactor emit code into smaller functions. 2. Preceding-whitespace utility is slightly easier to use. 3. Better casts and types in inferFromUsage make it easier to read.

Fix bogus newline

9457ebb

ghost reviewed Oct 24, 2018

View reviewed changes

src/compiler/factory.ts Outdated Show resolved Hide resolved

src/compiler/factory.ts Outdated Show resolved Hide resolved

Merge branch 'master' into infer-from-usage/existing-jsdoc-comments

163894f

Use @Andy-MS' cleanup annotateJSDocParameters

1968160

ghost approved these changes Oct 24, 2018

View reviewed changes

ghost mentioned this pull request Oct 24, 2018

Remove unused property ParameterInference#typeNode #28115

Merged

Merge branch 'master' into infer-from-usage/existing-jsdoc-comments

2b4f14f

sandersn merged commit fe2a33f into master Oct 24, 2018

sandersn deleted the infer-from-usage/existing-jsdoc-comments branch October 24, 2018 23:14

jessetrinity mentioned this pull request Nov 30, 2018

Visual Studio - Format on Enter no longer removes extra whitespace #28781

Closed

sandersn mentioned this pull request Dec 4, 2018

Revert over-eager bailout when trimming trailing whitespace #28848

Merged

swyxio mentioned this pull request May 21, 2019

[Minor bug] Comment merging should be joined by a newline #31492

Closed

Merge existing JSDoc comments #27978

Merge existing JSDoc comments #27978

Uh oh!

Conversation

sandersn commented Oct 18, 2018

Uh oh!

sandersn commented Oct 18, 2018

Uh oh!

Uh oh!

Uh oh!

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!

amcasey commented Oct 22, 2018

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sandersn commented Oct 24, 2018

Uh oh!

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 24, 2018

Uh oh!

Uh oh!

Uh oh!

sandersn commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

sandersn commented Oct 24, 2018 •

edited

Loading