Add prefer-function-over-method rule

docs/_data/rules.json

@@ -1018,6 +1018,24 @@
     "type": "functionality",
     "typescriptOnly": false
   },
+  {
+    "ruleName": "no-unused-this",
+    "description": "Warns for methods that do not use 'this'.",
+    "optionsDescription": "\n\"allow-public\" excludes public methods.\n\"allow-protected\" excludes protected methods.",
+    "options": {
+      "type": "string",
+      "enum": [
+        "allow-public",
+        "allow-protected"
+      ]
+    },
+    "optionExamples": [
+      "true",
+      "[true, \"allow-public\", \"allow-protected\"]"
+    ],
+    "type": "style",
+    "typescriptOnly": false
+  },
   {
     "ruleName": "no-unused-variable",
     "deprecationMessage": "Use the tsc compiler options --noUnusedParameters and --noUnusedLocals instead.",
@@ -1720,7 +1738,7 @@
     "ruleName": "whitespace",
     "description": "Enforces whitespace style conventions.",
     "rationale": "Helps maintain a readable, consistent style in your codebase.",
-    "optionsDescription": "\nSeven arguments may be optionally provided:\n\n* `\"check-branch\"` checks branching statements (`if`/`else`/`for`/`while`) are followed by whitespace.\n* `\"check-decl\"`checks that variable declarations have whitespace around the equals token.\n* `\"check-operator\"` checks for whitespace around operator tokens.\n* `\"check-module\"` checks for whitespace in import & export statements.\n* `\"check-separator\"` checks for whitespace after separator tokens (`,`/`;`).\n* `\"check-type\"` checks for whitespace before a variable type specification.\n* `\"check-typecast\"` checks for whitespace between a typecast and its target.",
+    "optionsDescription": "\nSeven arguments may be optionally provided:\n\n* `\"check-branch\"` checks branching statements (`if`/`else`/`for`/`while`) are followed by whitespace.\n* `\"check-decl\"`checks that variable declarations have whitespace around the equals token.\n* `\"check-operator\"` checks for whitespace around operator tokens.\n* `\"check-module\"` checks for whitespace in import & export statements.\n* `\"check-separator\"` checks for whitespace after separator tokens (`,`/`;`).\n* `\"check-type\"` checks for whitespace before a variable type specification.\n* `\"check-typecast\"` checks for whitespace between a typecast and its target.\n* `\"check-preblock\"` checks for whitespace before the opening brace of a block",
     "options": {
       "type": "array",
       "items": {
@@ -1732,7 +1750,8 @@
           "check-module",
           "check-separator",
           "check-type",
-          "check-typecast"
+          "check-typecast",
+          "check-preblock"
         ]
       },
       "minLength": 0,

docs/rules/no-unused-this/index.html

@@ -0,0 +1,28 @@
+---
+ruleName: no-unused-this
+description: Warns for methods that do not use 'this'.
+optionsDescription: |-
+
+  "allow-public" excludes public methods.
+  "allow-protected" excludes protected methods.
+options:
+  type: string
+  enum:
+    - allow-public
+    - allow-protected
+optionExamples:
+  - 'true'
+  - '[true, "allow-public", "allow-protected"]'
+type: style
+typescriptOnly: false
+layout: rule
+title: 'Rule: no-unused-this'
+optionsJSON: |-
+  {
+    "type": "string",
+    "enum": [
+      "allow-public",
+      "allow-protected"
+    ]
+  }
+---

docs/rules/whitespace/index.html

@@ -13,6 +13,7 @@
   * `"check-separator"` checks for whitespace after separator tokens (`,`/`;`).
   * `"check-type"` checks for whitespace before a variable type specification.
   * `"check-typecast"` checks for whitespace between a typecast and its target.
+  * `"check-preblock"` checks for whitespace before the opening brace of a block
 options:
   type: array
   items:
@@ -25,6 +26,7 @@
       - check-separator
       - check-type
       - check-typecast
+      - check-preblock
   minLength: 0
   maxLength: 7
 optionExamples:
@@ -45,7 +47,8 @@
         "check-module",
         "check-separator",
         "check-type",
-        "check-typecast"
+        "check-typecast",
+        "check-preblock"
       ]
     },
     "minLength": 0,

