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 01/10] 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", From 4c9f57a7e4e4301d6f46d98edc1256f7a7043bea Mon Sep 17 00:00:00 2001 From: Tim van der Lippe Date: Fri, 1 Oct 2021 17:54:12 +0100 Subject: [PATCH 02/10] Handle all edge cases per feedback Also remove the rule from recommended for now, as that is a breaking change. --- conf/eslint-recommended.js | 1 - lib/rules/no-unused-private-class-members.js | 119 ++++++++-- .../rules/no-unused-private-class-members.js | 207 +++++++++++++++++- 3 files changed, 303 insertions(+), 24 deletions(-) diff --git a/conf/eslint-recommended.js b/conf/eslint-recommended.js index cc6e7113e80..7d2161a50a7 100644 --- a/conf/eslint-recommended.js +++ b/conf/eslint-recommended.js @@ -64,7 +64,6 @@ 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/lib/rules/no-unused-private-class-members.js b/lib/rules/no-unused-private-class-members.js index 8997a595575..c4bbd8f251a 100644 --- a/lib/rules/no-unused-private-class-members.js +++ b/lib/rules/no-unused-private-class-members.js @@ -16,7 +16,7 @@ module.exports = { docs: { description: "disallow unused private class members", category: "Variables", - recommended: true, + recommended: false, url: "https://eslint.org/docs/rules/no-unused-private-class-members" }, @@ -28,8 +28,64 @@ module.exports = { }, create(context) { + const stackOfCurrentlyProcessingClassNodes = []; + + /** + * Check whether the current node is in a write only assignment. + * @param {ASTNode} privateIdentifierNode Node referring to a private identifier + * @param {boolean} isSetter Whether the property is declared as setter + * @returns {boolean} Whether the node is in a write only assignment + * @private + */ + function isWriteOnlyAssignmentStatementWithoutPerformingAnyReads(privateIdentifierNode, isSetter) { + if (privateIdentifierNode.parent.parent.type !== "AssignmentExpression") { + return false; + } + + /* + * Any assignment that writes to a setter is considered a read, as the setter can have + * side-effects in its definition. + */ + if (isSetter) { + return false; + } + + const assignmentExpression = privateIdentifierNode.parent.parent; + + // It is a write-only usage, since we still allow usages on the right for reads + if (assignmentExpression.left !== privateIdentifierNode.parent) { + return false; + } - const declaredAndUnusedPrivateMembers = new Map(); + // For any other operator (such as '+=') we still consider it a read operation + if (assignmentExpression.operator !== "=") { + + /* + * However, if the read operation is "discarded" in an empty statement, then + * we consider it write only. + */ + return assignmentExpression.parent.type === "ExpressionStatement"; + } + + return true; + } + + /** + * Check whether the current node is in a write only for-loop + * @param {ASTNode} privateIdentifierNode Node referring to a private identifier + * @returns {boolean} Whether the node is a write only for-loop + */ + function isWriteOnlyInAssignmentExpression(privateIdentifierNode) { + if (privateIdentifierNode.parent.parent.type !== "ForInStatement" && + privateIdentifierNode.parent.parent.type !== "ForOfStatement" && + privateIdentifierNode.parent.parent.type !== "AssignmentPattern") { + return false; + } + + const forLoopStatement = privateIdentifierNode.parent.parent; + + return forLoopStatement.left === privateIdentifierNode.parent; + } //-------------------------------------------------------------------------- // Public @@ -39,10 +95,16 @@ module.exports = { // Collect all declared members up front and assume they are all unused ClassBody(classBodyNode) { + const currentPrivateMembersPerClass = new Map(); + + stackOfCurrentlyProcessingClassNodes.unshift(currentPrivateMembersPerClass); for (const bodyMember of classBodyNode.body) { if (bodyMember.type === "PropertyDefinition" || bodyMember.type === "MethodDefinition") { if (bodyMember.key.type === "PrivateIdentifier") { - declaredAndUnusedPrivateMembers.set(bodyMember.key.name, bodyMember); + currentPrivateMembersPerClass.set(bodyMember.key.name, { + declaredNode: bodyMember, + isSetter: bodyMember.type === "MethodDefinition" && bodyMember.kind === "set" + }); } } } @@ -53,17 +115,47 @@ module.exports = { * `declaredAndUnusedPrivateMembers` if we deem it used. */ PrivateIdentifier(privateIdentifierNode) { - if ( - // The definition of the class member itself - privateIdentifierNode.parent.type !== "PropertyDefinition" && - privateIdentifierNode.parent.type !== "MethodDefinition" && + // The definition of the class member itself + if (privateIdentifierNode.parent.type === "PropertyDefinition" || + privateIdentifierNode.parent.type === "MethodDefinition") { + return; + } + + const isSetter = stackOfCurrentlyProcessingClassNodes[0].get(privateIdentifierNode.name)?.isSetter; - // Any usage of this member, except for writes (if it is a property) - privateIdentifierNode.parent.parent.type !== "AssignmentExpression") { + // Any assignments to this member, except for assignments that also read + if (isWriteOnlyAssignmentStatementWithoutPerformingAnyReads(privateIdentifierNode, isSetter)) { + return; + } + + // A statement which only increments (`this.#x++;`) + if (privateIdentifierNode.parent.parent.type === "UpdateExpression" && + privateIdentifierNode.parent.parent.parent.type === "ExpressionStatement") { + return; + } - declaredAndUnusedPrivateMembers.delete(privateIdentifierNode.name); + if (isWriteOnlyInAssignmentExpression(privateIdentifierNode)) { + return; } + + // ({ x: this.#usedInDestructuring } = bar); + if (privateIdentifierNode.parent.parent.type === "Property" && + privateIdentifierNode.parent.parent.parent.type === "ObjectPattern") { + return; + } + + // [...this.#unusedInRestPattern] = bar; + if (privateIdentifierNode.parent.parent.type === "RestElement") { + return; + } + + if (privateIdentifierNode.parent.parent.type === "ArrayPattern" && + privateIdentifierNode.parent.parent.parent.type === "AssignmentExpression") { + return; + } + + stackOfCurrentlyProcessingClassNodes[0].delete(privateIdentifierNode.name); }, /* @@ -72,16 +164,17 @@ module.exports = { * we can safely assume that all usages are within the current class body. */ "ClassBody:exit"() { - for (const [classMemberName, remainingDeclaredMember] of declaredAndUnusedPrivateMembers.entries()) { + const declaredAndUnusedPrivateMembers = stackOfCurrentlyProcessingClassNodes.shift(); + + for (const [classMemberName, { declaredNode }] of declaredAndUnusedPrivateMembers.entries()) { context.report({ - node: remainingDeclaredMember, + node: declaredNode, 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 index 881ac8dbace..827ec9f888a 100644 --- a/tests/lib/rules/no-unused-private-class-members.js +++ b/tests/lib/rules/no-unused-private-class-members.js @@ -35,41 +35,103 @@ function definedError(classMemberName) { ruleTester.run("no-unused-private-class-members", rule, { valid: [ "class Foo {}", - ` -class Foo { + `class Foo { publicMember = 42; }`, - ` -class Foo { + `class Foo { #usedMember = 42; method() { return this.#usedMember; } }`, - ` -class Foo { + `class Foo { #usedMember = 42; anotherMember = this.#usedMember; }`, - ` -class Foo { + `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 { + #unusedInAssignmentPattern; + method() { + [bar] = this.#unusedInAssignmentPattern; + } +}`, //-------------------------------------------------------------------------- // Method definitions //-------------------------------------------------------------------------- - ` -class Foo { + `class Foo { #usedMethod() { return 42; } anotherMethod() { return this.#usedMethod(); } +}`, + `class C { + set #x(value) { + doSomething(value); + } + + foo() { + this.#x = 1; + } }` ], invalid: [ @@ -109,6 +171,67 @@ class Second {}`, }`, 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 @@ -130,6 +253,70 @@ class Second {}`, } }`, 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")] } ] }); From d253c4e52fcb0ca917eead9bc17c19fa1c5df3bb Mon Sep 17 00:00:00 2001 From: Tim van der Lippe Date: Fri, 1 Oct 2021 18:03:06 +0100 Subject: [PATCH 03/10] Remove duplication in assignment checking --- lib/rules/no-unused-private-class-members.js | 38 ++++++-------------- 1 file changed, 11 insertions(+), 27 deletions(-) diff --git a/lib/rules/no-unused-private-class-members.js b/lib/rules/no-unused-private-class-members.js index c4bbd8f251a..a25c3fb1623 100644 --- a/lib/rules/no-unused-private-class-members.js +++ b/lib/rules/no-unused-private-class-members.js @@ -38,7 +38,13 @@ module.exports = { * @private */ function isWriteOnlyAssignmentStatementWithoutPerformingAnyReads(privateIdentifierNode, isSetter) { - if (privateIdentifierNode.parent.parent.type !== "AssignmentExpression") { + const parentStatement = privateIdentifierNode.parent.parent; + const isAssignmentExpression = parentStatement.type === "AssignmentExpression"; + + if (!isAssignmentExpression && + parentStatement.type !== "ForInStatement" && + parentStatement.type !== "ForOfStatement" && + parentStatement.type !== "AssignmentPattern") { return false; } @@ -50,43 +56,24 @@ module.exports = { return false; } - const assignmentExpression = privateIdentifierNode.parent.parent; - // It is a write-only usage, since we still allow usages on the right for reads - if (assignmentExpression.left !== privateIdentifierNode.parent) { + if (parentStatement.left !== privateIdentifierNode.parent) { return false; } // For any other operator (such as '+=') we still consider it a read operation - if (assignmentExpression.operator !== "=") { + if (isAssignmentExpression && parentStatement.operator !== "=") { /* * However, if the read operation is "discarded" in an empty statement, then * we consider it write only. */ - return assignmentExpression.parent.type === "ExpressionStatement"; + return parentStatement.parent.type === "ExpressionStatement"; } return true; } - /** - * Check whether the current node is in a write only for-loop - * @param {ASTNode} privateIdentifierNode Node referring to a private identifier - * @returns {boolean} Whether the node is a write only for-loop - */ - function isWriteOnlyInAssignmentExpression(privateIdentifierNode) { - if (privateIdentifierNode.parent.parent.type !== "ForInStatement" && - privateIdentifierNode.parent.parent.type !== "ForOfStatement" && - privateIdentifierNode.parent.parent.type !== "AssignmentPattern") { - return false; - } - - const forLoopStatement = privateIdentifierNode.parent.parent; - - return forLoopStatement.left === privateIdentifierNode.parent; - } - //-------------------------------------------------------------------------- // Public //-------------------------------------------------------------------------- @@ -135,10 +122,6 @@ module.exports = { return; } - if (isWriteOnlyInAssignmentExpression(privateIdentifierNode)) { - return; - } - // ({ x: this.#usedInDestructuring } = bar); if (privateIdentifierNode.parent.parent.type === "Property" && privateIdentifierNode.parent.parent.parent.type === "ObjectPattern") { @@ -150,6 +133,7 @@ module.exports = { return; } + // [this.#unusedInAssignmentPattern] = bar; if (privateIdentifierNode.parent.parent.type === "ArrayPattern" && privateIdentifierNode.parent.parent.parent.type === "AssignmentExpression") { return; From 185367576a2ac78bbc193f6422306d96cb5b25b8 Mon Sep 17 00:00:00 2001 From: Tim van der Lippe Date: Fri, 1 Oct 2021 18:16:25 +0100 Subject: [PATCH 04/10] Optimize check for property deletion This also removes the usage of optional chaining, which isn't supported yet on Node environments that ESLint supports. --- lib/rules/no-unused-private-class-members.js | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/rules/no-unused-private-class-members.js b/lib/rules/no-unused-private-class-members.js index a25c3fb1623..277da0294c8 100644 --- a/lib/rules/no-unused-private-class-members.js +++ b/lib/rules/no-unused-private-class-members.js @@ -102,6 +102,12 @@ module.exports = { * `declaredAndUnusedPrivateMembers` if we deem it used. */ PrivateIdentifier(privateIdentifierNode) { + const propertyDefinition = stackOfCurrentlyProcessingClassNodes[0].get(privateIdentifierNode.name); + + // In case any other usage was already detected, the property was already removed from the map + if (!propertyDefinition) { + return; + } // The definition of the class member itself if (privateIdentifierNode.parent.type === "PropertyDefinition" || @@ -109,10 +115,8 @@ module.exports = { return; } - const isSetter = stackOfCurrentlyProcessingClassNodes[0].get(privateIdentifierNode.name)?.isSetter; - // Any assignments to this member, except for assignments that also read - if (isWriteOnlyAssignmentStatementWithoutPerformingAnyReads(privateIdentifierNode, isSetter)) { + if (isWriteOnlyAssignmentStatementWithoutPerformingAnyReads(privateIdentifierNode, propertyDefinition.isSetter)) { return; } From 71d7a9b51267e90ad5af43ef374726ff9bb5526c Mon Sep 17 00:00:00 2001 From: Tim van der Lippe Date: Mon, 4 Oct 2021 11:23:27 +0100 Subject: [PATCH 05/10] Use more descriptive names --- lib/rules/no-unused-private-class-members.js | 37 +++++++++++--------- 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/lib/rules/no-unused-private-class-members.js b/lib/rules/no-unused-private-class-members.js index 277da0294c8..90f925df981 100644 --- a/lib/rules/no-unused-private-class-members.js +++ b/lib/rules/no-unused-private-class-members.js @@ -28,7 +28,7 @@ module.exports = { }, create(context) { - const stackOfCurrentlyProcessingClassNodes = []; + const trackedClasses = []; /** * Check whether the current node is in a write only assignment. @@ -37,7 +37,7 @@ module.exports = { * @returns {boolean} Whether the node is in a write only assignment * @private */ - function isWriteOnlyAssignmentStatementWithoutPerformingAnyReads(privateIdentifierNode, isSetter) { + function isWriteOnlyAssignment(privateIdentifierNode, isSetter) { const parentStatement = privateIdentifierNode.parent.parent; const isAssignmentExpression = parentStatement.type === "AssignmentExpression"; @@ -82,13 +82,13 @@ module.exports = { // Collect all declared members up front and assume they are all unused ClassBody(classBodyNode) { - const currentPrivateMembersPerClass = new Map(); + const privateMembers = new Map(); - stackOfCurrentlyProcessingClassNodes.unshift(currentPrivateMembersPerClass); + trackedClasses.unshift(privateMembers); for (const bodyMember of classBodyNode.body) { if (bodyMember.type === "PropertyDefinition" || bodyMember.type === "MethodDefinition") { if (bodyMember.key.type === "PrivateIdentifier") { - currentPrivateMembersPerClass.set(bodyMember.key.name, { + privateMembers.set(bodyMember.key.name, { declaredNode: bodyMember, isSetter: bodyMember.type === "MethodDefinition" && bodyMember.kind === "set" }); @@ -102,7 +102,7 @@ module.exports = { * `declaredAndUnusedPrivateMembers` if we deem it used. */ PrivateIdentifier(privateIdentifierNode) { - const propertyDefinition = stackOfCurrentlyProcessingClassNodes[0].get(privateIdentifierNode.name); + const propertyDefinition = trackedClasses[0].get(privateIdentifierNode.name); // In case any other usage was already detected, the property was already removed from the map if (!propertyDefinition) { @@ -116,34 +116,37 @@ module.exports = { } // Any assignments to this member, except for assignments that also read - if (isWriteOnlyAssignmentStatementWithoutPerformingAnyReads(privateIdentifierNode, propertyDefinition.isSetter)) { + if (isWriteOnlyAssignment(privateIdentifierNode, propertyDefinition.isSetter)) { return; } + const wrappingExpressionType = privateIdentifierNode.parent.parent.type; + const parentOfWrappingExpressionType = privateIdentifierNode.parent.parent.parent.type; + // A statement which only increments (`this.#x++;`) - if (privateIdentifierNode.parent.parent.type === "UpdateExpression" && - privateIdentifierNode.parent.parent.parent.type === "ExpressionStatement") { + if (wrappingExpressionType === "UpdateExpression" && + parentOfWrappingExpressionType === "ExpressionStatement") { return; } // ({ x: this.#usedInDestructuring } = bar); - if (privateIdentifierNode.parent.parent.type === "Property" && - privateIdentifierNode.parent.parent.parent.type === "ObjectPattern") { + if (wrappingExpressionType === "Property" && + parentOfWrappingExpressionType === "ObjectPattern") { return; } // [...this.#unusedInRestPattern] = bar; - if (privateIdentifierNode.parent.parent.type === "RestElement") { + if (wrappingExpressionType === "RestElement") { return; } // [this.#unusedInAssignmentPattern] = bar; - if (privateIdentifierNode.parent.parent.type === "ArrayPattern" && - privateIdentifierNode.parent.parent.parent.type === "AssignmentExpression") { + if (wrappingExpressionType === "ArrayPattern" && + parentOfWrappingExpressionType === "AssignmentExpression") { return; } - stackOfCurrentlyProcessingClassNodes[0].delete(privateIdentifierNode.name); + trackedClasses[0].delete(privateIdentifierNode.name); }, /* @@ -152,9 +155,9 @@ module.exports = { * we can safely assume that all usages are within the current class body. */ "ClassBody:exit"() { - const declaredAndUnusedPrivateMembers = stackOfCurrentlyProcessingClassNodes.shift(); + const unusedPrivateMembers = trackedClasses.shift(); - for (const [classMemberName, { declaredNode }] of declaredAndUnusedPrivateMembers.entries()) { + for (const [classMemberName, { declaredNode }] of unusedPrivateMembers.entries()) { context.report({ node: declaredNode, messageId: "unusedPrivateClassMember", From 6dd74040b848ce77cd695d74758c6c08a7bc56ab Mon Sep 17 00:00:00 2001 From: Tim van der Lippe Date: Mon, 4 Oct 2021 17:24:53 +0100 Subject: [PATCH 06/10] Handle more edge cases --- lib/rules/no-unused-private-class-members.js | 51 ++++++++++----- .../rules/no-unused-private-class-members.js | 65 ++++++++++++++++++- 2 files changed, 99 insertions(+), 17 deletions(-) diff --git a/lib/rules/no-unused-private-class-members.js b/lib/rules/no-unused-private-class-members.js index 90f925df981..ff16f6f2573 100644 --- a/lib/rules/no-unused-private-class-members.js +++ b/lib/rules/no-unused-private-class-members.js @@ -15,7 +15,6 @@ module.exports = { docs: { description: "disallow unused private class members", - category: "Variables", recommended: false, url: "https://eslint.org/docs/rules/no-unused-private-class-members" }, @@ -33,11 +32,11 @@ module.exports = { /** * Check whether the current node is in a write only assignment. * @param {ASTNode} privateIdentifierNode Node referring to a private identifier - * @param {boolean} isSetter Whether the property is declared as setter + * @param {boolean} isAccessor Whether the property is declared as setter * @returns {boolean} Whether the node is in a write only assignment * @private */ - function isWriteOnlyAssignment(privateIdentifierNode, isSetter) { + function isWriteOnlyAssignment(privateIdentifierNode, isAccessor) { const parentStatement = privateIdentifierNode.parent.parent; const isAssignmentExpression = parentStatement.type === "AssignmentExpression"; @@ -52,7 +51,7 @@ module.exports = { * Any assignment that writes to a setter is considered a read, as the setter can have * side-effects in its definition. */ - if (isSetter) { + if (isAccessor) { return false; } @@ -90,7 +89,8 @@ module.exports = { if (bodyMember.key.type === "PrivateIdentifier") { privateMembers.set(bodyMember.key.name, { declaredNode: bodyMember, - isSetter: bodyMember.type === "MethodDefinition" && bodyMember.kind === "set" + isAccessor: bodyMember.type === "MethodDefinition" && + (bodyMember.kind === "set" || bodyMember.kind === "get") }); } } @@ -102,10 +102,17 @@ module.exports = { * `declaredAndUnusedPrivateMembers` if we deem it used. */ PrivateIdentifier(privateIdentifierNode) { - const propertyDefinition = trackedClasses[0].get(privateIdentifierNode.name); + const classBody = trackedClasses.find(classProperties => classProperties.has(privateIdentifierNode.name)); - // In case any other usage was already detected, the property was already removed from the map - if (!propertyDefinition) { + // 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; } @@ -116,7 +123,7 @@ module.exports = { } // Any assignments to this member, except for assignments that also read - if (isWriteOnlyAssignment(privateIdentifierNode, propertyDefinition.isSetter)) { + if (isWriteOnlyAssignment(privateIdentifierNode, memberDefinition.isAccessor)) { return; } @@ -129,9 +136,15 @@ module.exports = { return; } - // ({ x: this.#usedInDestructuring } = bar); + /* + * ({ x: this.#usedInDestructuring } = bar); + * + * But should treat the following as a read: + * ({ [this.#x]: a } = foo); + */ if (wrappingExpressionType === "Property" && - parentOfWrappingExpressionType === "ObjectPattern") { + parentOfWrappingExpressionType === "ObjectPattern" && + privateIdentifierNode.parent.parent.value === privateIdentifierNode.parent) { return; } @@ -141,12 +154,17 @@ module.exports = { } // [this.#unusedInAssignmentPattern] = bar; - if (wrappingExpressionType === "ArrayPattern" && - parentOfWrappingExpressionType === "AssignmentExpression") { + if (wrappingExpressionType === "ArrayPattern") { return; } - trackedClasses[0].delete(privateIdentifierNode.name); + /* + * 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; }, /* @@ -157,7 +175,10 @@ module.exports = { "ClassBody:exit"() { const unusedPrivateMembers = trackedClasses.shift(); - for (const [classMemberName, { declaredNode }] of unusedPrivateMembers.entries()) { + for (const [classMemberName, { declaredNode, isUsed }] of unusedPrivateMembers.entries()) { + if (isUsed) { + continue; + } context.report({ node: declaredNode, messageId: "unusedPrivateClassMember", diff --git a/tests/lib/rules/no-unused-private-class-members.js b/tests/lib/rules/no-unused-private-class-members.js index 827ec9f888a..a8089854725 100644 --- a/tests/lib/rules/no-unused-private-class-members.js +++ b/tests/lib/rules/no-unused-private-class-members.js @@ -107,9 +107,47 @@ ruleTester.run("no-unused-private-class-members", rule, { } }`, `class Foo { - #unusedInAssignmentPattern; + #usedInAssignmentPattern; method() { - [bar] = this.#unusedInAssignmentPattern; + [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 C { + get #accessorWithGetterFirst() { + return something(); + } + set #accessorWithGetterFirst(value) { + doSomething(value); + } + method() { + this.#accessorWithGetterFirst += 1; + } +}`, + `class C { + #usedInInnerClass; + + method(a) { + return class { + foo = a.#usedInInnerClass; + } } }`, @@ -317,6 +355,29 @@ class Second {}`, } }`, 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 + }] } ] }); From 855484b71b287acbc8e6638354aeab59547f5bab Mon Sep 17 00:00:00 2001 From: Tim van der Lippe Date: Thu, 14 Oct 2021 20:57:33 +0100 Subject: [PATCH 07/10] Apply suggestions from code review Co-authored-by: Milos Djermanovic --- docs/rules/no-unused-private-class-members.md | 29 ++++++++++++++++--- lib/rules/no-unused-private-class-members.js | 1 + 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/docs/rules/no-unused-private-class-members.md b/docs/rules/no-unused-private-class-members.md index 1db141dec6e..2eb9f4bf485 100644 --- a/docs/rules/no-unused-private-class-members.md +++ b/docs/rules/no-unused-private-class-members.md @@ -4,16 +4,16 @@ Private class members that are declared and not used anywhere in the code are mo ## Rule Details -A private class member `#foo` is considered to be used if any of the following are true: +This rule reports unused private class members. -* It is called (`this.#foo()`) -* It is read (`this.#foo`) -* It is passed into a function as an argument (`doSomething(this.#foo)`) +* 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; } @@ -25,9 +25,21 @@ class Foo { } } +class Foo { + #usedOnlyToUpdateItself = 5; + method() { + this.#usedOnlyToUpdateItself++; + } +} + class Foo { #unusedMethod() {} } + +class Foo { + get #unusedAccessor() {} + set #unusedAccessor(value) {} +} ``` Examples of **correct** code for this rule: @@ -50,6 +62,15 @@ class Foo { return this.#usedMethod(); } } + +class Foo { + get #usedAccessor() {} + set #usedAccessor(value) {} + + method() { + this.#usedAccessor = 42; + } +} ``` ## When Not To Use It diff --git a/lib/rules/no-unused-private-class-members.js b/lib/rules/no-unused-private-class-members.js index ff16f6f2573..9b6c246f9f8 100644 --- a/lib/rules/no-unused-private-class-members.js +++ b/lib/rules/no-unused-private-class-members.js @@ -181,6 +181,7 @@ module.exports = { } context.report({ node: declaredNode, + loc: declaredNode.key.loc, messageId: "unusedPrivateClassMember", data: { classMemberName From 5b6b934a2bc6111b0be7ef6a3cd369c2e7c7733e Mon Sep 17 00:00:00 2001 From: Tim van der Lippe Date: Thu, 14 Oct 2021 20:57:45 +0100 Subject: [PATCH 08/10] Update lib/rules/no-unused-private-class-members.js Co-authored-by: Milos Djermanovic --- lib/rules/no-unused-private-class-members.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/rules/no-unused-private-class-members.js b/lib/rules/no-unused-private-class-members.js index 9b6c246f9f8..e686a4f6590 100644 --- a/lib/rules/no-unused-private-class-members.js +++ b/lib/rules/no-unused-private-class-members.js @@ -184,7 +184,7 @@ module.exports = { loc: declaredNode.key.loc, messageId: "unusedPrivateClassMember", data: { - classMemberName + classMemberName: `#${classMemberName}` } }); } From 273cbf6739d81066ce8cad27b2a662b46ffb3b41 Mon Sep 17 00:00:00 2001 From: Tim van der Lippe Date: Thu, 14 Oct 2021 20:58:12 +0100 Subject: [PATCH 09/10] Update no-unused-private-class-members.js --- tests/lib/rules/no-unused-private-class-members.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/lib/rules/no-unused-private-class-members.js b/tests/lib/rules/no-unused-private-class-members.js index a8089854725..b3b4109d63d 100644 --- a/tests/lib/rules/no-unused-private-class-members.js +++ b/tests/lib/rules/no-unused-private-class-members.js @@ -27,7 +27,7 @@ function definedError(classMemberName) { return { messageId: "unusedPrivateClassMember", data: { - classMemberName + classMemberName: `#${classMemberName}` } }; } From 5433a355538aef766e204762266a8ab04cdb9f92 Mon Sep 17 00:00:00 2001 From: Tim van der Lippe Date: Thu, 21 Oct 2021 10:34:12 +0100 Subject: [PATCH 10/10] Fix accessor handling --- lib/rules/no-unused-private-class-members.js | 24 +++++++++---------- .../rules/no-unused-private-class-members.js | 7 ++++++ 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/lib/rules/no-unused-private-class-members.js b/lib/rules/no-unused-private-class-members.js index e686a4f6590..74cf6ab694a 100644 --- a/lib/rules/no-unused-private-class-members.js +++ b/lib/rules/no-unused-private-class-members.js @@ -32,11 +32,10 @@ module.exports = { /** * Check whether the current node is in a write only assignment. * @param {ASTNode} privateIdentifierNode Node referring to a private identifier - * @param {boolean} isAccessor Whether the property is declared as setter * @returns {boolean} Whether the node is in a write only assignment * @private */ - function isWriteOnlyAssignment(privateIdentifierNode, isAccessor) { + function isWriteOnlyAssignment(privateIdentifierNode) { const parentStatement = privateIdentifierNode.parent.parent; const isAssignmentExpression = parentStatement.type === "AssignmentExpression"; @@ -47,14 +46,6 @@ module.exports = { return false; } - /* - * Any assignment that writes to a setter is considered a read, as the setter can have - * side-effects in its definition. - */ - if (isAccessor) { - 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; @@ -122,8 +113,17 @@ module.exports = { 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, memberDefinition.isAccessor)) { + if (isWriteOnlyAssignment(privateIdentifierNode)) { return; } @@ -181,7 +181,7 @@ module.exports = { } context.report({ node: declaredNode, - loc: declaredNode.key.loc, + 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 index b3b4109d63d..6e80baae968 100644 --- a/tests/lib/rules/no-unused-private-class-members.js +++ b/tests/lib/rules/no-unused-private-class-members.js @@ -129,6 +129,13 @@ ruleTester.run("no-unused-private-class-members", rule, { method() { this.#accessorWithSetterFirst += 1; } +}`, + `class Foo { + set #accessorUsedInMemberAccess(value) {} + + method(a) { + [this.#accessorUsedInMemberAccess] = a; + } }`, `class C { get #accessorWithGetterFirst() {