Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New: Add no-unused-private-class-members rule (fixes #14859) #14895

Merged
merged 12 commits into from Oct 22, 2021
78 changes: 78 additions & 0 deletions 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"*/
TimvdLippe marked this conversation as resolved.
Show resolved Hide resolved

class Foo {
#unusedMember = 5;
}

class Foo {
#usedOnlyInWrite = 5;
method() {
this.#usedOnlyInWrite = 42;
}
}

TimvdLippe marked this conversation as resolved.
Show resolved Hide resolved
class Foo {
#usedOnlyToUpdateItself = 5;
method() {
this.#usedOnlyToUpdateItself++;
}
}

class Foo {
#unusedMethod() {}
}
TimvdLippe marked this conversation as resolved.
Show resolved Hide resolved

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();
}
}
TimvdLippe marked this conversation as resolved.
Show resolved Hide resolved

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.
1 change: 1 addition & 0 deletions lib/rules/index.js
Expand Up @@ -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"),
Expand Down
194 changes: 194 additions & 0 deletions 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,
TimvdLippe marked this conversation as resolved.
Show resolved Hide resolved
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) {
TimvdLippe marked this conversation as resolved.
Show resolved Hide resolved
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,
TimvdLippe marked this conversation as resolved.
Show resolved Hide resolved
loc: declaredNode.key.loc,
messageId: "unusedPrivateClassMember",
data: {
classMemberName: `#${classMemberName}`
}
});
}
}
};
}
};