convertToEs6Module: Avoid replacing entire function (#22507)

* convertToEs6Module: Avoid replacing entire function

* Code review

* Fix typo

src/services/codefixes/convertToEs6Module.ts

@@ -114,8 +114,8 @@ namespace ts.codefix {
                         return false;
                     }
                     case SyntaxKind.BinaryExpression: {
-                        const { left, operatorToken, right } = expression as BinaryExpression;
-                        return operatorToken.kind === SyntaxKind.EqualsToken && convertAssignment(sourceFile, checker, statement as ExpressionStatement, left, right, changes, exports);
+                        const { operatorToken } = expression as BinaryExpression;
+                        return operatorToken.kind === SyntaxKind.EqualsToken && convertAssignment(sourceFile, checker, expression as BinaryExpression, changes, exports);
                     }
                 }
             }
@@ -144,7 +144,7 @@ namespace ts.codefix {
                 return convertPropertyAccessImport(name, initializer.name.text, initializer.expression.arguments[0], identifiers);
             }
             else {
-                // Move it out to its own variable statement.
+                // Move it out to its own variable statement. (This will not be used if `!foundImport`)
                 return createVariableStatement(/*modifiers*/ undefined, createVariableDeclarationList([decl], declarationList.flags));
             }
         });
@@ -177,12 +177,11 @@ namespace ts.codefix {
     function convertAssignment(
         sourceFile: SourceFile,
         checker: TypeChecker,
-        statement: ExpressionStatement,
-        left: Expression,
-        right: Expression,
+        assignment: BinaryExpression,
         changes: textChanges.ChangeTracker,
         exports: ExportRenames,
     ): ModuleExportsChanged {
+        const { left, right } = assignment;
         if (!isPropertyAccessExpression(left)) {
             return false;
         }
@@ -190,7 +189,7 @@ namespace ts.codefix {
         if (isExportsOrModuleExportsOrAlias(sourceFile, left)) {
             if (isExportsOrModuleExportsOrAlias(sourceFile, right)) {
                 // `const alias = module.exports;` or `module.exports = alias;` can be removed.
-                changes.deleteNode(sourceFile, statement);
+                changes.deleteNode(sourceFile, assignment.parent);
             }
             else {
                 let newNodes = isObjectLiteralExpression(right) ? tryChangeModuleExportsObject(right) : undefined;
@@ -198,12 +197,12 @@ namespace ts.codefix {
                 if (!newNodes) {
                     ([newNodes, changedToDefaultExport] = convertModuleExportsToExportDefault(right, checker));
                 }
-                changes.replaceNodeWithNodes(sourceFile, statement, newNodes);
+                changes.replaceNodeWithNodes(sourceFile, assignment.parent, newNodes);
                 return changedToDefaultExport;
             }
         }
         else if (isExportsOrModuleExportsOrAlias(sourceFile, left.expression)) {
-            convertNamedExport(sourceFile, statement, left.name, right, changes, exports);
+            convertNamedExport(sourceFile, assignment as BinaryExpression & { left: PropertyAccessExpression }, changes, exports);
         }
 
         return false;
@@ -223,7 +222,7 @@ namespace ts.codefix {
                 case SyntaxKind.SpreadAssignment:
                     return undefined;
                 case SyntaxKind.PropertyAssignment:
-                    return !isIdentifier(prop.name) ? undefined : convertExportsDotXEquals(prop.name.text, prop.initializer);
+                    return !isIdentifier(prop.name) ? undefined : convertExportsDotXEquals_replaceNode(prop.name.text, prop.initializer);
                 case SyntaxKind.MethodDeclaration:
                     return !isIdentifier(prop.name) ? undefined : functionExpressionToDeclaration(prop.name.text, [createToken(SyntaxKind.ExportKeyword)], prop);
                 default:
@@ -234,14 +233,12 @@ namespace ts.codefix {
 
     function convertNamedExport(
         sourceFile: SourceFile,
-        statement: Statement,
-        propertyName: Identifier,
-        right: Expression,
+        assignment: BinaryExpression & { left: PropertyAccessExpression },
         changes: textChanges.ChangeTracker,
         exports: ExportRenames,
     ): void {
         // If "originalKeywordKind" was set, this is e.g. `exports.
-        const { text } = propertyName;
+        const { text } = assignment.left.name;
         const rename = exports.get(text);
         if (rename !== undefined) {
             /*
@@ -249,13 +246,13 @@ namespace ts.codefix {
             export { _class as class };
             */
             const newNodes = [
-                makeConst(/*modifiers*/ undefined, rename, right),
+                makeConst(/*modifiers*/ undefined, rename, assignment.right),
                 makeExportDeclaration([createExportSpecifier(rename, text)]),
             ];
-            changes.replaceNodeWithNodes(sourceFile, statement, newNodes);
+            changes.replaceNodeWithNodes(sourceFile, assignment.parent, newNodes);
         }
         else {
-            changes.replaceNode(sourceFile, statement, convertExportsDotXEquals(text, right));
+            convertExportsPropertyAssignment(assignment, sourceFile, changes);
         }
     }
 
@@ -303,7 +300,27 @@ namespace ts.codefix {
         return makeExportDeclaration([createExportSpecifier(/*propertyName*/ undefined, "default")], moduleSpecifier);
     }
 
-    function convertExportsDotXEquals(name: string | undefined, exported: Expression): Statement {
+    function convertExportsPropertyAssignment({ left, right, parent }: BinaryExpression & { left: PropertyAccessExpression }, sourceFile: SourceFile, changes: textChanges.ChangeTracker): void {
+        const name = left.name.text;
+        if ((isFunctionExpression(right) || isArrowFunction(right) || isClassExpression(right)) && (!right.name || right.name.text === name)) {
+            // `exports.f = function() {}` -> `export function f() {}` -- Replace `exports.f = ` with `export `, and insert the name after `function`.
+            changes.replaceRange(sourceFile, { pos: left.getStart(sourceFile), end: right.getStart(sourceFile) }, createToken(SyntaxKind.ExportKeyword), { suffix: " " });
+
+            if (!right.name) changes.insertName(sourceFile, right, name);
+
+            const semi = findChildOfKind(parent, SyntaxKind.SemicolonToken, sourceFile);
+            if (semi) changes.deleteNode(sourceFile, semi, { useNonAdjustedEndPosition: true });
+        }
+        else {
+            // `exports.f = function g() {}` -> `export const f = function g() {}` -- just replace `exports.` with `export const `
+            changes.replaceNodeRangeWithNodes(sourceFile, left.expression, findChildOfKind(left, SyntaxKind.DotToken, sourceFile)!,
+                [createToken(SyntaxKind.ExportKeyword), createToken(SyntaxKind.ConstKeyword)],
+                { joiner: " ", suffix: " " });
+        }
+    }
+
+    // TODO: GH#22492 this will cause an error if a change has been made inside the body of the node.
+    function convertExportsDotXEquals_replaceNode(name: string | undefined, exported: Expression): Statement {
         const modifiers = [createToken(SyntaxKind.ExportKeyword)];
         switch (exported.kind) {
             case SyntaxKind.FunctionExpression: {

src/services/textChanges.ts

@@ -100,6 +100,10 @@ namespace ts.textChanges {
         preserveLeadingWhitespace?: boolean;
     }
 
+    export interface ReplaceWithMultipleNodesOptions extends InsertNodeOptions {
+        readonly joiner?: string;
+    }
+
     enum ChangeKind {
         Remove,
         ReplaceWithSingleNode,
@@ -130,7 +134,7 @@ namespace ts.textChanges {
     interface ReplaceWithMultipleNodes extends BaseChange {
         readonly kind: ChangeKind.ReplaceWithMultipleNodes;
         readonly nodes: ReadonlyArray<Node>;
-        readonly options?: InsertNodeOptions;
+        readonly options?: ReplaceWithMultipleNodesOptions;
     }
 
     interface ChangeText extends BaseChange {
@@ -303,7 +307,7 @@ namespace ts.textChanges {
             this.replaceRange(sourceFile, getAdjustedRange(sourceFile, startNode, endNode, options), newNode, options);
         }
 
-        private replaceRangeWithNodes(sourceFile: SourceFile, range: TextRange, newNodes: ReadonlyArray<Node>, options: InsertNodeOptions = {}) {
+        private replaceRangeWithNodes(sourceFile: SourceFile, range: TextRange, newNodes: ReadonlyArray<Node>, options: ReplaceWithMultipleNodesOptions & ConfigurableStartEnd = {}) {
             this.changes.push({ kind: ChangeKind.ReplaceWithMultipleNodes, sourceFile, range, options, nodes: newNodes });
             return this;
         }
@@ -312,7 +316,7 @@ namespace ts.textChanges {
             return this.replaceRangeWithNodes(sourceFile, getAdjustedRange(sourceFile, oldNode, oldNode, options), newNodes, options);
         }
 
-        public replaceNodeRangeWithNodes(sourceFile: SourceFile, startNode: Node, endNode: Node, newNodes: ReadonlyArray<Node>, options: ChangeNodeOptions = useNonAdjustedPositions) {
+        public replaceNodeRangeWithNodes(sourceFile: SourceFile, startNode: Node, endNode: Node, newNodes: ReadonlyArray<Node>, options: ReplaceWithMultipleNodesOptions & ConfigurableStartEnd = useNonAdjustedPositions) {
             return this.replaceRangeWithNodes(sourceFile, getAdjustedRange(sourceFile, startNode, endNode, options), newNodes, options);
         }
 
@@ -326,7 +330,7 @@ namespace ts.textChanges {
             this.replaceRange(sourceFile, createTextRange(pos), newNode, options);
         }
 
-        private insertNodesAt(sourceFile: SourceFile, pos: number, newNodes: ReadonlyArray<Node>, options: InsertNodeOptions = {}): void {
+        private insertNodesAt(sourceFile: SourceFile, pos: number, newNodes: ReadonlyArray<Node>, options: ReplaceWithMultipleNodesOptions = {}): void {
             this.changes.push({ kind: ChangeKind.ReplaceWithMultipleNodes, sourceFile, options, nodes: newNodes, range: { pos, end: pos } });
         }
 
@@ -483,6 +487,35 @@ namespace ts.textChanges {
             return Debug.failBadSyntaxKind(node); // We haven't handled this kind of node yet -- add it
         }
 
+        public insertName(sourceFile: SourceFile, node: FunctionExpression | ClassExpression | ArrowFunction, name: string): void {
+            Debug.assert(!node.name);
+            if (node.kind === SyntaxKind.ArrowFunction) {
+                const arrow = findChildOfKind(node, SyntaxKind.EqualsGreaterThanToken, sourceFile)!;
+                const lparen = findChildOfKind(node, SyntaxKind.OpenParenToken, sourceFile);
+                if (lparen) {
+                    // `() => {}` --> `function f() {}`
+                    this.insertNodesAt(sourceFile, lparen.getStart(sourceFile), [createToken(SyntaxKind.FunctionKeyword), createIdentifier(name)], { joiner: " " });
+                    this.deleteNode(sourceFile, arrow);
+                }
+                else {
+                    // `x => {}` -> `function f(x) {}`
+                    this.insertText(sourceFile, first(node.parameters).getStart(sourceFile), `function ${name}(`);
+                    // Replacing full range of arrow to get rid of the leading space -- replace ` =>` with `)`
+                    this.replaceRange(sourceFile, arrow, createToken(SyntaxKind.CloseParenToken));
+                }
+
+                if (node.body.kind !== SyntaxKind.Block) {
+                    // `() => 0` => `function f() { return 0; }`
+                    this.insertNodesAt(sourceFile, node.body.getStart(sourceFile), [createToken(SyntaxKind.OpenBraceToken), createToken(SyntaxKind.ReturnKeyword)], { joiner: " ", suffix: " " });
+                    this.insertNodesAt(sourceFile, node.body.end, [createToken(SyntaxKind.SemicolonToken), createToken(SyntaxKind.CloseBraceToken)], { joiner: " " });
+                }
+            }
+            else {
+                const pos = findChildOfKind(node, node.kind === SyntaxKind.FunctionExpression ? SyntaxKind.FunctionKeyword : SyntaxKind.ClassKeyword, sourceFile)!.end;
+                this.insertNodeAt(sourceFile, pos, createIdentifier(name), { prefix: " " });
+            }
+        }
+
         /**
          * This function should be used to insert nodes in lists when nodes don't carry separators as the part of the node range,
          * i.e. arguments in arguments lists, parameters in parameter lists etc.
@@ -652,7 +685,7 @@ namespace ts.textChanges {
             const { options = {}, range: { pos } } = change;
             const format = (n: Node) => getFormattedTextOfNode(n, sourceFile, pos, options, newLineCharacter, formatContext, validate);
             const text = change.kind === ChangeKind.ReplaceWithMultipleNodes
-                ? change.nodes.map(n => removeSuffix(format(n), newLineCharacter)).join(newLineCharacter)
+                ? change.nodes.map(n => removeSuffix(format(n), newLineCharacter)).join(change.options.joiner || newLineCharacter)
                 : format(change.node);
             // strip initial indentation (spaces or tabs) if text will be inserted in the middle of the line
             const noIndent = (options.preserveLeadingWhitespace || options.indentation !== undefined || getLineStartPositionForPosition(pos, sourceFile) === pos) ? text : text.replace(/^\s+/, "");

tests/cases/fourslash/refactorConvertToEs6Module_export_alias.ts

@@ -10,6 +10,6 @@
 verify.codeFix({
     description: "Convert to ES6 module",
     newFileContent: `
-export function f() { }
+export function f() {}
 `,
 });

tests/cases/fourslash/refactorConvertToEs6Module_export_named.ts

@@ -6,6 +6,10 @@
 ////[|exports.f = function() {}|];
 ////exports.C = class {};
 ////exports.x = 0;
+////exports.a1 = () => {};
+////exports.a2 = () => 0;
+////exports.a3 = x => { x; };
+////exports.a4 = x => x;
 
 verify.getSuggestionDiagnostics([{
     message: "File is a CommonJS module; it may be converted to an ES6 module.",
@@ -15,8 +19,11 @@ verify.getSuggestionDiagnostics([{
 verify.codeFix({
     description: "Convert to ES6 module",
     newFileContent:
-`export function f() { }
-export class C {
-}
-export const x = 0;`,
+`export function f() {}
+export class C {}
+export const x = 0;
+export function a1() {}
+export function a2() { return 0; }
+export function a3(x) { x; }
+export function a4(x) { return x; }`,
 });

tests/cases/fourslash/refactorConvertToEs6Module_export_namedClassExpression.ts

@@ -0,0 +1,14 @@
+/// <reference path='fourslash.ts' />
+
+// @allowJs: true
+
+// @Filename: /a.js
+////exports.C = class E { static instance = new E(); }
+////exports.D = class D { static instance = new D(); }
+
+verify.codeFix({
+    description: "Convert to ES6 module",
+    newFileContent:
+`export const C = class E { static instance = new E(); }
+export class D { static instance = new D(); }`,
+});

tests/cases/fourslash/refactorConvertToEs6Module_export_namedFunctionExpression.ts

@@ -9,6 +9,6 @@
 verify.codeFix({
     description: "Convert to ES6 module",
     newFileContent:
-`export const f = function g() { g(); };
+`export const f = function g() { g(); }
 export function h() { h(); }`,
 });

tests/cases/fourslash/refactorConvertToEs6Module_export_referenced.ts

@@ -12,7 +12,7 @@
 ////
 ////exports.z = 2;
 ////exports.f = function(z) {
-////    z;
+////    exports.z; z;
 ////}
 
 // TODO: GH#22492 Should be a able access `exports.z` inside `exports.f`
@@ -28,8 +28,9 @@ const _y = y;
 export { _y as y };
 _y;
 
-export const z = 2;
+const _z = 2;
+export { _z as z };
 export function f(z) {
-    z;
+    _z; z;
 }`,
 });

tests/cases/fourslash/refactorConvertToEs6Module_expressionToDeclaration.ts

@@ -10,7 +10,5 @@ verify.codeFix({
     description: "Convert to ES6 module",
     newFileContent:
 `export async function* f(p) { p; }
-export class C extends D {
-    m() { }
-}`,
+export class C extends D { m() {} }`,
 });