no-invalid-this: false positive for method syntax (#4034)

* 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

src/rules/noInvalidThisRule.ts

@@ -61,41 +61,63 @@ export class Rule extends Lint.Rules.AbstractRule {
     }
 }
 
+const enum parentType {
+    None,
+    Class,
+    ClassMethod,
+    BoundRegularFunction,
+    UnboundRegularFunction,
+}
+const thisAllowedParents = new Set([parentType.ClassMethod, parentType.BoundRegularFunction]);
+
 function walk(ctx: Lint.WalkContext<boolean>): void {
     const { sourceFile, options: checkFuncInMethod } = ctx;
+
+    let currentParent: parentType = parentType.None;
     let inClass = false;
-    let inFunctionInClass = false;
 
     ts.forEachChild(sourceFile, function cb(node: ts.Node) {
+        const originalParent = currentParent;
+        const originalInClass = inClass;
         switch (node.kind) {
             case ts.SyntaxKind.ClassDeclaration:
             case ts.SyntaxKind.ClassExpression:
-                if (!inClass) {
-                    inClass = true;
-                    ts.forEachChild(node, cb);
-                    inClass = false;
-                    return;
-                }
-                break;
+                inClass = true;
+                currentParent = parentType.Class;
+                ts.forEachChild(node, cb);
+                currentParent = originalParent;
+                inClass = originalInClass;
+                return;
 
+            case ts.SyntaxKind.MethodDeclaration:
+            case ts.SyntaxKind.GetAccessor:
+            case ts.SyntaxKind.SetAccessor:
+            case ts.SyntaxKind.Constructor:
+            case ts.SyntaxKind.PropertyDeclaration:
             case ts.SyntaxKind.FunctionDeclaration:
             case ts.SyntaxKind.FunctionExpression:
-                if ((node as ts.FunctionLikeDeclaration).parameters.some(isThisParameter)) {
+                if (currentParent === parentType.Class) {
+                    currentParent = parentType.ClassMethod;
+                    ts.forEachChild(node, cb);
+                    currentParent = originalParent;
                     return;
-                }
-                if (inClass) {
-                    inFunctionInClass = true;
+                } else {
+                    currentParent
+                        = (node as ts.FunctionLikeDeclaration).parameters.some(isThisParameter)
+                        ? parentType.BoundRegularFunction
+                        : parentType.UnboundRegularFunction;
                     ts.forEachChild(node, cb);
-                    inFunctionInClass = false;
+                    currentParent = originalParent;
                     return;
                 }
-                break;
 
             case ts.SyntaxKind.ThisKeyword:
-                if (!inClass) {
-                    ctx.addFailureAtNode(node, Rule.FAILURE_STRING_OUTSIDE);
-                } else if (checkFuncInMethod && inFunctionInClass) {
-                    ctx.addFailureAtNode(node, Rule.FAILURE_STRING_INSIDE);
+                if (!thisAllowedParents.has(currentParent)) {
+                    if (!inClass) {
+                        ctx.addFailureAtNode(node, Rule.FAILURE_STRING_OUTSIDE);
+                    } else if (checkFuncInMethod) {
+                        ctx.addFailureAtNode(node, Rule.FAILURE_STRING_INSIDE);
+                    }
                 }
                 return;
         }

test/rules/no-invalid-this/enabled/test.ts.lint

@@ -1,3 +1,23 @@
+function setSomeProperty(this: { someProperty: number }) {
+    this.someProperty = 7;
+}
+
+function bindProperty(this: { someProperty: number }, fn: Function) {
+    return fn.bind(this);
+}
+
+const objectLiteralStyle = {
+    bindProperty: function(this: { someProperty: number }, fn: Function) {
+        return fn.bind(this);
+    },
+};
+
+const objectMethodStyle = {
+    bindProperty(this: { someProperty: number }, fn: Function) {
+        return fn.bind(this);
+    },
+};
+
 export function f<T>(this: Observable<T>): Observable<T> {
     const nestedFunction = function(this) => {
         console.log(this)
@@ -10,6 +30,7 @@ class ContextualThisClass {
         let nestedFunction: (this: number[]) => number[] = function(this) {
             [3,4].forEach(function(nr){
                 console.log(this);
+                            ~~~~           [the "this" keyword is disallowed in function bodies inside class methods, use arrow functions instead]
             });
             return this.map(i=>i);
         };
@@ -67,6 +88,25 @@ class AClass {
         [3,4].forEach(badFunction);
         let badFunction = nr => console.log(this.x === nr);
     }
+
+    public fMethod() {
+        const objectLiteralStyle = {
+            bindProperty: function(this: { someProperty: number }, fn: Function) {
+                return fn.bind(this);
+            },
+        };
+
+        const objectMethodStyle = {
+            bindProperty(this: { someProperty: number }, fn: Function) {
+                return fn.bind(this);
+            },
+        };
+
+        return {
+            objectLiteralStyle,
+            objectMethodStyle,
+        };
+    }
 }
 
 const AClassExpression = class {