typescript-eslint/typescript-eslintPublic

NotificationsYou must be signed in to change notification settings
Fork2.8k
Star16k

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

Jump to bottom

Open

bradzacher wants to merge7 commits intomain

base:main

Choose a base branch

fromno-unused-private-class-members

Open

feat(eslint-plugin): [no-unused-private-class-members] new extension rule#10913

bradzacher wants to merge7 commits intomainfromno-unused-private-class-members

Conversation

Copy link

Member

bradzacher commentedMar 3, 2025

PR Checklist

Addresses an existing open issue: fixesRule proposal: no-unused-private-class-members #4571
That issue was marked asaccepting prs
Steps inContributing were taken

Overview

This PR implements an extension rule forno-unused-private-class-members that introduces support forprivate members.

feat(eslint-plugin): [no-unused-private-class-members] new extension …

34b77b8

…rule

bradzacher added the enhancement: new base rule extensionNew base rule extension required to handle a TS specific case label

Mar 3, 2025

Copy link

Contributor

typescript-eslintbot commentedMar 3, 2025

Thanks for the PR,@bradzacher!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

Copy link

netlifybot commentedMar 3, 2025•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

Name	Link
🔨 Latest commit	`a43dc70`
🔍 Latest deploy log	https://app.netlify.com/projects/typescript-eslint/deploys/689830a0d740ba0008843579
😎 Deploy Preview	https://deploy-preview-10913--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 93 (🟢 up 2 from production) Accessibility: 97 (no change from production) Best Practices: 100 (no change from production) SEO: 92 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to yourNetlify project configuration.

Copy link

nx-cloudbot commentedMar 3, 2025•
edited
Loading

🤖 Nx Cloud AI Fix Eligible

An automatically generated fix could have helped fix failing tasks for this run, but Self-healing CI is disabled for this workspace. Visit workspace settings to enable it and get automatic fixes in future runs.

_{To disable these notifications, a workspace admin can disable themin workspace settings.}

View yourCI Pipeline Execution ↗ for commita43dc70

Command	Status	Duration	Result
`nx test eslint-plugin --coverage=false`	❌ Failed	5m 23s	View ↗
`nx run-many -t lint`	❌ Failed	3m 25s	View ↗
`nx test eslint-plugin`	❌ Failed	4m 59s	View ↗
`nx run-many -t typecheck`	✅ Succeeded	2m 16s	View ↗
`nx test typescript-estree --coverage=false`	✅ Succeeded	2s	View ↗
`nx run types:build`	✅ Succeeded	5s	View ↗
`nx test eslint-plugin-internal --coverage=false`	✅ Succeeded	3s	View ↗
`nx run integration-tests:test`	✅ Succeeded	3s	View ↗
`Additional runs (26)`	✅ Succeeded	...	View ↗

☁️Nx Cloud last updated this comment at2025-08-10 05:47:58 UTC

Copy link

nx-cloudbot commentedMar 3, 2025

View yourCI Pipeline Execution ↗ for commit34b77b8.

☁️Nx Cloud last updated this comment at2025-03-03 02:18:54 UTC

JoshuaKGoldberg previously approved these changes

Mar 3, 2025

View reviewed changes

Copy link

Member

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

JoshuaKGoldberg added the 1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label

Mar 3, 2025

ronami reviewed

Mar 6, 2025

View reviewed changes

packages/eslint-plugin/src/rules/no-unused-private-class-members.ts OutdatedShow resolvedHide resolved

packages/eslint-plugin/src/rules/no-unused-private-class-members.ts Outdated

		// 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);

Copy link

Member

ronamiMar 6, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

[Bug]: I've noticed some missed reports with functions that have a differentthis than the class instance (deploy preview playground link):

classTest1{// should be reported but doesn't?privatebar:number;foo=function(){returnthis.bar;}}

Copy link

MemberAuthor

bradzacherMar 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This is because the base rule doesn't support that case becausethis.#bar is a syntax error inside a function expression!!

It's worth noting that TS doesn't actually catch this case either and reportsbar as unused because the type of thethis in the function expression isany!
If you turn onnoImplicitThis then TS will error on thatthis

We could support this if we wanted to. I'm leaning towards not because TS itself doesn't catch it either.

Copy link

Member

ronamiMar 9, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yes! I think the rule should reportbar as unused in this case. I've mentioned this since TypeScript reports it as unused but the rule doesn't report it as unused (regardless of the type error).

packages/eslint-plugin/src/rules/no-unused-private-class-members.ts Outdated

Comment on lines 219 to 221

		PrivateIdentifier(node): void {
		processPrivateIdentifier(node);
		},

Copy link

Member

ronamiMar 6, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

[Bug]: I think there's an inconsistency that creates false positives withstaticprivate properties (deploy preview playground link):

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;}

