From 5d9e87ba5d2f37fb4b1b2bc8d9da1cc6a077e506 Mon Sep 17 00:00:00 2001 From: Gyandeep Singh Date: Thu, 11 Aug 2016 09:09:28 -0500 Subject: [PATCH] New: `class-methods-use-this` rule (fixes #5139) --- conf/eslint.json | 1 + docs/rules/class-methods-use-this.md | 85 +++++++++++++++++++++++ lib/rules/class-methods-use-this.js | 79 +++++++++++++++++++++ packages/eslint-config-eslint/default.yml | 1 + tests/lib/rules/class-methods-use-this.js | 78 +++++++++++++++++++++ 5 files changed, 244 insertions(+) create mode 100644 docs/rules/class-methods-use-this.md create mode 100644 lib/rules/class-methods-use-this.js create mode 100644 tests/lib/rules/class-methods-use-this.js diff --git a/conf/eslint.json b/conf/eslint.json index 3d7779ea065d..12aef73dc0b6 100755 --- a/conf/eslint.json +++ b/conf/eslint.json @@ -140,6 +140,7 @@ "brace-style": "off", "callback-return": "off", "camelcase": "off", + "class-methods-use-this": "off", "comma-dangle": "off", "comma-spacing": "off", "comma-style": "off", diff --git a/docs/rules/class-methods-use-this.md b/docs/rules/class-methods-use-this.md new file mode 100644 index 000000000000..ec4106476260 --- /dev/null +++ b/docs/rules/class-methods-use-this.md @@ -0,0 +1,85 @@ +# Enforce that class methods utilize `this` (class-methods-use-this) + +If a class method does not use `this`, it can safely be made a static function. + +It's possible to have a class method which doesn't use `this`, such as: + +```js +class A { + constructor() { + this.a = "hi"; + } + + print() { + console.log(this.a); + } + + sayHi() { + console.log("hi"); + } +} + +let aObj = new A(); +a.sayHi(); // => "hi" +``` + +In the example above, the `sayHi` method doesn't use `this`, so we can make it a static method: + +```js +class A { + constructor() { + this.a = "hi"; + } + + print() { + console.log(this.a); + } + + static sayHi() { + console.log("hi"); + } +} + +A.sayHi(); // => "hi" +``` + +## Rule Details + +This rule is aimed to flag class methods that do not use `this`. + +Examples of **incorrect** code for this rule: + +```js +/*eslint class-methods-use-this: "error"*/ +/*eslint-env es6*/ + +class A { + foo() { + console.log("Hello World"); /*error Expected 'this' to be used by class method 'foo'.*/ + } +} +``` + +Examples of **correct** code for this rule: + +```js +/*eslint class-methods-use-this: "error"*/ +/*eslint-env es6*/ +class A { + foo() { + this.bar = "Hello World"; // OK, this is used + } +} + +class A { + constructor() { + super(); // OK. constructor is exempt + } +} + +class A { + static foo() { + // OK. static methods aren't expected to use this. + } +} +``` diff --git a/lib/rules/class-methods-use-this.js b/lib/rules/class-methods-use-this.js new file mode 100644 index 000000000000..473d70f74f7b --- /dev/null +++ b/lib/rules/class-methods-use-this.js @@ -0,0 +1,79 @@ +/** + * @fileoverview Rule to enforce that all class methods use 'this'. + * @author Patrick Williams + */ + +"use strict"; + +//------------------------------------------------------------------------------ +// Rule Definition +//------------------------------------------------------------------------------ + +module.exports = { + meta: { + docs: { + description: "enforce that class methods utilize `this`", + category: "Best Practices", + recommended: false + }, + schema: [] + }, + create(context) { + const stack = []; + + /** + * Initializes the current context to false and pushes it onto the stack. + * These booleans represent whether 'this' has been used in the context. + * @returns {void} + * @private + */ + function enterFunction() { + stack.push(false); + } + + /** + * Check if the node is an instance method + * @param {ASTNode} node - node to check + * @returns {boolean} True if its an instance method + * @private + */ + function isInstanceMethod(node) { + return !node.static && node.kind !== "constructor" && node.type === "MethodDefinition"; + } + + /** + * Checks if we are leaving a function that is a method, and reports if 'this' has not been used. + * Static methods and the constructor are exempt. + * Then pops the context off the stack. + * @param {ASTNode} node - A function node that was entered. + * @returns {void} + * @private + */ + function exitFunction(node) { + if (isInstanceMethod(node.parent) && !stack.pop()) { + context.report(node, "Expected 'this' to be used by class method '" + node.parent.key.name + "'."); + } + stack.pop(); + } + + /** + * Mark the current context as having used 'this'. + * @returns {void} + * @private + */ + function markThisUsed() { + if (stack.length) { + stack[stack.length - 1] = true; + } + } + + return { + FunctionDeclaration: enterFunction, + "FunctionDeclaration:exit": exitFunction, + FunctionExpression: enterFunction, + "FunctionExpression:exit": exitFunction, + ThisExpression: markThisUsed, + Super: markThisUsed + }; + } +}; diff --git a/packages/eslint-config-eslint/default.yml b/packages/eslint-config-eslint/default.yml index 429b872246a5..20c476eeb522 100644 --- a/packages/eslint-config-eslint/default.yml +++ b/packages/eslint-config-eslint/default.yml @@ -11,6 +11,7 @@ rules: brace-style: ["error", "1tbs"] camelcase: ["error", { properties: "never" }] callback-return: ["error", ["cb", "callback", "next"]] + class-methods-use-this: "error" comma-spacing: "error" comma-style: ["error", "last"] consistent-return: "error" diff --git a/tests/lib/rules/class-methods-use-this.js b/tests/lib/rules/class-methods-use-this.js new file mode 100644 index 000000000000..b2bb0708f118 --- /dev/null +++ b/tests/lib/rules/class-methods-use-this.js @@ -0,0 +1,78 @@ +/** + * @fileoverview Tests for class-methods-use-this rule. + * @author Patrick Williams + */ + +"use strict"; + +//------------------------------------------------------------------------------ +// Requirements +//------------------------------------------------------------------------------ + +const rule = require("../../../lib/rules/class-methods-use-this"); +const RuleTester = require("../../../lib/testers/rule-tester"); + +//------------------------------------------------------------------------------ +// Tests +//------------------------------------------------------------------------------ + +const ruleTester = new RuleTester(); + +ruleTester.run("class-methods-use-this", rule, { + valid: [ + {code: "class A { constructor() {} }", parserOptions: { ecmaVersion: 6 }}, + {code: "class A { foo() {this} }", parserOptions: { ecmaVersion: 6 }}, + {code: "class A { foo() {this.bar = 'bar';} }", parserOptions: { ecmaVersion: 6 }}, + {code: "class A { foo() {bar(this);} }", parserOptions: { ecmaVersion: 6 }}, + {code: "class A extends B { foo() {super.foo();} }", parserOptions: { ecmaVersion: 6 }}, + {code: "class A { foo() { if(true) { return this; } } }", parserOptions: { ecmaVersion: 6 }}, + {code: "class A { static foo() {} }", parserOptions: { ecmaVersion: 6 }}, + {code: "({ a(){} });", parserOptions: { ecmaVersion: 6 }}, + {code: "class A { foo() { () => this; } }", parserOptions: { ecmaVersion: 6 }}, + {code: "({ a: function () {} });", parserOptions: { ecmaVersion: 6 }} + ], + invalid: [ + { + code: "class A { foo() {} }", + parserOptions: { ecmaVersion: 6 }, + errors: [ + {type: "FunctionExpression", line: 1, column: 14, message: "Expected 'this' to be used by class method 'foo'."} + ] + }, + { + code: "class A { foo() {/**this**/} }", + parserOptions: { ecmaVersion: 6 }, + errors: [ + {type: "FunctionExpression", line: 1, column: 14, message: "Expected 'this' to be used by class method 'foo'."} + ] + }, + { + code: "class A { foo() {var a = function () {this};} }", + parserOptions: { ecmaVersion: 6 }, + errors: [ + {type: "FunctionExpression", line: 1, column: 14, message: "Expected 'this' to be used by class method 'foo'."} + ] + }, + { + code: "class A { foo() {window.this} }", + parserOptions: { ecmaVersion: 6 }, + errors: [ + {type: "FunctionExpression", line: 1, column: 14, message: "Expected 'this' to be used by class method 'foo'."} + ] + }, + { + code: "class A { foo() {that.this = 'this';} }", + parserOptions: { ecmaVersion: 6 }, + errors: [ + {type: "FunctionExpression", line: 1, column: 14, message: "Expected 'this' to be used by class method 'foo'."} + ] + }, + { + code: "class A { foo() { () => undefined; } }", + parserOptions: { ecmaVersion: 6 }, + errors: [ + {type: "FunctionExpression", line: 1, column: 14, message: "Expected 'this' to be used by class method 'foo'."} + ] + } + ] +});