Skip to content

Add codefix for --noImplicitThis #27565

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Mar 17, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -5517,6 +5517,30 @@
"category": "Message",
"code": 95101
},
"Add '@class' tag": {
"category": "Message",
"code": 95102
},
"Add '@this' tag": {
"category": "Message",
"code": 95103
},
"Add 'this' parameter.": {
"category": "Message",
"code": 95104
},
"Convert function expression '{0}' to arrow function": {
"category": "Message",
"code": 95105
},
"Convert function declaration '{0}' to arrow function": {
"category": "Message",
"code": 95106
},
"Fix all implicit-'this' errors": {
"category": "Message",
"code": 95107
},

"No value exists in scope for the shorthand property '{0}'. Either declare one or provide an initializer.": {
"category": "Error",
Expand Down
6 changes: 6 additions & 0 deletions src/compiler/factoryPublic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2466,6 +2466,12 @@ namespace ts {
return tag;
}

/* @internal */
export function createJSDocClassTag(): JSDocClassTag {
return createJSDocTag<JSDocClassTag>(SyntaxKind.JSDocClassTag, "class");
}


/* @internal */
export function createJSDocComment(comment?: string | undefined, tags?: NodeArray<JSDocTag> | undefined) {
const node = createSynthesizedNode(SyntaxKind.JSDocComment) as JSDoc;
Expand Down
61 changes: 61 additions & 0 deletions src/services/codefixes/fixImplicitThis.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/* @internal */
namespace ts.codefix {
const fixId = "fixImplicitThis";
const errorCodes = [Diagnostics.this_implicitly_has_type_any_because_it_does_not_have_a_type_annotation.code];
registerCodeFix({
errorCodes,
getCodeActions(context) {
const { sourceFile, program, span } = context;
let diagnostic: DiagnosticAndArguments | undefined;
const changes = textChanges.ChangeTracker.with(context, t => {
diagnostic = doChange(t, sourceFile, span.start, program.getTypeChecker());
});
return diagnostic ? [createCodeFixAction(fixId, changes, diagnostic, fixId, Diagnostics.Fix_all_implicit_this_errors)] : emptyArray;
},
fixIds: [fixId],
getAllCodeActions: context => codeFixAll(context, errorCodes, (changes, diag) => {
doChange(changes, diag.file, diag.start, context.program.getTypeChecker());
}),
});

function doChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, pos: number, checker: TypeChecker): DiagnosticAndArguments | undefined {
const token = getTokenAtPosition(sourceFile, pos);
Debug.assert(token.kind === SyntaxKind.ThisKeyword);

const fn = getThisContainer(token, /*includeArrowFunctions*/ false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we be in a default argument of the function? Does that matter?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a parameter initializer, 'this' refers to the same 'this' as inside the body of the function, not to 'this' of an outer function. So this will work just as well in that case.

if (!isFunctionDeclaration(fn) && !isFunctionExpression(fn)) return undefined;

if (!isSourceFile(getThisContainer(fn, /*includeArrowFunctions*/ false))) { // 'this' is defined outside, convert to arrow function
const fnKeyword = Debug.assertDefined(findChildOfKind(fn, SyntaxKind.FunctionKeyword, sourceFile));
const { name } = fn;
const body = Debug.assertDefined(fn.body); // Should be defined because the function contained a 'this' expression
if (isFunctionExpression(fn)) {
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;
}

// `function() {}` --> `() => {}`
changes.delete(sourceFile, fnKeyword);
if (name) {
changes.delete(sourceFile, name);
}
changes.insertText(sourceFile, body.pos, " =>");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't there a code fix or refactoring for converting a function to an error function? Can/should we share code?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There will be in #28250, we can combine these then.

return [Diagnostics.Convert_function_expression_0_to_arrow_function, name ? name.text : ANONYMOUS];
}
else {
// `function f() {}` => `const f = () => {}`
// `name` should be defined because we only do this in inner contexts, and name is only undefined for `export default function() {}`.
changes.replaceNode(sourceFile, fnKeyword, createToken(SyntaxKind.ConstKeyword));
changes.insertText(sourceFile, name!.end, " = ");
changes.insertText(sourceFile, body.pos, " =>");
return [Diagnostics.Convert_function_declaration_0_to_arrow_function, name!.text];
}
}
// No outer 'this', add a @class tag if in a JS constructor function
else if (isSourceFileJS(sourceFile) && isPropertyAccessExpression(token.parent) && isAssignmentExpression(token.parent.parent)) {
addJSDocTags(changes, sourceFile, fn, [createJSDocClassTag()]);
return Diagnostics.Add_class_tag;
}
}
}
2 changes: 1 addition & 1 deletion src/services/codefixes/inferFromUsage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,7 @@ namespace ts.codefix {
addJSDocTags(changes, sourceFile, signature, paramTags);
}

function addJSDocTags(changes: textChanges.ChangeTracker, sourceFile: SourceFile, parent: HasJSDoc, newTags: readonly JSDocTag[]): void {
export function addJSDocTags(changes: textChanges.ChangeTracker, sourceFile: SourceFile, parent: HasJSDoc, newTags: readonly JSDocTag[]): void {
const comments = mapDefined(parent.jsDoc, j => j.comment);
const oldTags = flatMapToMutable(parent.jsDoc, j => j.tags);
const unmergedNewTags = newTags.filter(newTag => !oldTags || !oldTags.some((tag, i) => {
Expand Down
8 changes: 4 additions & 4 deletions src/services/findAllReferences.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1174,16 +1174,16 @@ namespace ts.FindAllReferences {
}

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

export function eachSymbolReferenceInFile<T>(definition: Identifier, checker: TypeChecker, sourceFile: SourceFile, cb: (token: Identifier) => T): T | undefined {
export function eachSymbolReferenceInFile<T>(definition: Identifier, checker: TypeChecker, sourceFile: SourceFile, cb: (token: Identifier) => T, searchContainer: Node = sourceFile): T | undefined {
const symbol = isParameterPropertyDeclaration(definition.parent, definition.parent.parent)
? first(checker.getSymbolsOfParameterPropertyDeclaration(definition.parent, definition.text))
: checker.getSymbolAtLocation(definition);
if (!symbol) return undefined;
for (const token of getPossibleSymbolReferenceNodes(sourceFile, symbol.name)) {
for (const token of getPossibleSymbolReferenceNodes(sourceFile, symbol.name, searchContainer)) {
if (!isIdentifier(token) || token === definition || token.escapedText !== definition.escapedText) continue;
const referenceSymbol: Symbol = checker.getSymbolAtLocation(token)!; // See GH#19955 for why the type annotation is necessary
if (referenceSymbol === symbol
Expand Down
2 changes: 1 addition & 1 deletion src/services/refactors/extractSymbol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -677,7 +677,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:
Expand Down
28 changes: 23 additions & 5 deletions src/services/textChanges.ts
Original file line number Diff line number Diff line change
Expand Up @@ -338,8 +338,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): void {
this.insertNodeAt(sourceFile, getAdjustedStartPosition(sourceFile, before, {}), newNode, this.getOptionsForInsertNodeBefore(before, blankLineBetween));
this.insertNodeAt(sourceFile, getAdjustedStartPosition(sourceFile, before, {}), newNode, this.getOptionsForInsertNodeBefore(before, newNode, blankLineBetween));
}

public insertModifierBefore(sourceFile: SourceFile, modifier: SyntaxKind, before: Node): void {
Expand Down Expand Up @@ -426,15 +436,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: ", " };
Expand Down Expand Up @@ -1278,15 +1288,23 @@ namespace ts.textChanges {
deleteImportBinding(changes, sourceFile, node as NamespaceImport);
break;

case SyntaxKind.SemicolonToken:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could be miscounting the scopes, but why would deleteDeclaration be called on a semicolon?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

convertToEs6Module deletes semicolons when changing an assignment statement to a function declaration.

deleteNode(changes, sourceFile, node, { trailingTriviaOption: TrailingTriviaOption.Exclude });
break;

case SyntaxKind.FunctionKeyword:
deleteNode(changes, sourceFile, node, { leadingTriviaOption: LeadingTriviaOption.Exclude });
break;

default:
if (isImportClause(node.parent) && node.parent.name === node) {
deleteDefaultImport(changes, sourceFile, node.parent);
}
else if (isCallLikeExpression(node.parent)) {
else if (isCallExpression(node.parent) && contains(node.parent.arguments, node)) {
deleteNodeInList(changes, deletedNodesInLists, sourceFile, node);
}
else {
deleteNode(changes, sourceFile, node, node.kind === SyntaxKind.SemicolonToken ? { trailingTriviaOption: TrailingTriviaOption.Exclude } : undefined);
deleteNode(changes, sourceFile, node);
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion src/services/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,14 @@
"codefixes/addMissingInvocationForDecorator.ts",
"codefixes/addNameToNamelessParameter.ts",
"codefixes/annotateWithTypeFromJSDoc.ts",
"codefixes/inferFromUsage.ts",
"codefixes/convertFunctionToEs6Class.ts",
"codefixes/convertToAsyncFunction.ts",
"codefixes/convertToEs6Module.ts",
"codefixes/correctQualifiedNameToIndexedAccessType.ts",
"codefixes/convertToTypeOnlyExport.ts",
"codefixes/fixClassIncorrectlyImplementsInterface.ts",
"codefixes/importFixes.ts",
"codefixes/fixImplicitThis.ts",
"codefixes/fixSpelling.ts",
"codefixes/fixAddMissingMember.ts",
"codefixes/fixAddMissingNewOperator.ts",
Expand All @@ -82,6 +82,7 @@
"codefixes/fixJSDocTypes.ts",
"codefixes/fixMissingCallParentheses.ts",
"codefixes/fixAwaitInSyncFunction.ts",
"codefixes/inferFromUsage.ts",
"codefixes/disableJsDiagnostics.ts",
"codefixes/helpers.ts",
"codefixes/fixInvalidImportSyntax.ts",
Expand Down
2 changes: 2 additions & 0 deletions src/services/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2396,6 +2396,8 @@ namespace ts {
return checker.getTypeAtLocation(caseClause.parent.parent.expression);
}

export const ANONYMOUS = "anonymous function";

export function getTypeNodeIfAccessible(type: Type, enclosingScope: Node, program: Program, host: LanguageServiceHost): TypeNode | undefined {
const checker = program.getTypeChecker();
let typeIsAccessible = true;
Expand Down
36 changes: 36 additions & 0 deletions tests/cases/fourslash/codeFixImplicitThis_js_all.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/// <reference path='fourslash.ts' />

// @allowJs: true
// @checkJs: true
// @noImplicitThis: true

// @Filename: /a.js
////function f() {
//// this.x = 1;
////}
////class C {
//// m() {
//// function h() {
//// this;
//// }
//// }
////}

verify.codeFixAll({
fixId: "fixImplicitThis",
fixAllDescription: "Fix all implicit-'this' errors",
newFileContent:
`/**
* @class
*/
function f() {
this.x = 1;
}
class C {
m() {
const h = () => {
this;
}
}
}`,
});
22 changes: 22 additions & 0 deletions tests/cases/fourslash/codeFixImplicitThis_js_classTag.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/// <reference path='fourslash.ts' />

// @allowJs: true
// @checkJs: true
// @noImplicitThis: true

// @Filename: /a.js
////function f() {
//// this.x = 1;
////}

verify.codeFix({
description: "Add '@class' tag",
index: 0,
newFileContent:
`/**
* @class
*/
function f() {
this.x = 1;
}`,
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/// <reference path='fourslash.ts' />

// @allowJs: true
// @checkJs: true
// @noImplicitThis: true

// @Filename: /a.js
/////** Doc */
////function f() {
//// this.x = 1;
////}

verify.codeFix({
description: "Add '@class' tag",
index: 0,
newFileContent:
`/**
* Doc
* @class
*/
function f() {
this.x = 1;
}`,
});
30 changes: 30 additions & 0 deletions tests/cases/fourslash/codeFixImplicitThis_ts_all.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/// <reference path='fourslash.ts' />

// @noImplicitThis: true

////class C {
//// m() {
//// function f() {
//// this;
//// }
//// const g = function() {
//// this;
//// };
//// }
////}

verify.codeFixAll({
fixId: "fixImplicitThis",
fixAllDescription: "Fix all implicit-'this' errors",
newFileContent:
`class C {
m() {
const f = () => {
this;
}
const g = () => {
this;
};
}
}`,
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
/// <reference path='fourslash.ts' />

// @noImplicitThis: true

////this;

verify.codeFixAvailable([]);
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/// <reference path='fourslash.ts' />

// @noImplicitThis: true

////class C {
//// m() {
//// function f() {
//// this;
//// f(); // self-reference OK
//// }
//// }
////}

verify.codeFix({
description: "Convert function declaration 'f' to arrow function",
index: 0,
newFileContent:
`class C {
m() {
const f = () => {
this;
f(); // self-reference OK
}
}
}`,
});
Loading