-
Notifications
You must be signed in to change notification settings - Fork 13.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 1 commit
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,68 @@ | ||
| /* @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 (fn.name && FindAllReferences.Core.isSymbolReferencedInFile(fn.name, checker, sourceFile, body)) { | ||
|
Member
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. It's kind of confusing to refer to the same property as both |
||
| // 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, " =>"); | ||
|
Member
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?
Author
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>"]; | ||
|
Member
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. Do we have a constant for |
||
| } | ||
| 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]; | ||
| } | ||
| } | ||
| else { // No outer 'this', must add an annotation | ||
|
Member
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. Or a parameter? |
||
| if (isSourceFileJS(sourceFile)) { | ||
| const addClassTag = isPropertyAccessExpression(token.parent) && isAssignmentExpression(token.parent.parent); | ||
| changes.insertJsdocCommentBefore(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))); | ||
| return Diagnostics.Add_this_parameter; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -339,6 +339,22 @@ namespace ts.textChanges { | |
| this.insertText(sourceFile, token.getStart(sourceFile), text); | ||
| } | ||
|
|
||
| public insertJsdocCommentBefore(sourceFile: SourceFile, fn: FunctionDeclaration | FunctionExpression, tag: JSDocTag): void { | ||
|
Member
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 don't think this will work right for function expressions immediately preceded by text. |
||
| const existingJsdoc = fn.jsDoc && firstOrUndefined(fn.jsDoc); | ||
| if (existingJsdoc) { | ||
| // `/** foo */` --> `/**\n * @constructor\n * foo */` | ||
| const jsdocStart = existingJsdoc.getStart(sourceFile); | ||
| const indent = getIndent(sourceFile, jsdocStart); | ||
| const indentAsterisk = `${this.newLineCharacter}${indent} *`; | ||
| this.insertNodeAt(sourceFile, jsdocStart + "/**".length, tag, { prefix: `${indentAsterisk} `, suffix: indentAsterisk }); | ||
| } | ||
| else { | ||
| const fnStart = fn.getStart(sourceFile); | ||
| const indent = getIndent(sourceFile, fnStart); | ||
| this.insertNodeAt(sourceFile, fnStart, tag, { prefix: "/** ", suffix: ` */${this.newLineCharacter}${indent}` }); | ||
| } | ||
| } | ||
|
|
||
| public replaceRangeWithText(sourceFile: SourceFile, range: TextRange, text: string) { | ||
| this.changes.push({ kind: ChangeKind.Text, sourceFile, range, text }); | ||
| } | ||
|
|
@@ -720,6 +736,11 @@ namespace ts.textChanges { | |
| } | ||
| } | ||
|
|
||
| function getIndent(sourceFile: SourceFile, position: number): string { | ||
| const lineStart = getStartPositionOfLine(getLineAndCharacterOfPosition(sourceFile, position).line, sourceFile); | ||
| return sourceFile.text.slice(lineStart, position); | ||
| } | ||
|
|
||
| // find first non-whitespace position in the leading trivia of the node | ||
| function startPositionToDeleteNodeInList(sourceFile: SourceFile, node: Node): number { | ||
| return skipTrivia(sourceFile.text, getAdjustedStartPosition(sourceFile, node, {}, Position.FullStart), /*stopAfterLineBreak*/ false, /*stopAtComments*/ true); | ||
|
|
@@ -788,21 +809,35 @@ namespace ts.textChanges { | |
| } | ||
|
|
||
| /** Note: this may mutate `nodeIn`. */ | ||
| function getFormattedTextOfNode(nodeIn: Node, sourceFile: SourceFile, pos: number, { indentation, prefix, delta }: InsertNodeOptions, newLineCharacter: string, formatContext: formatting.FormatContext, validate: ValidateNonFormattedText | undefined): string { | ||
| const { node, text } = getNonformattedText(nodeIn, sourceFile, newLineCharacter); | ||
| if (validate) validate(node, text); | ||
| const { options: formatOptions } = formatContext; | ||
| const initialIndentation = | ||
| indentation !== undefined | ||
| ? indentation | ||
| : formatting.SmartIndenter.getIndentation(pos, sourceFile, formatOptions, prefix === newLineCharacter || getLineStartPositionForPosition(pos, sourceFile) === pos); | ||
| if (delta === undefined) { | ||
| delta = formatting.SmartIndenter.shouldIndentChildNode(formatContext.options, nodeIn) ? (formatOptions.indentSize || 0) : 0; | ||
| export function getFormattedTextOfNode(nodeIn: Node, sourceFile: SourceFile, pos: number, { indentation, prefix, delta }: InsertNodeOptions, newLineCharacter: string, formatContext: formatting.FormatContext, validate: ValidateNonFormattedText | undefined): string { | ||
|
Member
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. where is this used now that there is an |
||
| // Emitter doesn't handle JSDoc, so generate that here. | ||
| if (isJSDocTag(nodeIn)) { | ||
| switch (nodeIn.kind) { | ||
| case SyntaxKind.JSDocClassTag: | ||
| return "@class"; | ||
| case SyntaxKind.JSDocThisTag: | ||
| const { typeExpression } = nodeIn as JSDocThisTag; | ||
| return typeExpression ? `@this {${getNonformattedText(typeExpression.type, sourceFile, newLineCharacter).text}}` : "@this"; | ||
| default: | ||
| return Debug.fail(); // TODO (if this is needed) | ||
| } | ||
| } | ||
| else { | ||
| const { node, text } = getNonformattedText(nodeIn, sourceFile, newLineCharacter); | ||
| if (validate) validate(node, text); | ||
| const { options: formatOptions } = formatContext; | ||
| const initialIndentation = | ||
| indentation !== undefined | ||
| ? indentation | ||
| : formatting.SmartIndenter.getIndentation(pos, sourceFile, formatOptions, prefix === newLineCharacter || getLineStartPositionForPosition(pos, sourceFile) === pos); | ||
| if (delta === undefined) { | ||
| delta = formatting.SmartIndenter.shouldIndentChildNode(formatContext.options, nodeIn) ? (formatOptions.indentSize || 0) : 0; | ||
| } | ||
|
|
||
| const file: SourceFileLike = { text, getLineAndCharacterOfPosition(pos) { return getLineAndCharacterOfPosition(this, pos); } }; | ||
| const changes = formatting.formatNodeGivenIndentation(node, file, sourceFile.languageVariant, initialIndentation, delta, formatContext); | ||
| return applyChanges(text, changes); | ||
| const file: SourceFileLike = { text, getLineAndCharacterOfPosition(pos) { return getLineAndCharacterOfPosition(this, pos); } }; | ||
| const changes = formatting.formatNodeGivenIndentation(node, file, sourceFile.languageVariant, initialIndentation, delta, formatContext); | ||
| return applyChanges(text, changes); | ||
| } | ||
| } | ||
|
|
||
| /** Note: output node may be mutated input node. */ | ||
|
|
@@ -1103,15 +1138,23 @@ namespace ts.textChanges { | |
| deleteImportBinding(changes, sourceFile, node as NamespaceImport); | ||
| break; | ||
|
|
||
| case SyntaxKind.SemicolonToken: | ||
|
Member
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
Author
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, { useNonAdjustedEndPosition: true }); | ||
| break; | ||
|
|
||
| case SyntaxKind.FunctionKeyword: | ||
| deleteNode(changes, sourceFile, node, { useNonAdjustedStartPosition: true }); | ||
| 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 ? { useNonAdjustedEndPosition: true } : undefined); | ||
| deleteNode(changes, sourceFile, node); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| /// <reference path='fourslash.ts' /> | ||
|
|
||
| // @allowJs: true | ||
| // @checkJs: true | ||
| // @noImplicitThis: true | ||
|
|
||
| // @Filename: /a.js | ||
| ////function f() { | ||
| //// this.x = 1; | ||
| ////} | ||
| ////function g() { | ||
| //// this; | ||
| ////} | ||
| ////class C { | ||
| //// m() { | ||
| //// function h() { | ||
| //// this; | ||
| //// } | ||
| //// } | ||
| ////} | ||
|
|
||
| verify.codeFixAll({ | ||
| fixId: "fixImplicitThis", | ||
| fixAllDescription: "Fix all implicit-'this' errors", | ||
| newFileContent: | ||
| `/** @class */ | ||
| function f() { | ||
| this.x = 1; | ||
| } | ||
| /** @this {any} */ | ||
| function g() { | ||
| this; | ||
| } | ||
| class C { | ||
| m() { | ||
| const h = () => { | ||
| this; | ||
| } | ||
| } | ||
| }`, | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| /// <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,23 @@ | ||
| /// <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: | ||
| `/** | ||
| * @class | ||
| * Doc */ | ||
| function f() { | ||
| this.x = 1; | ||
| }`, | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| /// <reference path='fourslash.ts' /> | ||
|
|
||
| // @allowJs: true | ||
| // @checkJs: true | ||
| // @noImplicitThis: true | ||
|
|
||
| // @Filename: /a.js | ||
| ////function f() { | ||
| //// this; | ||
| ////} | ||
|
|
||
| verify.codeFix({ | ||
| description: "Add '@this' tag", | ||
| index: 0, | ||
| newFileContent: | ||
| `/** @this {any} */ | ||
| function f() { | ||
| this; | ||
| }`, | ||
| }); |
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.