Uh oh!
There was an error while loading. Please reload this page.
Add codefix for --noImplicitThis - #27565
Conversation
3748c71 to
ddc7797Compare
| 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)) { |
There was a problem hiding this comment.
It's kind of confusing to refer to the same property as both name and fn.name in this function.
| this.insertText(sourceFile, token.getStart(sourceFile), text); | ||
| } | ||
| public insertJsdocCommentBefore(sourceFile: SourceFile, fn: FunctionDeclaration | FunctionExpression, tag: JSDocTag): void { |
There was a problem hiding this comment.
I don't think this will work right for function expressions immediately preceded by text.
| : 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 { |
There was a problem hiding this comment.
where is this used now that there is an export here?
Nathan Shively-Sanders (sandersn)
left a comment
There was a problem hiding this comment.
I don't think the emit is correct for FunctionExpressions that are preceded by non-whitespace text on their line. Probably you should use code from #27610 and then I can grab it from master after this goes in and use it there.
| deleteImportBinding(changes, sourceFile, node as NamespaceImport); | ||
| break; | ||
| case SyntaxKind.SemicolonToken: |
There was a problem hiding this comment.
I could be miscounting the scopes, but why would deleteDeclaration be called on a semicolon?
There was a problem hiding this comment.
convertToEs6Module deletes semicolons when changing an assignment statement to a function declaration.
| const token = getTokenAtPosition(sourceFile, pos); | ||
| Debug.assert(token.kind === SyntaxKind.ThisKeyword); | ||
| const fn = getThisContainer(token, /*includeArrowFunctions*/ false); |
There was a problem hiding this comment.
Could we be in a default argument of the function? Does that matter?
There was a problem hiding this comment.
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.
| changes.delete(sourceFile, name); | ||
| } | ||
| changes.insertText(sourceFile, body.pos, " =>"); | ||
| return [Diagnostics.Convert_function_expression_0_to_arrow_function, name ? name.text : "<anonymous>"]; |
There was a problem hiding this comment.
Do we have a constant for "<anonymous>" somewhere? It seems unlikely that this is the first time we've had to name an anonymous symbol.
| if (name) { | ||
| changes.delete(sourceFile, name); | ||
| } | ||
| changes.insertText(sourceFile, body.pos, " =>"); |
There was a problem hiding this comment.
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.
There will be in #28250, we can combine these then.
| return [Diagnostics.Convert_function_declaration_0_to_arrow_function, name!.text]; | ||
| } | ||
| } | ||
| else { // No outer 'this', must add an annotation |
DOES NOT BUILD, because I only fixed the conflicts, not the build errors. I'm going to push this up and sync on my desktop machine because it's so much easier to work there.
This PR is old enough that it's taking a while to bring up-to-date. Currently:
|
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.
Don't need to add `@this` anymore either since inferFromUsage will do that.
From moving inferFromUsage down in priority I think?
thisis defined outside the containing function, convert the containing function to an arrow function, since that's the most likely error.@classif this is an assignment.Edit by Nathan Shively-Sanders (@sandersn):