From eff3bc882f8af302a1abaaea0cb84804c72240cc Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Fri, 20 Jan 2017 19:18:24 -0800 Subject: [PATCH] Add `prefer-function-over-method` rule (#2037) --- docs/rules/no-unused-this/index.html | 28 ++++ src/rules/preferFunctionOverMethodRule.ts | 132 ++++++++++++++++++ .../allow-protected/test.ts.lint | 9 ++ .../allow-protected/tslint.json | 5 + .../allow-public-and-protected/test.ts.lint | 8 ++ .../allow-public-and-protected/tslint.json | 5 + .../allow-public/test.ts.lint | 9 ++ .../allow-public/tslint.json | 5 + .../default/test.js.lint | 4 + .../default/test.ts.lint | 51 +++++++ .../default/tslint.json | 8 ++ 11 files changed, 264 insertions(+) create mode 100644 docs/rules/no-unused-this/index.html create mode 100644 src/rules/preferFunctionOverMethodRule.ts create mode 100644 test/rules/prefer-function-over-method/allow-protected/test.ts.lint create mode 100644 test/rules/prefer-function-over-method/allow-protected/tslint.json create mode 100644 test/rules/prefer-function-over-method/allow-public-and-protected/test.ts.lint create mode 100644 test/rules/prefer-function-over-method/allow-public-and-protected/tslint.json create mode 100644 test/rules/prefer-function-over-method/allow-public/test.ts.lint create mode 100644 test/rules/prefer-function-over-method/allow-public/tslint.json create mode 100644 test/rules/prefer-function-over-method/default/test.js.lint create mode 100644 test/rules/prefer-function-over-method/default/test.ts.lint create mode 100644 test/rules/prefer-function-over-method/default/tslint.json diff --git a/docs/rules/no-unused-this/index.html b/docs/rules/no-unused-this/index.html new file mode 100644 index 00000000000..85b84209125 --- /dev/null +++ b/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" + ] + } +--- \ No newline at end of file diff --git a/src/rules/preferFunctionOverMethodRule.ts b/src/rules/preferFunctionOverMethodRule.ts new file mode 100644 index 00000000000..a806a7b1c9f --- /dev/null +++ b/src/rules/preferFunctionOverMethodRule.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: "prefer-function-over-method", + 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; +} diff --git a/test/rules/prefer-function-over-method/allow-protected/test.ts.lint b/test/rules/prefer-function-over-method/allow-protected/test.ts.lint new file mode 100644 index 00000000000..e4df4b33c93 --- /dev/null +++ b/test/rules/prefer-function-over-method/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. diff --git a/test/rules/prefer-function-over-method/allow-protected/tslint.json b/test/rules/prefer-function-over-method/allow-protected/tslint.json new file mode 100644 index 00000000000..c5c1ff87b7e --- /dev/null +++ b/test/rules/prefer-function-over-method/allow-protected/tslint.json @@ -0,0 +1,5 @@ +{ + "rules": { + "prefer-function-over-method": [true, "allow-protected"] + } +} diff --git a/test/rules/prefer-function-over-method/allow-public-and-protected/test.ts.lint b/test/rules/prefer-function-over-method/allow-public-and-protected/test.ts.lint new file mode 100644 index 00000000000..0642288c026 --- /dev/null +++ b/test/rules/prefer-function-over-method/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. diff --git a/test/rules/prefer-function-over-method/allow-public-and-protected/tslint.json b/test/rules/prefer-function-over-method/allow-public-and-protected/tslint.json new file mode 100644 index 00000000000..c60d1b2b86e --- /dev/null +++ b/test/rules/prefer-function-over-method/allow-public-and-protected/tslint.json @@ -0,0 +1,5 @@ +{ + "rules": { + "prefer-function-over-method": [true, "allow-public", "allow-protected"] + } +} diff --git a/test/rules/prefer-function-over-method/allow-public/test.ts.lint b/test/rules/prefer-function-over-method/allow-public/test.ts.lint new file mode 100644 index 00000000000..f7f7f363d80 --- /dev/null +++ b/test/rules/prefer-function-over-method/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. diff --git a/test/rules/prefer-function-over-method/allow-public/tslint.json b/test/rules/prefer-function-over-method/allow-public/tslint.json new file mode 100644 index 00000000000..3b9071c84a8 --- /dev/null +++ b/test/rules/prefer-function-over-method/allow-public/tslint.json @@ -0,0 +1,5 @@ +{ + "rules": { + "prefer-function-over-method": [true, "allow-public"] + } +} diff --git a/test/rules/prefer-function-over-method/default/test.js.lint b/test/rules/prefer-function-over-method/default/test.js.lint new file mode 100644 index 00000000000..5fd64ecbc48 --- /dev/null +++ b/test/rules/prefer-function-over-method/default/test.js.lint @@ -0,0 +1,4 @@ +class C { + a() {} + ~ [Method does not use 'this'. Use a function instead.] +} diff --git a/test/rules/prefer-function-over-method/default/test.ts.lint b/test/rules/prefer-function-over-method/default/test.ts.lint new file mode 100644 index 00000000000..48ba5c89222 --- /dev/null +++ b/test/rules/prefer-function-over-method/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. diff --git a/test/rules/prefer-function-over-method/default/tslint.json b/test/rules/prefer-function-over-method/default/tslint.json new file mode 100644 index 00000000000..7feb00aed16 --- /dev/null +++ b/test/rules/prefer-function-over-method/default/tslint.json @@ -0,0 +1,8 @@ +{ + "rules": { + "prefer-function-over-method": true + }, + "jsRules": { + "prefer-function-over-method": true + } +}