Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
feat(eslint-plugin): [no-unused-private-class-members] new extension rule#10913
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Changes fromall commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| --- | ||
| description: 'Disallow unused private class members.' | ||
| --- | ||
| import Tabs from '@theme/Tabs'; | ||
| import TabItem from '@theme/TabItem'; | ||
| > 🛑 This file is source code, not the primary documentation location! 🛑 | ||
| > | ||
| > See **https://typescript-eslint.io/rules/no-unused-private-class-members** for documentation. | ||
| This rule extends the base [`eslint/no-unused-private-class-members`](https://eslint.org/docs/rules/no-unused-private-class-members) rule. | ||
| It adds support for members declared with TypeScript's `private` keyword. | ||
| ## Options | ||
| This rule has no options. | ||
| ## Examples | ||
| <Tabs> | ||
| <TabItem value="❌ Incorrect"> | ||
| ```ts | ||
| class A { | ||
| private foo = 123; | ||
| } | ||
| ``` | ||
| </TabItem> | ||
| <TabItem value="✅ Correct"> | ||
| ```tsx | ||
| class A { | ||
| private foo = 123; | ||
| constructor() { | ||
| console.log(this.foo); | ||
| } | ||
| } | ||
| ``` | ||
| </TabItem> | ||
| </Tabs> | ||
| ## Limitations | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. What do you think about also mentioning assigning classTest1{privatebar:number;foo(){constself=this;returnfunction(){// ❌ NOT detected as a usageself.bar;}}} | ||
| This rule does not detect the following cases: | ||
| (1) Nested classes using private members of the outer class: | ||
| ```ts | ||
| class Foo { | ||
| private prop = 123; | ||
| method(a: Foo) { | ||
| return class { | ||
| // ✅ Detected as a usage | ||
| prop = this.prop; | ||
| // ❌ NOT detected as a usage | ||
| innerProp = a.prop; | ||
| }; | ||
| } | ||
| } | ||
| ``` | ||
| (2) Usages of the private member outside of the class: | ||
| ```ts | ||
| class Foo { | ||
| private prop = 123; | ||
| } | ||
| const instance = new Foo(); | ||
| // ❌ NOT detected as a usage | ||
| console.log(foo['prop']); | ||
| ``` | ||
| ## 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. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,266 @@ | ||
| // The following code is adapted from the code in eslint/eslint. | ||
| // Source: https://github.com/eslint/eslint/blob/522d8a32f326c52886c531f43cf6a1ff15af8286/lib/rules/no-unused-private-class-members.js | ||
| // License: https://github.com/eslint/eslint/blob/522d8a32f326c52886c531f43cf6a1ff15af8286/LICENSE | ||
| import type { TSESTree } from '@typescript-eslint/utils'; | ||
| import { AST_NODE_TYPES } from '@typescript-eslint/utils'; | ||
| import { createRule } from '../util'; | ||
| type Options = []; | ||
| export type MessageIds = 'unusedPrivateClassMember'; | ||
| interface PrivateMember { | ||
| declaredNode: | ||
| | TSESTree.MethodDefinitionComputedName | ||
| | TSESTree.MethodDefinitionNonComputedName | ||
| | TSESTree.PropertyDefinitionComputedName | ||
| | TSESTree.PropertyDefinitionNonComputedName; | ||
| isAccessor: boolean; | ||
| } | ||
| export default createRule<Options, MessageIds>({ | ||
| name: 'no-unused-private-class-members', | ||
| meta: { | ||
| type: 'problem', | ||
| docs: { | ||
| description: 'Disallow unused private class members', | ||
| extendsBaseRule: true, | ||
| requiresTypeChecking: false, | ||
| }, | ||
| messages: { | ||
| unusedPrivateClassMember: | ||
| "'{{classMemberName}}' is defined but never used.", | ||
| }, | ||
| schema: [], | ||
| }, | ||
| defaultOptions: [], | ||
| create(context) { | ||
| const trackedClassMembers: Map<string, PrivateMember>[] = []; | ||
| /** | ||
| * The core ESLint rule tracks methods by adding an extra property of | ||
| * "isUsed" to the method, which is a bug according to Joshua Goldberg. | ||
| * Instead, in our extended rule, we create a separate data structure to | ||
| * track whether a method is being used. | ||
| */ | ||
| const trackedClassMembersUsed = new Set< | ||
| | TSESTree.MethodDefinitionComputedName | ||
| | TSESTree.MethodDefinitionNonComputedName | ||
| | TSESTree.PropertyDefinitionComputedName | ||
| | TSESTree.PropertyDefinitionNonComputedName | ||
| >(); | ||
| /** | ||
| * Check whether the current node is in a write only assignment. | ||
| * @param node Node referring to a private identifier | ||
| * @returns Whether the node is in a write only assignment | ||
| * @private | ||
| */ | ||
| function isWriteOnlyAssignment( | ||
| node: TSESTree.Identifier | TSESTree.PrivateIdentifier, | ||
| ): boolean { | ||
| const parentStatement = node.parent.parent; | ||
| if (parentStatement == null) { | ||
| return false; | ||
| } | ||
| const isAssignmentExpression = | ||
| parentStatement.type === AST_NODE_TYPES.AssignmentExpression; | ||
| if ( | ||
| !isAssignmentExpression && | ||
| parentStatement.type !== AST_NODE_TYPES.ForInStatement && | ||
| parentStatement.type !== AST_NODE_TYPES.ForOfStatement && | ||
| parentStatement.type !== AST_NODE_TYPES.AssignmentPattern | ||
| ) { | ||
| return false; | ||
| } | ||
| // It is a write-only usage, since we still allow usages on the right for | ||
| // reads. | ||
| if (parentStatement.left !== node.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 === AST_NODE_TYPES.ExpressionStatement | ||
| ); | ||
| } | ||
| return true; | ||
| } | ||
| //-------------------------------------------------------------------------- | ||
| // Public | ||
| //-------------------------------------------------------------------------- | ||
| function processPrivateIdentifier( | ||
| node: TSESTree.Identifier | TSESTree.PrivateIdentifier, | ||
| ): void { | ||
| const classBody = trackedClassMembers.find(classProperties => | ||
| classProperties.has(node.name), | ||
| ); | ||
| // Can't happen, as it is a parser error to have a missing class body, but | ||
| // let's code defensively here. | ||
| if (classBody == null) { | ||
| return; | ||
| } | ||
| // In case any other usage was already detected, we can short circuit the | ||
| // logic here. | ||
| const memberDefinition = classBody.get(node.name); | ||
| if (memberDefinition == null) { | ||
| return; | ||
| } | ||
| if (trackedClassMembersUsed.has(memberDefinition.declaredNode)) { | ||
| return; | ||
| } | ||
| // The definition of the class member itself | ||
| if ( | ||
| node.parent.type === AST_NODE_TYPES.PropertyDefinition || | ||
| node.parent.type === AST_NODE_TYPES.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) { | ||
| trackedClassMembersUsed.add(memberDefinition.declaredNode); | ||
| return; | ||
| } | ||
| // Any assignments to this member, except for assignments that also read | ||
| if (isWriteOnlyAssignment(node)) { | ||
| return; | ||
| } | ||
| const wrappingExpressionType = node.parent.parent?.type; | ||
| const parentOfWrappingExpressionType = node.parent.parent?.parent?.type; | ||
| // A statement which only increments (`this.#x++;`) | ||
| if ( | ||
| wrappingExpressionType === AST_NODE_TYPES.UpdateExpression && | ||
| parentOfWrappingExpressionType === AST_NODE_TYPES.ExpressionStatement | ||
| ) { | ||
| return; | ||
| } | ||
| /* | ||
| * ({ x: this.#usedInDestructuring } = bar); | ||
| * | ||
| * But should treat the following as a read: | ||
| * ({ [this.#x]: a } = foo); | ||
| */ | ||
| if ( | ||
| wrappingExpressionType === AST_NODE_TYPES.Property && | ||
| parentOfWrappingExpressionType === AST_NODE_TYPES.ObjectPattern && | ||
| node.parent.parent?.value === node.parent | ||
| ) { | ||
| return; | ||
| } | ||
| // [...this.#unusedInRestPattern] = bar; | ||
| if (wrappingExpressionType === AST_NODE_TYPES.RestElement) { | ||
| return; | ||
| } | ||
| // [this.#unusedInAssignmentPattern] = bar; | ||
| if (wrappingExpressionType === AST_NODE_TYPES.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. | ||
| trackedClassMembersUsed.add(memberDefinition.declaredNode); | ||
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. [Bug]: I've noticed some missed reports with functions that have a different classTest1{// should be reported but doesn't?privatebar:number;foo=function(){returnthis.bar;}} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. This is because the base rule doesn't support that case because It's worth noting that TS doesn't actually catch this case either and reports We could support this if we wanted to. I'm leaning towards not because TS itself doesn't catch it either. Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Yes! I think the rule should report | ||
| } | ||
| return { | ||
| // Collect all declared members/methods up front and assume they are all | ||
| // unused. | ||
| ClassBody(classBodyNode): void { | ||
| const privateMembers = new Map<string, PrivateMember>(); | ||
| trackedClassMembers.unshift(privateMembers); | ||
| for (const bodyMember of classBodyNode.body) { | ||
| if ( | ||
| (bodyMember.type === AST_NODE_TYPES.PropertyDefinition || | ||
| bodyMember.type === AST_NODE_TYPES.MethodDefinition) && | ||
| (bodyMember.key.type === AST_NODE_TYPES.PrivateIdentifier || | ||
| (bodyMember.key.type === AST_NODE_TYPES.Identifier && | ||
bradzacher marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
| bodyMember.accessibility === 'private')) | ||
bradzacher marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
| ) { | ||
| privateMembers.set(bodyMember.key.name, { | ||
| declaredNode: bodyMember, | ||
| isAccessor: | ||
| bodyMember.type === AST_NODE_TYPES.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(node): void { | ||
| processPrivateIdentifier(node); | ||
| }, | ||
Comment on lines +219 to +221 Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. [Bug]: I think there's an inconsistency that creates false positives with classTest1{publicstaticfoo(){returnTest1.#bar;}// correctly not reportedstatic #bar=1;}classTest2{publicstaticfoo(){returnTest2.bar;}// reported but shouldn't?privatestaticbar=1;} I've noticed a similar false positive with the following (deploy preview playground link): classTest1{publicfoo(a:Test1){returna.#bar;}// correctly not reported #bar=1;}classTest2{publicfoo(a:Test2){returna.bar;}// reported but shouldn't?privatebar=1;} MemberAuthor
| ||
| // Process all usages of the identifier and remove a member from | ||
| // `declaredAndUnusedPrivateMembers` if we deem it used. | ||
| MemberExpression(node): void { | ||
| if ( | ||
| !node.computed && | ||
| node.object.type === AST_NODE_TYPES.ThisExpression && | ||
| node.property.type === AST_NODE_TYPES.Identifier | ||
| ) { | ||
| processPrivateIdentifier(node.property); | ||
| } | ||
| }, | ||
| // 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'(): void { | ||
| const unusedPrivateMembers = trackedClassMembers.shift(); | ||
| if (unusedPrivateMembers == null) { | ||
| return; | ||
| } | ||
| for (const [ | ||
| classMemberName, | ||
| { declaredNode }, | ||
| ] of unusedPrivateMembers.entries()) { | ||
| if (trackedClassMembersUsed.has(declaredNode)) { | ||
| continue; | ||
| } | ||
| context.report({ | ||
| loc: declaredNode.key.loc, | ||
| node: declaredNode, | ||
| messageId: 'unusedPrivateClassMember', | ||
| data: { | ||
| classMemberName: | ||
| declaredNode.key.type === AST_NODE_TYPES.PrivateIdentifier | ||
| ? `#${classMemberName}` | ||
| : classMemberName, | ||
| }, | ||
| }); | ||
| } | ||
| }, | ||
| }; | ||
| }, | ||
| }); | ||
Some generated files are not rendered by default. Learn more abouthow customized files appear on GitHub.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.