GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Add codefix for --noImplicitThis · Pull Request #27565 · microsoft/TypeScript

Conversation

Deleted user (ghost) commented Oct 5, 2018 •

edited by sandersn

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

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.

Edit by Nathan Shively-Sanders (@sandersn):

Removed features that are redundant with inferFromUsage now.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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 Daniel Rosenwasser (DanielRosenwasser), Andrew Casey (amcasey) and Nathan Shively-Sanders (sandersn)

October 8, 2018 18:40

Nathan Shively-Sanders (sandersn) reviewed

Oct 8, 2018

View reviewed changes

Comment thread

src/services/codefixes/fixImplicitThis.ts Outdated

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

Nathan Shively-Sanders (sandersn) Oct 8, 2018

Copy link

Copy Markdown

Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Nathan Shively-Sanders (sandersn) self-assigned this

Oct 8, 2018

Nathan Shively-Sanders (sandersn) reviewed

Oct 8, 2018

View reviewed changes

Comment thread

src/services/textChanges.ts Outdated

		this.insertText(sourceFile, token.getStart(sourceFile), text);
		}

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

Nathan Shively-Sanders (sandersn) Oct 8, 2018

Copy link

Copy Markdown

Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Nathan Shively-Sanders (sandersn) reviewed

Oct 8, 2018

View reviewed changes

Comment thread

src/services/textChanges.ts Outdated

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

Nathan Shively-Sanders (sandersn) Oct 8, 2018

Copy link

Copy Markdown

Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Nathan Shively-Sanders (sandersn) requested changes

Oct 8, 2018

View reviewed changes

Nathan Shively-Sanders (sandersn) left a comment

Copy link

Copy Markdown

Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ghost mentioned this pull request

Oct 9, 2018

inferFromUsage codefix now emits JSDoc in JS files #27610

Merged

Andrew Casey (amcasey) reviewed

Oct 9, 2018

View reviewed changes

Comment thread

src/services/textChanges.ts

		deleteImportBinding(changes, sourceFile, node as NamespaceImport);
		break;

		case SyntaxKind.SemicolonToken:

Andrew Casey (amcasey) Oct 9, 2018

Copy link

Copy Markdown

Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Deleted user (ghost) Nov 13, 2018

Copy link

Copy Markdown

Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/services/codefixes/fixImplicitThis.ts

		const token = getTokenAtPosition(sourceFile, pos);
		Debug.assert(token.kind === SyntaxKind.ThisKeyword);

		const fn = getThisContainer(token, /includeArrowFunctions/ false);

Andrew Casey (amcasey) Oct 9, 2018

Copy link

Copy Markdown

Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Deleted user (ghost) Nov 13, 2018

Copy link

Copy Markdown

Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/services/codefixes/fixImplicitThis.ts Outdated

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

Andrew Casey (amcasey) Oct 9, 2018 •

edited by sandersn

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/services/codefixes/fixImplicitThis.ts

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

Andrew Casey (amcasey) Oct 9, 2018

Copy link

Copy Markdown

Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Deleted user (ghost) Nov 13, 2018

Copy link

Copy Markdown

Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/services/codefixes/fixImplicitThis.ts Outdated

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

Andrew Casey (amcasey) Oct 9, 2018

Copy link

Copy Markdown

Member

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.

Or a parameter?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

Nathan Shively-Sanders (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

Nathan Shively-Sanders (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

Nathan Shively-Sanders (sandersn) commented Mar 16, 2020

Copy link

Copy Markdown

Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Nathan Shively-Sanders (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

Nathan Shively-Sanders (sandersn) approved these changes

Mar 17, 2020

View reviewed changes

Nathan Shively-Sanders (sandersn) merged commit b0450ae into master

Mar 17, 2020

Nathan Shively-Sanders (sandersn) deleted the fixImplicitThis branch

March 17, 2020 20:00

Microsoft (microsoft) locked as resolved and limited conversation to collaborators

Oct 21, 2025

Reviewers

Daniel Rosenwasser (DanielRosenwasser)

Awaiting requested review from DanielRosenwasser

Uh oh!

There was an error while loading. Please reload this page.

Assignees

Nathan Shively-Sanders (sandersn)

Labels

For Backlog Bug

PRs that fix a backlog bug

Projects

PR Backlog

Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

Deleted user (ghost) commented Oct 5, 2018 • edited by sandersn Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nathan Shively-Sanders (sandersn) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andrew Casey (amcasey) Oct 9, 2018 • edited by sandersn Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nathan Shively-Sanders (sandersn) commented Mar 16, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Deleted user (ghost) commented Oct 5, 2018 •

edited by sandersn

Loading

Uh oh!

There was an error while loading. Please reload this page.

Andrew Casey (amcasey) Oct 9, 2018 •

edited by sandersn

Loading

Uh oh!

There was an error while loading. Please reload this page.