Skip to content

Commit b0450ae

Browse files
Andy Hansonsandersn
Andy Hanson
and
sandersn
authored
Add codefix for --noImplicitThis (#27565)
* Add codefix for --noImplicitThis * Code review * Back to building post-merge * Remove redundant functions + update tests Infer-from-usage also inserts `this: any` parameters when needed, so I removed that from fixImplicitThis. Otherwise, fixImplicitThis has better suggestions than inferFromUsage, so I moved inferFromUsage later in the suggestion order. * More redundancy removal Don't need to add `@this` anymore either since inferFromUsage will do that. * More baseline updates From moving inferFromUsage down in priority I think? * remove now-redundant ad-hoc jsdoc emit * fix more bad merge Co-authored-by: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com>
1 parent ec426ee commit b0450ae

18 files changed

+334
-12
lines changed

‎src/compiler/diagnosticMessages.json

+24
Original file line numberDiff line numberDiff line change
@@ -5517,6 +5517,30 @@
55175517
"category": "Message",
55185518
"code": 95101
55195519
},
5520+
"Add '@class' tag": {
5521+
"category": "Message",
5522+
"code": 95102
5523+
},
5524+
"Add '@this' tag": {
5525+
"category": "Message",
5526+
"code": 95103
5527+
},
5528+
"Add 'this' parameter.": {
5529+
"category": "Message",
5530+
"code": 95104
5531+
},
5532+
"Convert function expression '{0}' to arrow function": {
5533+
"category": "Message",
5534+
"code": 95105
5535+
},
5536+
"Convert function declaration '{0}' to arrow function": {
5537+
"category": "Message",
5538+
"code": 95106
5539+
},
5540+
"Fix all implicit-'this' errors": {
5541+
"category": "Message",
5542+
"code": 95107
5543+
},
55205544