src/rules/noUnusedThisRule.ts

@@ -0,0 +1,132 @@
+/**
+ * @license
+ * Copyright 2017 Palantir Technologies, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import * as ts from "typescript";
+
+import * as Lint from "../index";
+
+const OPTION_ALLOW_PUBLIC = "allow-public";
+const OPTION_ALLOW_PROTECTED = "allow-protected";
+
+export class Rule extends Lint.Rules.AbstractRule {
+    /* tslint:disable:object-literal-sort-keys */
+    public static metadata: Lint.IRuleMetadata = {
+        ruleName: "no-unused-this",
+        description: "Warns for methods that do not use 'this'.",
+        optionsDescription: Lint.Utils.dedent`
+            "${OPTION_ALLOW_PUBLIC}" excludes public methods.
+            "${OPTION_ALLOW_PROTECTED}" excludes protected methods.`,
+        options: {
+            type: "string",
+            enum: [OPTION_ALLOW_PUBLIC, OPTION_ALLOW_PROTECTED],
+        },
+        optionExamples: [
+            "true",
+            `[true, "${OPTION_ALLOW_PUBLIC}", "${OPTION_ALLOW_PROTECTED}"]`,
+        ],
+        type: "style",
+        typescriptOnly: false,
+    };
+    /* tslint:enable:object-literal-sort-keys */
+
+    public static FAILURE_STRING = "Method does not use 'this'. Use a function instead.";
+
+    public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] {
+        return this.applyWithWalker(new Walker(sourceFile, this.getOptions()));
+    }
+}
+
+class Walker extends Lint.RuleWalker {
+    private allowPublic = this.hasOption(OPTION_ALLOW_PUBLIC);
+    private allowProtected = this.hasOption(OPTION_ALLOW_PROTECTED);
+    private stack: ThisUsed[] = [];
+
+    public visitNode(node: ts.Node) {
+        switch (node.kind) {
+            case ts.SyntaxKind.ThisKeyword:
+            case ts.SyntaxKind.SuperKeyword:
+                this.setThisUsed(node);
+                break;
+
+            case ts.SyntaxKind.MethodDeclaration:
+                const { name } = node as ts.MethodDeclaration;
+                const usesThis = this.withThisScope(
+                    name.kind === ts.SyntaxKind.Identifier ? name.text : undefined,
+                    () => super.visitNode(node));
+                if (!usesThis &&
+                    node.parent!.kind !== ts.SyntaxKind.ObjectLiteralExpression &&
+                    this.shouldWarnForModifiers(node as ts.MethodDeclaration)) {
+                    this.addFailureAtNode((node as ts.MethodDeclaration).name, Rule.FAILURE_STRING);
+                }
+                break;
+
+            case ts.SyntaxKind.FunctionDeclaration:
+            case ts.SyntaxKind.FunctionExpression:
+                this.withThisScope(undefined, () => super.visitNode(node));
+                break;
+
+            default:
+                super.visitNode(node);
+        }
+    }
+
+    private setThisUsed(node: ts.Node) {
+        const cur = this.stack[this.stack.length - 1];
+        if (cur && !isRecursiveCall(node, cur)) {
+            cur.isThisUsed = true;
+        }
+    }
+
+    private withThisScope(name: string | undefined, recur: () => void): boolean {
+        this.stack.push({ name, isThisUsed: false });
+        recur();
+        return this.stack.pop()!.isThisUsed;
+    }
+
+    private shouldWarnForModifiers(node: ts.MethodDeclaration): boolean {
+        if (Lint.hasModifier(node.modifiers, ts.SyntaxKind.StaticKeyword)) {
+            return false;
+        }
+        // TODO: Also return false if it's marked "override" (https://github.com/palantir/tslint/pull/2037)
+
+        switch (methodVisibility(node)) {
+            case Visibility.Public:
+                return !this.allowPublic;
+            case Visibility.Protected:
+                return !this.allowProtected;
+            default:
+                return true;
+        }
+    }
+}
+
+interface ThisUsed { readonly name: string | undefined; isThisUsed: boolean; }
+
+function isRecursiveCall(thisOrSuper: ts.Node, cur: ThisUsed) {
+    const parent = thisOrSuper.parent!;
+    return thisOrSuper.kind === ts.SyntaxKind.ThisKeyword &&
+        parent.kind === ts.SyntaxKind.PropertyAccessExpression &&
+        (parent as ts.PropertyAccessExpression).name.text === cur.name;
+}
+
+const enum Visibility { Public, Protected, Private }
+
+function methodVisibility(node: ts.MethodDeclaration): Visibility {
+    return Lint.hasModifier(node.modifiers, ts.SyntaxKind.PrivateKeyword) ? Visibility.Private :
+        Lint.hasModifier(node.modifiers, ts.SyntaxKind.ProtectedKeyword) ? Visibility.Protected :
+        Visibility.Public;
+}