Copy link

MemberAuthor

bradzacherMar 7, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

For your second example -- this is expected and is the same purposely not covered case as here:
https://github.com/typescript-eslint/typescript-eslint/pull/10913/files#diff-914330b2e3f9d71dfb628800b850412c3c5560900b13cf8a2925ff7af0905e53R50-R66

I made this rule purposely NOT use type info because the cases that type info enable are mostly going to be rarer edge cases.

Copy link

MemberAuthor

bradzacherMar 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This first example of static private member access we could handle though with scope analysis.

ronami reviewed

Mar 6, 2025

View reviewed changes

Copy link

Member

ronami left a comment•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Looks amazing! Excited to enable this on projects I work on 🚀🚀🚀

(I accidentally posted the review earlier while still adding comments 🙈)

packages/eslint-plugin/docs/rules/no-unused-private-class-members.mdx

		</TabItem>
		</Tabs>

		## Limitations

Copy link

Member

ronamiMar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

What do you think about also mentioning assigningthis to a variable as a limitaiton?

classTest1{privatebar:number;foo(){constself=this;returnfunction(){// ❌ NOT detected as a usageself.bar;}}}

support accessor properties

0854007

Copy link

MemberAuthor

bradzacher commentedMar 10, 2025

FYI I'm going to make@Josh-Cena very happy - I'm going to build a "class scope analyser" so we can do this right rather than just hacking it in.

This does mean that I'm going to rewrite the rule from the ground up -- but it'll be worht it

Copy link

Member

kirkwaiblinger commentedMar 19, 2025•
edited
Loading

FYI I'm going to make@Josh-Cena very happy - I'm going to build a "class scope analyser" so we can do this right rather than just hacking it in.
This does mean that I'm going to rewrite the rule from the ground up -- but it'll be worht it

Do you intend for this to proceed as-is, with this work intended to happen in the future, or should we pause/draft it pending this work?

Copy link

MemberAuthor

bradzacher commentedMar 19, 2025

Let's draft it.
We need to at least handle the static property case.
Might have some time soon to pick the work back up.

kirkwaiblinger added the awaiting responseIssues waiting for a reply from the OP or another party label

Mar 19, 2025

kirkwaiblinger marked this pull request as draft

March 19, 2025 09:39

JoshuaKGoldberg removed the 1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label

Mar 24, 2025

WIP class scope analyser

df85b86

bradzacher dismissedJoshuaKGoldberg’sstale review viadf85b86

July 6, 2025 02:52

bradzacher added2 commits

July 6, 2025 16:34

WIP

8710810

WIP

fe8c300

bradzacher removed the awaiting responseIssues waiting for a reply from the OP or another party label

Jul 13, 2025

got it all working

e72b89a

bradzacher marked this pull request as ready for review

August 10, 2025 05:39

Merge branch 'main' into no-unused-private-class-members

a43dc70

bradzacher requested review fromJoshuaKGoldberg andronami

August 10, 2025 05:39

ronami reviewed

Aug 23, 2025

View reviewed changes

Copy link

Member

ronami left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Looks amazing and a very beneficial rule to have; just a few small comments from me 🚀🚀🚀

packages/eslint-plugin/src/rules/no-unused-private-class-members.ts

		meta: {
		type: 'problem',
		docs: {
		description: 'Disallow unused private class members',

Copy link

Member

ronamiAug 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

What do you think about covering the following use-case? (deploy preview link)

classTest1{// should be reported but doesn't?constructor(privatefoo:number){}}

packages/eslint-plugin/src/util/class-scope-analyzer/classScopeAnalyzer.ts

Comment on lines +446 to +464

		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;
		}
		}
		}

Copy link

Member

ronamiAug 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This doesn't seem to have a test, and it throws for a function declaration/expression with no arguments (deploy preview link):

functionfoo(){}

packages/eslint-plugin/src/util/class-scope-analyzer/classScopeAnalyzer.ts

Comment on lines +248 to +256

		// 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;
		// }
		// }

Copy link

Member

ronamiAug 23, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Ironically, it seems like TypeScript incorrectly reports this as unused (playground link):

classFoo{privateprop:number;method(thing:Foo){thing.prop=1;}}

Seems like a bug though, unless I'm totally missing something 😅

Edit: Interestingly, seems like the assignment was the culprit, and the following isn't reported by TypeScript (playground link):

classFoo{privateprop:number;method(thing:Foo){returnthing.prop;}}

Copy link

MemberAuthor

bradzacherAug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

also looks like my comment is wrong and I did actually add support for this case haha

ronami reviewed

Aug 24, 2025

View reviewed changes

packages/eslint-plugin/src/rules/no-unused-private-class-members.ts

		meta: {
		type: 'problem',
		docs: {
		description: 'Disallow unused private class members',

Copy link

Member

ronamiAug 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think this is a bug and shouldn't be reported (deploy preview playground link):

classTest1{// reported but shouldn't?privatefoo:number|null;publicbar(){this.foo??=1;}}

Copy link

MemberAuthor

bradzacherAug 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This is a weird case that isn't currently reported byno-unused-vars but really should be.

It is reported by the baseno-unused-private-class-members rule

Copy link

Member

kirkwaiblingerAug 27, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

There was just a recent eslint bug report about these; I guess it's intentional for no-unused-vars sincex ?? = yreadsx first (equivalent tox != null ? x : (x = y)) rather thanunconditionally assigning (x = x != null ? x : y). Seeeslint/eslint#20029. But presumably the two rules should agree?

Copy link

Member

kirkwaiblingerAug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Oh, but do note that these are reported by no-useless-assignment so 🤷‍♂️ 🤷‍♂️

(playground)

Copy link

MemberAuthor

bradzacherAug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

we should probably file a bug for NUPCM then so it matches NUV?

kirkwaiblinger mentioned this pull request

Sep 12, 2025

feat(eslint-plugin): [no-unused-private-class-members] new extension rule#11598

Closed

3 tasks

JoshuaKGoldberg added the awaiting responseIssues waiting for a reply from the OP or another party label

Sep 29, 2025

JoshuaKGoldberg requested changes

Sep 29, 2025

View reviewed changes

Copy link

Member

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I have very little to add that Kirk and Ronami didn't already. Very exciting!! ⚡

packages/eslint-plugin/src/util/class-scope-analyzer/extractComputedName.ts

		}
		/**
		* Extracts the string name for a member.
		* @returns `null` if the name cannot be extracted due to it being a computed.

Copy link

Member

JoshuaKGoldbergSep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

[Docs] Small grammar point:

Suggested change

	* @returns`null`ifthenamecannotbeextractedduetoitbeingacomputed.
	* @returns`null`ifthenamecannotbeextractedduetoitbeingcomputed.

packages/eslint-plugin/src/util/class-scope-analyzer/extractComputedName.ts

		*/
		export function extractNameForMember(node: MemberNode): [Key, string] \| null {
		if (node.computed) {
		return extractComputedName(node.key);

Copy link

Member

JoshuaKGoldbergSep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Just noting,#11006 has some other logic around computed keys. It might be useful to unify?

Labels

awaiting response

Issues waiting for a reply from the OP or another party

enhancement: new base rule extension

New base rule extension required to handle a TS specific case

4 participants

Movatterモバイル変換

Uh oh!

feat(eslint-plugin): [no-unused-private-class-members] new extension rule#10913

Are you sure you want to change the base?

feat(eslint-plugin): [no-unused-private-class-members] new extension rule#10913

Uh oh!

Conversation

bradzacher commentedMar 3, 2025

PR Checklist

Overview

Uh oh!

typescript-eslintbot commentedMar 3, 2025

Uh oh!

netlifybot commentedMar 3, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

✅ Deploy Preview fortypescript-eslint ready!

Uh oh!

nx-cloudbot commentedMar 3, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

🤖 Nx Cloud AI Fix Eligible

Uh oh!

nx-cloudbot commentedMar 3, 2025

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ronamiMar 6, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronamiMar 9, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronamiMar 6, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradzacherMar 7, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronami left a comment• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradzacher commentedMar 10, 2025

Uh oh!

kirkwaiblinger commentedMar 19, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

bradzacher commentedMar 19, 2025

Uh oh!

ronami left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronamiAug 23, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kirkwaiblingerAug 27, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

netlifybot commentedMar 3, 2025•
edited
Loading

nx-cloudbot commentedMar 3, 2025•
edited
Loading

ronamiMar 6, 2025•
edited
Loading

ronamiMar 9, 2025•
edited
Loading

ronamiMar 6, 2025•
edited
Loading

bradzacherMar 7, 2025•
edited
Loading

ronami left a comment•
edited
Loading

kirkwaiblinger commentedMar 19, 2025•
edited
Loading

ronamiAug 23, 2025•
edited
Loading

kirkwaiblingerAug 27, 2025•
edited
Loading