55215545
"No value exists in scope for the shorthand property '{0}'. Either declare one or provide an initializer.": {
55225546
"category": "Error",

‎src/compiler/factoryPublic.ts

+6
Original file line numberDiff line numberDiff line change
@@ -2466,6 +2466,12 @@ namespace ts {
24662466
return tag;
24672467
}
24682468

2469+
/* @internal */
2470+
export function createJSDocClassTag(): JSDocClassTag {
2471+
return createJSDocTag<JSDocClassTag>(SyntaxKind.JSDocClassTag, "class");
2472+
}
2473+
2474+
24692475
/* @internal */
24702476
export function createJSDocComment(comment?: string | undefined, tags?: NodeArray<JSDocTag> | undefined) {
24712477
const node = createSynthesizedNode(SyntaxKind.JSDocComment) as JSDoc;

‎src/services/codefixes/fixImplicitThis.ts

+61
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
/* @internal */
2+
namespace ts.codefix {
3+
const fixId = "fixImplicitThis";
4+
const errorCodes = [Diagnostics.this_implicitly_has_type_any_because_it_does_not_have_a_type_annotation.code];
5+
registerCodeFix({
6+
errorCodes,
7+
getCodeActions(context) {
8+
const { sourceFile, program, span } = context;
9+
let diagnostic: DiagnosticAndArguments | undefined;
10+
const changes = textChanges.ChangeTracker.with(context, t => {
11+
diagnostic = doChange(t, sourceFile, span.start, program.getTypeChecker());
12+
});
13+
return diagnostic ? [createCodeFixAction(fixId, changes, diagnostic, fixId, Diagnostics.Fix_all_implicit_this_errors)] : emptyArray;
14+
},
15+
fixIds: [fixId],
16+
getAllCodeActions: context => codeFixAll(context, errorCodes, (changes, diag) => {
17+
doChange(changes, diag.file, diag.start, context.program.getTypeChecker());
18+
}),
19+
});
20+
21+
function doChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, pos: number, checker: TypeChecker): DiagnosticAndArguments | undefined {
22+
const token = getTokenAtPosition(sourceFile, pos);
23+
Debug.assert(token.kind === SyntaxKind.ThisKeyword);
24+
25+
const fn = getThisContainer(token, /*includeArrowFunctions*/ false);
26+
if (!isFunctionDeclaration(fn) && !isFunctionExpression(fn)) return undefined;
27+
28+
if (!isSourceFile(getThisContainer(fn, /*includeArrowFunctions*/ false))) { // 'this' is defined outside, convert to arrow function
29+
const fnKeyword = Debug.assertDefined(findChildOfKind(fn, SyntaxKind.FunctionKeyword, sourceFile));
30+
const { name } = fn;
31+
const body = Debug.assertDefined(fn.body); // Should be defined because the function contained a 'this' expression
32+
if (isFunctionExpression(fn)) {
33+
if (name && FindAllReferences.Core.isSymbolReferencedInFile(name, checker, sourceFile, body)) {
34+
// Function expression references itself. To fix we would have to extract it to a const.
35+
return undefined;
36+
}
37+
38+
// `function() {}` --> `() => {}`
39+
changes.delete(sourceFile, fnKeyword);
40+
if (name) {
41+
changes.delete(sourceFile, name);
42+
}
43+
changes.insertText(sourceFile, body.pos, " =>");
44+
return [Diagnostics.Convert_function_expression_0_to_arrow_function, name ? name.text : ANONYMOUS];
45+
}
46+
else {
47+
// `function f() {}` => `const f = () => {}`
48+
// `name` should be defined because we only do this in inner contexts, and name is only undefined for `export default function() {}`.
49+
changes.replaceNode(sourceFile, fnKeyword, createToken(SyntaxKind.ConstKeyword));
50+
changes.insertText(sourceFile, name!.end, " = ");
51+
changes.insertText(sourceFile, body.pos, " =>");
52+
return [Diagnostics.Convert_function_declaration_0_to_arrow_function, name!.text];
53+
}
54+
}
55+
// No outer 'this', add a @class tag if in a JS constructor function
56+
else if (isSourceFileJS(sourceFile) && isPropertyAccessExpression(token.parent) && isAssignmentExpression(token.parent.parent)) {
57+
addJSDocTags(changes, sourceFile, fn, [createJSDocClassTag()]);
58+
return Diagnostics.Add_class_tag;
59+
}
60+
}
61+
}

‎src/services/codefixes/inferFromUsage.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -351,7 +351,7 @@ namespace ts.codefix {
351351
addJSDocTags(changes, sourceFile, signature, paramTags);
352352
}
353353

354-
function addJSDocTags(changes: textChanges.ChangeTracker, sourceFile: SourceFile, parent: HasJSDoc, newTags: readonly JSDocTag[]): void {
354+
export function addJSDocTags(changes: textChanges.ChangeTracker, sourceFile: SourceFile, parent: HasJSDoc, newTags: readonly JSDocTag[]): void {
355355
const comments = mapDefined(parent.jsDoc, j => j.comment);
356356
const oldTags = flatMapToMutable(parent.jsDoc, j => j.tags);
357357
const unmergedNewTags = newTags.filter(newTag => !oldTags || !oldTags.some((tag, i) => {

‎src/services/findAllReferences.ts

+4-4
Original file line numberDiff line numberDiff line change
@@ -1174,16 +1174,16 @@ namespace ts.FindAllReferences {
11741174
}
11751175

11761176
/** Used as a quick check for whether a symbol is used at all in a file (besides its definition). */
1177-
export function isSymbolReferencedInFile(definition: Identifier, checker: TypeChecker, sourceFile: SourceFile): boolean {
1178-
return eachSymbolReferenceInFile(definition, checker, sourceFile, () => true) || false;
1177+
export function isSymbolReferencedInFile(definition: Identifier, checker: TypeChecker, sourceFile: SourceFile, searchContainer: Node = sourceFile): boolean {
1178+
return eachSymbolReferenceInFile(definition, checker, sourceFile, () => true, searchContainer) || false;
11791179
}
11801180

1181-
export function eachSymbolReferenceInFile<T>(definition: Identifier, checker: TypeChecker, sourceFile: SourceFile, cb: (token: Identifier) => T): T | undefined {
1181+
export function eachSymbolReferenceInFile<T>(definition: Identifier, checker: TypeChecker, sourceFile: SourceFile, cb: (token: Identifier) => T, searchContainer: Node = sourceFile): T | undefined {
11821182
const symbol = isParameterPropertyDeclaration(definition.parent, definition.parent.parent)
11831183
? first(checker.getSymbolsOfParameterPropertyDeclaration(definition.parent, definition.text))
11841184
: checker.getSymbolAtLocation(definition);
11851185
if (!symbol) return undefined;
1186-
for (const token of getPossibleSymbolReferenceNodes(sourceFile, symbol.name)) {
1186+
for (const token of getPossibleSymbolReferenceNodes(sourceFile, symbol.name, searchContainer)) {
11871187
if (!isIdentifier(token) || token === definition || token.escapedText !== definition.escapedText) continue;
11881188
const referenceSymbol: Symbol = checker.getSymbolAtLocation(token)!; // See GH#19955 for why the type annotation is necessary
11891189
if (referenceSymbol === symbol

‎src/services/refactors/extractSymbol.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -677,7 +677,7 @@ namespace ts.refactor.extractSymbol {
677677
case SyntaxKind.FunctionDeclaration:
678678
return scope.name
679679
? `function '${scope.name.text}'`
680-
: "anonymous function";
680+
: ANONYMOUS;
681681
case SyntaxKind.ArrowFunction:
682682
return "arrow function";
683683
case SyntaxKind.MethodDeclaration:

‎src/services/textChanges.ts

+23-5
Original file line numberDiff line numberDiff line change
@@ -338,8 +338,18 @@ namespace ts.textChanges {
338338
});
339339
}
340340

341+
public insertFirstParameter(sourceFile: SourceFile, parameters: NodeArray<ParameterDeclaration>, newParam: ParameterDeclaration): void {
342+
const p0 = firstOrUndefined(parameters);
343+
if (p0) {
344+
this.insertNodeBefore(sourceFile, p0, newParam);
345+
}
346+
else {
347+
this.insertNodeAt(sourceFile, parameters.pos, newParam);
348+
}
349+
}
350+
341351
public insertNodeBefore(sourceFile: SourceFile, before: Node, newNode: Node, blankLineBetween = false): void {
342-
this.insertNodeAt(sourceFile, getAdjustedStartPosition(sourceFile, before, {}), newNode, this.getOptionsForInsertNodeBefore(before, blankLineBetween));
352+
this.insertNodeAt(sourceFile, getAdjustedStartPosition(sourceFile, before, {}), newNode, this.getOptionsForInsertNodeBefore(before, newNode, blankLineBetween));
343353
}
344354

345355
public insertModifierBefore(sourceFile: SourceFile, modifier: SyntaxKind, before: Node): void {
@@ -426,15 +436,15 @@ namespace ts.textChanges {
426436
this.insertNodesAt(sourceFile, start, typeParameters, { prefix: "<", suffix: ">" });
427437
}
428438

429-
private getOptionsForInsertNodeBefore(before: Node, doubleNewlines: boolean): InsertNodeOptions {
439+
private getOptionsForInsertNodeBefore(before: Node, inserted: Node, doubleNewlines: boolean): InsertNodeOptions {
430440
if (isStatement(before) || isClassElement(before)) {
431441
return { suffix: doubleNewlines ? this.newLineCharacter + this.newLineCharacter : this.newLineCharacter };
432442
}
433443
else if (isVariableDeclaration(before)) { // insert `x = 1, ` into `const x = 1, y = 2;
434444
return { suffix: ", " };
435445
}
436446
else if (isParameter(before)) {
437-
return {};
447+
return isParameter(inserted) ? { suffix: ", " } : {};
438448
}
439449
else if (isStringLiteral(before) && isImportDeclaration(before.parent) || isNamedImports(before)) {
440450
return { suffix: ", " };
@@ -1278,15 +1288,23 @@ namespace ts.textChanges {
12781288
deleteImportBinding(changes, sourceFile, node as NamespaceImport);
12791289
break;
12801290

1291+
case SyntaxKind.SemicolonToken:
1292+
deleteNode(changes, sourceFile, node, { trailingTriviaOption: TrailingTriviaOption.Exclude });
1293+
break;
1294+
1295+
case SyntaxKind.FunctionKeyword:
1296+
deleteNode(changes, sourceFile, node, { leadingTriviaOption: LeadingTriviaOption.Exclude });
1297+
break;
1298+
12811299
default:
12821300
if (isImportClause(node.parent) && node.parent.name === node) {
12831301
deleteDefaultImport(changes, sourceFile, node.parent);
12841302
}
1285-
else if (isCallLikeExpression(node.parent)) {
1303+
else if (isCallExpression(node.parent) && contains(node.parent.arguments, node)) {
12861304
deleteNodeInList(changes, deletedNodesInLists, sourceFile, node);
12871305
}
12881306
else {
1289-
deleteNode(changes, sourceFile, node, node.kind === SyntaxKind.SemicolonToken ? { trailingTriviaOption: TrailingTriviaOption.Exclude } : undefined);
1307+
deleteNode(changes, sourceFile, node);
12901308
}
12911309
}
12921310
}

‎src/services/tsconfig.json

+2-1
Original file line numberDiff line numberDiff line change
@@ -55,14 +55,14 @@
5555
"codefixes/addMissingInvocationForDecorator.ts",
5656
"codefixes/addNameToNamelessParameter.ts",
5757
"codefixes/annotateWithTypeFromJSDoc.ts",
58-
"codefixes/inferFromUsage.ts",
5958
"codefixes/convertFunctionToEs6Class.ts",
6059
"codefixes/convertToAsyncFunction.ts",
6160
"codefixes/convertToEs6Module.ts",
6261
"codefixes/correctQualifiedNameToIndexedAccessType.ts",
6362
"codefixes/convertToTypeOnlyExport.ts",
6463
"codefixes/fixClassIncorrectlyImplementsInterface.ts",
6564
"codefixes/importFixes.ts",
65+
"codefixes/fixImplicitThis.ts",
6666
"codefixes/fixSpelling.ts",
6767
"codefixes/fixAddMissingMember.ts",
6868
"codefixes/fixAddMissingNewOperator.ts",
@@ -82,6 +82,7 @@
8282
"codefixes/fixJSDocTypes.ts",
8383
"codefixes/fixMissingCallParentheses.ts",
8484
"codefixes/fixAwaitInSyncFunction.ts",
85+
"codefixes/inferFromUsage.ts",
8586
"codefixes/disableJsDiagnostics.ts",
8687
"codefixes/helpers.ts",
8788
"codefixes/fixInvalidImportSyntax.ts",

‎src/services/utilities.ts

+2
Original file line numberDiff line numberDiff line change
@@ -2396,6 +2396,8 @@ namespace ts {
23962396
return checker.getTypeAtLocation(caseClause.parent.parent.expression);
23972397
}
23982398

2399+
export const ANONYMOUS = "anonymous function";
2400+
23992401
export function getTypeNodeIfAccessible(type: Type, enclosingScope: Node, program: Program, host: LanguageServiceHost): TypeNode | undefined {
24002402
const checker = program.getTypeChecker();
24012403
let typeIsAccessible = true;

‎tests/cases/fourslash/codeFixImplicitThis_js_all.ts

+36
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
/// <reference path='fourslash.ts' />
2+
3+
// @allowJs: true
4+
// @checkJs: true
5+
// @noImplicitThis: true
6+
7+
// @Filename: /a.js
8+
////function f() {
9+
//// this.x = 1;
10+
////}
11+
////class C {
12+
//// m() {
13+
//// function h() {
14+
//// this;
15+
//// }
16+
//// }
17+
////}
18+
19+
verify.codeFixAll({
20+
fixId: "fixImplicitThis",
21+
fixAllDescription: "Fix all implicit-'this' errors",
22+
newFileContent:
23+
`/**
24+
* @class
25+
*/
26+
function f() {
27+
this.x = 1;
28+
}
29+
class C {
30+
m() {
31+
const h = () => {
32+
this;
33+
}
34+
}
35+
}`,
36+
});

‎tests/cases/fourslash/codeFixImplicitThis_js_classTag.ts

+22
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
/// <reference path='fourslash.ts' />
2+
3+
// @allowJs: true
4+
// @checkJs: true
5+
// @noImplicitThis: true
6+
7+
// @Filename: /a.js
8+
////function f() {
9+
//// this.x = 1;
10+
////}
11+
12+
verify.codeFix({
13+
description: "Add '@class' tag",
14+
index: 0,
15+
newFileContent:
16+
`/**
17+
* @class
18+
*/
19+
function f() {
20+
this.x = 1;
21+
}`,
22+
});

‎tests/cases/fourslash/codeFixImplicitThis_js_classTag_addToComment.ts

+24
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
/// <reference path='fourslash.ts' />
2+
3+
// @allowJs: true
4+
// @checkJs: true
5+
// @noImplicitThis: true
6+
7+
// @Filename: /a.js
8+
/////** Doc */
9+
////function f() {
10+
//// this.x = 1;
11+
////}
12+
13+
verify.codeFix({
14+
description: "Add '@class' tag",
15+
index: 0,
16+
newFileContent:
17+
`/**
18+
* Doc
19+
* @class
20+
*/
21+
function f() {
22+
this.x = 1;
23+
}`,
24+
});

‎tests/cases/fourslash/codeFixImplicitThis_ts_all.ts

+30
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
/// <reference path='fourslash.ts' />
2+
3+
// @noImplicitThis: true
4+
5+
////class C {
6+
//// m() {
7+
//// function f() {
8+
//// this;
9+
//// }
10+
//// const g = function() {
11+
//// this;
12+
//// };
13+
//// }
14+
////}
15+
16+
verify.codeFixAll({
17+
fixId: "fixImplicitThis",
18+
fixAllDescription: "Fix all implicit-'this' errors",
19+
newFileContent:
20+
`class C {
21+
m() {
22+
const f = () => {
23+
this;
24+
}
25+
const g = () => {
26+
this;
27+
};
28+
}
29+
}`,
30+
});

‎tests/cases/fourslash/codeFixImplicitThis_ts_cantFixNonFunction.ts

+7
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
/// <reference path='fourslash.ts' />
2+
3+
// @noImplicitThis: true
4+
5+
////this;
6+
7+
verify.codeFixAvailable([]);

‎tests/cases/fourslash/codeFixImplicitThis_ts_functionDeclaration.ts

+26
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
/// <reference path='fourslash.ts' />
2+
3+
// @noImplicitThis: true
4+
5+
////class C {
6+
//// m() {
7+
//// function f() {
8+
//// this;
9+
//// f(); // self-reference OK
10+
//// }
11+
//// }
12+
////}
13+
14+
verify.codeFix({
15+
description: "Convert function declaration 'f' to arrow function",
16+
index: 0,
17+
newFileContent:
18+
`class C {
19+
m() {
20+
const f = () => {
21+
this;
22+
f(); // self-reference OK
23+
}
24+
}
25+
}`,
26+
});

0 commit comments

Comments
 (0)