From 3d370fb3596ccd3463c29f1a7a1e3f321dd8083a Mon Sep 17 00:00:00 2001 From: Tim van der Lippe Date: Fri, 22 Oct 2021 14:17:52 +0100 Subject: [PATCH] New: Add no-unused-private-class-members rule (fixes #14859) (#14895) * 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. * Handle all edge cases per feedback Also remove the rule from recommended for now, as that is a breaking change. * Remove duplication in assignment checking * Optimize check for property deletion This also removes the usage of optional chaining, which isn't supported yet on Node environments that ESLint supports. * Use more descriptive names * Handle more edge cases * Apply suggestions from code review Co-authored-by: Milos Djermanovic * Update lib/rules/no-unused-private-class-members.js Co-authored-by: Milos Djermanovic * Update no-unused-private-class-members.js * Fix accessor handling Co-authored-by: Milos Djermanovic --- docs/rules/no-unused-private-class-members.md | 78 ++++ lib/rules/index.js | 1 + lib/rules/no-unused-private-class-members.js | 194 +++++++++ .../rules/no-unused-private-class-members.js | 390 ++++++++++++++++++ tools/rule-types.json | 1 + 5 files changed, 664 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/docs/rules/no-unused-private-class-members.md b/docs/rules/no-unused-private-class-members.md new file mode 100644 index 00000000000..2eb9f4bf485 --- /dev/null +++ b/docs/rules/no-unused-private-class-members.md @@ -0,0 +1,78 @@ +# 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 + +This rule reports unused private class members. + +* A private field or method is considered to be unused if its value is never read. +* A private accessor is considered to be unused if it is never accessed (read or write). + +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 { + #usedOnlyToUpdateItself = 5; + method() { + this.#usedOnlyToUpdateItself++; + } +} + +class Foo { + #unusedMethod() {} +} + +class Foo { + get #unusedAccessor() {} + set #unusedAccessor(value) {} +} +``` + +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(); + } +} + +class Foo { + get #usedAccessor() {} + set #usedAccessor(value) {} + + method() { + this.#usedAccessor = 42; + } +} +``` + +## 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 c88febd85b9..ed322a4120a 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..74cf6ab694a --- /dev/null +++ b/lib/rules/no-unused-private-class-members.js @@ -0,0 +1,194 @@ +/** + * @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", + recommended: false, + url: "https://eslint.org/docs/rules/no-unused-private-class-members" + }, + + schema: [], + + messages: { + unusedPrivateClassMember: "'{{classMemberName}}' is defined but never used." + } + }, + + create(context) { + const trackedClasses = []; + + /** + * Check whether the current node is in a write only assignment. + * @param {ASTNode} privateIdentifierNode Node referring to a private identifier + * @returns {boolean} Whether the node is in a write only assignment + * @private + */ + function isWriteOnlyAssignment(privateIdentifierNode) { + const parentStatement = privateIdentifierNode.parent.parent; + const isAssignmentExpression = parentStatement.type === "AssignmentExpression"; + + if (!isAssignmentExpression && + parentStatement.type !== "ForInStatement" && + parentStatement.type !== "ForOfStatement" && + parentStatement.type !== "AssignmentPattern") { + return false; + } + + // It is a write-only usage, since we still allow usages on the right for reads + if (parentStatement.left !== privateIdentifierNode.parent) { + return false; + } + + // For any other operator (such as '+=') we still consider it a read operation + if (isAssignmentExpression && parentStatement.operator !== "=") { + + /* + * However, if the read operation is "discarded" in an empty statement, then + * we consider it write only. + */ + return parentStatement.parent.type === "ExpressionStatement"; + } + + return true; + } + + //-------------------------------------------------------------------------- + // Public + //-------------------------------------------------------------------------- + + return { + + // Collect all declared members up front and assume they are all unused + ClassBody(classBodyNode) { + const privateMembers = new Map(); + + trackedClasses.unshift(privateMembers); + for (const bodyMember of classBodyNode.body) { + if (bodyMember.type === "PropertyDefinition" || bodyMember.type === "MethodDefinition") { + if (bodyMember.key.type === "PrivateIdentifier") { + privateMembers.set(bodyMember.key.name, { + declaredNode: bodyMember, + isAccessor: bodyMember.type === "MethodDefinition" && + (bodyMember.kind === "set" || bodyMember.kind === "get") + }); + } + } + } + }, + + /* + * Process all usages of the private identifier and remove a member from + * `declaredAndUnusedPrivateMembers` if we deem it used. + */ + PrivateIdentifier(privateIdentifierNode) { + const classBody = trackedClasses.find(classProperties => classProperties.has(privateIdentifierNode.name)); + + // Can't happen, as it is a parser to have a missing class body, but let's code defensively here. + if (!classBody) { + return; + } + + // In case any other usage was already detected, we can short circuit the logic here. + const memberDefinition = classBody.get(privateIdentifierNode.name); + + if (memberDefinition.isUsed) { + return; + } + + // The definition of the class member itself + if (privateIdentifierNode.parent.type === "PropertyDefinition" || + privateIdentifierNode.parent.type === "MethodDefinition") { + return; + } + + /* + * Any usage of an accessor is considered a read, as the getter/setter can have + * side-effects in its definition. + */ + if (memberDefinition.isAccessor) { + memberDefinition.isUsed = true; + return; + } + + // Any assignments to this member, except for assignments that also read + if (isWriteOnlyAssignment(privateIdentifierNode)) { + return; + } + + const wrappingExpressionType = privateIdentifierNode.parent.parent.type; + const parentOfWrappingExpressionType = privateIdentifierNode.parent.parent.parent.type; + + // A statement which only increments (`this.#x++;`) + if (wrappingExpressionType === "UpdateExpression" && + parentOfWrappingExpressionType === "ExpressionStatement") { + return; + } + + /* + * ({ x: this.#usedInDestructuring } = bar); + * + * But should treat the following as a read: + * ({ [this.#x]: a } = foo); + */ + if (wrappingExpressionType === "Property" && + parentOfWrappingExpressionType === "ObjectPattern" && + privateIdentifierNode.parent.parent.value === privateIdentifierNode.parent) { + return; + } + + // [...this.#unusedInRestPattern] = bar; + if (wrappingExpressionType === "RestElement") { + return; + } + + // [this.#unusedInAssignmentPattern] = bar; + if (wrappingExpressionType === "ArrayPattern") { + return; + } + + /* + * We can't delete the memberDefinition, as we need to keep track of which member we are marking as used. + * In the case of nested classes, we only mark the first member we encounter as used. If you were to delete + * the member, then any subsequent usage could incorrectly mark the member of an encapsulating parent class + * as used, which is incorrect. + */ + memberDefinition.isUsed = true; + }, + + /* + * 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"() { + const unusedPrivateMembers = trackedClasses.shift(); + + for (const [classMemberName, { declaredNode, isUsed }] of unusedPrivateMembers.entries()) { + if (isUsed) { + continue; + } + context.report({ + node: declaredNode, + loc: declaredNode.key.loc, + messageId: "unusedPrivateClassMember", + data: { + classMemberName: `#${classMemberName}` + } + }); + } + } + }; + } +}; 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..6e80baae968 --- /dev/null +++ b/tests/lib/rules/no-unused-private-class-members.js @@ -0,0 +1,390 @@ +/** + * @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: `#${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; + foo() { + anotherMember = this.#usedMember; + } +}`, + `class C { + #usedMember; + + foo() { + bar(this.#usedMember += 1); + } +}`, + `class Foo { + #usedMember = 42; + method() { + return someGlobalMethod(this.#usedMember); + } +}`, + `class C { + #usedInOuterClass; + + foo() { + return class {}; + } + + bar() { + return this.#usedInOuterClass; + } +}`, + `class Foo { + #usedInForInLoop; + method() { + for (const bar in this.#usedInForInLoop) { + + } + } +}`, + `class Foo { + #usedInForOfLoop; + method() { + for (const bar of this.#usedInForOfLoop) { + + } + } +}`, + `class Foo { + #usedInAssignmentPattern; + method() { + [bar = 1] = this.#usedInAssignmentPattern; + } +}`, + `class Foo { + #usedInArrayPattern; + method() { + [bar] = this.#usedInArrayPattern; + } +}`, + `class Foo { + #usedInAssignmentPattern; + method() { + [bar] = this.#usedInAssignmentPattern; + } +}`, + `class C { + #usedInObjectAssignment; + + method() { + ({ [this.#usedInObjectAssignment]: a } = foo); + } +}`, + `class C { + set #accessorWithSetterFirst(value) { + doSomething(value); + } + get #accessorWithSetterFirst() { + return something(); + } + method() { + this.#accessorWithSetterFirst += 1; + } +}`, + `class Foo { + set #accessorUsedInMemberAccess(value) {} + + method(a) { + [this.#accessorUsedInMemberAccess] = a; + } +}`, + `class C { + get #accessorWithGetterFirst() { + return something(); + } + set #accessorWithGetterFirst(value) { + doSomething(value); + } + method() { + this.#accessorWithGetterFirst += 1; + } +}`, + `class C { + #usedInInnerClass; + + method(a) { + return class { + foo = a.#usedInInnerClass; + } + } +}`, + + //-------------------------------------------------------------------------- + // Method definitions + //-------------------------------------------------------------------------- + `class Foo { + #usedMethod() { + return 42; + } + anotherMethod() { + return this.#usedMethod(); + } +}`, + `class C { + set #x(value) { + doSomething(value); + } + + foo() { + this.#x = 1; + } +}` + ], + 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")] + }, + { + code: `class Foo { + #usedOnlyInWriteStatement = 5; + method() { + this.#usedOnlyInWriteStatement += 42; + } +}`, + errors: [definedError("usedOnlyInWriteStatement")] + }, + { + code: `class C { + #usedOnlyInIncrement; + + foo() { + this.#usedOnlyInIncrement++; + } +}`, + errors: [definedError("usedOnlyInIncrement")] + }, + { + code: `class C { + #unusedInOuterClass; + + foo() { + return class { + #unusedInOuterClass; + + bar() { + return this.#unusedInOuterClass; + } + }; + } +}`, + errors: [definedError("unusedInOuterClass")] + }, + { + code: `class C { + #unusedOnlyInSecondNestedClass; + + foo() { + return class { + #unusedOnlyInSecondNestedClass; + + bar() { + return this.#unusedOnlyInSecondNestedClass; + } + }; + } + + baz() { + return this.#unusedOnlyInSecondNestedClass; + } + + bar() { + return class { + #unusedOnlyInSecondNestedClass; + } + } +}`, + errors: [definedError("unusedOnlyInSecondNestedClass")] + }, + + //-------------------------------------------------------------------------- + // Unused method definitions + //-------------------------------------------------------------------------- + { + code: `class Foo { + #unusedMethod() {} +}`, + errors: [definedError("unusedMethod")] + }, + { + code: `class Foo { + #unusedMethod() {} + #usedMethod() { + return 42; + } + publicMethod() { + return this.#usedMethod(); + } +}`, + errors: [definedError("unusedMethod")] + }, + { + code: `class Foo { + set #unusedSetter(value) {} +}`, + errors: [definedError("unusedSetter")] + }, + { + code: `class Foo { + #unusedForInLoop; + method() { + for (this.#unusedForInLoop in bar) { + + } + } +}`, + errors: [definedError("unusedForInLoop")] + }, + { + code: `class Foo { + #unusedForOfLoop; + method() { + for (this.#unusedForOfLoop of bar) { + + } + } +}`, + errors: [definedError("unusedForOfLoop")] + }, + { + code: `class Foo { + #unusedInDestructuring; + method() { + ({ x: this.#unusedInDestructuring } = bar); + } +}`, + errors: [definedError("unusedInDestructuring")] + }, + { + code: `class Foo { + #unusedInRestPattern; + method() { + [...this.#unusedInRestPattern] = bar; + } +}`, + errors: [definedError("unusedInRestPattern")] + }, + { + code: `class Foo { + #unusedInAssignmentPattern; + method() { + [this.#unusedInAssignmentPattern = 1] = bar; + } +}`, + errors: [definedError("unusedInAssignmentPattern")] + }, + { + code: `class Foo { + #unusedInAssignmentPattern; + method() { + [this.#unusedInAssignmentPattern] = bar; + } +}`, + errors: [definedError("unusedInAssignmentPattern")] + }, + { + code: `class C { + #usedOnlyInTheSecondInnerClass; + + method(a) { + return class { + #usedOnlyInTheSecondInnerClass; + + method2(b) { + foo = b.#usedOnlyInTheSecondInnerClass; + } + + method3(b) { + foo = b.#usedOnlyInTheSecondInnerClass; + } + } + } +}`, + errors: [{ + ...definedError("usedOnlyInTheSecondInnerClass"), + line: 2 + }] + } + ] +}); 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",