-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Changes from all commits
cb30622
8ae463f
e74d5ee
87f791f
665ffc0
dc29946
8074c74
d0b9505
1a347bf
7fb1bd2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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); | ||
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, " =>"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 { | ||
|
@@ -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: ", " }; | ||
|
@@ -1278,15 +1288,23 @@ namespace ts.textChanges { | |
deleteImportBinding(changes, sourceFile, node as NamespaceImport); | ||
break; | ||
|
||
case SyntaxKind.SemicolonToken: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I could be miscounting the scopes, but why would There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
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); | ||
} | ||
} | ||
} | ||
|
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; | ||
} | ||
} | ||
}`, | ||
}); |
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; | ||
}`, | ||
}); |
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 | ||
} | ||
} | ||
}`, | ||
}); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.