From 8b2d49fe3387883b22e3bd7be38fb5c6a28be230 Mon Sep 17 00:00:00 2001 From: Tim van der Lippe Date: Fri, 6 Aug 2021 15:52:59 +0100 Subject: [PATCH] New: Report unused private class members (fixes #14859) For any private class property or method, report those that are unused. Since private class members can only be accessed in the same class body, we can safely assume that all usages are processed when we leave the ClassBody scope. --- conf/eslint-recommended.js | 1 + docs/rules/no-unused-private-class-members.md | 57 ++++++++ lib/rules/index.js | 1 + lib/rules/no-unused-private-class-members.js | 88 ++++++++++++ .../rules/no-unused-private-class-members.js | 135 ++++++++++++++++++ tools/rule-types.json | 1 + 6 files changed, 283 insertions(+) create mode 100644 docs/rules/no-unused-private-class-members.md create mode 100644 lib/rules/no-unused-private-class-members.js create mode 100644 tests/lib/rules/no-unused-private-class-members.js diff --git a/conf/eslint-recommended.js b/conf/eslint-recommended.js index 7d2161a50a7..cc6e7113e80 100644 --- a/conf/eslint-recommended.js +++ b/conf/eslint-recommended.js @@ -64,6 +64,7 @@ module.exports = { "no-unsafe-negation": "error", "no-unsafe-optional-chaining": "error", "no-unused-labels": "error", + "no-unused-private-class-members": "error", "no-unused-vars": "error", "no-useless-backreference": "error", "no-useless-catch": "error", diff --git a/docs/rules/no-unused-private-class-members.md b/docs/rules/no-unused-private-class-members.md new file mode 100644 index 00000000000..1db141dec6e --- /dev/null +++ b/docs/rules/no-unused-private-class-members.md @@ -0,0 +1,57 @@ +# Disallow Unused Private Class Members (no-unused-private-class-members) + +Private class members that are declared and not used anywhere in the code are most likely an error due to incomplete refactoring. Such class members take up space in the code and can lead to confusion by readers. + +## Rule Details + +A private class member `#foo` is considered to be used if any of the following are true: + +* It is called (`this.#foo()`) +* It is read (`this.#foo`) +* It is passed into a function as an argument (`doSomething(this.#foo)`) + +Examples of **incorrect** code for this rule: + +```js +/*eslint no-unused-private-class-members: "error"*/ +class Foo { + #unusedMember = 5; +} + +class Foo { + #usedOnlyInWrite = 5; + method() { + this.#usedOnlyInWrite = 42; + } +} + +class Foo { + #unusedMethod() {} +} +``` + +Examples of **correct** code for this rule: + +```js +/*eslint no-unused-private-class-members: "error"*/ + +class Foo { + #usedMember = 42; + method() { + return this.#usedMember; + } +} + +class Foo { + #usedMethod() { + return 42; + } + anotherMethod() { + return this.#usedMethod(); + } +} +``` + +## When Not To Use It + +If you don't want to be notified about unused private class members, you can safely turn this rule off. diff --git a/lib/rules/index.js b/lib/rules/index.js index 35af38fd108..15566a64740 100644 --- a/lib/rules/index.js +++ b/lib/rules/index.js @@ -221,6 +221,7 @@ module.exports = new LazyLoadingRuleMap(Object.entries({ "no-unsafe-optional-chaining": () => require("./no-unsafe-optional-chaining"), "no-unused-expressions": () => require("./no-unused-expressions"), "no-unused-labels": () => require("./no-unused-labels"), + "no-unused-private-class-members": () => require("./no-unused-private-class-members"), "no-unused-vars": () => require("./no-unused-vars"), "no-use-before-define": () => require("./no-use-before-define"), "no-useless-backreference": () => require("./no-useless-backreference"), diff --git a/lib/rules/no-unused-private-class-members.js b/lib/rules/no-unused-private-class-members.js new file mode 100644 index 00000000000..8997a595575 --- /dev/null +++ b/lib/rules/no-unused-private-class-members.js @@ -0,0 +1,88 @@ +/** + * @fileoverview Rule to flag declared but unused private class members + * @author Tim van der Lippe + */ + +"use strict"; + +//------------------------------------------------------------------------------ +// Rule Definition +//------------------------------------------------------------------------------ + +module.exports = { + meta: { + type: "problem", + + docs: { + description: "disallow unused private class members", + category: "Variables", + recommended: true, + url: "https://eslint.org/docs/rules/no-unused-private-class-members" + }, + + schema: [], + + messages: { + unusedPrivateClassMember: "'{{classMemberName}}' is defined but never used." + } + }, + + create(context) { + + const declaredAndUnusedPrivateMembers = new Map(); + + //-------------------------------------------------------------------------- + // Public + //-------------------------------------------------------------------------- + + return { + + // Collect all declared members up front and assume they are all unused + ClassBody(classBodyNode) { + for (const bodyMember of classBodyNode.body) { + if (bodyMember.type === "PropertyDefinition" || bodyMember.type === "MethodDefinition") { + if (bodyMember.key.type === "PrivateIdentifier") { + declaredAndUnusedPrivateMembers.set(bodyMember.key.name, bodyMember); + } + } + } + }, + + /* + * Process all usages of the private identifier and remove a member from + * `declaredAndUnusedPrivateMembers` if we deem it used. + */ + PrivateIdentifier(privateIdentifierNode) { + if ( + + // The definition of the class member itself + privateIdentifierNode.parent.type !== "PropertyDefinition" && + privateIdentifierNode.parent.type !== "MethodDefinition" && + + // Any usage of this member, except for writes (if it is a property) + privateIdentifierNode.parent.parent.type !== "AssignmentExpression") { + + declaredAndUnusedPrivateMembers.delete(privateIdentifierNode.name); + } + }, + + /* + * Post-process the class members and report any remaining members. + * Since private members can only be accessed in the current class context, + * we can safely assume that all usages are within the current class body. + */ + "ClassBody:exit"() { + for (const [classMemberName, remainingDeclaredMember] of declaredAndUnusedPrivateMembers.entries()) { + context.report({ + node: remainingDeclaredMember, + messageId: "unusedPrivateClassMember", + data: { + classMemberName + } + }); + } + declaredAndUnusedPrivateMembers.clear(); + } + }; + } +}; diff --git a/tests/lib/rules/no-unused-private-class-members.js b/tests/lib/rules/no-unused-private-class-members.js new file mode 100644 index 00000000000..881ac8dbace --- /dev/null +++ b/tests/lib/rules/no-unused-private-class-members.js @@ -0,0 +1,135 @@ +/** + * @fileoverview Tests for no-unused-private-class-members rule. + * @author Tim van der Lippe + */ + +"use strict"; + +//------------------------------------------------------------------------------ +// Requirements +//------------------------------------------------------------------------------ + +const rule = require("../../../lib/rules/no-unused-private-class-members"), + { RuleTester } = require("../../../lib/rule-tester"); + +//------------------------------------------------------------------------------ +// Tests +//------------------------------------------------------------------------------ + +const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 2022 } }); + +/** + * Returns an expected error for defined-but-not-used private class member. + * @param {string} classMemberName The name of the class member + * @returns {Object} An expected error object + */ +function definedError(classMemberName) { + return { + messageId: "unusedPrivateClassMember", + data: { + classMemberName + } + }; +} + +ruleTester.run("no-unused-private-class-members", rule, { + valid: [ + "class Foo {}", + ` +class Foo { + publicMember = 42; +}`, + ` +class Foo { + #usedMember = 42; + method() { + return this.#usedMember; + } +}`, + ` +class Foo { + #usedMember = 42; + anotherMember = this.#usedMember; +}`, + ` +class Foo { + #usedMember = 42; + method() { + return someGlobalMethod(this.#usedMember); + } +}`, + + //-------------------------------------------------------------------------- + // Method definitions + //-------------------------------------------------------------------------- + ` +class Foo { + #usedMethod() { + return 42; + } + anotherMethod() { + return this.#usedMethod(); + } +}` + ], + invalid: [ + { + code: `class Foo { + #unusedMember = 5; +}`, + errors: [definedError("unusedMember")] + }, + { + code: `class First {} +class Second { + #unusedMemberInSecondClass = 5; +}`, + errors: [definedError("unusedMemberInSecondClass")] + }, + { + code: `class First { + #unusedMemberInFirstClass = 5; +} +class Second {}`, + errors: [definedError("unusedMemberInFirstClass")] + }, + { + code: `class First { + #firstUnusedMemberInSameClass = 5; + #secondUnusedMemberInSameClass = 5; +}`, + errors: [definedError("firstUnusedMemberInSameClass"), definedError("secondUnusedMemberInSameClass")] + }, + { + code: `class Foo { + #usedOnlyInWrite = 5; + method() { + this.#usedOnlyInWrite = 42; + } +}`, + errors: [definedError("usedOnlyInWrite")] + }, + + //-------------------------------------------------------------------------- + // Unused method definitions + //-------------------------------------------------------------------------- + { + code: `class Foo { + #unusedMethod() {} +}`, + errors: [definedError("unusedMethod")] + }, + { + code: `class Foo { + #unusedMethod() {} + #usedMethod() { + return 42; + } + publicMethod() { + return this.#usedMethod(); + } +}`, + errors: [definedError("unusedMethod")] + } + ] +}); diff --git a/tools/rule-types.json b/tools/rule-types.json index d35446d9dfb..4a71a8b09ed 100644 --- a/tools/rule-types.json +++ b/tools/rule-types.json @@ -208,6 +208,7 @@ "no-unsafe-optional-chaining": "problem", "no-unused-expressions": "suggestion", "no-unused-labels": "suggestion", + "no-unused-private-class-members": "problem", "no-unused-vars": "problem", "no-use-before-define": "problem", "no-useless-backreference": "problem",