This repository has been archived by the owner on Mar 25, 2021. It is now read-only.
[bug-fix] no-invalid-this / ignore functions w "this" param #3267
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aervin
changed the title
ajafff
suggested changes
Oct 1, 2017
src/rules/noInvalidThisRule.ts
Outdated
| @@ -76,6 +78,9 @@ function walk(ctx: Lint.WalkContext<boolean>): void { | |||
|
|
|||
| case ts.SyntaxKind.FunctionDeclaration: | |||
| case ts.SyntaxKind.FunctionExpression: | |||
| if (hasContextualThis(node as ts.FunctionDeclaration)) { | |||
There was a problem hiding this comment.
ts.FunctionLikeDeclaration for correctness
src/rules/noInvalidThisRule.ts
Outdated
| @@ -96,3 +101,7 @@ function walk(ctx: Lint.WalkContext<boolean>): void { | |||
| ts.forEachChild(node, cb); | |||
| }); | |||
| } | |||
|
|
|||
| function hasContextualThis(node: ts.FunctionDeclaration): boolean { | |||
| return node.parameters.some((param) => param.name.getText() === "this"); | |||
There was a problem hiding this comment.
return node.parameters.some(isThisParameter);| @@ -1,3 +1,21 @@ | |||
| export function f<T>(this: Observable<T>): Observable<T> { | |||
| const nestedFunction = (this) => { | |||
There was a problem hiding this comment.
arrow functions cannot have a this parameter
We don't need to worry about it, because TypeScript shows an error. But we shouldn't use it in any test
src/rules/noInvalidThisRule.ts
Outdated
| @@ -96,3 +101,7 @@ function walk(ctx: Lint.WalkContext<boolean>): void { | |||
| ts.forEachChild(node, cb); | |||
| }); | |||
| } | |||
|
|
|||
| function hasContextualThis(node: ts.FunctionDeclaration): boolean { | |||
There was a problem hiding this comment.
please rename to hasThisParameter (or completely inline it)
This has nothing to do with contextual this. It's about explicitly declared this parameters.
Contextual this looks as follows:
declare function callWithThis(fn: (this: Array<any>) => void): void;
callWithThis(function() {
console.log(this.length); // this is allowed, because `this` is contextually typed to be an array
});To detect that you would need the TypeChecker. I don't think that's worth it, because TypeScript provides the option --noImplicitThis which makes this rule obsolete.
… arrow function w this param from test
aervin
changed the title
aervin
changed the title
ajafff
approved these changes
Oct 2, 2017
|
Thanks @aervin |
HyphnKnight
pushed a commit
to HyphnKnight/tslint
that referenced
this pull request
Apr 9, 2018
amacleay
added a commit
to amacleay/tslint
that referenced
this pull request
Jul 12, 2018
* Keep track of evaluation context: look to the closest parent function and decide if it is allowed to refer to `this` * Fix a false negative in the tests from palantir#3267
4 tasks
amacleay
added a commit
to amacleay/tslint
that referenced
this pull request
Jul 12, 2018
* Keep track of evaluation context: look to the closest parent function and decide if it is allowed to refer to `this` * Fix a false negative in the tests from palantir#3267
amacleay
added a commit
to amacleay/tslint
that referenced
this pull request
Nov 7, 2018
* Keep track of evaluation context: look to the closest parent function and decide if it is allowed to refer to `this` * Fix a false negative in the tests from palantir#3267
ericanderson
pushed a commit
that referenced
this pull request
Dec 12, 2018
* no-invalid-this: false positive for method syntax * Keep track of evaluation context: look to the closest parent function and decide if it is allowed to refer to `this` * Fix a false negative in the tests from #3267 * no-invalid-this: stylistic cleanup
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
PR checklist
Overview of change:
Functions with contextual "this" will be ignored.
Is there anything you'd like reviewers to focus on?
Are additional test cases needed?