test/rules/no-unused-this/allow-protected/test.ts.lint

@@ -0,0 +1,9 @@
+class C {
+    a() {}
+    ~ [0]
+    protected b() {}
+    private c() {}
+            ~ [0]
+}
+
+[0]: Method does not use 'this'. Use a function instead.

test/rules/no-unused-this/allow-protected/tslint.json

@@ -0,0 +1,5 @@
+{
+  "rules": {
+    "no-unused-this": [true, "allow-protected"]
+  }
+}

test/rules/no-unused-this/allow-public-and-protected/test.ts.lint

@@ -0,0 +1,8 @@
+class C {
+    a() {}
+    protected b() {}
+    private c() {}
+            ~ [0]
+}
+
+[0]: Method does not use 'this'. Use a function instead.

test/rules/no-unused-this/allow-public-and-protected/tslint.json

@@ -0,0 +1,5 @@
+{
+  "rules": {
+    "no-unused-this": [true, "allow-public", "allow-protected"]
+  }
+}

test/rules/no-unused-this/allow-public/test.ts.lint

@@ -0,0 +1,9 @@
+class C {
+    a() {}
+    protected b() {}
+              ~ [0]
+    private c() {}
+            ~ [0]
+}
+
+[0]: Method does not use 'this'. Use a function instead.

test/rules/no-unused-this/allow-public/tslint.json

@@ -0,0 +1,5 @@
+{
+  "rules": {
+    "no-unused-this": [true, "allow-public"]
+  }
+}

test/rules/no-unused-this/default/test.js.lint

@@ -0,0 +1,4 @@
+class C {
+    a() {}
+    ~ [Method does not use 'this'. Use a function instead.]
+}

test/rules/no-unused-this/default/test.ts.lint

@@ -0,0 +1,51 @@
+class C {
+    static s() {}
+
+    plain() {}
+    ~~~~~ [0]
+
+    thisInFunction() {
+    ~~~~~~~~~~~~~~ [0]
+        function() {
+            this;
+        }
+    }
+
+    thisInObjectMethod() {
+    ~~~~~~~~~~~~~~~~~~ [0]
+        return { x() { this; } }
+    }
+
+    recur() {
+    ~~~~~ [0]
+        this.recur();
+    }
+
+    thisInParameter(this: string) {}
+    ~~~~~~~~~~~~~~~ [0]
+
+    thisInParameterDefault(x = this) {}
+
+    thisUsed() {
+        this;
+    }
+
+    super() {
+        super;
+    }
+
+    protected protected() {}
+              ~~~~~~~~~ [0]
+
+    private private() {}
+            ~~~~~~~ [0]
+
+    [Symbol.iterator]() {}
+    ~~~~~~~~~~~~~~~~~ [0]
+}
+
+const o = {
+    x() {}
+}
+
+[0]: Method does not use 'this'. Use a function instead.

test/rules/no-unused-this/default/tslint.json

@@ -0,0 +1,8 @@
+{
+  "rules": {
+    "no-unused-this": true
+  },
+  "jsRules": {
+    "no-unused-this": true
+  }
+}