Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

Draft
bradzacher wants to merge1 commit intomain
base:main
Choose a base branch
Loading
fromno-unused-private-class-members

Conversation

bradzacher
Copy link
Member

PR Checklist

Overview

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

zanminkian, ronami, LvChengbin, and kddsultan reacted with thumbs up emojikirkwaiblinger and ronami reacted with heart emoji
@bradzacherbradzacher added the enhancement: new base rule extensionNew base rule extension required to handle a TS specific case labelMar 3, 2025
@typescript-eslint
Copy link
Contributor

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 onhttps://opencollective.com/typescript-eslint.

@netlifyNetlify
Copy link

netlifybot commentedMar 3, 2025•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

NameLink
🔨 Latest commit34b77b8
🔍 Latest deploy loghttps://app.netlify.com/sites/typescript-eslint/deploys/67c511602d5b8e00088058ec
😎 Deploy Previewhttps://deploy-preview-10913--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse		1 paths audited
Performance: 99 (🟢 up 1 from production)
Accessibility: 100 (no change from production)
Best Practices: 100 (🟢 up 8 from production)
SEO: 98 (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 site configuration.

@nx-cloudNx Cloud
Copy link

nx-cloudbot commentedMar 3, 2025•
edited
Loading

View yourCI Pipeline Execution ↗ for commit34b77b8.

CommandStatusDurationResult
nx run-many --target=build --exclude website --...✅ Succeeded1sView ↗
nx run-many --target=clean✅ Succeeded10sView ↗

☁️Nx Cloud last updated this comment at2025-03-24 11:52:46 UTC

@nx-cloudNx Cloud
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

Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Conan O'Brien frankly saying 'yep'

ronami reacted with thumbs up emoji
@JoshuaKGoldbergJoshuaKGoldberg added the 1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge labelMar 3, 2025
// 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

@ronamironamiMar 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

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

@ronamironamiMar 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).

Comment on lines +219 to +221
PrivateIdentifier(node): void {
processPrivateIdentifier(node);
},
Copy link
Member

@ronamironamiMar 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

@bradzacherbradzacherMar 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.

ronami reacted with thumbs up emoji
Copy link
MemberAuthor

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 reacted with thumbs up emoji
Copy link
Member

@ronamironami 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 🙈)

</TabItem>
</Tabs>

## Limitations
Copy link
Member

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

bradzacher reacted with thumbs up emoji
@bradzacher
Copy link
MemberAuthor

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

kirkwaiblinger reacted with laugh emoji

@kirkwaiblinger
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?

@bradzacher
Copy link
MemberAuthor

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 and JoshuaKGoldberg reacted with thumbs up emoji

@kirkwaiblingerkirkwaiblinger added the awaiting responseIssues waiting for a reply from the OP or another party labelMar 19, 2025
@kirkwaiblingerkirkwaiblinger marked this pull request as draftMarch 19, 2025 09:39
@JoshuaKGoldbergJoshuaKGoldberg removed the 1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge labelMar 24, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@ronamironamironami left review comments

@JoshuaKGoldbergJoshuaKGoldbergJoshuaKGoldberg approved these changes

Assignees
No one assigned
Labels
awaiting responseIssues waiting for a reply from the OP or another partyenhancement: new base rule extensionNew base rule extension required to handle a TS specific case
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@bradzacher@kirkwaiblinger@JoshuaKGoldberg@ronami
[8]ページ先頭
©2009-2025 Movatter.jp