Code review

Author: Andy Hanson

Diff for: src/services/codefixes/fixImplicitThis.ts

@@ -30,7 +30,7 @@ namespace ts.codefix {
             const { name } = fn;
             const body = Debug.assertDefined(fn.body); // Should be defined because the function contained a 'this' expression
             if (isFunctionExpression(fn)) {
-                if (fn.name && FindAllReferences.Core.isSymbolReferencedInFile(fn.name, checker, sourceFile, body)) {
+                if (name && FindAllReferences.Core.isSymbolReferencedInFile(name, checker, sourceFile, body)) {
                     // Function expression references itself. To fix we would have to extract it to a const.
                     return undefined;
                 }
@@ -41,7 +41,7 @@ namespace ts.codefix {
                     changes.delete(sourceFile, name);
                 }
                 changes.insertText(sourceFile, body.pos, " =>");
-                return [Diagnostics.Convert_function_expression_0_to_arrow_function, name ? name.text : "<anonymous>"];
+                return [Diagnostics.Convert_function_expression_0_to_arrow_function, name ? name.text : ANONYMOUS];
             }
             else {
                 // `function f() {}` => `const f = () => {}`
@@ -52,14 +52,14 @@ namespace ts.codefix {
                 return [Diagnostics.Convert_function_declaration_0_to_arrow_function, name!.text];
             }
         }
-        else { // No outer 'this', must add an annotation
+        else { // No outer 'this', must add a jsdoc tag / parameter
             if (isSourceFileJS(sourceFile)) {
                 const addClassTag = isPropertyAccessExpression(token.parent) && isAssignmentExpression(token.parent.parent);
                 addJSDocTags(changes, sourceFile, fn, [addClassTag ? createJSDocClassTag() : createJSDocThisTag(createKeywordTypeNode(SyntaxKind.AnyKeyword))]);
                 return addClassTag ? Diagnostics.Add_class_tag : Diagnostics.Add_this_tag;
             }
             else {
-                changes.insertNodeAt(sourceFile, fn.parameters.pos, makeParameter("this", createKeywordTypeNode(SyntaxKind.AnyKeyword)));
+                changes.insertFirstParameter(sourceFile, fn.parameters, makeParameter("this", createKeywordTypeNode(SyntaxKind.AnyKeyword)));
                 return Diagnostics.Add_this_parameter;
             }
         }

Diff for: src/services/codefixes/generateTypes.ts

@@ -191,6 +191,7 @@ namespace ts {
         ];
         return { parameters, returnType: hasReturn ? anyType() : createKeywordTypeNode(SyntaxKind.VoidKeyword) };
     }
+    /* @internal */
     export function makeParameter(name: string, type: TypeNode): ParameterDeclaration {
         return createParameter(/*decorators*/ undefined, /*modifiers*/ undefined, /*dotDotDotToken*/ undefined, name, /*questionToken*/ undefined, type);
     }

Diff for: src/services/refactors/extractSymbol.ts

@@ -673,7 +673,7 @@ namespace ts.refactor.extractSymbol {
             case SyntaxKind.FunctionDeclaration:
                 return scope.name
                     ? `function '${scope.name.text}'`
-                    : "anonymous function";
+                    : ANONYMOUS;
             case SyntaxKind.ArrowFunction:
                 return "arrow function";
             case SyntaxKind.MethodDeclaration:

Diff for: src/services/textChanges.ts

@@ -310,8 +310,18 @@ namespace ts.textChanges {
             });
         }
 
+        public insertFirstParameter(sourceFile: SourceFile, parameters: NodeArray<ParameterDeclaration>, newParam: ParameterDeclaration): void {
+            const p0 = firstOrUndefined(parameters);
+            if (p0) {
+                this.insertNodeBefore(sourceFile, p0, newParam);
+            }
+            else {
+                this.insertNodeAt(sourceFile, parameters.pos, newParam);
+            }
+        }
+
         public insertNodeBefore(sourceFile: SourceFile, before: Node, newNode: Node, blankLineBetween = false) {
-            this.insertNodeAt(sourceFile, getAdjustedStartPosition(sourceFile, before, {}, Position.Start), newNode, this.getOptionsForInsertNodeBefore(before, blankLineBetween));
+            this.insertNodeAt(sourceFile, getAdjustedStartPosition(sourceFile, before, {}, Position.Start), newNode, this.getOptionsForInsertNodeBefore(before, newNode, blankLineBetween));
         }
 
         public insertModifierBefore(sourceFile: SourceFile, modifier: SyntaxKind, before: Node): void {
@@ -390,15 +400,15 @@ namespace ts.textChanges {
             this.insertNodesAt(sourceFile, start, typeParameters, { prefix: "<", suffix: ">" });
         }
 
-        private getOptionsForInsertNodeBefore(before: Node, doubleNewlines: boolean): InsertNodeOptions {
+        private getOptionsForInsertNodeBefore(before: Node, inserted: Node, doubleNewlines: boolean): InsertNodeOptions {
             if (isStatement(before) || isClassElement(before)) {
                 return { suffix: doubleNewlines ? this.newLineCharacter + this.newLineCharacter : this.newLineCharacter };
             }
             else if (isVariableDeclaration(before)) { // insert `x = 1, ` into `const x = 1, y = 2;
                 return { suffix: ", " };
             }
             else if (isParameter(before)) {
-                return {};
+                return isParameter(inserted) ? { suffix: ", " } : {};
             }
             else if (isStringLiteral(before) && isImportDeclaration(before.parent) || isNamedImports(before)) {
                 return { suffix: ", " };
@@ -807,7 +817,7 @@ namespace ts.textChanges {
         }
 
         /** Note: this may mutate `nodeIn`. */
-        export function getFormattedTextOfNode(nodeIn: Node, sourceFile: SourceFile, pos: number, { indentation, prefix, delta }: InsertNodeOptions, newLineCharacter: string, formatContext: formatting.FormatContext, validate: ValidateNonFormattedText | undefined): string {
+        function getFormattedTextOfNode(nodeIn: Node, sourceFile: SourceFile, pos: number, { indentation, prefix, delta }: InsertNodeOptions, newLineCharacter: string, formatContext: formatting.FormatContext, validate: ValidateNonFormattedText | undefined): string {
             // Emitter doesn't handle JSDoc, so generate that here.
             if (isJSDocTag(nodeIn)) {
                 switch (nodeIn.kind) {

Diff for: src/services/utilities.ts

@@ -1908,4 +1908,6 @@ namespace ts {
     export function getSwitchedType(caseClause: CaseClause, checker: TypeChecker): Type | undefined {
         return checker.getTypeAtLocation(caseClause.parent.parent.expression);
     }
+
+    export const ANONYMOUS = "anonymous function";
 }

Diff for: tests/cases/fourslash/codeFixImplicitThis_ts_functionExpression_noName.ts

@@ -11,7 +11,7 @@
 ////}
 
 verify.codeFix({
-    description: "Convert function expression '<anonymous>' to arrow function",
+    description: "Convert function expression 'anonymous function' to arrow function",
     index: 0,
     newFileContent:
 `class C {

Diff for: tests/cases/fourslash/codeFixImplicitThis_ts_thisParameter2.ts

@@ -0,0 +1,16 @@
+/// <reference path='fourslash.ts' />
+
+// @noImplicitThis: true
+
+////function f(x: number) {
+////    this;
+////}
+
+verify.codeFix({
+    description: "Add 'this' parameter.",
+    index: 0,
+    newFileContent:
+`function f(this: any, x: number) {
+    this;
+}`,
+});