From 34b77b871b759d2ee9344ac245b39dc5f4a40535 Mon Sep 17 00:00:00 2001 From: bradzacher Date: Mon, 3 Mar 2025 12:28:13 +1030 Subject: [PATCH 1/6] feat(eslint-plugin): [no-unused-private-class-members] new extension rule --- .../rules/no-unused-private-class-members.mdx | 82 +++ packages/eslint-plugin/src/configs/all.ts | 2 + packages/eslint-plugin/src/rules/index.ts | 2 + .../rules/no-unused-private-class-members.ts | 266 +++++++++ .../no-unused-private-class-members.shot | 24 + .../no-unused-private-class-members.test.ts | 520 ++++++++++++++++++ packages/typescript-eslint/src/configs/all.ts | 2 + 7 files changed, 898 insertions(+) create mode 100644 packages/eslint-plugin/docs/rules/no-unused-private-class-members.mdx create mode 100644 packages/eslint-plugin/src/rules/no-unused-private-class-members.ts create mode 100644 packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-unused-private-class-members.shot create mode 100644 packages/eslint-plugin/tests/rules/no-unused-private-class-members.test.ts diff --git a/packages/eslint-plugin/docs/rules/no-unused-private-class-members.mdx b/packages/eslint-plugin/docs/rules/no-unused-private-class-members.mdx new file mode 100644 index 000000000000..b055ecbf3a15 --- /dev/null +++ b/packages/eslint-plugin/docs/rules/no-unused-private-class-members.mdx @@ -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 + + + + +```ts +class A { + private foo = 123; +} +``` + + + + +```tsx +class A { + private foo = 123; + + constructor() { + console.log(this.foo); + } +} +``` + + + + +## Limitations + +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. diff --git a/packages/eslint-plugin/src/configs/all.ts b/packages/eslint-plugin/src/configs/all.ts index 6febc52e3977..e7a349994486 100644 --- a/packages/eslint-plugin/src/configs/all.ts +++ b/packages/eslint-plugin/src/configs/all.ts @@ -111,6 +111,8 @@ export = { '@typescript-eslint/no-unsafe-unary-minus': 'error', 'no-unused-expressions': 'off', '@typescript-eslint/no-unused-expressions': 'error', + 'no-unused-private-class-members': 'off', + '@typescript-eslint/no-unused-private-class-members': 'error', 'no-unused-vars': 'off', '@typescript-eslint/no-unused-vars': 'error', 'no-use-before-define': 'off', diff --git a/packages/eslint-plugin/src/rules/index.ts b/packages/eslint-plugin/src/rules/index.ts index f1587b5d2b1f..9f05b7ea6469 100644 --- a/packages/eslint-plugin/src/rules/index.ts +++ b/packages/eslint-plugin/src/rules/index.ts @@ -87,6 +87,7 @@ import noUnsafeReturn from './no-unsafe-return'; import noUnsafeTypeAssertion from './no-unsafe-type-assertion'; import noUnsafeUnaryMinus from './no-unsafe-unary-minus'; import noUnusedExpressions from './no-unused-expressions'; +import noUnusedPrivateClassMembers from './no-unused-private-class-members'; import noUnusedVars from './no-unused-vars'; import noUseBeforeDefine from './no-use-before-define'; import noUselessConstructor from './no-useless-constructor'; @@ -220,6 +221,7 @@ const rules = { 'no-unsafe-type-assertion': noUnsafeTypeAssertion, 'no-unsafe-unary-minus': noUnsafeUnaryMinus, 'no-unused-expressions': noUnusedExpressions, + 'no-unused-private-class-members': noUnusedPrivateClassMembers, 'no-unused-vars': noUnusedVars, 'no-use-before-define': noUseBeforeDefine, 'no-useless-constructor': noUselessConstructor, diff --git a/packages/eslint-plugin/src/rules/no-unused-private-class-members.ts b/packages/eslint-plugin/src/rules/no-unused-private-class-members.ts new file mode 100644 index 000000000000..34f909ddb435 --- /dev/null +++ b/packages/eslint-plugin/src/rules/no-unused-private-class-members.ts @@ -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({ + 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[] = []; + + /** + * 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); + } + + return { + // Collect all declared members/methods up front and assume they are all + // unused. + ClassBody(classBodyNode): void { + const privateMembers = new Map(); + + 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 && + bodyMember.accessibility === 'private')) + ) { + 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); + }, + + // 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, + }, + }); + } + }, + }; + }, +}); diff --git a/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-unused-private-class-members.shot b/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-unused-private-class-members.shot new file mode 100644 index 000000000000..1bea7a556d71 --- /dev/null +++ b/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-unused-private-class-members.shot @@ -0,0 +1,24 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`Validating rule docs no-unused-private-class-members.mdx code examples ESLint output 1`] = ` +"Incorrect + +class A { + private foo = 123; + ~~~ 'foo' is defined but never used. +} +" +`; + +exports[`Validating rule docs no-unused-private-class-members.mdx code examples ESLint output 2`] = ` +"Correct + +class A { + private foo = 123; + + constructor() { + console.log(this.foo); + } +} +" +`; diff --git a/packages/eslint-plugin/tests/rules/no-unused-private-class-members.test.ts b/packages/eslint-plugin/tests/rules/no-unused-private-class-members.test.ts new file mode 100644 index 000000000000..5cd87e322619 --- /dev/null +++ b/packages/eslint-plugin/tests/rules/no-unused-private-class-members.test.ts @@ -0,0 +1,520 @@ +// The following tests are adapted from the tests in eslint. +// Original Code: https://github.com/eslint/eslint/blob/522d8a32f326c52886c531f43cf6a1ff15af8286/tests/lib/rules/no-unused-private-class-members.js +// License : https://github.com/eslint/eslint/blob/522d8a32f326c52886c531f43cf6a1ff15af8286/LICENSE + +import type { TestCaseError } from '@typescript-eslint/rule-tester'; + +import { RuleTester } from '@typescript-eslint/rule-tester'; + +import type { MessageIds } from '../../src/rules/no-unused-private-class-members'; + +import rule from '../../src/rules/no-unused-private-class-members'; + +const ruleTester = new RuleTester(); + +/** Returns an expected error for defined-but-not-used private class member. */ +function definedError( + classMemberName: string, + addHashTag = true, +): TestCaseError { + return { + data: { + classMemberName: addHashTag ? `#${classMemberName}` : classMemberName, + }, + messageId: 'unusedPrivateClassMember', + }; +} + +ruleTester.run('no-unused-private-class-members', rule, { + valid: [ + 'class Foo {}', + ` +class Foo { + publicMember = 42; +} + `, + ` +class Foo { + public publicMember = 42; +} + `, + ` +class Foo { + protected publicMember = 42; +} + `, + ` +class C { + #usedInInnerClass; + + method(a) { + return class { + foo = a.#usedInInnerClass; + }; + } +} + `, + + ...[ + ` +class Foo { + @# = 42; + method() { + return this.#; + } +} + `, + ` +class Foo { + @# = 42; + anotherMember = this.#; +} + `, + ` +class Foo { + @# = 42; + foo() { + anotherMember = this.#; + } +} + `, + ` +class C { + @#; + + foo() { + bar((this.# += 1)); + } +} + `, + ` +class Foo { + @# = 42; + method() { + return someGlobalMethod(this.#); + } +} + `, + ` +class C { + @#; + + foo() { + return class {}; + } + + bar() { + return this.#; + } +} + `, + ` +class Foo { + @#; + method() { + for (const bar in this.#) { + } + } +} + `, + ` +class Foo { + @#; + method() { + for (const bar of this.#) { + } + } +} + `, + ` +class Foo { + @#; + method() { + [bar = 1] = this.#; + } +} + `, + ` +class Foo { + @#; + method() { + [bar] = this.#; + } +} + `, + ` +class C { + @#; + + method() { + ({ [this.#]: a } = foo); + } +} + `, + ` +class C { + @set #(value) { + doSomething(value); + } + @get #() { + return something(); + } + method() { + this.# += 1; + } +} + `, + ` +class Foo { + @set #(value) {} + + method(a) { + [this.#] = a; + } +} + `, + ` +class C { + @get #() { + return something(); + } + @set #(value) { + doSomething(value); + } + method() { + this.# += 1; + } +} + `, + + //-------------------------------------------------------------------------- + // Method definitions + //-------------------------------------------------------------------------- + ` +class Foo { + @#() { + return 42; + } + anotherMethod() { + return this.#(); + } +} + `, + ` +class C { + @set #(value) { + doSomething(value); + } + + foo() { + this.# = 1; + } +} + `, + ].flatMap(code => [ + code.replaceAll('#', '#privateMember').replaceAll('@', ''), + code.replaceAll('#', 'privateMember').replaceAll('@', 'private '), + ]), + ], + invalid: [ + { + 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')], + }, + { + code: ` +class C { + #usedOnlyInTheSecondInnerClass; + + method(a) { + return class { + #usedOnlyInTheSecondInnerClass; + + method2(b) { + foo = b.#usedOnlyInTheSecondInnerClass; + } + + method3(b) { + foo = b.#usedOnlyInTheSecondInnerClass; + } + }; + } +} + `, + errors: [ + { + ...definedError('usedOnlyInTheSecondInnerClass'), + line: 3, + }, + ], + }, + + // intentionally not handled cases + { + code: ` +class C { + private usedInInnerClass; + + method(a: C) { + return class { + foo = a.usedInInnerClass; + }; + } +} + `, + errors: [definedError('usedInInnerClass', false)], + }, + { + code: ` +class C { + private usedOutsideClass; +} + +const instance = new C(); +console.log(instance.usedOutsideClass); + `, + errors: [definedError('usedOutsideClass', false)], + }, + + ...[ + { + code: ` +class Foo { + @# = 5; +} + `, + errors: [definedError('#', false)], + }, + { + code: ` +class First {} +class Second { + @# = 5; +} + `, + errors: [definedError('#', false)], + }, + { + code: ` +class First { + @# = 5; +} +class Second {} + `, + errors: [definedError('#', false)], + }, + { + code: ` +class First { + @# = 5; + @#2 = 5; +} + `, + errors: [definedError('#', false), definedError('#2', false)], + }, + { + code: ` +class Foo { + @# = 5; + method() { + this.# = 42; + } +} + `, + errors: [definedError('#', false)], + }, + { + code: ` +class Foo { + @# = 5; + method() { + this.# += 42; + } +} + `, + errors: [definedError('#', false)], + }, + { + code: ` +class C { + @#; + + foo() { + this.#++; + } +} + `, + errors: [definedError('#', false)], + }, + + //-------------------------------------------------------------------------- + // Unused method definitions + //-------------------------------------------------------------------------- + { + code: ` +class Foo { + @#() {} +} + `, + errors: [definedError('#', false)], + }, + { + code: ` +class Foo { + @#() {} + @#Used() { + return 42; + } + publicMethod() { + return this.#Used(); + } +} + `, + errors: [definedError('#', false)], + }, + { + code: ` +class Foo { + @set #(value) {} +} + `, + errors: [definedError('#', false)], + }, + { + code: ` +class Foo { + @#; + method() { + for (this.# in bar) { + } + } +} + `, + errors: [definedError('#', false)], + }, + { + code: ` +class Foo { + @#; + method() { + for (this.# of bar) { + } + } +} + `, + errors: [definedError('#', false)], + }, + { + code: ` +class Foo { + @#; + method() { + ({ x: this.# } = bar); + } +} + `, + errors: [definedError('#', false)], + }, + { + code: ` +class Foo { + @#; + method() { + [...this.#] = bar; + } +} + `, + errors: [definedError('#', false)], + }, + { + code: ` +class Foo { + @#; + method() { + [this.# = 1] = bar; + } +} + `, + errors: [definedError('#', false)], + }, + { + code: ` +class Foo { + @#; + method() { + [this.#] = bar; + } +} + `, + errors: [definedError('#', false)], + }, + ].flatMap(({ code, errors }) => [ + { + code: code.replaceAll('#', '#privateMember').replaceAll('@', ''), + errors: errors.map(error => ({ + ...error, + data: { + classMemberName: (error.data?.classMemberName as string).replaceAll( + '#', + '#privateMember', + ), + }, + })), + }, + { + code: code.replaceAll('#', 'privateMember').replaceAll('@', 'private '), + errors: errors.map(error => ({ + ...error, + data: { + classMemberName: (error.data?.classMemberName as string).replaceAll( + '#', + 'privateMember', + ), + }, + })), + }, + ]), + ], +}); diff --git a/packages/typescript-eslint/src/configs/all.ts b/packages/typescript-eslint/src/configs/all.ts index 4bf1a3b4c5d4..557f725da3f8 100644 --- a/packages/typescript-eslint/src/configs/all.ts +++ b/packages/typescript-eslint/src/configs/all.ts @@ -125,6 +125,8 @@ export default ( '@typescript-eslint/no-unsafe-unary-minus': 'error', 'no-unused-expressions': 'off', '@typescript-eslint/no-unused-expressions': 'error', + 'no-unused-private-class-members': 'off', + '@typescript-eslint/no-unused-private-class-members': 'error', 'no-unused-vars': 'off', '@typescript-eslint/no-unused-vars': 'error', 'no-use-before-define': 'off', From 08540073bdffc3651d4898bff2773cb6a8688f3d Mon Sep 17 00:00:00 2001 From: Brad Zacher Date: Sun, 9 Mar 2025 12:02:13 +1030 Subject: [PATCH 2/6] support accessor properties --- .../rules/no-unused-private-class-members.mdx | 12 ++--- .../rules/no-unused-private-class-members.ts | 20 ++++---- .../no-unused-private-class-members.test.ts | 47 +++++++++++++++++++ 3 files changed, 63 insertions(+), 16 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/no-unused-private-class-members.mdx b/packages/eslint-plugin/docs/rules/no-unused-private-class-members.mdx index b055ecbf3a15..aad7b5bac464 100644 --- a/packages/eslint-plugin/docs/rules/no-unused-private-class-members.mdx +++ b/packages/eslint-plugin/docs/rules/no-unused-private-class-members.mdx @@ -47,20 +47,18 @@ class A { This rule does not detect the following cases: -(1) Nested classes using private members of the outer class: +(1) Private members only used using private only without `this`: ```ts class Foo { private prop = 123; method(a: Foo) { - return class { - // ✅ Detected as a usage - prop = this.prop; + // ✅ Detected as a usage + const prop = this.prop; - // ❌ NOT detected as a usage - innerProp = a.prop; - }; + // ❌ NOT detected as a usage + const otherProp = a.prop; } } ``` diff --git a/packages/eslint-plugin/src/rules/no-unused-private-class-members.ts b/packages/eslint-plugin/src/rules/no-unused-private-class-members.ts index 34f909ddb435..daed826ebddf 100644 --- a/packages/eslint-plugin/src/rules/no-unused-private-class-members.ts +++ b/packages/eslint-plugin/src/rules/no-unused-private-class-members.ts @@ -11,15 +11,6 @@ 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({ name: 'no-unused-private-class-members', meta: { @@ -39,6 +30,15 @@ export default createRule({ }, defaultOptions: [], create(context) { + interface PrivateMember { + declaredNode: + | TSESTree.AccessorProperty + | TSESTree.MethodDefinitionComputedName + | TSESTree.MethodDefinitionNonComputedName + | TSESTree.PropertyDefinitionComputedName + | TSESTree.PropertyDefinitionNonComputedName; + isAccessor: boolean; + } const trackedClassMembers: Map[] = []; /** @@ -48,6 +48,7 @@ export default createRule({ * track whether a method is being used. */ const trackedClassMembersUsed = new Set< + | TSESTree.AccessorProperty | TSESTree.MethodDefinitionComputedName | TSESTree.MethodDefinitionNonComputedName | TSESTree.PropertyDefinitionComputedName @@ -199,6 +200,7 @@ export default createRule({ for (const bodyMember of classBodyNode.body) { if ( (bodyMember.type === AST_NODE_TYPES.PropertyDefinition || + bodyMember.type === AST_NODE_TYPES.AccessorProperty || bodyMember.type === AST_NODE_TYPES.MethodDefinition) && (bodyMember.key.type === AST_NODE_TYPES.PrivateIdentifier || (bodyMember.key.type === AST_NODE_TYPES.Identifier && diff --git a/packages/eslint-plugin/tests/rules/no-unused-private-class-members.test.ts b/packages/eslint-plugin/tests/rules/no-unused-private-class-members.test.ts index 5cd87e322619..b25b947cc7c1 100644 --- a/packages/eslint-plugin/tests/rules/no-unused-private-class-members.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unused-private-class-members.test.ts @@ -54,6 +54,37 @@ class C { } } `, + ` +class C { + private accessor accessorMember = 42; + + method() { + return this.accessorMember; + } +} + `, + ` +class C { + private static staticMember = 42; + + static method() { + return this.staticMember; + } +} + `, + { + code: ` +class C { + private static staticMember = 42; + + method() { + return C.staticMember; + } +} + `, + // TODO make this work + skip: true, + }, ...[ ` @@ -290,6 +321,22 @@ class C { }, ], }, + { + code: ` +class C { + private accessor accessorMember = 42; +} + `, + errors: [definedError('accessorMember', false)], + }, + { + code: ` +class C { + private static staticMember = 42; +} + `, + errors: [definedError('staticMember', false)], + }, // intentionally not handled cases { From df85b86e7d8273d6bcac27a8d6d0d5b20da09a33 Mon Sep 17 00:00:00 2001 From: Brad Zacher Date: Sun, 6 Jul 2025 12:22:44 +0930 Subject: [PATCH 3/6] WIP class scope analyser --- .../classScopeAnalyzer.ts | 325 ++++++++++++++++++ .../extractComputedName.ts | 56 +++ .../src/util/class-scope-analyzer/types.ts | 23 ++ 3 files changed, 404 insertions(+) create mode 100644 packages/eslint-plugin/src/util/class-scope-analyzer/classScopeAnalyzer.ts create mode 100644 packages/eslint-plugin/src/util/class-scope-analyzer/extractComputedName.ts create mode 100644 packages/eslint-plugin/src/util/class-scope-analyzer/types.ts diff --git a/packages/eslint-plugin/src/util/class-scope-analyzer/classScopeAnalyzer.ts b/packages/eslint-plugin/src/util/class-scope-analyzer/classScopeAnalyzer.ts new file mode 100644 index 000000000000..d538563a83c2 --- /dev/null +++ b/packages/eslint-plugin/src/util/class-scope-analyzer/classScopeAnalyzer.ts @@ -0,0 +1,325 @@ +import type { ScopeManager } from '@typescript-eslint/scope-manager'; +import type { TSESTree } from '@typescript-eslint/utils'; + +import { Visitor } from '@typescript-eslint/scope-manager'; +import { AST_NODE_TYPES } from '@typescript-eslint/utils'; + +import type { ClassNode, FunctionNode, Key, MemberNode } from './types'; + +import { + extractNameForMember, + extractNameForMemberExpression, +} from './extractComputedName'; +import { privateKey } from './types'; + +export class Member { + /** + * The node that declares this member + */ + public readonly node: MemberNode; + + /** + * The member name, as given in the source code. + */ + public readonly name: Key; + + /** + * The number of references to this member. + */ + public referenceCount = 0; + + private constructor(node: MemberNode, name: Key) { + this.node = node; + this.name = name; + } + public static create(node: MemberNode): Member | null { + const name = extractNameForMember(node); + if (name == null) { + return null; + } + return new Member(node, name); + } + + /** + * True if the variable is considered an accessor + */ + public isAccessor(): boolean { + return ( + this.node.type === AST_NODE_TYPES.AccessorProperty || + ('kind' in this.node && + (this.node.kind === 'get' || this.node.kind === 'set')) + ); + } + + /** + * True if the variable has the `private` accessibility modifier. + */ + public isPrivate(): boolean { + return this.node.accessibility === 'private'; + } + /** + * True if the variable has the `protected` accessibility modifier. + */ + public isProtected(): boolean { + return this.node.accessibility === 'protected'; + } + + /** + * True if the variable has the `public` accessibility modifier. + */ + public isPublic(): boolean { + return this.node.accessibility === 'public'; + } +} + +abstract class ThisScope extends Visitor { + /** + * The classes directly declared within this class -- for example a class declared within a method. + * This does not include grandchild classes. + */ + public readonly childScopes: ThisScope[] = []; + + /** + * The scope manager instance used to resolve variables to improve discovery of usages. + */ + public readonly scopeManager: ScopeManager; + + /** + * The parent class scope if one exists. + */ + public readonly upper: ThisScope | null; + + /** + * The context of the `this` reference in the current scope. + */ + protected readonly thisContext: ClassScope | null; + + constructor( + scopeManager: ScopeManager, + upper: ThisScope | null, + thisContext: 'none' | 'self' | ClassScope, + ) { + super({}); + this.scopeManager = scopeManager; + this.upper = upper; + + if (thisContext === 'self') { + this.thisContext = this as unknown as ClassScope; + } else if (thisContext === 'none') { + this.thisContext = null; + } else { + this.thisContext = thisContext; + } + } + + private getObjectClass( + node: TSESTree.MemberExpression, + ): { thisContext: ThisScope; type: 'instance' | 'static' } | null { + switch (node.object.type) { + case AST_NODE_TYPES.ThisExpression: { + if (this.thisContext == null) { + return null; + } + return { thisContext: this.thisContext, type: 'instance' }; + } + + case AST_NODE_TYPES.Identifier: { + const thisContext = this.findClassScopeWithName(node.object.name); + if (thisContext != null) { + return { thisContext, type: 'static' }; + } + return null; + } + case AST_NODE_TYPES.MemberExpression: + // TODO - we could probably recurse here to do some more complex analysis and support like + return null; + + default: + return null; + } + } + + private visitClass(node: ClassNode): void { + const classScope = new ClassScope(node, this, this.scopeManager); + this.childScopes.push(classScope); + classScope.visit(node); + } + + private visitFunction(node: FunctionNode): void { + const functionScope = new FunctionScope(this.scopeManager, this, node); + this.childScopes.push(functionScope); + functionScope.visit(node); + } + + /** + * Gets the nearest class scope with the given name. + */ + public findClassScopeWithName(name: string): ClassScope | null { + // eslint-disable-next-line @typescript-eslint/no-this-alias + let currentScope: ThisScope | null = this; + while (currentScope != null) { + if ( + currentScope instanceof ClassScope && + currentScope.className === name + ) { + return currentScope; + } + currentScope = currentScope.upper; + } + return null; + } + + ///////////////////// + // Visit selectors // + ///////////////////// + + protected ClassDeclaration(node: TSESTree.ClassDeclaration): void { + this.visitClass(node); + } + + protected ClassExpression(node: TSESTree.ClassExpression): void { + this.visitClass(node); + } + + protected FunctionDeclaration(node: TSESTree.FunctionDeclaration): void { + this.visitFunction(node); + } + + protected FunctionExpression(node: TSESTree.FunctionExpression): void { + if ( + (node.parent.type === AST_NODE_TYPES.MethodDefinition || + node.parent.type === AST_NODE_TYPES.PropertyDefinition) && + node.parent.value === node + ) { + // if the function is a method's implementation then the `this` is guaranteed to be the class + this.visit(node); + } else { + // TODO(bradzacher): + // - we could handle manual bound functions like `(function () { ... }).bind(this)` + // - we could handle auto-bound methods like `[].map(function () {}, this)` + this.visitFunction(node); + } + } + + protected MemberExpression(node: TSESTree.MemberExpression): void { + const propertyName = extractNameForMemberExpression(node); + if (propertyName == null) { + return; + } + + const objectClassName = this.getObjectClass(node); + } + + protected PrivateIdentifier(node: TSESTree.PrivateIdentifier): void { + const member = this.thisContext?.members.get(privateKey(node)); + if (member == null) { + return; + } + + member.referenceCount += 1; + } +} + +/** + * When we visit a function declaration/expression the `this` reference is + * rebound so it no longer refers to the class. + * + * This also supports a function's `this` parameter. + */ +export class FunctionScope extends ThisScope { + constructor( + scopeManager: ScopeManager, + upper: ThisScope, + node: FunctionNode, + ) { + if ( + node.params[0].type !== AST_NODE_TYPES.Identifier || + node.params[0].name !== 'this' + ) { + super(scopeManager, upper, 'none'); + return; + } + + const thisType = node.params[0].typeAnnotation?.typeAnnotation; + if ( + thisType == null || + thisType.type !== AST_NODE_TYPES.TSTypeReference || + thisType.typeName.type !== AST_NODE_TYPES.Identifier + ) { + super(scopeManager, upper, 'none'); + return; + } + + const thisContext = upper.findClassScopeWithName(thisType.typeName.name); + if (thisContext == null) { + super(scopeManager, upper, 'none'); + return; + } + + super(scopeManager, upper, thisContext); + } +} + +export class ClassScope extends ThisScope { + /** + * The classes name as given in the source code. + * If this is `null` then the class is an anonymous class. + */ + public readonly className: string | null; + + /** + * The class's members, keyed by their name + */ + public readonly members = new Map(); + + /** + * The node that declares this class. + */ + public readonly theClass: ClassNode; + + public constructor( + theClass: ClassNode, + upper: ClassScope | FunctionScope | null, + scopeManager: ScopeManager, + ) { + super(scopeManager, upper, 'self'); + + this.theClass = theClass; + this.className = theClass.id?.name ?? null; + + for (const memberNode of theClass.body.body) { + switch (memberNode.type) { + case AST_NODE_TYPES.AccessorProperty: + case AST_NODE_TYPES.MethodDefinition: + case AST_NODE_TYPES.PropertyDefinition: + case AST_NODE_TYPES.TSAbstractAccessorProperty: + case AST_NODE_TYPES.TSAbstractMethodDefinition: + case AST_NODE_TYPES.TSAbstractPropertyDefinition: { + const member = Member.create(memberNode); + if (member == null) { + continue; + } + this.members.set(member.name, member); + break; + } + + case AST_NODE_TYPES.StaticBlock: + // static blocks declare no members + continue; + + case AST_NODE_TYPES.TSIndexSignature: + // index signatures are type signatures only and are fully computed + continue; + } + } + } +} + +export function analyzeClassMemberUsage( + theClass: ClassNode, + scopeManager: ScopeManager, +): ClassScope { + const analyzer = new ClassScope(theClass, null, scopeManager); + analyzer.visit(theClass); + return analyzer; +} diff --git a/packages/eslint-plugin/src/util/class-scope-analyzer/extractComputedName.ts b/packages/eslint-plugin/src/util/class-scope-analyzer/extractComputedName.ts new file mode 100644 index 000000000000..020d3bf1fa6e --- /dev/null +++ b/packages/eslint-plugin/src/util/class-scope-analyzer/extractComputedName.ts @@ -0,0 +1,56 @@ +import type { TSESTree } from '@typescript-eslint/utils'; + +import { AST_NODE_TYPES } from '@typescript-eslint/utils'; + +import type { Key, MemberNode } from './types'; + +import { privateKey, publicKey } from './types'; + +function extractComputedName(computedName: TSESTree.Expression): Key | null { + if (computedName.type === AST_NODE_TYPES.Literal) { + return publicKey(computedName.value?.toString() ?? 'null'); + } + if ( + computedName.type === AST_NODE_TYPES.TemplateLiteral && + computedName.expressions.length === 0 + ) { + return publicKey(computedName.quasis[0].value.raw); + } + return null; +} +function extractNonComputedName( + nonComputedName: TSESTree.Identifier | TSESTree.PrivateIdentifier, +): Key { + if (nonComputedName.type === AST_NODE_TYPES.PrivateIdentifier) { + return privateKey(nonComputedName); + } + return publicKey(nonComputedName.name); +} +/** + * Extracts the string name for a member. + * @returns `null` if the name cannot be extracted due to it being a computed. + */ +export function extractNameForMember(node: MemberNode): Key | null { + if (node.computed) { + return extractComputedName(node.key); + } + + if (node.key.type === AST_NODE_TYPES.Literal) { + return extractComputedName(node.key); + } + + return extractNonComputedName(node.key); +} +/** + * Extracts the string property name for a member. + * @returns `null` if the name cannot be extracted due to it being a computed. + */ +export function extractNameForMemberExpression( + node: TSESTree.MemberExpression, +): Key | null { + if (node.computed) { + return extractComputedName(node.property); + } + + return extractNonComputedName(node.property); +} diff --git a/packages/eslint-plugin/src/util/class-scope-analyzer/types.ts b/packages/eslint-plugin/src/util/class-scope-analyzer/types.ts new file mode 100644 index 000000000000..69db0fa37cfb --- /dev/null +++ b/packages/eslint-plugin/src/util/class-scope-analyzer/types.ts @@ -0,0 +1,23 @@ +import type { TSESTree } from '@typescript-eslint/utils'; + +export type ClassNode = TSESTree.ClassDeclaration | TSESTree.ClassExpression; +export type FunctionNode = + | TSESTree.FunctionDeclaration + | TSESTree.FunctionExpression; +export type MemberNode = + | TSESTree.AccessorProperty + | TSESTree.MethodDefinition + | TSESTree.PropertyDefinition + | TSESTree.TSAbstractAccessorProperty + | TSESTree.TSAbstractMethodDefinition + | TSESTree.TSAbstractPropertyDefinition; +export type PrivateKey = string & { __brand: 'private-key' }; +export type PublicKey = string & { __brand: 'public-key' }; +export type Key = PrivateKey | PublicKey; + +export function publicKey(node: string): PublicKey { + return node as PublicKey; +} +export function privateKey(node: TSESTree.PrivateIdentifier): PrivateKey { + return `#private@@${node.name}` as PrivateKey; +} From 8710810d01f6e14a6172c78666f0d9e616c93827 Mon Sep 17 00:00:00 2001 From: Brad Zacher Date: Sun, 6 Jul 2025 16:34:01 +0930 Subject: [PATCH 4/6] WIP --- .../classScopeAnalyzer.ts | 269 ++++++++++++------ .../src/util/class-scope-analyzer/types.ts | 3 - 2 files changed, 188 insertions(+), 84 deletions(-) diff --git a/packages/eslint-plugin/src/util/class-scope-analyzer/classScopeAnalyzer.ts b/packages/eslint-plugin/src/util/class-scope-analyzer/classScopeAnalyzer.ts index d538563a83c2..41d101d90935 100644 --- a/packages/eslint-plugin/src/util/class-scope-analyzer/classScopeAnalyzer.ts +++ b/packages/eslint-plugin/src/util/class-scope-analyzer/classScopeAnalyzer.ts @@ -1,10 +1,11 @@ +/* eslint-disable @typescript-eslint/no-this-alias -- we do a bunch of "start iterating from here" code which isn't actually aliasing `this` */ import type { ScopeManager } from '@typescript-eslint/scope-manager'; import type { TSESTree } from '@typescript-eslint/utils'; import { Visitor } from '@typescript-eslint/scope-manager'; import { AST_NODE_TYPES } from '@typescript-eslint/utils'; -import type { ClassNode, FunctionNode, Key, MemberNode } from './types'; +import type { ClassNode, Key, MemberNode } from './types'; import { extractNameForMember, @@ -40,39 +41,28 @@ export class Member { return new Member(node, name); } - /** - * True if the variable is considered an accessor - */ - public isAccessor(): boolean { - return ( - this.node.type === AST_NODE_TYPES.AccessorProperty || - ('kind' in this.node && - (this.node.kind === 'get' || this.node.kind === 'set')) - ); - } - - /** - * True if the variable has the `private` accessibility modifier. - */ public isPrivate(): boolean { return this.node.accessibility === 'private'; } - /** - * True if the variable has the `protected` accessibility modifier. - */ - public isProtected(): boolean { - return this.node.accessibility === 'protected'; - } - /** - * True if the variable has the `public` accessibility modifier. - */ - public isPublic(): boolean { - return this.node.accessibility === 'public'; + public isStatic(): boolean { + return this.node.static; } } +type IntermediateNode = + | TSESTree.FunctionDeclaration + | TSESTree.FunctionExpression + | TSESTree.Program + | TSESTree.StaticBlock; + abstract class ThisScope extends Visitor { + /** + * True if the context is considered a static context and so `this` refers to + * the class and not an instance (eg a static method or a static block). + */ + protected readonly isStaticThisContext: boolean; + /** * The classes directly declared within this class -- for example a class declared within a method. * This does not include grandchild classes. @@ -98,13 +88,18 @@ abstract class ThisScope extends Visitor { scopeManager: ScopeManager, upper: ThisScope | null, thisContext: 'none' | 'self' | ClassScope, + isStaticThisContext: boolean, ) { super({}); this.scopeManager = scopeManager; this.upper = upper; + this.isStaticThisContext = isStaticThisContext; if (thisContext === 'self') { - this.thisContext = this as unknown as ClassScope; + if (!(this instanceof ClassScope)) { + throw new Error('Cannot use `self` unless it is in a ClassScope'); + } + this.thisContext = this; } else if (thisContext === 'none') { this.thisContext = null; } else { @@ -112,9 +107,10 @@ abstract class ThisScope extends Visitor { } } - private getObjectClass( - node: TSESTree.MemberExpression, - ): { thisContext: ThisScope; type: 'instance' | 'static' } | null { + private getObjectClass(node: TSESTree.MemberExpression): { + thisContext: ThisScope['thisContext']; + type: 'instance' | 'static'; + } | null { switch (node.object.type) { case AST_NODE_TYPES.ThisExpression: { if (this.thisContext == null) { @@ -128,10 +124,21 @@ abstract class ThisScope extends Visitor { if (thisContext != null) { return { thisContext, type: 'static' }; } + + // TODO -- use scope analysis to do some basic type resolution to handle cases like: + // ``` + // class Foo { + // private prop: number; + // method(thing: Foo) { + // // this references the private instance member but not via `this` so we can't see it + // thing.prop = 1; + // } + // } + return null; } case AST_NODE_TYPES.MemberExpression: - // TODO - we could probably recurse here to do some more complex analysis and support like + // TODO - we could probably recurse here to do some more complex analysis and support like `foo.bar.baz` nested references return null; default: @@ -145,17 +152,20 @@ abstract class ThisScope extends Visitor { classScope.visit(node); } - private visitFunction(node: FunctionNode): void { - const functionScope = new FunctionScope(this.scopeManager, this, node); - this.childScopes.push(functionScope); - functionScope.visit(node); + private visitIntermediate(node: IntermediateNode): void { + const intermediateScope = new IntermediateScope( + this.scopeManager, + this, + node, + ); + this.childScopes.push(intermediateScope); + intermediateScope.visit(node); } /** * Gets the nearest class scope with the given name. */ public findClassScopeWithName(name: string): ClassScope | null { - // eslint-disable-next-line @typescript-eslint/no-this-alias let currentScope: ThisScope | null = this; while (currentScope != null) { if ( @@ -182,95 +192,173 @@ abstract class ThisScope extends Visitor { } protected FunctionDeclaration(node: TSESTree.FunctionDeclaration): void { - this.visitFunction(node); + this.visitIntermediate(node); } protected FunctionExpression(node: TSESTree.FunctionExpression): void { - if ( - (node.parent.type === AST_NODE_TYPES.MethodDefinition || - node.parent.type === AST_NODE_TYPES.PropertyDefinition) && - node.parent.value === node - ) { - // if the function is a method's implementation then the `this` is guaranteed to be the class - this.visit(node); - } else { - // TODO(bradzacher): - // - we could handle manual bound functions like `(function () { ... }).bind(this)` - // - we could handle auto-bound methods like `[].map(function () {}, this)` - this.visitFunction(node); - } + this.visitIntermediate(node); } protected MemberExpression(node: TSESTree.MemberExpression): void { + if (node.property.type === AST_NODE_TYPES.PrivateIdentifier) { + // will be handled by the PrivateIdentifier visitor + return; + } + const propertyName = extractNameForMemberExpression(node); if (propertyName == null) { return; } const objectClassName = this.getObjectClass(node); - } + if (objectClassName == null) { + return; + } - protected PrivateIdentifier(node: TSESTree.PrivateIdentifier): void { - const member = this.thisContext?.members.get(privateKey(node)); + if (objectClassName.thisContext == null) { + return; + } + + const members = + objectClassName.type === 'instance' + ? objectClassName.thisContext.members.instance + : objectClassName.thisContext.members.static; + const member = members.get(propertyName); if (member == null) { return; } member.referenceCount += 1; } + + protected PrivateIdentifier(node: TSESTree.PrivateIdentifier): void { + // We can actually be pretty loose with our code here thanks to how private + // members are designed. + // + // 1) classes CANNOT have a static and instance private member with the + // same name, so we don't need to match up static access. + // 2) nested classes CANNOT access a private member of their parent class if + // the member has the same name as a private member of the nested class. + // + // together this means that we can just look for the member upwards until we + // find a match and we know that will be the correct match! + + let currentScope: ThisScope | null = this; + const key = privateKey(node); + while (currentScope != null) { + if (currentScope.thisContext != null) { + const member = + currentScope.thisContext.members.instance.get(key) ?? + currentScope.thisContext.members.static.get(key); + if (member != null) { + member.referenceCount += 1; + return; + } + } + + currentScope = currentScope.upper; + } + } + + protected StaticBlock(node: TSESTree.StaticBlock): void { + this.visitIntermediate(node); + } } /** + * Any other scope that is not a class scope + * * When we visit a function declaration/expression the `this` reference is * rebound so it no longer refers to the class. * * This also supports a function's `this` parameter. */ -export class FunctionScope extends ThisScope { +class IntermediateScope extends ThisScope { constructor( scopeManager: ScopeManager, - upper: ThisScope, - node: FunctionNode, + upper: ThisScope | null, + node: IntermediateNode, ) { - if ( - node.params[0].type !== AST_NODE_TYPES.Identifier || - node.params[0].name !== 'this' - ) { - super(scopeManager, upper, 'none'); + if (node.type === AST_NODE_TYPES.Program) { + super(scopeManager, upper, 'none', false); + return; + } + + if (node.type === AST_NODE_TYPES.StaticBlock) { + if (upper == null || !(upper instanceof ClassScope)) { + throw new Error( + 'Cannot have a static block without an upper ClassScope', + ); + } + super(scopeManager, upper, upper, true); return; } - const thisType = node.params[0].typeAnnotation?.typeAnnotation; + // method definition if ( - thisType == null || - thisType.type !== AST_NODE_TYPES.TSTypeReference || - thisType.typeName.type !== AST_NODE_TYPES.Identifier + (node.parent.type === AST_NODE_TYPES.MethodDefinition || + node.parent.type === AST_NODE_TYPES.PropertyDefinition) && + node.parent.value === node ) { - super(scopeManager, upper, 'none'); + if (upper == null || !(upper instanceof ClassScope)) { + throw new Error( + 'Cannot have a class method/property without an upper ClassScope', + ); + } + super(scopeManager, upper, upper, node.parent.static); return; } - const thisContext = upper.findClassScopeWithName(thisType.typeName.name); - if (thisContext == null) { - super(scopeManager, upper, 'none'); - return; + // function with a `this` parameter + if ( + upper != null && + node.params[0].type === AST_NODE_TYPES.Identifier && + node.params[0].name === 'this' + ) { + const thisType = node.params[0].typeAnnotation?.typeAnnotation; + if ( + thisType?.type === AST_NODE_TYPES.TSTypeReference && + thisType.typeName.type === AST_NODE_TYPES.Identifier + ) { + const thisContext = upper.findClassScopeWithName( + thisType.typeName.name, + ); + if (thisContext != null) { + super(scopeManager, upper, thisContext, false); + return; + } + } } - super(scopeManager, upper, thisContext); + super(scopeManager, upper, 'none', false); } } -export class ClassScope extends ThisScope { +export interface ClassScopeResult { /** * The classes name as given in the source code. * If this is `null` then the class is an anonymous class. */ + readonly className: string | null; + /** + * The class's members, keyed by their name + */ + readonly members: { + readonly instance: Map; + readonly static: Map; + }; +} + +class ClassScope extends ThisScope implements ClassScopeResult { public readonly className: string | null; /** * The class's members, keyed by their name */ - public readonly members = new Map(); + public readonly members: ClassScopeResult['members'] = { + instance: new Map(), + static: new Map(), + }; /** * The node that declares this class. @@ -279,10 +367,10 @@ export class ClassScope extends ThisScope { public constructor( theClass: ClassNode, - upper: ClassScope | FunctionScope | null, + upper: ClassScope | IntermediateScope | null, scopeManager: ScopeManager, ) { - super(scopeManager, upper, 'self'); + super(scopeManager, upper, 'self', false); this.theClass = theClass; this.className = theClass.id?.name ?? null; @@ -299,7 +387,11 @@ export class ClassScope extends ThisScope { if (member == null) { continue; } - this.members.set(member.name, member); + if (member.isStatic()) { + this.members.static.set(member.name, member); + } else { + this.members.instance.set(member.name, member); + } break; } @@ -316,10 +408,25 @@ export class ClassScope extends ThisScope { } export function analyzeClassMemberUsage( - theClass: ClassNode, + program: TSESTree.Program, scopeManager: ScopeManager, -): ClassScope { - const analyzer = new ClassScope(theClass, null, scopeManager); - analyzer.visit(theClass); - return analyzer; +): ReadonlyMap { + const rootScope = new IntermediateScope(scopeManager, null, program); + rootScope.visit(program); + return traverseScopes(rootScope); +} + +function traverseScopes( + currentScope: ThisScope, + analysisResults = new Map(), +) { + if (currentScope instanceof ClassScope) { + analysisResults.set(currentScope.theClass, currentScope); + } + + for (const childScope of currentScope.childScopes) { + traverseScopes(childScope, analysisResults); + } + + return analysisResults; } diff --git a/packages/eslint-plugin/src/util/class-scope-analyzer/types.ts b/packages/eslint-plugin/src/util/class-scope-analyzer/types.ts index 69db0fa37cfb..481aff1a4a20 100644 --- a/packages/eslint-plugin/src/util/class-scope-analyzer/types.ts +++ b/packages/eslint-plugin/src/util/class-scope-analyzer/types.ts @@ -1,9 +1,6 @@ import type { TSESTree } from '@typescript-eslint/utils'; export type ClassNode = TSESTree.ClassDeclaration | TSESTree.ClassExpression; -export type FunctionNode = - | TSESTree.FunctionDeclaration - | TSESTree.FunctionExpression; export type MemberNode = | TSESTree.AccessorProperty | TSESTree.MethodDefinition From fe8c3006db55ce40a80d6fd009d398e329dd8175 Mon Sep 17 00:00:00 2001 From: Brad Zacher Date: Sun, 6 Jul 2025 20:09:19 +0930 Subject: [PATCH 5/6] WIP --- .../rules/no-unused-private-class-members.ts | 263 ++---------------- .../classScopeAnalyzer.ts | 44 ++- .../extractComputedName.ts | 21 +- .../no-unused-private-class-members.test.ts | 2 +- 4 files changed, 76 insertions(+), 254 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-unused-private-class-members.ts b/packages/eslint-plugin/src/rules/no-unused-private-class-members.ts index daed826ebddf..ba3e94cb34b2 100644 --- a/packages/eslint-plugin/src/rules/no-unused-private-class-members.ts +++ b/packages/eslint-plugin/src/rules/no-unused-private-class-members.ts @@ -1,12 +1,7 @@ -// 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 { ESLintUtils } from '@typescript-eslint/utils'; import { createRule } from '../util'; +import { analyzeClassMemberUsage } from '../util/class-scope-analyzer/classScopeAnalyzer'; type Options = []; export type MessageIds = 'unusedPrivateClassMember'; @@ -23,244 +18,40 @@ export default createRule({ messages: { unusedPrivateClassMember: - "'{{classMemberName}}' is defined but never used.", + "Private class member '{{classMemberName}}' is defined but never used.", }, schema: [], }, defaultOptions: [], create(context) { - interface PrivateMember { - declaredNode: - | TSESTree.AccessorProperty - | TSESTree.MethodDefinitionComputedName - | TSESTree.MethodDefinitionNonComputedName - | TSESTree.PropertyDefinitionComputedName - | TSESTree.PropertyDefinitionNonComputedName; - isAccessor: boolean; - } - const trackedClassMembers: Map[] = []; - - /** - * 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.AccessorProperty - | 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); - } - return { - // Collect all declared members/methods up front and assume they are all - // unused. - ClassBody(classBodyNode): void { - const privateMembers = new Map(); - - trackedClassMembers.unshift(privateMembers); - for (const bodyMember of classBodyNode.body) { - if ( - (bodyMember.type === AST_NODE_TYPES.PropertyDefinition || - bodyMember.type === AST_NODE_TYPES.AccessorProperty || - bodyMember.type === AST_NODE_TYPES.MethodDefinition) && - (bodyMember.key.type === AST_NODE_TYPES.PrivateIdentifier || - (bodyMember.key.type === AST_NODE_TYPES.Identifier && - bodyMember.accessibility === 'private')) - ) { - 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); - }, - - // 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; - } + 'Program:exit'(node) { + const result = analyzeClassMemberUsage( + node, + ESLintUtils.nullThrows( + context.sourceCode.scopeManager, + 'Missing required scope manager', + ), + ); - for (const [ - classMemberName, - { declaredNode }, - ] of unusedPrivateMembers.entries()) { - if (trackedClassMembersUsed.has(declaredNode)) { - continue; + for (const classScope of result.values()) { + for (const member of [ + ...classScope.members.instance.values(), + ...classScope.members.static.values(), + ]) { + if (!member.isPrivate() && !member.isHashPrivate()) { + continue; + } + + if (member.referenceCount === 0) { + context.report({ + node: member.node.key, + messageId: 'unusedPrivateClassMember', + data: { classMemberName: member.name }, + }); + } } - context.report({ - loc: declaredNode.key.loc, - node: declaredNode, - messageId: 'unusedPrivateClassMember', - data: { - classMemberName: - declaredNode.key.type === AST_NODE_TYPES.PrivateIdentifier - ? `#${classMemberName}` - : classMemberName, - }, - }); } }, }; diff --git a/packages/eslint-plugin/src/util/class-scope-analyzer/classScopeAnalyzer.ts b/packages/eslint-plugin/src/util/class-scope-analyzer/classScopeAnalyzer.ts index 41d101d90935..1204855f3900 100644 --- a/packages/eslint-plugin/src/util/class-scope-analyzer/classScopeAnalyzer.ts +++ b/packages/eslint-plugin/src/util/class-scope-analyzer/classScopeAnalyzer.ts @@ -19,18 +19,24 @@ export class Member { */ public readonly node: MemberNode; + /** + * The resolved, unique key for this member. + */ + public readonly key: Key; + /** * The member name, as given in the source code. */ - public readonly name: Key; + public readonly name: string; /** * The number of references to this member. */ public referenceCount = 0; - private constructor(node: MemberNode, name: Key) { + private constructor(node: MemberNode, key: Key, name: string) { this.node = node; + this.key = key; this.name = name; } public static create(node: MemberNode): Member | null { @@ -38,7 +44,11 @@ export class Member { if (name == null) { return null; } - return new Member(node, name); + return new Member(node, ...name); + } + + public isHashPrivate(): boolean { + return this.node.key.type === AST_NODE_TYPES.PrivateIdentifier; } public isPrivate(): boolean { @@ -116,7 +126,10 @@ abstract class ThisScope extends Visitor { if (this.thisContext == null) { return null; } - return { thisContext: this.thisContext, type: 'instance' }; + return { + thisContext: this.thisContext, + type: this.isStaticThisContext ? 'static' : 'instance', + }; } case AST_NODE_TYPES.Identifier: { @@ -149,7 +162,7 @@ abstract class ThisScope extends Visitor { private visitClass(node: ClassNode): void { const classScope = new ClassScope(node, this, this.scopeManager); this.childScopes.push(classScope); - classScope.visit(node); + classScope.visitChildren(node); } private visitIntermediate(node: IntermediateNode): void { @@ -159,7 +172,7 @@ abstract class ThisScope extends Visitor { node, ); this.childScopes.push(intermediateScope); - intermediateScope.visit(node); + intermediateScope.visitChildren(node); } /** @@ -200,6 +213,8 @@ abstract class ThisScope extends Visitor { } protected MemberExpression(node: TSESTree.MemberExpression): void { + this.visitChildren(node); + if (node.property.type === AST_NODE_TYPES.PrivateIdentifier) { // will be handled by the PrivateIdentifier visitor return; @@ -223,7 +238,7 @@ abstract class ThisScope extends Visitor { objectClassName.type === 'instance' ? objectClassName.thisContext.members.instance : objectClassName.thisContext.members.static; - const member = members.get(propertyName); + const member = members.get(propertyName[0]); if (member == null) { return; } @@ -232,6 +247,17 @@ abstract class ThisScope extends Visitor { } protected PrivateIdentifier(node: TSESTree.PrivateIdentifier): void { + this.visitChildren(node); + + if ( + (node.parent.type === AST_NODE_TYPES.MethodDefinition || + node.parent.type === AST_NODE_TYPES.PropertyDefinition) && + node.parent.key === node + ) { + // ignore the member definition + return; + } + // We can actually be pretty loose with our code here thanks to how private // members are designed. // @@ -388,9 +414,9 @@ class ClassScope extends ThisScope implements ClassScopeResult { continue; } if (member.isStatic()) { - this.members.static.set(member.name, member); + this.members.static.set(member.key, member); } else { - this.members.instance.set(member.name, member); + this.members.instance.set(member.key, member); } break; } diff --git a/packages/eslint-plugin/src/util/class-scope-analyzer/extractComputedName.ts b/packages/eslint-plugin/src/util/class-scope-analyzer/extractComputedName.ts index 020d3bf1fa6e..3b3aca0acdaa 100644 --- a/packages/eslint-plugin/src/util/class-scope-analyzer/extractComputedName.ts +++ b/packages/eslint-plugin/src/util/class-scope-analyzer/extractComputedName.ts @@ -6,31 +6,36 @@ import type { Key, MemberNode } from './types'; import { privateKey, publicKey } from './types'; -function extractComputedName(computedName: TSESTree.Expression): Key | null { +function extractComputedName( + computedName: TSESTree.Expression, +): [Key, string] | null { if (computedName.type === AST_NODE_TYPES.Literal) { - return publicKey(computedName.value?.toString() ?? 'null'); + const name = computedName.value?.toString() ?? 'null'; + return [publicKey(name), name]; } if ( computedName.type === AST_NODE_TYPES.TemplateLiteral && computedName.expressions.length === 0 ) { - return publicKey(computedName.quasis[0].value.raw); + const name = computedName.quasis[0].value.raw; + return [publicKey(name), name]; } return null; } function extractNonComputedName( nonComputedName: TSESTree.Identifier | TSESTree.PrivateIdentifier, -): Key { +): [Key, string] | null { + const name = nonComputedName.name; if (nonComputedName.type === AST_NODE_TYPES.PrivateIdentifier) { - return privateKey(nonComputedName); + return [privateKey(nonComputedName), `#${name}`]; } - return publicKey(nonComputedName.name); + return [publicKey(name), name]; } /** * Extracts the string name for a member. * @returns `null` if the name cannot be extracted due to it being a computed. */ -export function extractNameForMember(node: MemberNode): Key | null { +export function extractNameForMember(node: MemberNode): [Key, string] | null { if (node.computed) { return extractComputedName(node.key); } @@ -47,7 +52,7 @@ export function extractNameForMember(node: MemberNode): Key | null { */ export function extractNameForMemberExpression( node: TSESTree.MemberExpression, -): Key | null { +): [Key, string] | null { if (node.computed) { return extractComputedName(node.property); } diff --git a/packages/eslint-plugin/tests/rules/no-unused-private-class-members.test.ts b/packages/eslint-plugin/tests/rules/no-unused-private-class-members.test.ts index b25b947cc7c1..2fa6d9ee9111 100644 --- a/packages/eslint-plugin/tests/rules/no-unused-private-class-members.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unused-private-class-members.test.ts @@ -254,7 +254,7 @@ class C { #unusedInOuterClass; foo() { - return class { + return class D { #unusedInOuterClass; bar() { From e72b89a34357ff79e502daa12b3eb54d2fae7a3a Mon Sep 17 00:00:00 2001 From: Brad Zacher Date: Sun, 10 Aug 2025 14:56:49 +0930 Subject: [PATCH 6/6] got it all working --- .../rules/no-unused-private-class-members.ts | 17 +-- .../classScopeAnalyzer.ts | 115 +++++++++++++++++- .../no-unused-private-class-members.test.ts | 2 - 3 files changed, 120 insertions(+), 14 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-unused-private-class-members.ts b/packages/eslint-plugin/src/rules/no-unused-private-class-members.ts index ba3e94cb34b2..d5bb6e9f2379 100644 --- a/packages/eslint-plugin/src/rules/no-unused-private-class-members.ts +++ b/packages/eslint-plugin/src/rules/no-unused-private-class-members.ts @@ -40,17 +40,18 @@ export default createRule({ ...classScope.members.instance.values(), ...classScope.members.static.values(), ]) { - if (!member.isPrivate() && !member.isHashPrivate()) { + if ( + (!member.isPrivate() && !member.isHashPrivate()) || + member.isUsed() + ) { continue; } - if (member.referenceCount === 0) { - context.report({ - node: member.node.key, - messageId: 'unusedPrivateClassMember', - data: { classMemberName: member.name }, - }); - } + context.report({ + node: member.node.key, + messageId: 'unusedPrivateClassMember', + data: { classMemberName: member.name }, + }); } } }, diff --git a/packages/eslint-plugin/src/util/class-scope-analyzer/classScopeAnalyzer.ts b/packages/eslint-plugin/src/util/class-scope-analyzer/classScopeAnalyzer.ts index 1204855f3900..752b64709396 100644 --- a/packages/eslint-plugin/src/util/class-scope-analyzer/classScopeAnalyzer.ts +++ b/packages/eslint-plugin/src/util/class-scope-analyzer/classScopeAnalyzer.ts @@ -6,6 +6,7 @@ import { Visitor } from '@typescript-eslint/scope-manager'; import { AST_NODE_TYPES } from '@typescript-eslint/utils'; import type { ClassNode, Key, MemberNode } from './types'; +import { nullThrows, NullThrowsReasons } from '..'; import { extractNameForMember, @@ -30,9 +31,14 @@ export class Member { public readonly name: string; /** - * The number of references to this member. + * The number of writes to this member. */ - public referenceCount = 0; + public writeCount = 0; + + /** + * The number of reads from this member. + */ + public readCount = 0; private constructor(node: MemberNode, key: Key, name: string) { this.node = node; @@ -58,6 +64,107 @@ export class Member { public isStatic(): boolean { return this.node.static; } + + public isAccessor(): boolean { + if ( + this.node.type === AST_NODE_TYPES.MethodDefinition || + this.node.type === AST_NODE_TYPES.TSAbstractMethodDefinition + ) { + return this.node.kind === 'set' || this.node.kind === 'get'; + } + + return ( + this.node.type === AST_NODE_TYPES.AccessorProperty || + this.node.type === AST_NODE_TYPES.TSAbstractAccessorProperty + ); + } + + public isUsed(): boolean { + return ( + this.readCount > 0 || + // any usage of an accessor is considered a usage as accessor can have side effects + (this.writeCount > 0 && this.isAccessor()) + ); + } +} + +function isWriteOnlyUsage(node: TSESTree.Node, parent: TSESTree.Node): boolean { + if ( + parent.type !== AST_NODE_TYPES.AssignmentExpression && + parent.type !== AST_NODE_TYPES.ForInStatement && + parent.type !== AST_NODE_TYPES.ForOfStatement && + parent.type !== AST_NODE_TYPES.AssignmentPattern + ) { + return false; + } + + // If it's on the right then it's a read not a write + if (parent.left !== node) { + return false; + } + + if ( + parent.type === AST_NODE_TYPES.AssignmentExpression && + // For any other operator (such as '+=') we still consider it a read operation + parent.operator !== '=' + ) { + // if the read operation is "discarded" in an empty statement, then it is write only. + return parent.parent.type === AST_NODE_TYPES.ExpressionStatement; + } + + return true; +} + +function countReference(identifierParent: TSESTree.Node, member: Member) { + const identifierGrandparent = nullThrows( + identifierParent.parent, + NullThrowsReasons.MissingParent, + ); + + if (isWriteOnlyUsage(identifierParent, identifierGrandparent)) { + member.writeCount += 1; + return; + } + + const identifierGreatGrandparent = identifierGrandparent.parent; + + // A statement which only increments (`this.#x++;`) + if ( + identifierGrandparent.type === AST_NODE_TYPES.UpdateExpression && + identifierGreatGrandparent?.type === AST_NODE_TYPES.ExpressionStatement + ) { + member.writeCount += 1; + return; + } + + /* + * ({ x: this.#usedInDestructuring } = bar); + * + * But should treat the following as a read: + * ({ [this.#x]: a } = foo); + */ + if ( + identifierGrandparent.type === AST_NODE_TYPES.Property && + identifierGreatGrandparent?.type === AST_NODE_TYPES.ObjectPattern && + identifierGrandparent.value === identifierParent + ) { + member.writeCount += 1; + return; + } + + // [...this.#unusedInRestPattern] = bar; + if (identifierGrandparent.type === 'RestElement') { + member.writeCount += 1; + return; + } + + // [this.#unusedInAssignmentPattern] = bar; + if (identifierGrandparent.type === 'ArrayPattern') { + member.writeCount += 1; + return; + } + + member.readCount += 1; } type IntermediateNode = @@ -243,7 +350,7 @@ abstract class ThisScope extends Visitor { return; } - member.referenceCount += 1; + countReference(node, member); } protected PrivateIdentifier(node: TSESTree.PrivateIdentifier): void { @@ -277,7 +384,7 @@ abstract class ThisScope extends Visitor { currentScope.thisContext.members.instance.get(key) ?? currentScope.thisContext.members.static.get(key); if (member != null) { - member.referenceCount += 1; + countReference(node.parent, member); return; } } diff --git a/packages/eslint-plugin/tests/rules/no-unused-private-class-members.test.ts b/packages/eslint-plugin/tests/rules/no-unused-private-class-members.test.ts index 2fa6d9ee9111..73eef14eb20f 100644 --- a/packages/eslint-plugin/tests/rules/no-unused-private-class-members.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unused-private-class-members.test.ts @@ -82,8 +82,6 @@ class C { } } `, - // TODO make this work - skip: true, }, ...[