Merge existing JSDoc comments (#27978)

* Correct indentation, using correct (I hope) indentation code

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

* Remove some possibly-superfluous code

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

* Restrict indentation change to avoid breaking baselines

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

Still doesn't merge correctly though

* Add @return tags to emitter

* Merge multiple jsdocs

(not for @param yet)

* Merge multiple jsdoc for parameters too

* Emit more jsdoc tags

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

And single tests (at least) for all tags

* Cleanup in textChanges.ts

* Cleanup in formatting.ts

(Plus a little more in textChanges.ts)

* Cleanup in inferFromUsage.ts

* Fix minor omissions

* Separate merged top-level JSDoc comments with \n

instead of space.

* Don't delete intrusive non-jsdoc comments

* Cleanup from PR comments

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

* Use @andy-ms' cleanup annotateJSDocParameters

src/services/codefixes/inferFromUsage.ts

@@ -73,7 +73,9 @@ namespace ts.codefix {
                     const type = inferTypeForVariableFromUsage(parent.name, program, cancellationToken);
                     const typeNode = type && getTypeNodeIfAccessible(type, parent, program, host);
                     if (typeNode) {
-                        changes.tryInsertJSDocType(sourceFile, parent, typeNode);
+                        // Note that the codefix will never fire with an existing `@type` tag, so there is no need to merge tags
+                        const typeTag = createJSDocTypeTag(createJSDocTypeExpression(typeNode), /*comment*/ "");
+                        addJSDocTags(changes, sourceFile, cast(parent.parent.parent, isExpressionStatement), [typeTag]);
                     }
                     return parent;
                 }
@@ -192,7 +194,13 @@ namespace ts.codefix {
         const typeNode = type && getTypeNodeIfAccessible(type, declaration, program, host);
         if (typeNode) {
             if (isInJSFile(sourceFile) && declaration.kind !== SyntaxKind.PropertySignature) {
-                changes.tryInsertJSDocType(sourceFile, declaration, typeNode);
+                const parent = isVariableDeclaration(declaration) ? tryCast(declaration.parent.parent, isVariableStatement) : declaration;
+                if (!parent) {
+                    return;
+                }
+                const typeExpression = createJSDocTypeExpression(typeNode);
+                const typeTag = isGetAccessorDeclaration(declaration) ? createJSDocReturnTag(typeExpression, "") : createJSDocTypeTag(typeExpression, "");
+                addJSDocTags(changes, sourceFile, parent, [typeTag]);
             }
             else {
                 changes.tryInsertTypeAnnotation(sourceFile, declaration, typeNode);
@@ -200,16 +208,52 @@ namespace ts.codefix {
         }
     }
 
-    function annotateJSDocParameters(changes: textChanges.ChangeTracker, sourceFile: SourceFile, parameterInferences: ParameterInference[], program: Program, host: LanguageServiceHost): void {
-        const result = mapDefined(parameterInferences, inference => {
+    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(parameterInferences, inference => {
             const param = inference.declaration;
+            // only infer parameters that have (1) no type and (2) an accessible inferred type
+            if (param.initializer || getJSDocType(param) || !isIdentifier(param.name)) return;
+
             const typeNode = inference.type && getTypeNodeIfAccessible(inference.type, param, program, host);
-            return typeNode && !param.initializer && !getJSDocType(param) ? { ...inference, typeNode } : undefined;
+            return typeNode && createJSDocParamTag(param.name, !!inference.isOptional, createJSDocTypeExpression(typeNode), "");
         });
-        changes.tryInsertJSDocParameters(sourceFile, result);
+        addJSDocTags(changes, sourceFile, signature, paramTags);
     }
 
-   function getTypeNodeIfAccessible(type: Type, enclosingScope: Node, program: Program, host: LanguageServiceHost): TypeNode | undefined {
+    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;
+                const newParam = newTag as JSDocParameterTag;
+                return isIdentifier(oldParam.name) && isIdentifier(newParam.name) && oldParam.name.escapedText === newParam.name.escapedText
+                    ? createJSDocParamTag(newParam.name, newParam.isBracketed, newParam.typeExpression, oldParam.comment)
+                    : undefined;
+            }
+            case SyntaxKind.JSDocReturnTag:
+                return createJSDocReturnTag((newTag as JSDocReturnTag).typeExpression, oldTag.comment);
+        }
+    }
+
+    function getTypeNodeIfAccessible(type: Type, enclosingScope: Node, program: Program, host: LanguageServiceHost): TypeNode | undefined {
         const checker = program.getTypeChecker();
         let typeIsAccessible = true;
         const notAccessible = () => { typeIsAccessible = false; };

src/services/formatting/formatting.ts

@@ -365,16 +365,21 @@ namespace ts.formatting {
     function formatSpan(originalRange: TextRange, sourceFile: SourceFile, formatContext: FormatContext, requestKind: FormattingRequestKind): TextChange[] {
         // find the smallest node that fully wraps the range and compute the initial indentation for the node
         const enclosingNode = findEnclosingNode(originalRange, sourceFile);
-        return getFormattingScanner(sourceFile.text, sourceFile.languageVariant, getScanStartPosition(enclosingNode, originalRange, sourceFile), originalRange.end, scanner => formatSpanWorker(
-            originalRange,
-            enclosingNode,
-            SmartIndenter.getIndentationForNode(enclosingNode, originalRange, sourceFile, formatContext.options),
-            getOwnOrInheritedDelta(enclosingNode, formatContext.options, sourceFile),
-            scanner,
-            formatContext,
-            requestKind,
-            prepareRangeContainsErrorFunction(sourceFile.parseDiagnostics, originalRange),
-            sourceFile));
+        return getFormattingScanner(
+            sourceFile.text,
+            sourceFile.languageVariant,
+            getScanStartPosition(enclosingNode, originalRange, sourceFile),
+            originalRange.end,
+            scanner => formatSpanWorker(
+                originalRange,
+                enclosingNode,
+                SmartIndenter.getIndentationForNode(enclosingNode, originalRange, sourceFile, formatContext.options),
+                getOwnOrInheritedDelta(enclosingNode, formatContext.options, sourceFile),
+                scanner,
+                formatContext,
+                requestKind,
+                prepareRangeContainsErrorFunction(sourceFile.parseDiagnostics, originalRange),
+                sourceFile));
     }
 
     function formatSpanWorker(originalRange: TextRange,
@@ -413,7 +418,8 @@ namespace ts.formatting {
         if (!formattingScanner.isOnToken()) {
             const leadingTrivia = formattingScanner.getCurrentLeadingTrivia();
             if (leadingTrivia) {
-                processTrivia(leadingTrivia, enclosingNode, enclosingNode, /*dynamicIndentation*/ undefined!); // TODO: GH#18217
+                indentTriviaItems(leadingTrivia, initialIndentation, /*indentNextTokenOrTrivia*/ false,
+                                  item => processRange(item, sourceFile.getLineAndCharacterOfPosition(item.pos), enclosingNode, enclosingNode, /*dynamicIndentation*/ undefined!));
                 trimTrailingWhitespacesForRemainingRange();
             }
         }
@@ -814,27 +820,8 @@ namespace ts.formatting {
                     let indentNextTokenOrTrivia = true;
                     if (currentTokenInfo.leadingTrivia) {
                         const commentIndentation = dynamicIndentation.getIndentationForComment(currentTokenInfo.token.kind, tokenIndentation, container);
-
-                        for (const triviaItem of currentTokenInfo.leadingTrivia) {
-                            const triviaInRange = rangeContainsRange(originalRange, triviaItem);
-                            switch (triviaItem.kind) {
-                                case SyntaxKind.MultiLineCommentTrivia:
-                                    if (triviaInRange) {
-                                        indentMultilineCommentOrJsxText(triviaItem, commentIndentation, /*firstLineIsIndented*/ !indentNextTokenOrTrivia);
-                                    }
-                                    indentNextTokenOrTrivia = false;
-                                    break;
-                                case SyntaxKind.SingleLineCommentTrivia:
-                                    if (indentNextTokenOrTrivia && triviaInRange) {
-                                        insertIndentation(triviaItem.pos, commentIndentation, /*lineAdded*/ false);
-                                    }
-                                    indentNextTokenOrTrivia = false;
-                                    break;
-                                case SyntaxKind.NewLineTrivia:
-                                    indentNextTokenOrTrivia = true;
-                                    break;
-                            }
-                        }
+                        indentNextTokenOrTrivia = indentTriviaItems(currentTokenInfo.leadingTrivia, commentIndentation, indentNextTokenOrTrivia,
+                                                                    item => insertIndentation(item.pos, commentIndentation, /*lineAdded*/ false));
                     }
 
                     // indent token only if is it is in target range and does not overlap with any error ranges
@@ -852,6 +839,34 @@ namespace ts.formatting {
             }
         }
 
+        function indentTriviaItems(
+            trivia: TextRangeWithKind[],
+            commentIndentation: number,
+            indentNextTokenOrTrivia: boolean,
+            indentSingleLine: (item: TextRangeWithKind) => void) {
+            for (const triviaItem of trivia) {
+                const triviaInRange = rangeContainsRange(originalRange, triviaItem);
+                switch (triviaItem.kind) {
+                    case SyntaxKind.MultiLineCommentTrivia:
+                        if (triviaInRange) {
+                            indentMultilineCommentOrJsxText(triviaItem, commentIndentation, /*firstLineIsIndented*/ !indentNextTokenOrTrivia);
+                        }
+                        indentNextTokenOrTrivia = false;
+                        break;
+                    case SyntaxKind.SingleLineCommentTrivia:
+                        if (indentNextTokenOrTrivia && triviaInRange) {
+                            indentSingleLine(triviaItem);
+                        }
+                        indentNextTokenOrTrivia = false;
+                        break;
+                    case SyntaxKind.NewLineTrivia:
+                        indentNextTokenOrTrivia = true;
+                        break;
+                }
+            }
+            return indentNextTokenOrTrivia;
+        }
+
         function processTrivia(trivia: TextRangeWithKind[], parent: Node, contextNode: Node, dynamicIndentation: DynamicIndentation): void {
             for (const triviaItem of trivia) {
                 if (isComment(triviaItem.kind) && rangeContainsRange(originalRange, triviaItem)) {
@@ -861,7 +876,6 @@ namespace ts.formatting {
             }
         }
 
-        // TODO: GH#18217 use an enum instead of `boolean | undefined`
         function processRange(range: TextRangeWithKind,
             rangeStart: LineAndCharacter,
             parent: Node,
@@ -1072,7 +1086,10 @@ namespace ts.formatting {
          * Trimming will be done for lines after the previous range
          */
         function trimTrailingWhitespacesForRemainingRange() {
-            const startPosition = previousRange ? previousRange.end : originalRange.pos;
+            if (!previousRange) {
+                return;
+            }
+            const startPosition = previousRange.end;
 
             const startLine = sourceFile.getLineAndCharacterOfPosition(startPosition).line;
             const endLine = sourceFile.getLineAndCharacterOfPosition(originalRange.end).line;

src/services/formatting/smartIndenter.ts

@@ -79,14 +79,14 @@ namespace ts.formatting {
                 return findFirstNonWhitespaceColumn(getStartPositionOfLine(commentStartLine, sourceFile), position, sourceFile, options);
             }
 
-            const startPostionOfLine = getStartPositionOfLine(previousLine, sourceFile);
-            const { column, character } = findFirstNonWhitespaceCharacterAndColumn(startPostionOfLine, position, sourceFile, options);
+            const startPositionOfLine = getStartPositionOfLine(previousLine, sourceFile);
+            const { column, character } = findFirstNonWhitespaceCharacterAndColumn(startPositionOfLine, position, sourceFile, options);
 
             if (column === 0) {
                 return column;
             }
 
-            const firstNonWhitespaceCharacterCode = sourceFile.text.charCodeAt(startPostionOfLine + character);
+            const firstNonWhitespaceCharacterCode = sourceFile.text.charCodeAt(startPositionOfLine + character);
             return firstNonWhitespaceCharacterCode === CharacterCodes.asterisk ? column - 1 : column;
         }
 

src/services/textChanges.ts

@@ -209,12 +209,6 @@ namespace ts.textChanges {
 
     export type TypeAnnotatable = SignatureDeclaration | VariableDeclaration | ParameterDeclaration | PropertyDeclaration | PropertySignature;
 
-    interface JSDocParameter {
-        declaration: ParameterDeclaration;
-        typeNode: TypeNode;
-        isOptional?: boolean;
-    }
-
     export class ChangeTracker {
         private readonly changes: Change[] = [];
         private readonly newFiles: { readonly oldFile: SourceFile | undefined, readonly fileName: string, readonly statements: ReadonlyArray<Statement> }[] = [];
@@ -345,10 +339,19 @@ namespace ts.textChanges {
             this.insertText(sourceFile, token.getStart(sourceFile), text);
         }
 
-        public insertCommentThenNewline(sourceFile: SourceFile, character: number, position: number, commentText: string): void {
-            const token = getTouchingToken(sourceFile, position);
-            const text = "/**" + commentText + "*/" + this.newLineCharacter + repeatString(" ", character);
-            this.insertText(sourceFile, token.getStart(sourceFile), text);
+        public insertJsdocCommentBefore(sourceFile: SourceFile, node: HasJSDoc, tag: JSDoc) {
+            const fnStart = node.getStart(sourceFile);
+            if (node.jsDoc) {
+                for (const jsdoc of node.jsDoc) {
+                    this.deleteRange(sourceFile, {
+                        pos: getLineStartPositionForPosition(jsdoc.getStart(sourceFile), sourceFile),
+                        end: getAdjustedEndPosition(sourceFile, jsdoc, /*options*/ {})
+                    });
+                }
+            }
+            const startPosition = getPrecedingNonSpaceCharacterPosition(sourceFile.text, fnStart - 1);
+            const indent = sourceFile.text.slice(startPosition, fnStart);
+            this.insertNodeAt(sourceFile, fnStart, tag, { preserveLeadingWhitespace: false, suffix: this.newLineCharacter + indent });
         }
 
         public replaceRangeWithText(sourceFile: SourceFile, range: TextRange, text: string) {
@@ -359,23 +362,6 @@ namespace ts.textChanges {
             this.replaceRangeWithText(sourceFile, createRange(pos), text);
         }
 
-        public tryInsertJSDocParameters(sourceFile: SourceFile, parameters: JSDocParameter[]) {
-            if (parameters.length === 0) {
-                return;
-            }
-            const parent = parameters[0].declaration.parent;
-            const indent = getLineAndCharacterOfPosition(sourceFile, parent.getStart()).character;
-            let commentText = "\n";
-            for (const { declaration, typeNode, isOptional } of parameters) {
-                if (isIdentifier(declaration.name)) {
-                    const printed = changesToText.getNonformattedText(typeNode, sourceFile, this.newLineCharacter).text;
-                    commentText += this.printJSDocParameter(indent, printed, declaration.name, isOptional);
-                }
-            }
-            commentText += repeatString(" ", indent + 1);
-            this.insertCommentThenNewline(sourceFile, indent, parent.getStart(), commentText);
-        }
-
         /** Prefer this over replacing a node with another that has a type annotation, as it avoids reformatting the other parts of the node. */
         public tryInsertTypeAnnotation(sourceFile: SourceFile, node: TypeAnnotatable, type: TypeNode): void {
             let endNode: Node | undefined;
@@ -394,27 +380,6 @@ namespace ts.textChanges {
             this.insertNodeAt(sourceFile, endNode.end, type, { prefix: ": " });
         }
 
-        public tryInsertJSDocType(sourceFile: SourceFile, node: Node, type: TypeNode): void {
-            const printed = changesToText.getNonformattedText(type, sourceFile, this.newLineCharacter).text;
-            let commentText;
-            if (isGetAccessorDeclaration(node)) {
-                commentText = ` @return {${printed}} `;
-            }
-            else {
-                commentText = ` @type {${printed}} `;
-                node = node.parent;
-            }
-            this.insertCommentThenNewline(sourceFile, getLineAndCharacterOfPosition(sourceFile, node.getStart(sourceFile)).character, node.getStart(sourceFile), commentText);
-        }
-
-        private printJSDocParameter(indent: number, printed: string, name: Identifier, isOptionalParameter: boolean | undefined) {
-            let printName = unescapeLeadingUnderscores(name.escapedText);
-            if (isOptionalParameter) {
-                printName = `[${printName}]`;
-            }
-            return repeatString(" ", indent) + ` * @param {${printed}} ${printName}\n`;
-        }
-
         public insertTypeParameters(sourceFile: SourceFile, node: SignatureDeclaration, typeParameters: ReadonlyArray<TypeParameterDeclaration>): void {
             // If no `(`, is an arrow function `x => x`, so use the pos of the first parameter
             const start = (findChildOfKind(node, SyntaxKind.OpenParenToken, sourceFile) || first(node.parameters)).getStart(sourceFile);

src/services/utilities.ts

@@ -1672,6 +1672,13 @@ namespace ts {
         return position;
     }
 
+    export function getPrecedingNonSpaceCharacterPosition(text: string, position: number) {
+        while (position > -1 && isWhiteSpaceSingleLine(text.charCodeAt(position))) {
+            position -= 1;
+        }
+        return position + 1;
+    }
+
     /**
      * Creates a deep, memberwise clone of a node with no source map location.
      *