More refactoring tests, some comment preservation and some fixed form…

…atting of multiline tuples

src/compiler/emitter.ts

@@ -4980,7 +4980,7 @@ namespace ts {
         }
 
         function emitLeadingSynthesizedComment(comment: SynthesizedComment) {
-            if (comment.kind === SyntaxKind.SingleLineCommentTrivia) {
+            if (comment.hasLeadingNewline || comment.kind === SyntaxKind.SingleLineCommentTrivia) {
                 writer.writeLine();
             }
             writeSynthesizedComment(comment);

src/compiler/types.ts

@@ -2600,6 +2600,7 @@ namespace ts {
         text: string;
         pos: -1;
         end: -1;
+        hasLeadingNewline?: boolean;
     }
 
     // represents a top level: { type } expression in a JSDoc comment.

src/services/formatting/smartIndenter.ts

@@ -574,7 +574,7 @@ namespace ts.formatting {
                     return childKind !== SyntaxKind.JsxClosingFragment;
                 case SyntaxKind.IntersectionType:
                 case SyntaxKind.UnionType:
-                    if (childKind === SyntaxKind.TypeLiteral) {
+                    if (childKind === SyntaxKind.TypeLiteral || childKind === SyntaxKind.TupleType) {
                         return false;
                     }
                     // falls through

src/services/refactors/convertOverloadListToSingleSignature.ts

@@ -25,6 +25,8 @@ namespace ts.refactor.addOrRemoveBracesToArrowFunction {
         const signatureDecls = getConvertableOverloadListAtPosition(file, startPosition, program);
         if (!signatureDecls) return undefined;
 
+        const checker = program.getTypeChecker();
+
         const lastDeclaration = signatureDecls[signatureDecls.length - 1];
         let updated = lastDeclaration;
         switch (lastDeclaration.kind) {
@@ -39,6 +41,21 @@ namespace ts.refactor.addOrRemoveBracesToArrowFunction {
                 );
                 break;
             }
+            case SyntaxKind.MethodDeclaration: {
+                updated = updateMethod(
+                    lastDeclaration,
+                    lastDeclaration.decorators,
+                    lastDeclaration.modifiers,
+                    lastDeclaration.asteriskToken,
+                    lastDeclaration.name,
+                    lastDeclaration.questionToken,
+                    lastDeclaration.typeParameters,
+                    getNewParametersForCombinedSignature(signatureDecls),
+                    lastDeclaration.type,
+                    lastDeclaration.body
+                );
+                break;
+            }
             case SyntaxKind.CallSignature: {
                 updated = updateCallSignature(
                     lastDeclaration,
@@ -48,6 +65,16 @@ namespace ts.refactor.addOrRemoveBracesToArrowFunction {
                 );
                 break;
             }
+            case SyntaxKind.Constructor: {
+                updated = updateConstructor(
+                    lastDeclaration,
+                    lastDeclaration.decorators,
+                    lastDeclaration.modifiers,
+                    getNewParametersForCombinedSignature(signatureDecls),
+                    lastDeclaration.body
+                );
+                break;
+            }
             case SyntaxKind.ConstructSignature: {
                 updated = updateConstructSignature(
                     lastDeclaration,
@@ -84,7 +111,12 @@ namespace ts.refactor.addOrRemoveBracesToArrowFunction {
 
         return { renameFilename: undefined, renameLocation: undefined, edits };
 
-        function getNewParametersForCombinedSignature(signatureDeclarations: (MethodSignature | CallSignatureDeclaration | ConstructSignatureDeclaration | FunctionDeclaration)[]): NodeArray<ParameterDeclaration> {
+        function getNewParametersForCombinedSignature(signatureDeclarations: (MethodSignature | MethodDeclaration | CallSignatureDeclaration | ConstructorDeclaration | ConstructSignatureDeclaration | FunctionDeclaration)[]): NodeArray<ParameterDeclaration> {
+            const lastSig = signatureDeclarations[signatureDeclarations.length - 1];
+            if (isFunctionLikeDeclaration(lastSig) && lastSig.body) {
+                // Trim away implementation signature arguments (they should already be compatible with overloads, but are likely less precise to guarantee compatability with the overloads)
+                signatureDeclarations = signatureDeclarations.slice(0, signatureDeclarations.length - 1);
+            }
             return createNodeArray([
                 createParameter(
                     /*decorators*/ undefined,
@@ -97,24 +129,56 @@ namespace ts.refactor.addOrRemoveBracesToArrowFunction {
             ]);
         }
 
-        function convertSignatureParametersToTuple(decl: MethodSignature | CallSignatureDeclaration | ConstructSignatureDeclaration | FunctionDeclaration): TupleTypeNode {
-            return setEmitFlags(createTupleTypeNode(map(decl.parameters, convertParameterToNamedTupleMember)), EmitFlags.SingleLine);
+        function convertSignatureParametersToTuple(decl: MethodSignature | MethodDeclaration | CallSignatureDeclaration | ConstructorDeclaration | ConstructSignatureDeclaration | FunctionDeclaration): TupleTypeNode {
+            const members = map(decl.parameters, convertParameterToNamedTupleMember);
+            return setEmitFlags(createTupleTypeNode(members), some(members, m => !!length(getSyntheticLeadingComments(m))) ? EmitFlags.None : EmitFlags.SingleLine);
         }
 
         function convertParameterToNamedTupleMember(p: ParameterDeclaration): NamedTupleMember {
             Debug.assert(isIdentifier(p.name)); // This is checked during refactoring applicability checking
-            return setTextRange(createNamedTupleMember(
+            const result = setTextRange(createNamedTupleMember(
                 p.dotDotDotToken,
                 p.name,
                 p.questionToken,
                 p.type || createKeywordTypeNode(SyntaxKind.AnyKeyword)
             ), p);
+            const parameterDocComment = p.symbol && p.symbol.getDocumentationComment(checker);
+            if (parameterDocComment) {
+                const newComment = displayPartsToString(parameterDocComment);
+                if (newComment.length) {
+                    setSyntheticLeadingComments(result, [{
+                        text: `*
+${newComment.split("\n").map(c => ` * ${c}`).join("\n")}
+ `,
+                        kind: SyntaxKind.MultiLineCommentTrivia,
+                        pos: -1,
+                        end: -1,
+                        hasTrailingNewLine: true,
+                        hasLeadingNewline: true,
+                    }]);
+                }
+            }
+            return result;
         }
+
+    }
+
+    function isConvertableSignatureDeclaration(d: Node): d is MethodSignature | MethodDeclaration | CallSignatureDeclaration | ConstructorDeclaration | ConstructSignatureDeclaration | FunctionDeclaration {
+        switch (d.kind) {
+            case SyntaxKind.MethodSignature:
+            case SyntaxKind.MethodDeclaration:
+            case SyntaxKind.CallSignature:
+            case SyntaxKind.Constructor:
+            case SyntaxKind.ConstructSignature:
+            case SyntaxKind.FunctionDeclaration:
+                return true;
+        }
+        return false;
     }
 
     function getConvertableOverloadListAtPosition(file: SourceFile, startPosition: number, program: Program) {
         const node = getTokenAtPosition(file, startPosition);
-        const containingDecl = findAncestor(node, n => isFunctionLikeDeclaration(n));
+        const containingDecl = findAncestor(node, isConvertableSignatureDeclaration);
         if (!containingDecl) {
             return;
         }
@@ -130,14 +194,14 @@ namespace ts.refactor.addOrRemoveBracesToArrowFunction {
         if (!every(decls, d => getSourceFileOfNode(d) === file)) {
             return;
         }
-        const kindOne = decls[0].kind;
-        if (kindOne !== SyntaxKind.MethodSignature && kindOne !== SyntaxKind.CallSignature && kindOne !== SyntaxKind.ConstructSignature && kindOne !== SyntaxKind.FunctionDeclaration) {
+        if (!isConvertableSignatureDeclaration(decls[0])) {
             return;
         }
+        const kindOne = decls[0].kind;
         if (!every(decls, d => d.kind === kindOne)) {
             return;
         }
-        const signatureDecls = decls as (MethodSignature | CallSignatureDeclaration | ConstructSignatureDeclaration | FunctionDeclaration)[];
+        const signatureDecls = decls as (MethodSignature | MethodDeclaration | CallSignatureDeclaration | ConstructorDeclaration | ConstructSignatureDeclaration | FunctionDeclaration)[];
         if (some(signatureDecls, d => !!d.typeParameters || some(d.parameters, p => !!p.decorators || !!p.modifiers || !isIdentifier(p.name)))) {
             return;
         }

tests/baselines/reference/api/tsserverlibrary.d.ts

@@ -1586,6 +1586,7 @@ declare namespace ts {
         text: string;
         pos: -1;
         end: -1;
+        hasLeadingNewline?: boolean;
     }
     export interface JSDocTypeExpression extends TypeNode {
         kind: SyntaxKind.JSDocTypeExpression;

tests/baselines/reference/api/typescript.d.ts

@@ -1586,6 +1586,7 @@ declare namespace ts {
         text: string;
         pos: -1;
         end: -1;
+        hasLeadingNewline?: boolean;
     }
     export interface JSDocTypeExpression extends TypeNode {
         kind: SyntaxKind.JSDocTypeExpression;

tests/cases/fourslash/refactorOverloadListToSingleSignature2.ts

@@ -20,30 +20,30 @@ edit.applyRefactor({
     refactorName: "Convert overload list to single signature",
     actionName: "Convert overload list to single signature",
     actionDescription: ts.Diagnostics.Convert_overload_list_to_single_signature.message,
-// Aspirational:
-//    newContent: `declare function foo(...args: [] | [
-//    /**
-//     * a string param doc
-//     */
-//    a: string
-//] | [
-//    /**
-//     * a number param doc
-//     */
-//    a: number,
-//    /**
-//     * b number param doc
-//     */
-//    b: number
-//] | [
-//    /**
-//     * rest param doc
-//     */
-//    ...rest: symbol[]
-//]): void;`,
-// Actual:
-    newContent: `/**
+// we don't delete the param comment on the signature we update because deleting *part* of a comment is... hard
+// and we definitely don't want to delete the whole comment. This is probably a good argument for why jsdoc should
+// really be uniformly handled as AST nodes, and transformed as such :(
+newContent: `/**
  * @param rest rest param doc
  */
-declare function foo(...args: [] | [a: string] | [a: number, b: number] | [...rest: symbol[]]): void;`
+declare function foo(...args: [] | [
+    /**
+     * a string param doc
+     */
+    a: string
+] | [
+    /**
+     * a number param doc
+     */
+    a: number,
+    /**
+     * b number param doc
+     */
+    b: number
+] | [
+    /**
+     * rest param doc
+     */
+    ...rest: symbol[]
+]): void;`,
 });

tests/cases/fourslash/refactorOverloadListToSingleSignature3.ts

@@ -0,0 +1,19 @@
+/// <reference path='fourslash.ts' />
+
+/////*a*/function foo(): void;
+////function foo(a: string): void;
+////function foo(a: number, b: number): void;
+////function foo(...rest: symbol[]): void;/*b*/
+////function foo(...args: any[]): void {
+////  // body
+////}
+
+goTo.select("a", "b");
+edit.applyRefactor({
+    refactorName: "Convert overload list to single signature",
+    actionName: "Convert overload list to single signature",
+    actionDescription: ts.Diagnostics.Convert_overload_list_to_single_signature.message,
+    newContent: `function foo(...args: [] | [a: string] | [a: number, b: number] | [...rest: symbol[]]): void {
+    // body
+}`,
+});

tests/cases/fourslash/refactorOverloadListToSingleSignature4.ts

@@ -0,0 +1,23 @@
+/// <reference path='fourslash.ts' />
+
+////class A {
+////    /*a*/foo(): void;
+////    foo(a: string): void;
+////    foo(a: number, b: number): void;
+////    foo(...rest: symbol[]): void;/*b*/
+////    foo(...args: any[]): void {
+////      // body
+////    }
+////}
+
+goTo.select("a", "b");
+edit.applyRefactor({
+    refactorName: "Convert overload list to single signature",
+    actionName: "Convert overload list to single signature",
+    actionDescription: ts.Diagnostics.Convert_overload_list_to_single_signature.message,
+    newContent: `class A {
+    foo(...args: [] | [a: string] | [a: number, b: number] | [...rest: symbol[]]): void {
+        // body
+    }
+}`,
+});

tests/cases/fourslash/refactorOverloadListToSingleSignature5.ts

@@ -0,0 +1,23 @@
+/// <reference path='fourslash.ts' />
+
+////class A {
+////    /*a*/constructor();
+////    constructor(a: string);
+////    constructor(a: number, b: number);
+////    constructor(...rest: symbol[]);/*b*/
+////    constructor(...args: any[]) {
+////      // body
+////    }
+////}
+
+goTo.select("a", "b");
+edit.applyRefactor({
+    refactorName: "Convert overload list to single signature",
+    actionName: "Convert overload list to single signature",
+    actionDescription: ts.Diagnostics.Convert_overload_list_to_single_signature.message,
+    newContent: `class A {
+    constructor(...args: [] | [a: string] | [a: number, b: number] | [...rest: symbol[]]) {
+        // body
+    }
+}`,
+});

tests/cases/fourslash/refactorOverloadListToSingleSignature6.ts

@@ -0,0 +1,18 @@
+/// <reference path='fourslash.ts' />
+
+////interface A {
+////    /*a*/(): void;
+////    (a: string): void;
+////    (a: number, b: number): void;
+////    (...rest: symbol[]): void;/*b*/
+////}
+
+goTo.select("a", "b");
+edit.applyRefactor({
+    refactorName: "Convert overload list to single signature",
+    actionName: "Convert overload list to single signature",
+    actionDescription: ts.Diagnostics.Convert_overload_list_to_single_signature.message,
+    newContent: `interface A {
+    (...args: [] | [a: string] | [a: number, b: number] | [...rest: symbol[]]): void;
+}`,
+});

tests/cases/fourslash/refactorOverloadListToSingleSignature7.ts

@@ -0,0 +1,18 @@
+/// <reference path='fourslash.ts' />
+
+////interface A {
+////    /*a*/new (): void;
+////    new (a: string): void;
+////    new (a: number, b: number): void;
+////    new (...rest: symbol[]): void;/*b*/
+////}
+
+goTo.select("a", "b");
+edit.applyRefactor({
+    refactorName: "Convert overload list to single signature",
+    actionName: "Convert overload list to single signature",
+    actionDescription: ts.Diagnostics.Convert_overload_list_to_single_signature.message,
+    newContent: `interface A {
+    new(...args: [] | [a: string] | [a: number, b: number] | [...rest: symbol[]]): void;
+}`,
+});

tests/cases/fourslash/refactorOverloadListToSingleSignature8.ts

@@ -0,0 +1,18 @@
+/// <reference path='fourslash.ts' />
+
+////interface A {
+////    /*a*/foo(): void;
+////    foo(a: string): void;
+////    foo(a: number, b: number): void;
+////    foo(...rest: symbol[]): void;/*b*/
+////}
+
+goTo.select("a", "b");
+edit.applyRefactor({
+    refactorName: "Convert overload list to single signature",
+    actionName: "Convert overload list to single signature",
+    actionDescription: ts.Diagnostics.Convert_overload_list_to_single_signature.message,
+    newContent: `interface A {
+    foo(...args: [] | [a: string] | [a: number, b: number] | [...rest: symbol[]]): void;
+}`,
+});