no-invalid-this: false positive for method syntax #4034
Conversation
|
Thanks for your interest in palantir/tslint, @amacleay! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request. |
amacleay
force-pushed
the
no-invalid-this-method-declaration
branch
from
8167440 to
65e2dfd
Compare
|
cc @aervin - I changed a line you added which I believe to be a mistake (https://github.com/palantir/tslint/pull/4034/files#diff-7062f9042aeae109e75e6f78d7430418R33) so my changes deserve extra scrutiny because I'm pretty often wrong! |
|
Hi all! Feedback would be much appreciated if someone has an opportunity to provide it. I think this should be ready to go as-is if the changes seem appropriate to fix the bug (or, what I believe to be a bug). |
src/rules/noInvalidThisRule.ts
Outdated
| @@ -59,39 +59,64 @@ export class Rule extends Lint.Rules.AbstractRule { | |||
|
|
|||
| function walk(ctx: Lint.WalkContext<boolean>): void { | |||
| const { sourceFile, options: checkFuncInMethod } = ctx; | |||
|
|
|||
| const enum parentType { | |||
There was a problem hiding this comment.
Seems semantically odd to recreate this each time walk is called. We don't need that, so this should be moved outside of the walk function.
src/rules/noInvalidThisRule.ts
Outdated
| UnboundRegularFunction, | ||
| } | ||
|
|
||
| function thisIsAllowed(parent: parentType): boolean { |
There was a problem hiding this comment.
Nit: instead of this three-line function, how about const thisAllowedTypes = new Set([ParentType.ClassMethod, ParentType.BoundRegularFunction]);, and use its .has?
|
|
* 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
|
Pushing a rebase to hopefully resolve the failure |
amacleay
force-pushed
the
no-invalid-this-method-declaration
branch
from
cad840a to
17229fc
Compare
| @@ -61,41 +61,63 @@ export class Rule extends Lint.Rules.AbstractRule { | |||
| } | |||
| } | |||
|
|
|||
| const enum parentType { | |||
There was a problem hiding this comment.
nit, enums should be CamelCase: ParentType.
| const enum parentType { | |
| const enum ParentType { |
and decide if it is allowed to refer to
thisPR checklist
Overview of change:
Previously, the
no-invalid-thisrule only had shallow tracking of context.thisshould be allowed if any only iff the closest parent non-arrow function is either a) a class method, or b) a function with athistype parameter.There were several drawbacks of not tracking context directly:
thistype parameter and a function inside a function with athistype parameter - in fact, there was a test checked in which enforced bad behavior:tslint/test/rules/no-invalid-this/enabled/test.ts.lint
Line 12 in b104e5a
thison this line does not inherit type parameterIs there anything you'd like reviewers to focus on?
CHANGELOG.md entry:
[bugfix]
no-invalid-thisrule fixes false positives on method-like syntax and false negatives on nested functions