Add codefix for --noImplicitThis #27565

ghost · 2018-10-05T02:00:29Z

If there's no containing function, no fix.
If this is defined outside the containing function, convert the containing function to an arrow function, since that's the most likely error.
If not, inside a JS function:
- Tag as a @class if this is an assignment.

Removed features that are redundant with inferFromUsage now.

sandersn · 2018-10-08T23:01:18Z

src/services/codefixes/fixImplicitThis.ts

+            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)) {


It's kind of confusing to refer to the same property as both name and fn.name in this function.

sandersn · 2018-10-08T23:20:54Z

src/services/textChanges.ts

@@ -339,6 +339,22 @@ namespace ts.textChanges {
            this.insertText(sourceFile, token.getStart(sourceFile), text);
        }

+        public insertJsdocCommentBefore(sourceFile: SourceFile, fn: FunctionDeclaration | FunctionExpression, tag: JSDocTag): void {


I don't think this will work right for function expressions immediately preceded by text.

sandersn · 2018-10-08T23:23:43Z

src/services/textChanges.ts

-                    : 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 {


where is this used now that there is an export here?

sandersn

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.

amcasey · 2018-10-09T23:32:32Z

src/services/textChanges.ts

@@ -1103,15 +1138,23 @@ namespace ts.textChanges {
                    deleteImportBinding(changes, sourceFile, node as NamespaceImport);
                    break;

+                case SyntaxKind.SemicolonToken:


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

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

amcasey · 2018-10-09T23:37:03Z

src/services/codefixes/fixImplicitThis.ts

+        const token = getTokenAtPosition(sourceFile, pos);
+        Debug.assert(token.kind === SyntaxKind.ThisKeyword);
+
+        const fn = getThisContainer(token, /*includeArrowFunctions*/ false);


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

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.

amcasey · 2018-10-09T23:38:46Z

src/services/codefixes/fixImplicitThis.ts

+                    changes.delete(sourceFile, name);
+                }
+                changes.insertText(sourceFile, body.pos, " =>");
+                return [Diagnostics.Convert_function_expression_0_to_arrow_function, name ? name.text : "<anonymous>"];


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.

amcasey · 2018-10-09T23:39:27Z

src/services/codefixes/fixImplicitThis.ts

+                if (name) {
+                    changes.delete(sourceFile, name);
+                }
+                changes.insertText(sourceFile, body.pos, " =>");


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

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

amcasey · 2018-10-09T23:40:40Z

src/services/codefixes/fixImplicitThis.ts

+                return [Diagnostics.Convert_function_declaration_0_to_arrow_function, name!.text];
+            }
+        }
+        else { // No outer 'this', must add an annotation


Or a parameter?

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.

sandersn · 2020-03-16T18:08:22Z

This PR is old enough that it's taking a while to bring up-to-date. Currently:

Merged from master.
Builds.
All tests passes except for the newly added test, plus a test for another codefix (infer from usage) that now interferes with it.

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?

ghost force-pushed the fixImplicitThis branch 2 times, most recently from 3748c71 to ddc7797 Compare October 8, 2018 18:00

Add codefix for --noImplicitThis

cb30622

ghost force-pushed the fixImplicitThis branch from ddc7797 to cb30622 Compare October 8, 2018 18:08

ghost requested review from amcasey, DanielRosenwasser and sandersn October 8, 2018 18:40

sandersn reviewed Oct 8, 2018

View reviewed changes

sandersn self-assigned this Oct 8, 2018

sandersn reviewed Oct 8, 2018

View reviewed changes

sandersn requested changes Oct 8, 2018

View reviewed changes

ghost mentioned this pull request Oct 9, 2018

inferFromUsage codefix now emits JSDoc in JS files #27610

Merged

amcasey reviewed Oct 9, 2018

View reviewed changes

Andy Hanson added 2 commits November 13, 2018 11:17

Merge branch 'master' into fixImplicitThis

8ae463f

Code review

e74d5ee

ghost force-pushed the fixImplicitThis branch from d5cdff9 to e74d5ee Compare November 13, 2018 22:12

sandersn added For Milestone Bug PRs that fix a bug with a specific milestone For Backlog Bug PRs that fix a backlog bug and removed For Milestone Bug PRs that fix a bug with a specific milestone labels Feb 1, 2020

sandersn added 2 commits March 16, 2020 08:17

Merge branch 'master' into fixImplicitThis

87f791f

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.

Back to building post-merge

665ffc0

sandersn added 5 commits March 17, 2020 08:33

Remove redundant functions + update tests

dc29946

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

8074c74

Don't need to add `@this` anymore either since inferFromUsage will do that.

More baseline updates

d0b9505

From moving inferFromUsage down in priority I think?

remove now-redundant ad-hoc jsdoc emit

1a347bf

fix more bad merge

7fb1bd2

sandersn approved these changes Mar 17, 2020

View reviewed changes

sandersn merged commit b0450ae into master Mar 17, 2020

sandersn deleted the fixImplicitThis branch March 17, 2020 20:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add codefix for --noImplicitThis #27565

Add codefix for --noImplicitThis #27565

ghost commented Oct 5, 2018 •

edited by sandersn

Loading

sandersn Oct 8, 2018

sandersn Oct 8, 2018

sandersn Oct 8, 2018

sandersn left a comment

amcasey Oct 9, 2018

ghost Nov 13, 2018

amcasey Oct 9, 2018

ghost Nov 13, 2018

amcasey Oct 9, 2018 •

edited by sandersn

Loading

amcasey Oct 9, 2018

ghost Nov 13, 2018

amcasey Oct 9, 2018

sandersn commented Mar 16, 2020

Add codefix for --noImplicitThis #27565

Add codefix for --noImplicitThis #27565

Conversation

ghost commented Oct 5, 2018 • edited by sandersn Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey Oct 9, 2018 • edited by sandersn Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn commented Mar 16, 2020

ghost commented Oct 5, 2018 •

edited by sandersn

Loading

amcasey Oct 9, 2018 •

edited by sandersn

Loading